GNOME Bugzilla – Bug 796836
avwait: Add recording property
Last modified: 2018-07-24 10:25:47 UTC
Still WIP, documentation still needed, but a preliminary review would be helpful.
Created attachment 373093 [details] [review] avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through.
(In reply to Vivia Nikolaidou from comment #1) > Created attachment 373093 [details] [review] [review] > avwait: Add recording property > > It works like a valve in front of the actual avwait. When recording == > TRUE, other rules are then examined. When recording == FALSE, nothing is > passing through. I assume when recording is set to FALSE it then also makes sure that both streams end at more or less the same time?
Created attachment 373095 [details] [review] avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through.
(In reply to Sebastian Dröge (slomo) from comment #2) > I assume when recording is set to FALSE it then also makes sure that both > streams end at more or less the same time? Yes. At least that's the plan, I've only made approximate testing so far.
Created attachment 373096 [details] [review] avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through.
Review of attachment 373096 [details] [review]: ::: gst/timecode/gstavwait.c @@ +785,3 @@ } + + self->audio_running_time_to_wait_for = self->running_time_to_wait_for; All this code could use some comments. Why is this done, what's the purpose? :) @@ +790,3 @@ + if (self->was_recording) { + GST_INFO_OBJECT (self, "Recording stopped at %" GST_TIME_FORMAT, + GST_TIME_ARGS (running_time)); Don't we have to tell the audio when exactly recording was stopped so that it can also stop at that very same time (and clip the samples accordingly)? That is, unsetting audio_running_time_to_wait_for below seems wrong also @@ +796,3 @@ + inbuf = NULL; + } + self->audio_running_time_to_wait_for = GST_CLOCK_TIME_NONE; This especially means that below the audio would currently wait forever after recording stopped @@ +804,3 @@ + GST_TIME_ARGS (self->running_time_to_wait_for), inbuf); + } + } So we start/stop recording on the video side always immediately. This means that the audio side must be throttled by the video, and must always be behind the video @@ +885,3 @@ current_running_time, vsign, video_running_time) == 1 /* Wait if we don't even know what to wait for yet */ + || self->audio_running_time_to_wait_for == GST_CLOCK_TIME_NONE)) { here you would wait forever when recording is stopped instead of advancing the audio at the speed of the video @@ +914,3 @@ } } + if (self->audio_running_time_to_wait_for == GST_CLOCK_TIME_NONE How would we ever get here considering the waiting above?
Created attachment 373109 [details] [review] avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through.
Thanks, fixed it a bit. Remaining problem: Sometimes it fails to preroll - I think that the video chain function is called exactly once for one buffer and then never again..?! Missing things: * Updating the element message (recording might be FALSE) * Documentation * Code comments * Unit tests * Make sure audio starts/stops exactly where the audio does, and not approximately
Created attachment 373121 [details] [review] avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through.
(In reply to Vivia Nikolaidou from comment #8) > Thanks, fixed it a bit. > > Remaining problem: Sometimes it fails to preroll - I think that the video > chain function is called exactly once for one buffer and then never again..?! Turned out to be a bug in the test application. :) > Missing things: > * Updating the element message (recording might be FALSE) > * Documentation > * Code comments Done > * Unit tests Still TODO, but please feedback in the meantime! > * Make sure audio starts/stops exactly where the audio does, and not > approximately (That will be checked together with the unit tests)
Review of attachment 373121 [details] [review]: ::: gst/timecode/gstavwait.c @@ +36,3 @@ + * everything else. If recording is FALSE, all buffers are dropped regardless + * of settings. If recording is TRUE, the other settings (mode, + * target-timecode, target-running-time, etc) are taken into account. Mention what effect this has on the audio here @@ +514,3 @@ self->running_time_to_wait_for = self->target_running_time; + if (self->recording) { + self->audio_running_time_to_wait_for = self->running_time_to_wait_for; Isn't there anything else needed here for making sure that audio starts after the video? What if the video is already at 12s, the audio at 10s and the time that is set here is 11s? Then there will be 1s of audio before the video starts. Maybe these should always only ever be set from the video chain function? @@ +541,3 @@ + if (self->recording) { + self->audio_running_time_to_wait_for = + self->running_time_to_wait_for; Same here @@ +589,3 @@ self->running_time_to_wait_for = GST_CLOCK_TIME_NONE; self->running_time_to_end_at = GST_CLOCK_TIME_NONE; + self->audio_running_time_to_wait_for = GST_CLOCK_TIME_NONE; These 3 are set from the properties and but reset on flushes. This seems suspicious and error-prone (you need multiple places to update them and one could easily be forgotten). Would it make more sense to not set them from properties and only ever update them based on the property values from the chain functions (and reset them here, etc)? @@ +759,3 @@ self->running_time_to_end_at = gst_segment_to_running_time (&self->vsegment, GST_FORMAT_TIME, self->vsegment.position); It seems like this value is never used anywhere, only the one for audio. And the same for the to_wait_for (which is also handled in the property setter, but probably should only be done in the chain functions?) @@ +828,3 @@ + } else if (running_time < self->running_time_to_wait_for + && self->running_time_to_wait_for != GST_CLOCK_TIME_NONE) { + /* Don't bother waiting */ Shouldn't audio wait then especially to make sure it stays behind the video, so that once we start recording the audio can be made sure to start with the video? @@ +829,3 @@ + && self->running_time_to_wait_for != GST_CLOCK_TIME_NONE) { + /* Don't bother waiting */ + self->audio_running_time_to_wait_for = GST_CLOCK_TIME_NONE - 1; GST_CLOCK_TIME_NONE - 1 is a typo? Don't do calculations with GST_CLOCK_TIME_NONE
Created attachment 373125 [details] [review] avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through.
Thanks for the review! (In reply to Sebastian Dröge (slomo) from comment #11) > Review of attachment 373121 [details] [review] [review]: > > ::: gst/timecode/gstavwait.c > @@ +36,3 @@ > + * everything else. If recording is FALSE, all buffers are dropped > regardless > + * of settings. If recording is TRUE, the other settings (mode, > + * target-timecode, target-running-time, etc) are taken into account. > > Mention what effect this has on the audio here Done. > > @@ +514,3 @@ > self->running_time_to_wait_for = self->target_running_time; > + if (self->recording) { > + self->audio_running_time_to_wait_for = > self->running_time_to_wait_for; > > Isn't there anything else needed here for making sure that audio starts > after the video? What if the video is already at 12s, the audio at 10s and > the time that is set here is 11s? Then there will be 1s of audio before the > video starts. Good point. Made target-running-time, target-tc, and mode only changeable in READY state, to avoid such weird behaviour. end-tc doesn't matter because the actual value is only set from the video chain function anyway. > Maybe these should always only ever be set from the video chain function? They are edge-triggered currently, so they are set from set-property for mode=running-time (in READY so it won't collide with the video chain function), and from the video chain function when a change has been detected (e.g. target-timecode has been reached, or recording was changed). > @@ +541,3 @@ > + if (self->recording) { > + self->audio_running_time_to_wait_for = > + self->running_time_to_wait_for; > > Same here See point above. > @@ +589,3 @@ > self->running_time_to_wait_for = GST_CLOCK_TIME_NONE; > self->running_time_to_end_at = GST_CLOCK_TIME_NONE; > + self->audio_running_time_to_wait_for = GST_CLOCK_TIME_NONE; > > These 3 are set from the properties and but reset on flushes. This seems > suspicious and error-prone (you need multiple places to update them and one > could easily be forgotten). > > Would it make more sense to not set them from properties and only ever > update them based on the property values from the chain functions (and reset > them here, etc)? See point above. :) > @@ +759,3 @@ > self->running_time_to_end_at = > gst_segment_to_running_time (&self->vsegment, > GST_FORMAT_TIME, > self->vsegment.position); > > It seems like this value is never used anywhere, only the one for audio. And > the same for the to_wait_for (which is also handled in the property setter, > but probably should only be done in the chain functions?) We need to remember the previous state. Imagine that we have running-time-to-wait-for set to 3 seconds, but we keep switching recording on and off, both before and after the 3 seconds. After recording is switched to on, we need to know whether the 3 seconds have been reached or not. > @@ +828,3 @@ > + } else if (running_time < self->running_time_to_wait_for > + && self->running_time_to_wait_for != GST_CLOCK_TIME_NONE) { > + /* Don't bother waiting */ > > Shouldn't audio wait then especially to make sure it stays behind the video, > so that once we start recording the audio can be made sure to start with the > video? Comment changed to clarify this point. > @@ +829,3 @@ > + && self->running_time_to_wait_for != GST_CLOCK_TIME_NONE) { > + /* Don't bother waiting */ > + self->audio_running_time_to_wait_for = GST_CLOCK_TIME_NONE - 1; > > GST_CLOCK_TIME_NONE - 1 is a typo? Don't do calculations with > GST_CLOCK_TIME_NONE Same as point above, they belong together.
Review of attachment 373125 [details] [review]: Seems fine except for a couple of similar memory problems when running the test in valgrind. E.g. in line 741 you unref the buffer, but from previously in line 731 you still have a reference to the timecode stored in its meta (which is invalid now) and you use the timecode (that is now freed) in line 758. It's an old problem but thanks to the test this shows up now. It seems to be the only problem reported for the test, just a couple of dozen times.
Created attachment 373136 [details] [review] avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through.
(In reply to Sebastian Dröge (slomo) from comment #14) > Review of attachment 373125 [details] [review] [review]: > > Seems fine except for a couple of similar memory problems when running the > test in valgrind. > > E.g. in line 741 you unref the buffer, but from previously in line 731 you > still have a reference to the timecode stored in its meta (which is invalid > now) and you use the timecode (that is now freed) in line 758. It's an old > problem but thanks to the test this shows up now. > > It seems to be the only problem reported for the test, just a couple of > dozen times. Thanks, fixed.
Created attachment 373137 [details] [review] avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through.
Review of attachment 373137 [details] [review]: commit 854baf4fdbff7a69aee82648f1e064a49c74a1ac (HEAD -> master, origin/master, origin/HEAD) Author: Vivia Nikolaidou <vivia@ahiru.eu> Date: Thu Jul 19 18:34:40 2018 +0300 avwait: Add recording property It works like a valve in front of the actual avwait. When recording == TRUE, other rules are then examined. When recording == FALSE, nothing is passing through. https://bugzilla.gnome.org/show_bug.cgi?id=796836