[Bug 774049] New features/functionality in the Aggregator and VideoAggregator

[GStreamer (GNOME Bugzilla)]

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

Olivier Crête <olivier.crete at ocrete.ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #339240|none                        |needs-work
             status|                            |

--- Comment #3 from Olivier Crête <olivier.crete at ocrete.ca> ---
Review of attachment 339240:
 --> (https://bugzilla.gnome.org/review?bug=774049&attachment=339240)

::: gst-libs/gst/video/gstvideoaggregator.c
@@ +1310,3 @@
+  int buffer_nr = 0;
+
+  //check the timestamp delay

No // comments, please do /* ... */

@@ +1312,3 @@
+  //check the timestamp delay
+  GstClockTime timeNow =
+      gst_util_get_timestamp () - GST_ELEMENT (vagg)->base_time;

you need to hold the object lock to get the base_time..
Also you can't compare gst_util_get_timestamp() with it. I'd just drop that
line entirely.

@@ +1329,3 @@
+    if (buf) {
+      buffer_timestamps[buffer_nr] = GST_BUFFER_TIMESTAMP (buf);
+      if (buffer_timestamps[buffer_nr] == -1) {

Please use GST_CLOCK_TIME_NONE instead of -1

@@ +1333,3 @@
+        GST_DEBUG_OBJECT (pad, "Need timestamped buffers!");
+        GST_OBJECT_UNLOCK (vagg);
+        return GST_FLOW_ERROR;

if you return GST_FLOW_ERROR, you also need ot post an error with
GST_ELEMENT_ERROR() so that application can know why.

@@ +2304,3 @@
+      g_param_spec_boolean ("check-timestamps", "Check Timestamps",
+          "Checks that the timestamps from all sink pads match. Ignores the
stream fps, will only output a new frame when they all match (won't duplicate
any buffers).",
+          FALSE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

My impression is that this really is specific to the subclass and not to the
application? So it should probably be a regular setter function instead of a
property.

Name is also not so clear, maybe something like "only-equal-timestamps" ?

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