VSE: Process audio strips waveforms in parallel #108877

Merged
Richard Antalik merged 13 commits from Yup_Lucas/blender:Yup_Lucas/vse-waveform-task-pool into main 2023-07-13 03:22:58 +02:00
Contributor

This PR is part of the GSOC 2023 project to improve the speed of waveform drawing.

As a first step, we're pushing strips to be processed in parallel. The current implementation processes strips serially.


A TaskPool is used to execute tasks in the backghround.

Whenever a new audio strip needs their waveform to be computed, it's
added as a task to the TaskPool.

Once the PreviewJob has submitted all available tasks to the task pool,
it waits for all of them to finish before exiting.

The TaskPool is configured to start tasks as soon as they are pushed
onto the task queue.


Testing

  1. Added three different videos to the sequencer (~350MB each)
  2. Turned on waveform display
  3. Waited for waveforms to appear

NOTE: These demos were recorded in debug mode.

Single-threaded mode (main branch) demo

Multi-threaded mode (this PR) demo

This PR is part of the GSOC 2023 project to improve the speed of waveform drawing. As a first step, we're pushing strips to be processed in parallel. The current implementation processes strips serially. ---- A TaskPool is used to execute tasks in the backghround. Whenever a new audio strip needs their waveform to be computed, it's added as a task to the TaskPool. Once the PreviewJob has submitted all available tasks to the task pool, it waits for all of them to finish before exiting. The TaskPool is configured to start tasks as soon as they are pushed onto the task queue. --- ### Testing 1. Added three different videos to the sequencer (~350MB each) 2. Turned on waveform display 3. Waited for waveforms to appear **NOTE: These demos were recorded in debug mode.** #### Single-threaded mode (main branch) demo <video src="/attachments/c53067cb-9690-4f0c-adf7-5af94dc74677" title="VSE Single-threaded demo.mp4" controls></video> #### Multi-threaded mode (this PR) demo <video src="/attachments/1b8b4d3f-2d16-409e-bfd6-cacc56cf9c08" title="VSE Mult-threaded task pool demo.mp4" controls></video>
Lucas Tadeu added 1 commit 2023-06-12 09:55:30 +02:00
A TaskPool is used to execute tasks in the backghround.

Whenever a new audio strip needs their waveform to be computed, it's
added as a task to the TaskPool.

Once the PreviewJob has submitted all available tasks to the task pool,
it waits for all of them to finish before exiting.

The TaskPool is configured to start tasks as soon as they are pushed
onto the task queue.
Lucas Tadeu requested review from Richard Antalik 2023-06-12 09:55:46 +02:00

I have tested the patch on random .blend file I have, and noticed, that only few strips are processed and then job quits.
This seems to be due to nature of current setup - Each time sequencer_preview_add_sound() is called, PreviewJobAudio is added to queue as long as PreviewJob exists, but preview_startjob() is called only once. So with task pool, jobs are processed very quickly and BLI_task_pool_work_and_wait() is executed while new jobs are still pushed in.

I didn't realize this would be an issue. You may try to not wait for tasks to finish and suspend PreviewJob thread (see BLI_condition_wait(), I used it in sequencer prefetch.c). I am not sure if you can poll whether all tasks of task pool are finished.
If polling is not possilbe, I suppose, it would be possible to advance counter of finished tasks from free_read_sound_waveform_task() and resume PreviewJob thread. Then it can exit once all tasks are done and queue is empty.
I would imagine, this must be already solved in some area. What comes to mind is file browser and thumbnail rendering - it's similar problem. So perhaps there already is API for it.

You could also gather all strips to be processed in advance, and then create the job. But user can still request new set of strips to be processed while a job is running, so this is not much more helpful.

I have tested the patch on random .blend file I have, and noticed, that only few strips are processed and then job quits. This seems to be due to nature of current setup - Each time `sequencer_preview_add_sound()` is called, `PreviewJobAudio` is added to queue as long as `PreviewJob` exists, but `preview_startjob()` is called only once. So with task pool, jobs are processed very quickly and `BLI_task_pool_work_and_wait()` is executed while new jobs are still pushed in. I didn't realize this would be an issue. You may try to not wait for tasks to finish and suspend `PreviewJob` thread (see `BLI_condition_wait()`, I used it in sequencer `prefetch.c`). I am not sure if you can poll whether all tasks of task pool are finished. If polling is not possilbe, I suppose, it would be possible to advance counter of finished tasks from `free_read_sound_waveform_task()` and resume `PreviewJob` thread. Then it can exit once all tasks are done and queue is empty. I would imagine, this must be already solved in some area. What comes to mind is file browser and thumbnail rendering - it's similar problem. So perhaps there already is API for it. You could also gather all strips to be processed in advance, and then create the job. But user can still request new set of strips to be processed while a job is running, so this is not much more helpful.
Author
Contributor

@iss thanks for catching that. Yeah I think the condition variable could work, I just need to check if this would impact the task pool in some way.

Regarding polling, I haven't found anything in the BLI_task.h API, but I'll take a look at TBB's documentation. Maybe they have support for that kind of thing and it's just not exposed through Blender's API. If it turns out we can poll the TaskPool, then maybe we can:

  1. use something like std::condition_variable_any::wait_until to block with a timeout.
  2. When the time is up in the PreviewJob, we push all new jobs to the task queue and we poll the queue to see how many tasks have completed.
  3. Then loop back and wait on on the condition variable with the timeout.

I'd just have to write a wrapper around that behaviour if it doesn't exist already.

I'm gonna take a look at the file browser and thumbnail rendering code and see how it was done over there.

@iss thanks for catching that. Yeah I think the condition variable could work, I just need to check if this would impact the task pool in some way. Regarding polling, I haven't found anything in the BLI_task.h API, but I'll take a look at TBB's documentation. Maybe they have support for that kind of thing and it's just not exposed through Blender's API. If it turns out we can poll the TaskPool, then maybe we can: 1. use something like `std::condition_variable_any::wait_until` to block with a timeout. 2. When the time is up in the PreviewJob, we push all new jobs to the task queue and we poll the queue to see how many tasks have completed. 3. Then loop back and wait on on the condition variable with the timeout. I'd just have to write a wrapper around that behaviour if it doesn't exist already. I'm gonna take a look at the file browser and thumbnail rendering code and see how it was done over there.
Author
Contributor

Just an update here: No luck with the TBB docs.

There doesn't seem to be a way to just poll the threads.

I'm working on the changes to make it so that the PreviewJob waits on a condition variable like you did in the prefetch.c file.

Whenever a new audio job is added to the queue, the condition variable is triggered.
Whenever audio job is completed, the condition variable is triggered.

When there are no more audio jobs to process, the preview job ends.

Just an update here: No luck with the TBB docs. There doesn't seem to be a way to just poll the threads. I'm working on the changes to make it so that the PreviewJob waits on a condition variable like you did in the prefetch.c file. Whenever a new audio job is added to the queue, the condition variable is triggered. Whenever audio job is completed, the condition variable is triggered. When there are no more audio jobs to process, the preview job ends.
Lucas Tadeu added 1 commit 2023-06-16 16:58:53 +02:00
Author
Contributor

Following up on this:

I read through sequencer_thumbnails.c but had no luck there.
The code that computes the thumbnail data creates a new WM job and creates a job that goes over a ghash one item at time.
It doesn't appear to be thread safe as all it does is iterate over the keys in the ghash. Looking through the ghash file I don't see any locking mechanism present in the interface.

filelist.cc also uses a WM job to process the list of files, but all it does is lock the entire list and process them one by one. there's no polling done there. This is probably fine since this should be fairly quick.

fsmenu.c also uses a WM job to validate bookmarks. Doesn't do any kind of locking.

Following up on this: I read through [sequencer_thumbnails.c](https://projects.blender.org/Yup_Lucas/blender/src/branch/main/source/blender/editors/space_sequencer/sequencer_thumbnails.c) but had no luck there. The code that computes the thumbnail data creates a new WM job and creates a job that goes over a [ghash](https://projects.blender.org/Yup_Lucas/blender/src/branch/main/source/blender/blenlib/BLI_ghash.h) one item at time. It doesn't appear to be thread safe as all it does is iterate over the keys in the ghash. Looking through the ghash file I don't see any locking mechanism present in the interface. [filelist.cc](https://projects.blender.org/Yup_Lucas/blender/src/branch/main/source/blender/editors/space_file/filelist.cc) also uses a WM job to process the list of files, but all it does is lock the entire list and process them one by one. there's no polling done there. This is probably fine since this should be fairly quick. [fsmenu.c](https://projects.blender.org/Yup_Lucas/blender/src/branch/main/source/blender/editors/space_file/fsmenu.c) also uses a WM job to validate bookmarks. Doesn't do any kind of locking.
Lucas Tadeu added 1 commit 2023-06-19 15:17:21 +02:00
Author
Contributor

@iss the current state in this PR sort of works, but I think there's still a race condition I wasn't able to trigger.

What I think can happen is this:

  1. The PreviewJob thread sees that all outstanding jobs are processed and prepares to exit (keep_looping = false)
  2. In the main thread, a new audio file gets added.
  3. The sequencer_preview_add_sound sees the PreviewJob is still active and tries to lock the mutex to add a new file to the list. This doesn't go through because the mutex is still locked by the PreviewJob that is terminating.
  4. The PreviewJob finishes.
  5. The sequencer_preview_add_sound adds the new audio file to the PreviewJob pointer it received initially. This may or may not crash. If the PreviewJob is deallocated concurrently, we could be trying to access an invalid pointer. If the PreviewJob is only deallocated in the main thread, the new audio file gets added but leaks.

I'm not sure how to mitigate this issue.

I'm considering two options:

  1. Never terminate the preview job and just let it block on the conditon_wait call.
  2. Add some state flag to the preview job (like bool terminating): When the flag is true sequencer_preview_add_sound keeps waiting until the job done. It then creates a new preview job
@iss the current state in this PR sort of works, but I think there's still a race condition I wasn't able to trigger. What I think can happen is this: 1. The PreviewJob thread sees that all outstanding jobs are processed and prepares to exit (keep_looping = false) 2. In the main thread, a new audio file gets added. 3. The `sequencer_preview_add_sound` sees the PreviewJob is still active and tries to lock the mutex to add a new file to the list. This doesn't go through because the mutex is still locked by the PreviewJob that is terminating. 4. The PreviewJob finishes. 5. The `sequencer_preview_add_sound` adds the new audio file to the `PreviewJob` pointer it received initially. This may or may not crash. If the PreviewJob is deallocated concurrently, we could be trying to access an invalid pointer. If the PreviewJob is only deallocated in the main thread, the new audio file gets added but leaks. I'm not sure how to mitigate this issue. I'm considering two options: 1. Never terminate the preview job and just let it block on the conditon_wait call. 2. Add some state flag to the preview job (like `bool terminating`): When the flag is true `sequencer_preview_add_sound` keeps waiting until the job done. It then creates a new preview job

I read through [sequencer_thumbnails.c]
I've meant filebrowser thumbnail rendering. I Should have been more clearer about that. That said, sequencer thumbnail's thread safety should be provided by sequencer cache.

filelist.cc also uses a WM job to process the list of files, but all it does is lock the entire list and process them one by one. there's no polling done there. This is probably fine since this should be fairly quick.

fsmenu.c also uses a WM job to validate bookmarks. Doesn't do any kind of locking.

Sorry, I have lead you astray here. I could have checked, that filebrowser actually didn't do any on demand job updates like we do for waveforms.

I'm considering two options:

  1. Never terminate the preview job and just let it block on the conditon_wait call.
  2. Add some state flag to the preview job (like bool terminating): When the flag is true sequencer_preview_add_sound keeps waiting until the job done. It then creates a new preview job

I have checked the latest patch and I think the scenario you have described in previous post is correct. Personally I would go with option 2 - check whether new job has to be created. This is because I would reserve resident threads to low latency tasks or when updating job data is expensive. I think, that resident thread would also require handling when new .blend file is open and that would complicate things (Thread could be referenced in SpaceSeq.runtime and terminated in sequencer_free()).

In general, the design principle is, that main thread should never lag for user. In this patch after keep_looping is set to false job practically immediately quits, because all tasks are done. Function free_preview_job() is called from main thread, so checking state of PreviewJob is safe. However, that also means you can't just use while loop to wait until the job is finished. This can be resolved by using event system to form a loop:

diff --git a/source/blender/editors/space_sequencer/sequencer_preview.c b/source/blender/editors/space_sequencer/sequencer_preview.c
index f3d09c33bea..32b20f4ca62 100644
--- a/source/blender/editors/space_sequencer/sequencer_preview.c
+++ b/source/blender/editors/space_sequencer/sequencer_preview.c
@@ -126,6 +126,11 @@ void sequencer_preview_add_sound(const bContext *C, Sequence *seq)
   /* Get the preview job if it exists. */
   pj = WM_jobs_customdata_get(wm_job);

+  /* Wait until job terminates completely before starting new one. */
+  if (pj != NULL && !pj->keep_looping) {
+    WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, NULL);
+    return;
+  }
+
   if (!pj) {
     pj = MEM_callocN(sizeof(PreviewJob), "preview rebuild job");

WM_event_add_notifier() tags editor to do redraw, which is not very efficient, but only 1-2 redraws should be needed.

In any case, so far the patch looks very promising!

> I read through [sequencer_thumbnails.c] I've meant filebrowser thumbnail rendering. I Should have been more clearer about that. That said, sequencer thumbnail's thread safety should be provided by sequencer cache. > [filelist.cc](https://projects.blender.org/Yup_Lucas/blender/src/branch/main/source/blender/editors/space_file/filelist.cc) also uses a WM job to process the list of files, but all it does is lock the entire list and process them one by one. there's no polling done there. This is probably fine since this should be fairly quick. > > [fsmenu.c](https://projects.blender.org/Yup_Lucas/blender/src/branch/main/source/blender/editors/space_file/fsmenu.c) also uses a WM job to validate bookmarks. Doesn't do any kind of locking. Sorry, I have lead you astray here. I could have checked, that filebrowser actually didn't do any on demand job updates like we do for waveforms. > I'm considering two options: > 1. Never terminate the preview job and just let it block on the conditon_wait call. > 2. Add some state flag to the preview job (like `bool terminating`): When the flag is true `sequencer_preview_add_sound` keeps waiting until the job done. It then creates a new preview job I have checked the latest patch and I think the scenario you have described in previous post is correct. Personally I would go with option 2 - check whether new job has to be created. This is because I would reserve resident threads to low latency tasks or when updating job data is expensive. I think, that resident thread would also require handling when new .blend file is open and that would complicate things (Thread could be referenced in `SpaceSeq.runtime` and terminated in `sequencer_free()`). In general, the design principle is, that main thread should never lag for user. In this patch after `keep_looping` is set to `false` job practically immediately quits, because all tasks are done. Function `free_preview_job()` is called from main thread, so checking state of `PreviewJob` is safe. However, that also means you can't just use `while` loop to wait until the job is finished. This can be resolved by using event system to form a loop: ```Diff diff --git a/source/blender/editors/space_sequencer/sequencer_preview.c b/source/blender/editors/space_sequencer/sequencer_preview.c index f3d09c33bea..32b20f4ca62 100644 --- a/source/blender/editors/space_sequencer/sequencer_preview.c +++ b/source/blender/editors/space_sequencer/sequencer_preview.c @@ -126,6 +126,11 @@ void sequencer_preview_add_sound(const bContext *C, Sequence *seq) /* Get the preview job if it exists. */ pj = WM_jobs_customdata_get(wm_job); + /* Wait until job terminates completely before starting new one. */ + if (pj != NULL && !pj->keep_looping) { + WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, NULL); + return; + } + if (!pj) { pj = MEM_callocN(sizeof(PreviewJob), "preview rebuild job"); ``` `WM_event_add_notifier()` tags editor to do redraw, which is not very efficient, but only 1-2 redraws should be needed. In any case, so far the patch looks very promising!
Lucas Tadeu added 1 commit 2023-06-26 09:43:16 +02:00
If a sound is added to the queue when the preview job is done processing
its current list of sounds, a leak could happen: The new sound gets
added to the queue but the job never actually processes it.

This change follows @iss's suggestion of using the WM_event_add_notifier
function to force a redraw when that is that case.

If the PreviewJob is terminating (running = false):
- First clear the sound's loading tag: Without this, the code in
  sequencer_draw.c will not be able to push the sound for processing in
  the second pass.
- Add the event notifier for the current scene.
Author
Contributor

Alright, this took me a while but it's finally working as I wanted.

Context

Just using the event notifier doesn't work because in sequencer_draw.c we check if the sound is marked as loading before submitting it to the PreviewJob.

Took me some time to realize that was an issue. Without doing that the sound never gets pushed to the PreviewJob after the first attempt.

Alright, this took me a while but it's finally working as I wanted. ## Context Just using the event notifier doesn't work because in [sequencer_draw.c](https://projects.blender.org/Yup_Lucas/blender/src/branch/main/source/blender/editors/space_sequencer/sequencer_draw.c#L273-L277) we check if the sound is marked as loading before submitting it to the PreviewJob. Took me some time to realize that was an issue. Without doing that the sound never gets pushed to the PreviewJob after the first attempt.
Lucas Tadeu changed title from WIP: VSE: Process audio strips waveforms in parallel to VSE: Process audio strips waveforms in parallel 2023-06-26 09:47:42 +02:00
Lucas Tadeu added 1 commit 2023-06-26 09:49:46 +02:00
Mark job as running by default and move comment around
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
e926125a69

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR108877) when ready.
Author
Contributor

@iss I just saw buildbot found a merge conflict here.

Does blender require any special process for this sort of stuff or can I just go ahead and merge master into this branch to sort out the conflict?

@iss I just saw buildbot found a merge conflict here. Does blender require any special process for this sort of stuff or can I just go ahead and merge master into this branch to sort out the conflict?
Richard Antalik requested changes 2023-07-04 03:03:47 +02:00
Richard Antalik left a comment
Member

Please use clang-format to format the changes or run make format.
This file has been converted to C++ in main branch, so please merge it to your branch. For C++ there will be only few places where static_cast has to be used for type casting, otherwise code should work.

Functionality wise I have no concerns, but I have few suggestions to improve code readability.

Please use clang-format to format the changes or run `make format`. This file has been converted to C++ in main branch, so please merge it to your branch. For C++ there will be only few places where `static_cast` has to be used for type casting, otherwise code should work. Functionality wise I have no concerns, but I have few suggestions to improve code readability.
@ -23,6 +24,8 @@
#include "MEM_guardedalloc.h"
#include "atomic_ops.h"

This include is not used.

This include is not used.
Yup_Lucas marked this conversation as resolved
@ -44,1 +49,4 @@
typedef struct ReadSoundWaveformTask {
PreviewJob *wm_job;
bool *wm_do_update;

wm_do_update, wm_progress and are unused in this struct. These can be removed

`wm_do_update`, `wm_progress` and are unused in this struct. These can be removed
Yup_Lucas marked this conversation as resolved
@ -57,2 +109,3 @@
TaskPool *task_pool = BLI_task_pool_create(NULL, TASK_PRIORITY_LOW);
PreviewJob *pj = data;
PreviewJobAudio *previewjb;
bool keep_looping = true;

This variable mirrors PreviewJob::running, so you can use just one of these.

This variable mirrors `PreviewJob::running`, so you can use just one of these.
Yup_Lucas marked this conversation as resolved
@ -67,0 +117,4 @@
while (keep_looping) {
/* Wait until there's either a new audio job to process or one of the previously submitted jobs is done.*/
BLI_mutex_lock(pj->mutex);
while (pj->previews.first == NULL && previous_processed == pj->processed) {

Use BLI_listbase_is_empty(&pj->previews)

I was bit confused by previous_processed == pj->processed part. Unless I missed something, this is only for updating progress, so I would suggest following:

    while (BLI_listbase_is_empty(&pj->previews) && pj->processed != pj->total) {
      float current_progress = (pj->total > 0) ? (float)pj->processed / (float)pj->total : 1.0f;

      if (current_progress != *progress) {
        *progress = current_progress;
        *do_update = true;
      }

      BLI_condition_wait(&pj->preview_suspend_cond, pj->mutex);
    }

Having update_progress() function would be even better.

Use `BLI_listbase_is_empty(&pj->previews)` I was bit confused by `previous_processed == pj->processed` part. Unless I missed something, this is only for updating progress, so I would suggest following: ``` while (BLI_listbase_is_empty(&pj->previews) && pj->processed != pj->total) { float current_progress = (pj->total > 0) ? (float)pj->processed / (float)pj->total : 1.0f; if (current_progress != *progress) { *progress = current_progress; *do_update = true; } BLI_condition_wait(&pj->preview_suspend_cond, pj->mutex); } ``` Having `update_progress()` function would be even better.
Yup_Lucas marked this conversation as resolved
@ -79,2 +132,2 @@
sound->tags &= ~SOUND_TAGS_WAVEFORM_LOADING;
BLI_spin_unlock(sound->spinlock);
if (pj->processed == pj->total) {
keep_looping = false;

Bit of nitpick, but I think the code would be bit more readable:
Here you can just break out of while loop.
Following else if could have been just normal if statement also breaking out of while loop. Moving code to cancel_tasks() function would also make this much clearer.
Final else statement you can then remove and unindent.

With that, you may also use while(true) loop, since condition will never evaluate to false.

Bit of nitpick, but I think the code would be bit more readable: Here you can just break out of while loop. Following `else if` could have been just normal if statement also breaking out of while loop. Moving code to `cancel_tasks()` function would also make this much clearer. Final `else` statement you can then remove and unindent. With that, you may also use `while(true)` loop, since condition will never evaluate to false.
Author
Contributor

I can't directly break there because the mutex is locked at that point.

The options is to either have BLI_mutex_unlock call at each "break out" point or to just have the if-elseif-else clause to make sure that always happens.

We also need to set pj->running = false inside the lock to make sure the main thread (the one inserting jobs into the list) actually sees the change.

I'll make the change to split the nested ifs, but I'll have to replicate the running = false and mutex unlock calls.

I can't directly break there because the mutex is locked at that point. The options is to either have BLI_mutex_unlock call at each "break out" point or to just have the if-elseif-else clause to make sure that always happens. We also need to set `pj->running = false` inside the lock to make sure the main thread (the one inserting jobs into the list) actually sees the change. I'll make the change to split the nested ifs, but I'll have to replicate the running = false and mutex unlock calls.
iss marked this conversation as resolved
@ -83,2 +138,2 @@
previewjb = previewjb->next;
BLI_mutex_unlock(pj->mutex);
/* Clear all pending jobs data. */
for (PreviewJobAudio *previewjb = pj->previews.first; previewjb != NULL; previewjb = previewjb->next) {

You can use LISTBASE_FOREACH macro here, it's bit less noisy than plain for loop:
LISTBASE_FOREACH (PreviewJobAudio *, previewjb, &pj->previews){}

You can use `LISTBASE_FOREACH` macro here, it's bit less noisy than plain for loop: `LISTBASE_FOREACH (PreviewJobAudio *, previewjb, &pj->previews){}`
Yup_Lucas marked this conversation as resolved
@ -91,0 +148,4 @@
} else {
/* Push all available jobs to the processing queue. */
PreviewJobAudio *previewjb = pj->previews.first;
while (previewjb) {

Foreach macro can be used here too, just use LISTBASE_FOREACH_MUTABLE since you are modifying linked list.

Also if you make function for pushing task, you won't need to clarify what code does with comment:

    LISTBASE_FOREACH_MUTABLE (PreviewJobAudio *, previewjb, &pj->previews) {
      push_task(task_pool, pj, previewjb);
    }
Foreach macro can be used here too, just use `LISTBASE_FOREACH_MUTABLE` since you are modifying linked list. Also if you make function for pushing task, you won't need to clarify what code does with comment: ``` LISTBASE_FOREACH_MUTABLE (PreviewJobAudio *, previewjb, &pj->previews) { push_task(task_pool, pj, previewjb); } ```
Yup_Lucas marked this conversation as resolved
@ -130,0 +199,4 @@
BLI_mutex_lock(pj->mutex);
/* If the job exists but is not running, bail and try again on the next draw call. */
if (!pj->running) {

I think these conditions could be simplified a bit. I would suggest

  if (pj && !pj->running) {
    /* Clear the sound loading tag to that it can be reattempted. */
    clear_sound_waveform_loading_tag(seq->sound);
    WM_event_add_notifier(C, NC_SCENE | ND_SPACE_SEQUENCER, CTX_data_scene(C));
    return;
  }

  if (!pj) { /* There's no existig preview job. */
  ...
  }
I think these conditions could be simplified a bit. I would suggest ``` if (pj && !pj->running) { /* Clear the sound loading tag to that it can be reattempted. */ clear_sound_waveform_loading_tag(seq->sound); WM_event_add_notifier(C, NC_SCENE | ND_SPACE_SEQUENCER, CTX_data_scene(C)); return; } if (!pj) { /* There's no existig preview job. */ ... } ```
Author
Contributor

But don't we need to lock the struct to access the running variable?
It could be changed concurrently by the thread running the WM job.

But don't we need to lock the struct to access the running variable? It could be changed concurrently by the thread running the WM job.

Right, you have to lock, but if running is false, there would be no point. I would say, the reason for locking is rather to prevent PreviewJob from getting done until you add new PreviewJobAudio to list. In this patch it could happen.

So I would correct myself to

  if (pj && !pj->running) {
    /* Clear the sound loading tag to that it can be reattempted. */
    clear_sound_waveform_loading_tag(seq->sound);
    WM_event_add_notifier(C, NC_SCENE | ND_SPACE_SEQUENCER, CTX_data_scene(C));
    return;
  }

  if (pj) { 
  BLI_mutex_lock(pj->mutex); /* Just lock, will add files later. */
  } else {
    /* Create the job (and lock mutex). */
  }

  BLI_addtail(&pj->previews, audiojob);
  pj->total++;
  BLI_mutex_unlock(pj->mutex);
  ...

My point is mainly to eliminate nested if statements if possible.

Right, you have to lock, but if `running` is false, there would be no point. I would say, the reason for locking is rather to prevent `PreviewJob` from getting done until you add new `PreviewJobAudio` to list. In this patch it could happen. So I would correct myself to ``` if (pj && !pj->running) { /* Clear the sound loading tag to that it can be reattempted. */ clear_sound_waveform_loading_tag(seq->sound); WM_event_add_notifier(C, NC_SCENE | ND_SPACE_SEQUENCER, CTX_data_scene(C)); return; } if (pj) { BLI_mutex_lock(pj->mutex); /* Just lock, will add files later. */ } else { /* Create the job (and lock mutex). */ } BLI_addtail(&pj->previews, audiojob); pj->total++; BLI_mutex_unlock(pj->mutex); ... ``` My point is mainly to eliminate nested `if` statements if possible.

Sorry, I have realized, that even my post above is not totally thread safe, since running can change from time first condition is checked to second one so please disregard that.

Sorry, I have realized, that even my post above is not totally thread safe, since `running` can change from time first condition is checked to second one so please disregard that.

@iss I just saw buildbot found a merge conflict here.

Does blender require any special process for this sort of stuff or can I just go ahead and merge master into this branch to sort out the conflict?

I have noticed that locally. I don't think it would allow you to merge changes if there are conflicts. You will have merge main to your branch, resolve conflicts (I did copy paste whole file from your patch, then redo C++ conversion, because it took me less then resolving conflicts. Just double check unintended changes then). Then push to your remote branch to update patch. After that it should allow you to merge to main.

> @iss I just saw buildbot found a merge conflict here. > > Does blender require any special process for this sort of stuff or can I just go ahead and merge master into this branch to sort out the conflict? I have noticed that locally. I don't think it would allow you to merge changes if there are conflicts. You will have merge main to your branch, resolve conflicts (I did copy paste whole file from your patch, then redo C++ conversion, because it took me less then resolving conflicts. Just double check unintended changes then). Then push to your remote branch to update patch. After that it should allow you to merge to main.
Lucas Tadeu added 3 commits 2023-07-04 14:32:49 +02:00
Lucas Tadeu added 1 commit 2023-07-04 14:34:10 +02:00
Author
Contributor

@iss I sorted out the conflict and applied most of the changes you requested.

I kept setting the pj->running = false inside the lock and the check in the function that pushes new audio jobs.

If we don't really need to lock at insertion, then I'll make the changes and just get rid of the locking there.
I thought we would have to lock to ensure the function reads an up-to-date value. I might be misunderstanding something though.

@iss I sorted out the conflict and applied most of the changes you requested. I kept setting the `pj->running = false` inside the lock and the check in the function that pushes new audio jobs. If we don't really need to lock at insertion, then I'll make the changes and just get rid of the locking there. I thought we would have to lock to ensure the function reads an up-to-date value. I might be misunderstanding something though.
Lucas Tadeu requested review from Richard Antalik 2023-07-04 14:35:41 +02:00
Richard Antalik requested changes 2023-07-05 02:55:56 +02:00
Richard Antalik left a comment
Member

Found one more minor issue.

Found one more minor issue.
@ -102,0 +165,4 @@
PreviewJobAudio *next_previewjb = previewjb->next;
BLI_remlink(&pj->previews, previewjb);
previewjb = next_previewjb;

Don't modify previewjb, it is done by iterator macro. next_previewjb can be removed. You can also use some iterable C++ type here like blender::Vector, to eliminate macros if you hate them :) I don't mind either way.

Don't modify `previewjb`, it is done by iterator macro. `next_previewjb` can be removed. You can also use some iterable C++ type here like `blender::Vector`, to eliminate macros if you hate them :) I don't mind either way.
Yup_Lucas marked this conversation as resolved

@iss I sorted out the conflict and applied most of the changes you requested.

I kept setting the pj->running = false inside the lock and the check in the function that pushes new audio jobs.

If we don't really need to lock at insertion, then I'll make the changes and just get rid of the locking there.
I thought we would have to lock to ensure the function reads an up-to-date value. I might be misunderstanding something though.

I think you are referring to sequencer_preview_add_sound() here? I have clarified this in review comment, so I think locking is actually needed.

> @iss I sorted out the conflict and applied most of the changes you requested. > > I kept setting the `pj->running = false` inside the lock and the check in the function that pushes new audio jobs. > > If we don't really need to lock at insertion, then I'll make the changes and just get rid of the locking there. > I thought we would have to lock to ensure the function reads an up-to-date value. I might be misunderstanding something though. I think you are referring to `sequencer_preview_add_sound()` here? I have clarified this in review comment, so I think locking is actually needed.
Lucas Tadeu added 2 commits 2023-07-10 12:44:31 +02:00
Author
Contributor

@iss I've applied the changes you and merged Blender's main branch into this one.
I ran a build and test this morning and it works as expected.

Thanks a bunch for the review pointers.

@iss I've applied the changes you and merged Blender's main branch into this one. I ran a build and test this morning and it works as expected. Thanks a bunch for the review pointers.
Lucas Tadeu requested review from Richard Antalik 2023-07-10 12:49:01 +02:00
Richard Antalik approved these changes 2023-07-11 00:59:07 +02:00
Richard Antalik left a comment
Member

I still think, that function sequencer_preview_add_sound() is unsafe since PreviewJob::running can change state (to true) until it gets to BLI_addtail(&pj->previews, audiojob); from point where it is checked first.
Would appreciate that change before committing.

AFAIK you should have commit rights, so go ahead with this one.

I still think, that function `sequencer_preview_add_sound()` is unsafe since `PreviewJob::running` can change state (to true) until it gets to `BLI_addtail(&pj->previews, audiojob);` from point where it is checked first. Would appreciate that change before committing. AFAIK you should have commit rights, so go ahead with this one.
Lucas Tadeu added 1 commit 2023-07-11 08:51:41 +02:00
Author
Contributor

Oh, I finally understood what you meant there. I've made the code change to ensure the PreviewJob is locked inside the sequencer_preview_add_sound() function until the new sound is added.

I ran another build with the code changes and it worked. I'll merge the PR once the build passes

Oh, I finally understood what you meant there. I've made the code change to ensure the PreviewJob is locked inside the `sequencer_preview_add_sound()` function until the new sound is added. I ran another build with the code changes and it worked. I'll merge the PR once the build passes
Lucas Tadeu added 1 commit 2023-07-11 08:54:10 +02:00
Author
Contributor

@iss I just noticed I don't actually have commit rights. Can you merge this PR ?

@iss I just noticed I don't actually have commit rights. Can you merge this PR ?

@iss I just noticed I don't actually have commit rights. Can you merge this PR ?

Sure, I assume, that commit message would be:

A TaskPool is used to execute tasks in the backghround.

Whenever a new audio strip needs their waveform to be computed, it's
added as a task to the TaskPool.

Once the PreviewJob has submitted all available tasks to the task pool,
it waits for all of them to finish before exiting.

The TaskPool is configured to start tasks as soon as they are pushed
onto the task queue.

Seems Ok to me, not sure if I have to add some details.

I have reached out whether you actually should have commit rights, I don't remember how this worked last time...

> @iss I just noticed I don't actually have commit rights. Can you merge this PR ? Sure, I assume, that commit message would be: ``` A TaskPool is used to execute tasks in the backghround. Whenever a new audio strip needs their waveform to be computed, it's added as a task to the TaskPool. Once the PreviewJob has submitted all available tasks to the task pool, it waits for all of them to finish before exiting. The TaskPool is configured to start tasks as soon as they are pushed onto the task queue. ``` Seems Ok to me, not sure if I have to add some details. I have reached out whether you actually should have commit rights, I don't remember how this worked last time...
Richard Antalik merged commit b11540a65f into main 2023-07-13 03:22:58 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#108877
No description provided.