GNOME Bugzilla – Bug 797145
avwait: Send dropping=true message after all streams stopped
Last modified: 2018-10-04 09:15:26 UTC
See commit message
Created attachment 373655 [details] [review] avwait: Send dropping=true message after all streams stopped Previously it was dispatched before the last video buffer, and audio buffers would follow afterwards. It's misleading to send the dropping=true message before both streams have really stopped, it can lead to races when someone is e.g. waiting for that message to send EOS. Also added some debug output.
Created attachment 373656 [details] [review] avwait: Send dropping=true message after all streams stopped Previously it was dispatched before the last video buffer, and audio buffers would follow afterwards. It's misleading to send the dropping=true message before both streams have really stopped, it can lead to races when someone is e.g. waiting for that message to send EOS. Also added some debug output.
Review of attachment 373656 [details] [review]: ::: gst/timecode/gstavwait.c @@ +809,3 @@ if (self->recording) { self->audio_running_time_to_end_at = self->running_time_to_end_at; + self->must_send_end_message = 1; Should be |= @@ +871,3 @@ /* We just stopped recording: synchronise the audio */ self->audio_running_time_to_end_at = running_time; + self->must_send_end_message = 1; Should be |= @@ +931,3 @@ + } + g_mutex_lock (&self->mutex); + if ((self->must_send_end_message & 4) != 0) { This should probably check 2 | 4? @@ +936,3 @@ + gst_avwait_send_element_message (self, TRUE, + self->audio_running_time_to_end_at); + } else if (self->must_send_end_message == 1) { == 1? Or & 1? @@ +1093,3 @@ gst_audio_buffer_clip (inbuf, &asegment2, self->ainfo.rate, self->ainfo.bpf); + if (self->must_send_end_message >= 1) { For flags always use bitwise operations. It's semantically not an integer @@ +1094,3 @@ self->ainfo.bpf); + if (self->must_send_end_message >= 1) { + send_element_message = TRUE; This should probably be |= 4 with the flags? The boolean seems unnecessary ::: gst/timecode/gstavwait.h @@ +93,3 @@ + * Then the next chain function pushes its own buffer and sets it back to 0 + */ + gint must_send_end_message; Please use #defines for this :)
Created attachment 373672 [details] [review] avwait: Send dropping=true message after all streams stopped Previously it was dispatched before the last video buffer, and audio buffers would follow afterwards. It's misleading to send the dropping=true message before both streams have really stopped, it can lead to races when someone is e.g. waiting for that message to send EOS. Also added some debug output.
Thanks, please review again. (In reply to Sebastian Dröge (slomo) from comment #3) > Review of attachment 373656 [details] [review] [review]: > > ::: gst/timecode/gstavwait.c > @@ +931,3 @@ > + } > + g_mutex_lock (&self->mutex); > + if ((self->must_send_end_message & 4) != 0) { > > This should probably check 2 | 4? > > @@ +1094,3 @@ > self->ainfo.bpf); > + if (self->must_send_end_message >= 1) { > + send_element_message = TRUE; > > This should probably be |= 4 with the flags? The boolean seems unnecessary It is necessary because we unlock the mutex in the meantime. That may lead to a race where the video chain might trigger the message with the next (dropped) incoming video buffer, even though audio hasn't called gst_pad_push yet. Added a comment to clarify this.
Review of attachment 373672 [details] [review]: ::: gst/timecode/gstavwait.c @@ +880,3 @@ /* We just stopped recording: synchronise the audio */ self->audio_running_time_to_end_at = running_time; + self->must_send_end_message = END_MESSAGE_STREAM_ENDED; Shouldn't this and above not be |= ? And what if audio (or video) ended long ago already due to EOS, this does not seem to be handled
Created attachment 373728 [details] [review] avwait: Send dropping=true message after all streams stopped Previously it was dispatched before the last video buffer, and audio buffers would follow afterwards. It's misleading to send the dropping=true message before both streams have really stopped, it can lead to races when someone is e.g. waiting for that message to send EOS. Also added some debug output.
Thanks, changed. Please review again.
Review of attachment 373728 [details] [review]: Just two minor things left ::: gst/timecode/gstavwait.c @@ +818,3 @@ if (self->recording) { self->audio_running_time_to_end_at = self->running_time_to_end_at; + self->must_send_end_message = END_MESSAGE_STREAM_ENDED; |= here too @@ +1132,3 @@ + if (send_element_message) { + g_mutex_lock (&self->mutex); + if (self->must_send_end_message & END_MESSAGE_VIDEO_PUSHED || Put the x & y into parenthesis
Created attachment 373729 [details] [review] avwait: Send dropping=true message after all streams stopped Previously it was dispatched before the last video buffer, and audio buffers would follow afterwards. It's misleading to send the dropping=true message before both streams have really stopped, it can lead to races when someone is e.g. waiting for that message to send EOS. Also added some debug output.
Review of attachment 373729 [details] [review]: commit b1b4a043388188cd7dd0ab87d1771d89d12b3ca4 (HEAD -> upstream_master, upstream/master) Author: Vivia Nikolaidou <vivia@ahiru.eu> Date: Fri Sep 14 16:05:20 2018 +0300 avwait: Send dropping=true message after all streams stopped Previously it was dispatched before the last video buffer, and audio buffers would follow afterwards. It's misleading to send the dropping=true message before both streams have really stopped, it can lead to races when someone is e.g. waiting for that message to send EOS. Also added some debug output. https://bugzilla.gnome.org/show_bug.cgi?id=797145
Review of attachment 373729 [details] [review]: oops, it wasn't this one that I committed :)
Created attachment 373730 [details] [review] avwait: Send dropping=true message after all streams stopped Previously it was dispatched before the last video buffer, and audio buffers would follow afterwards. It's misleading to send the dropping=true message before both streams have really stopped, it can lead to races when someone is e.g. waiting for that message to send EOS. Also added some debug output.
Review of attachment 373730 [details] [review]: commit b1b4a043388188cd7dd0ab87d1771d89d12b3ca4 (HEAD -> upstream_master, upstream/master) Author: Vivia Nikolaidou <vivia@ahiru.eu> Date: Fri Sep 14 16:05:20 2018 +0300 avwait: Send dropping=true message after all streams stopped Previously it was dispatched before the last video buffer, and audio buffers would follow afterwards. It's misleading to send the dropping=true message before both streams have really stopped, it can lead to races when someone is e.g. waiting for that message to send EOS. Also added some debug output. https://bugzilla.gnome.org/show_bug.cgi?id=797145
Created attachment 373843 [details] [review] avwait: Fix sending of dropping=true messages If the first audio buffer to be dropped started right between two video buffers (after the end of the first but before the start of the second, as is often the case with N/1001 video frame rates), we would miss sending the dropping=true message.
Created attachment 373844 [details] [review] avwait: Fix sending of dropping=true messages If the first audio buffer to be dropped started right between two video buffers (after the end of the first but before the start of the second, as is often the case with N/1001 video frame rates), we would miss sending the dropping=true message.