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

[GStreamer (GNOME Bugzilla)]

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

--- Comment #16 from Reynaldo H. Verdejo Pinochet <reynaldo at opendot.cl> ---
Review of attachment 302093:
 --> (https://bugzilla.gnome.org/review?bug=748259&attachment=302093)

Just cosmetic/formatting remarks, nothing blocking but
worth considering

::: gst/alevel/gstalevel.c
@@ +331,3 @@
+  GstALevel *self = GST_ALEVEL (parent);
+  GST_LOG_OBJECT (pad, "Got %s event", GST_EVENT_TYPE_NAME (event));
+

Maybe drop the braces on the case statements bellow

@@ +375,3 @@
+  GstALevel *self = GST_ALEVEL (parent);
+  GST_LOG_OBJECT (pad, "Got %s event", GST_EVENT_TYPE_NAME (event));
+

ditto

@@ +434,3 @@
+      self->total_frames = 0;
+      if (self->CS)
+        g_free (self->CS);

I also prefer not to use braces on single statement
if blocks, maybe you can check the rest of the code
and make it consistent? There are multiple times in
which you do

@@ +536,3 @@
+  g_mutex_lock (&self->mutex);
+  self->vsegment.position = timestamp;
+  duration = GST_BUFFER_DURATION (inbuf);

Like bellow but happens several times

@@ +720,3 @@
+        available_bytes, bytes);
+
+    if (available_bytes >= bytes) {

proly if available_bytes < bytes with a goto done to #746

::: tests/check/elements/alevel.c
@@ +214,3 @@
+    } else if (i == 4 && audio_nondiscont) {
+      timestamp += 30 * GST_MSECOND;
+    }

See previous comment on {}

@@ +259,3 @@
+      } else if (video_overlaps) {
+        timestamp -= 10 * GST_MSECOND;
+      }

ditto

@@ +272,3 @@
+on_message (GstBus * bus, GstMessage * message, gpointer user_data)
+{
+  if (message->type == GST_MESSAGE_ELEMENT) {

if message->type != GST_MESSAGE_ELEMENT with an immediate
return GST_BUS_PASS or alternatively a goto to #312

@@ +282,3 @@
+    guint i;
+
+    if (strcmp (name, "alevel") == 0) {

Same as above with != 0

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