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

[GStreamer (GNOME Bugzilla)]

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

--- Comment #24 from Vivia Nikolaidou <vivia at ahiru.eu> ---
Sorry it took so long - replying now!

(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #23)
> Review of attachment 306566 [details] [review]:
> 
> ::: 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 :)

Made it FIXME 2.0 for now :)

> @@ +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);

Cannot use this because the vtimeq is allocated in self and not created using
g_queue_new.

> 
> @@ +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)

Actually it's needed here to calculate available_bytes, which is needed in the
"if" 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'.

OK, fixed.

> @@ +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?

Oops. Nice catch, thanks :)

> @@ +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 ..."

Fixed too, thanks :)

Attaching new patch...

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