mltframework / shotcut

cross-platform (Qt), open-source (GPLv3) video editor
https://www.shotcut.org
GNU General Public License v3.0
10.88k stars 1.13k forks source link

Concurrency? issue in audio scopes #188

Closed j-b-m closed 8 years ago

j-b-m commented 8 years ago

Hi all, There is a small problem in Shotcut's audio scopes that I reproduced in Kdenlive when using some of your code. When using more than one audio scope, the audio data delivered to the scopes is sometimes corrupted. This problem does not happen when only one audio scope is used.

This can be triggered by opening all three audio scopes (level, wave, spectrum) and playing an audio file. You can notice a small flickering (you need to watch closely) in the scopes. Also, the spectrum will display errors in the terminal like: [Error ] [filter fft] Unsupported format 0

I traced the problem to the filters used for the scopes, like "fft" and "audiolevel". Sometimes, when processing these filters for the scopes, the call to

int error = mlt_frame_get_audio( frame, buffer, format, frequency, channels, samples );

returns a value of 0 for format and samples, causing the flicker in the scopes because it is interpreted as no audio.

I think this is related to the SharedFrame copies that are used by the scopes, because the issue is not present when only one scope is enabled, but have been unable to really understand what is happening. If anyone has a clue, that is more than welcome.

bmatherly commented 8 years ago

I might have a chance to look into this in the next week or two.

In the meantime, if you are looking for a suggestion...

I have always been suspicious of the clone function: Mlt::Frame SharedFrame::clone(bool audio, bool image, bool alpha) const

The function is const, but it calls a bunch of non-const functions under the hood. In the case of three active monitors, the clone function will get called three times - once for each monitor. I never tested that the clone function is perfectly thread save. Consider adding a critical section around the clone to see if that helps.

Also, the clone function copies some properties - including the producer:

cloneFrame.inherit(d->f);
cloneFrame.set("_producer", d->f.get_data("_producer", size), 0, NULL, NULL);
cloneFrame.set("movit.convert", d->f.get_data("movit.convert", size), 0, NULL, NULL);
cloneFrame.get_frame()->convert_image = d->f.get_frame()->convert_image;
cloneFrame.get_frame()->convert_audio = d->f.get_frame()->convert_audio;

I'm not sure if there might be a side effect of having three frames that have inherited the same properties like that. You could try commenting out some of those and see if it makes any difference.

j-b-m commented 8 years ago

Thanks for the lighning fast answer. I was also suspecting something around the clone() function. You were right. Locking the properties before calling inherit seems to fix the problem:

d->f.lock();
cloneFrame.inherit(d->f);
d->f.unlock();

I am glad that I can stop watching at those blinking scopes trying to debug the problem!

bmatherly commented 8 years ago

Nice. I'm glad you were able to figure it out.

bmatherly commented 8 years ago

Thinking about this some more... I wonder if we should put the lock/unlock in the inherit function itself.

ddennedy commented 8 years ago

Yeah, Brian, that is a better idea, in mlt_properties_inherit(). However, that will not let Kdenlive have a short term fix.

bmatherly commented 8 years ago

I was able to reproduce the issue in shotcut and resolve it by adding lock to the inherit function.

https://github.com/mltframework/mlt/commit/ae7af0239d378e4ccc8da402687d735d2f944784