VSE: Process audio strips waveforms in parallel #108877
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#108877
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "Yup_Lucas/blender:Yup_Lucas/vse-waveform-task-pool"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
NOTE: These demos were recorded in debug mode.
Single-threaded mode (main branch) demo
Multi-threaded mode (this PR) demo
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 asPreviewJob
exists, butpreview_startjob()
is called only once. So with task pool, jobs are processed very quickly andBLI_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 (seeBLI_condition_wait()
, I used it in sequencerprefetch.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 resumePreviewJob
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.
@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:
std::condition_variable_any::wait_until
to block with a 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.
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.
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.
@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:
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.sequencer_preview_add_sound
adds the new audio file to thePreviewJob
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:
bool terminating
): When the flag is truesequencer_preview_add_sound
keeps waiting until the job done. It then creates a new preview jobSorry, 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 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 insequencer_free()
).In general, the design principle is, that main thread should never lag for user. In this patch after
keep_looping
is set tofalse
job practically immediately quits, because all tasks are done. Functionfree_preview_job()
is called from main thread, so checking state ofPreviewJob
is safe. However, that also means you can't just usewhile
loop to wait until the job is finished. This can be resolved by using event system to form a loop: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!
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.
WIP: VSE: Process audio strips waveforms in parallelto VSE: Process audio strips waveforms in parallel@blender-bot package
Package build started. Download here when ready.
@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?
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.
@ -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@ -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.@ -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:Having
update_progress()
function would be even better.@ -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 tocancel_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.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.
@ -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){}
@ -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:
@ -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
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 preventPreviewJob
from getting done until you add newPreviewJobAudio
to list. In this patch it could happen.So I would correct myself to
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.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 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.
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 likeblender::Vector
, to eliminate macros if you hate them :) I don't mind either way.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'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.
I still think, that function
sequencer_preview_add_sound()
is unsafe sincePreviewJob::running
can change state (to true) until it gets toBLI_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.
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
@iss I just noticed I don't actually have commit rights. Can you merge this PR ?
Sure, I assume, that commit message would be:
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...