[gstreamer-bugs] [Bug 613589] typefind: Export AAC level in caps

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=613589
  GStreamer | gst-plugins-base | git

Tim-Philipp Müller <t.i.m> changed:

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

--- Comment #4 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-04-06 18:55:22 UTC ---
Review of attachment 156762:
 --> (https://bugzilla.gnome.org/review?bug=613589&attachment=156762)

Couple of nitpicks:

::: gst/typefind/Makefile.am
@@ -1,3 @@
 plugin_LTLIBRARIES = libgsttypefindfunctions.la

-libgsttypefindfunctions_la_SOURCES = gsttypefindfunctions.c

The gstaacutil.h header file will need to be added somewhere too, otherwise it
won't be disted.

::: gst/typefind/gstaacutil.c
@@ +108,3 @@
+      break;
+    default:
+      g_return_val_if_reached (-1);

g_return_val_if_reached() and friends are mostly meant as guards for bad input
to public API, and for programming mistakes.

In case of bad data input, we should just silently return -1 (but may do a
GST_WARNING or so if we want to).

If we want to guard against bad arguments from other GStreamer functions (e.g..
if they should have checked bad data already), then a g_assert_if_reached()
might be better.

Not sure which one is the case here.

@@ +114,3 @@
+    /* 0xd and 0xe are reserved. 0xf means the sample frequency is directly
+     * specified in the header (FIXME) */
+    g_return_val_if_reached (-1);

See above.

::: gst/typefind/gsttypefindfunctions.c
@@ +687,3 @@
       snc = GST_READ_UINT16_BE (c.data + len);
       if ((snc & 0xfff6) == 0xfff0) {
+        gint mpegversion, profile, level, sample_freq_idx, channel_config;

We should make the ones that are always unsigned a guint IMHO.

@@ +714,3 @@
+              "profile", G_TYPE_STRING, profile_to_string[profile],
+              "level", G_TYPE_STRING,
+              g_ascii_dtostr (buf, sizeof (buf), (gdouble) level),

Ew :-) I don't think we should be using g_ascii_dstrostr() for integers here,
even if the returns-buffer thing is rather convenient (also, if we use it, we
should probably use the provided define for the buffer size). I would suggest a
plain g_snprintf() before the _suggest_simple().

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.