GNOME Bugzilla – Bug 729882
opusenc: use aux vars to minimize critical region
Last modified: 2014-05-26 07:26:13 UTC
This commit minimize critical region in gstopusenc(gst_opus_enc_encode) with the main goal to avoid dead lock between gst_audio_encoder_finish_frame and gst_opus_enc_get_property. This dead lock has been shown debugging the pipeline graph (GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS), where gst_opus_enc_get_property is called. Also, this allow block the element less time.
Created attachment 276249 [details] [review] A patch to solve the bug
Review of attachment 276249 [details] [review]: ::: ext/opus/gstopusenc.c @@ +805,3 @@ + n_channels = enc->n_channels; + frame_samples = enc->frame_samples; + state = enc->state; Note that the number of channels does not need to be protected here (it's only modified from the streaming thread), and that you need to ensure that the state is also only changed from the streaming thread (which it probably is, I didn't change the code). If the latter is not true, releasing the lock here is introducing a race condition... but if it is true you also don't need to protect it with the mutex at all. As far as I see this here you only need to copy the values set from the GObject properties and have those protected with a mutex, everything else is just fine.
Review of attachment 276249 [details] [review]: ::: ext/opus/gstopusenc.c @@ +805,3 @@ + n_channels = enc->n_channels; + frame_samples = enc->frame_samples; + state = enc->state; About n_channels Its value is modified into a protected section: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/opus/gstopusenc.c?h=1.2#n613 About state with this change can be a problem because it can be destroyed (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/opus/gstopusenc.c?h=1.2#n621) before use it (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/opus/gstopusenc.c?h=1.2#n834). So I am going to do a change in this patch and upload it again.
set_format() is always called from the streaming thread, as is the encoding code (and thus protected with the stream lock already). I think that's ok then, but as for n_channels it's not necessary to protect it with any mutex manually (it already is). However that's really just nitpicking :)
Created attachment 276285 [details] [review] Fix dead lock managing critical section properly
Review of attachment 276285 [details] [review]: ::: ext/opus/gstopusenc.c @@ +845,3 @@ + frame_samples, omap.data, max_payload_size * enc->n_channels); + + g_mutex_unlock (&enc->property_lock); If I'm not mistaken you can unlock already right before "buf" is mapped
Review of attachment 276285 [details] [review]: ::: ext/opus/gstopusenc.c @@ +845,3 @@ + frame_samples, omap.data, max_payload_size * enc->n_channels); + + g_mutex_unlock (&enc->property_lock); It was done in the previous patch, and I have had to change it because opus_multistream_encode uses enc->state and it can be destroyed before use it in this function. So we must unlock here.
How can enc->state be destroyed before use in this function?
enc->state can be destroyed (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/opus/gstopusenc.c?h=1.2#n621) before using it (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/opus/gstopusenc.c?h=1.2#n834).
Both functions are guaranteed to be called from the streaming thread
So, is the first patch that I uploaded OK? I have changed it because of the Comment 2 (https://bugzilla.gnome.org/show_bug.cgi?id=729882#c2)
My point there was that you don't need to lock anything for access to enc->state, enc->n_channels and enc->sample_rate... but only to those fields that are set as part of a GObject property.
Created attachment 276289 [details] [review] Fix dead lock managing critical section properly
Sorry, I didn't understand you. I hope the new patch is OK ;).
Sorry for not being clearer with what I meant, and thanks for your patience and the patch :) commit 93ba600ba930997cd9244caca2372602020faefe Author: Miguel París Díaz <mparisdiaz@gmail.com> Date: Sat May 10 18:32:28 2014 +0200 opusenc: Use aux vars to minimize critical region This avoid dead lock between gst_audio_encoder_finish_frame() and gst_opus_enc_get_property(). Also, now bytes var is set into protected section. https://bugzilla.gnome.org/show_bug.cgi?id=729882