[Bug 644768] [dcaparse] implement 14-to-16-bit conversion

[GStreamer (bugzilla.gnome.org)]

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

Arun Raghavan <arun> changed:

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

--- Comment #14 from Arun Raghavan <arun at accosted.net> 2011-09-19 07:34:00 UTC ---
Review of attachment 196701:
 --> (https://bugzilla.gnome.org/review?bug=644768&attachment=196701)

Comments inline, but a couple of other things. 1) Are the changes other than
the two new variables to dcaparse.h required? 2) The convention on commit
messages is to have a one-line quick description (usually <= 80 characters),
followed by an empty line, followed by a more detailed description, bug link,
etc.

::: gst/audioparsers/gstdcaparse.c
@@ +52,3 @@
 #include <gst/base/gstbitreader.h>

+// byte-order safe word reading and writing macros

We use C-style comments everywhere.

@@ +54,3 @@
+// byte-order safe word reading and writing macros
+#define GST_READ_UINT16__(ENDIANNESS, X)  ((ENDIANNESS) == (BIG_ENDIAN) ?
(GST_READ_UINT16_BE((X))) : (GST_READ_UINT16_LE((X))))
+#define GST_WRITE_UINT16__(ENDIANNESS, P, X)  {if ((ENDIANNESS) ==
(BIG_ENDIAN)) {GST_WRITE_UINT16_BE((P), (X));} else {GST_WRITE_UINT16_LE((P),
(X));}}

Presumably the trailing __ is to prevent potential future clashes with
GStreamer macros. Might as well just drop the GST_ and the trailing
underscores, IMO.

Also, convention is to use lower-case for macro arguments.

@@ +57,3 @@
+
+// expand 14 to 16 bits by copying bit 0xD to highest 2 bits and write
+#define DCA_WRITE_UINT14(ENDIANNESS, P, X)  {if ((X) & 0x2000) {(X) |=
0xC000;} GST_WRITE_UINT16__((ENDIANNESS), (P), (X))}

More spaces to conform with coding style, please.

@@ +186,3 @@
+  if (intersect_caps && !gst_caps_is_empty (intersect_caps)) {
+    GstStructure *s = gst_caps_get_structure (intersect_caps, 0);
+    gint depth, endianness;

Empty lines between declarations and between conditions would assist
readability (applies to all subsequent bits too).

@@ +466,3 @@
+      GST_DEBUG_OBJECT (dcaparse,
+          "stream endianness is %i, downstream negotiated %i... byteswapping",
+          *endianness, dcaparse->endianness_negotiated);

Do we really need to dump the debug output in this function for every frame
(it's already dumped at negotiation time). No additional information here.

@@ +473,3 @@
+      *endianness =
+          (*endianness == G_BIG_ENDIAN) ? G_LITTLE_ENDIAN : G_BIG_ENDIAN;
+

Couldn't this just be *endianness = dcparse->endianness_negotiated?

If the depth is unchanged, can't we byteswap in place? Would be more efficient.

@@ +477,3 @@
+      gst_buffer_unref (buf);
+      frame->buffer = new_buf;
+      buf = new_buf;

Is this assignment necessary (occurs again below as well)?

@@ +490,3 @@
+    gint bo = dcaparse->endianness_negotiated;
+    if (*depth == 14 && dcaparse->depth_negotiated == 16) {
+      new_size = (*size / 16) * 14;

This will round down, losing some bits. And if size < 16, you'll lose all the
data.

@@ +497,3 @@
+          "stream depth is 14 bit, downstream negotiated 16 bit...
upconverting (endianness %i->%i)",
+          *endianness, bo);
+      while (pos16 <= new_size - 14) {

The last 14 bytes will be lost here.

@@ +501,3 @@
+            ((GST_READ_UINT16__ (*endianness,
+                    data + pos14) << 2) | ((GST_READ_UINT16__ (*endianness,
+                        data + pos14 + 2) >> 12) & 0x0003));

Might be easier to read if you weren't spelling out "endianness" twice per
line. Or better yet, reading all the data you need into a fixed buffer and
using that.

@@ +554,3 @@
+        pos16 += 2;
+      }
+    } else if (*depth == 16 && dcaparse->depth_negotiated == 14) {

All the same comments from the previous block apply here as well.

@@ +645,3 @@
+  }
+
+  gst_dca_parse_convert_frame (frame, dcaparse, &depth, &endianness, &size);

Maybe this should this be called only if required (with a G_UNLIKELY on the
condition).

-- 
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.