[Bug 748259] [PATCH] New audio/video level element

[GStreamer (GNOME Bugzilla)]

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

Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> changed:

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

--- Comment #23 from Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> ---
Review of attachment 306566:
 --> (https://bugzilla.gnome.org/review?bug=748259&attachment=306566)

::: gst/videoframe_audiolevel/gstvideoframe-audiolevel.c
@@ +43,3 @@
+/* FIXME 0.11: suppress warnings for deprecated API such as GValueArray
+ * with newer GLib versions (>= 2.31.0) */
+#define GLIB_DISABLE_DEPRECATION_WARNINGS

Can we use a GPtrArray instead. If this works well, we can also add this code
to GstLevel when we do gstreamer-2.x. The FIXME should also target 2.X and not
0.11 :)

@@ +236,3 @@
+      gst_adapter_clear (self->adapter);
+      g_queue_foreach (&self->vtimeq, (GFunc) g_free, NULL);
+      g_queue_clear (&self->vtimeq);

g_queue_free_full (&self->vtimeq, (GFunc) g_free);

also below. Since elements are fixed size and potentially allocated and freed a
lot, you could consider using g_slice here.

@@ +680,3 @@
+      guint num_frames =
+          gst_util_uint64_scale (*vt0 - cur_time, rate, GST_SECOND);
+      bytes = num_frames * GST_AUDIO_INFO_BPF (&self->ainfo);

you could move this down where it is used (after the next condition)

@@ +689,3 @@
+        cur_time = *vt0;
+      } else {
+        GST_DEBUG_OBJECT (self, "We flush %ld out of %ld bytes", bytes,

nit: I'd prefer just the facts :) "flushed %ld out ..." that is no 'We'.

@@ +694,3 @@
+        self->total_frames += num_frames;
+        if (available_bytes <= bytes) {
+          num_frames = available_bytes / GST_AUDIO_INFO_BPF (&self->ainfo);

why is this updated here?

@@ +711,3 @@
+    }
+    available_bytes = gst_adapter_available (self->adapter);
+    GST_DEBUG_OBJECT (self, "We have %ld out of %ld bytes",

nit: like above, maybe "Adapter contains %ld out ..."

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