[Bug 796836] avwait: Add recording property

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=796836

--- Comment #13 from Vivia Nikolaidou <vivia at ahiru.eu> ---
Thanks for the review!

(In reply to Sebastian Dröge (slomo) from comment #11)
> 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

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.