Fixed race condition when aligning tracks based on audio
Needs ReviewPublic
Actions

Authored by raphaelm on Apr 14 2017, 7:14 PM.

Details

Reviewers

mardelle
alcinos

Summary

The bug happens when one clicks 'Align Audio to Reference' before
the computations triggered by 'Set Audio Reference' have completed.
In this case, the alignment can be completely arbitrary.

Namely, aligning a child audio track ('Align Audio to Reference')
triggers AudioCorrelation::addChild(), which ultimately calls
AudioCorrelation::slotProcessChild(). This function uses the reference
envelope (m_mainTrackEnvelope->envelope()) in order to compute
the cross-correlation, which might contain arbitrary data if the
computation of the main envelope is not finished. We fix this by
ensuring that m_mainTrackEnvelope->envelope() blocks until the envelope
is computed.

In this commit we also do small code cleanups, and fix another (tiny)
race condition where we connect the signal corresponding to the end
of the envelope computation after the computation has started. If the
computation is really fast, it means the signal could be lost.

Test Plan

Tested: crafted a project that reliably triggers the race condition. Verified that the alignment is arbitrary before this change, and correct after this change.

Diff Detail

Repository

R158 Kdenlive

Lint

Lint Skipped

Unit

Unit Tests Skipped

raphaelm created this revision.Apr 14 2017, 7:14 PM

alcinos added inline comments.Apr 15 2017, 4:41 PM

src/lib/audio/audioEnvelope.cpp
37	The use of smart_ptrs is a plus, I'm also trying to use them more where possible. However, some care has to be taken at the boundaries of the code. When calling connect(), it doesn't matter to .get() the unique_ptr, but here, it might be more of a problem because we break ownership. If in the future someone change something making it possible fo AudioInfo to outlive AudioEnvelope, then we have a problem. I suggest either making the unique_ptr a shared one or borrowing a reference to the unique_ptr if that seems possible.
46	This can be deleted now.
125	Its probably better to make this computation in the loop above, isn't it ?

Addressed alcinos's comments. Added more comments in the code about other race conditions not addressed in this change.

raphaelm added inline comments.Apr 15 2017, 9:52 PM

src/lib/audio/audioEnvelope.cpp
37	Note that the switch to a unique_ptr for m_info is semantically equivalent to the previous code with new/delete: in both cases, m_info is going to be destroyed when AudioEnvelope is destroyed. Does your comment refer to the connect() on line 33? It is indeed an issue if the thread that computes the envelope is started but not finished, and the AudioEnvelope is destroyed (but this issue it not specific to m_info). This can happen if you set the audio reference twice, second time being before the computation of the first envelope has finished: the second call to setAudioAlignReference() will lead to the deletion of the running envelope. I added a partial fix to this pre-existing issue; a partial fix is probably more involved and is probably best done in another patch. Regarding potential future changes that would make AudioInfo outlive the AudioEnvelope, I think we should worry about that when (if) this will happen. Do you have a reason to think this will happen in the near future? If we apply this reasoning to all member objects, we would use shared_ptrs everywhere by default. Let me know if that makes sense, I was a bit confused by your comment, so I might have mis-interpreted.
46	I added back some code to wait for the async computations to be finished.
125	Done. Note that amplitudeMax was only the maximum of the positive values, as in the code before this change. I changed it to be the max of the absolute values, which seems better given the usage of amplitudeMax in drawEnvelope().

Hello,
This fix still hasn't been merged!
Can I apply it to Applications/18.04 and master already?
Then we can see what's needed for refactoring_timeline...

Revision Contents
Changeset List

			Path	Packages
M			src/lib/audio/audioCorrelation.h (33 lines)
M			src/lib/audio/audioCorrelation.cpp (44 lines)
M			src/lib/audio/audioEnvelope.h (82 lines)
M			src/lib/audio/audioEnvelope.cpp (184 lines)
M			src/timeline/customtrackview.cpp (13 lines)