[Bug 796836] avwait: Add recording property

[GStreamer (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #373121|none                        |reviewed
             status|                            |

--- Comment #11 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 373121:
 --> (https://bugzilla.gnome.org/review?bug=796836&attachment=373121)

::: 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

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