dcparse moved to -good

Created attachment 190519 [details] [review]
experimentally add 14-to-16-bit and endianess conversion (without doing any actual caps negotiation)

this algo should technically be able to do the conversion between 14 and 16 bit streams. however, i'n not exactly sure who the payload really is memorywise organized in the GstBuffer. and one point in gst_dca_parse_parse_header the frame size is also simply re-calculated if it's 14 bit...
however, the payload from a 14 bit LE .wav container that comes out in the end still differs from what it should look like if it had been packed in a .mka as 16 bit BE (mkv)

Review of attachment 190519 [details] [review]:

Run gst-indent on your changes before committing and use "git format-patch" style patches.

For the negotiation you can take a look at h264parse, it negotiates the format and alignment with downstream.

::: gst/audioparsers/gstdcaparse.c
@@ +422,3 @@
+    tmp = data[pos];
+    data[pos] = data[pos+1];
+    data[pos+1] = tmp;

Just do GST_WRITE_UINT16_LE(data, GST_READ_UINT16_BE (data)) here. Also, do it at the same time when converting between 14<->16 bits

Any progress on this? Also you should add "Converter" to the classification of the element class to have it used properly by decodebin2 now.

Created attachment 195860 [details]
*very draftish not working version of the patch

Created attachment 195861 [details]
limited caps dot graph

Created attachment 195869 [details]
debug log

GST_DEBUG=playbin2:5,decodebin2:5,*parse*:5,*dvb*:5 gst-launch -v playbin2 uri="file:///media
/hdd/movie/wav/Nightwish - Dark Passion Play - DTS/(03) [Nightwish] Amaranth.wav" >/testProgs/644768_caps.log 2>&1

Created attachment 195870 [details]
new dot graph

Created attachment 195921 [details] [review]
add endianness and depth downstream negotiation and implement byteswapping or 14-to-16-bit upconversion if needed

Refs bug #644768 (also requires core a121713f0c347caa483c213d7055a9c9c5aa46b8 and base 0e54d2c3438863ec871cc83d815e87fd9826fef7)

Review of attachment 195921 [details] [review]:

::: gst/audioparsers/gstdcaparse.c
@@ +172,3 @@
+  tcaps = gst_static_pad_template_get_caps (&src_template);
+  intersect_caps =
+      gst_caps_intersect_full (peercaps, tcaps, GST_CAPS_INTERSECT_FIRST);

This will fail if peercaps are NULL

@@ +174,3 @@
+      gst_caps_intersect_full (peercaps, tcaps, GST_CAPS_INTERSECT_FIRST);
+
+  if (intersect_caps && gst_caps_get_size (intersect_caps) > 0) {

Use && !gst_caps_is_empty() instead of the size check

@@ +479,3 @@
+      for (pos = 0; pos < size - 2; pos += 2) {
+        tmp = GST_READ_UINT16_BE (data + pos);
+        GST_WRITE_UINT16_LE (data + pos, tmp);

The loops could be the same for both conversions, they are doing exactly the same

@@ +495,3 @@
+          ((GST_READ_UINT16_BE (data +
+                      pos14) << 2) | ((GST_READ_UINT16_BE (data + pos14 +
+                          2) >> 12) & 0x0003)));

This will not work. You have to put the READ result into a temporary variable first

Created attachment 196047 [details] [review]
    [dcaparse] add endianness and depth downstream negotiation and implement byteswapping or 14-to-16-bit upconversion if needed

this incorporates slomo's annotations (thx for that btw.)

Review of attachment 196047 [details] [review]:

::: gst/audioparsers/gstdcaparse.c
@@ +171,3 @@
+  peercaps = gst_pad_peer_get_caps (GST_BASE_PARSE_SRC_PAD (dcaparse));
+  if (peercaps == NULL)
+    return;

Don't you need to set the _negotiated fields of the instance to something sensible here? Otherwise negotiate will be called for every single buffer

@@ +481,3 @@
+    } else {
+      endianness = G_LITTLE_ENDIAN;
+    }

Maybe use a ternary operator here ;)

@@ +484,3 @@
+  }
+
+  if (depth == 14 && dcaparse->depth_negotiated == 16) {

Don't you also need downwards converting code from 16 to 14 bits here? And is this code working for both endiannesses?

Created attachment 196701 [details] [review]
[dcaparse] add endianness and depth downstream negotiation and implement byteswapping and conversion between 14 and 16 bit streams

i'll give it another shot. there's up and downconversion now (so in case anybody decides to give wavenc dts input caps it'll be usable right away) and everything is endianness-proof back and forth
conversion is also moved from parse_frame into a convert_frame function

Review of attachment 196701 [details] [review]:

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

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/41.