The parser still fails in my tests on some files, but it would be nice to get already a code review.

I'm also unsure about the line:

   *skipsize = pushSize;
    // return gst_base_parse_finish_frame (parse, frame, pushSize);
    return gst_base_parse_push_frame (parse, frame);

which seems to be the reason of some failures in my test:

[gst-master] [peter@core2 tests]$ gst-launch-1.0 filesrc location=./files/s1.caf ! cafparse ! audioconvert dithering=0 ! wavenc ! filesink location=./files/s2.wav
Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...

** (gst-launch-1.0:14581): WARNING **: audioconvert0: size 65536 is not a multiple of unit size 12
0:00:00.036802853 14581      0x16f9590 ERROR          basetransform gstbasetransform.c:1669:default_prepare_output_buffer:<audioconvert0> unknown output size
ERROR: from element /GstPipeline:pipeline0/GstCafParse:cafparse0: GStreamer encountered a general stream error.

Created attachment 288549 [details] [review]
Patch to implement a CAF parser

Fixed a length issue.
My PCM test files are now parsed OK.

Review of attachment 288549 [details] [review]:

General stylistic comment, in GStreamer, we normally don't use CamelCase, but it's not a big deal. Also, this not really being a parser (but a demuxer), I'm not sure if GstBaseParse is really the right base class, but we have no demuxer base class, so if it works, I'd leave it as-is.

If you have one a typefind function for the typefind plugin would also be appreciated, so it can "just work".

::: gst/audioparsers/gstcafparse.c
@@ +54,3 @@
+    "channels = (int) [ 1, MAX ]," \
+    "layout = interleaved, " \
+    "channel-mask = (bitmask) 0"

Leave that channel mask out I think. 0 sounds wrong.

@@ +95,3 @@
+
+  gst_element_class_set_static_metadata (element_class, "CAF audio parser",
+      "Codec/Parser/Audio", "Parses a Core Audio (CAF) audio stream",

In GStreamer nomenclature, it's really a Demuxer, not a parser (so it wavparse).

@@ +109,3 @@
+{
+  gst_caf_parse_reset (caf);
+  GST_PAD_SET_ACCEPT_INTERSECT (GST_BASE_PARSE_SINK_PAD (caf));

This is not needed, the sink caps have no properties.

@@ +222,3 @@
+      "rate", G_TYPE_INT, rate, "channels", G_TYPE_INT, channelsPerFrame,
+      "layout", G_TYPE_STRING, "interleaved",
+      "channel-mask", GST_TYPE_BITMASK, G_GUINT64_CONSTANT (0), NULL);

To create the audio caps, its safer to do gst_audio_info_init(); gst_audio_info_set_format(); gst_audio_info_to_caps();

@@ +223,3 @@
+      "layout", G_TYPE_STRING, "interleaved",
+      "channel-mask", GST_TYPE_BITMASK, G_GUINT64_CONSTANT (0), NULL);
+  gst_pad_set_caps (GST_BASE_PARSE_SRC_PAD (GST_BASE_PARSE (caf)), caps);

This gst_pad_set_caps() is redundant. In GStreamer 1.x, it's just a compat macro to gst_pad_push_event(gst_event_new_caps()).

@@ +263,3 @@
+    case GST_MAKE_FOURCC ('a', 'l', 'a', 'c'):
+      GST_DEBUG_OBJECT (caf, "format '%" GST_FOURCC_FORMAT
+          "' not yet implemented", GST_FOURCC_ARGS (formatID));

If you return a GST_FLOW_ERROR, you must post an error message with GST_ELEMENT_ERROR()

@@ +267,3 @@
+    default:
+      GST_DEBUG_OBJECT (caf, "unknown format '%" GST_FOURCC_FORMAT "'",
+          GST_FOURCC_ARGS (formatID));

This one too.

Created attachment 288851 [details] [review]
Demuxer for CAF

Incorporated all comments from Olivier (BTW: many thanks for the review).

Implemented
- Reading of "free", "chan", "desc" and "data" chunk
- Handling of uncompressed PCM data
- Basic channel mask settings

Still missing
- Handling of compressed data
- More complete channel mask setting (but CAF is here more
  flexible than gstreamer)
- Reading of chunks with more metadata (time code etc.)

Any comments?

Created attachment 289335 [details] [review]
cafdemux: new element to demux Core Audio Format

Renamed to cafdemux, since it is a demuxer
Moved to own directory.

The code looks good but I'm still bothered by the fact that you inherited from baseparse. Was there any reason or benefit for doing this?

++ on having a typefind function to allow this element being autoplugged with playbin to work directly on CAF files.

Could you convert the CamelCase in functions and variable names to GStreamer style please, and also make the patch against gst-plugins-bad instead of -good? (-bad is where new elements land usually).

Does seeking work already? Is the format really syncable?

Typefind is already committed: https://bugzilla.gnome.org/show_bug.cgi?id=739840

On inherting from baseparse, well, at that time I thought it was a good idea and it seems to work.
But I don't claim to understand gstreamer internals, so I'm open for suggestions.

And I guess using BaseParse gives you seekability almost for free, and we're still lacking a BaseDemux...

Took a quick look at the CAF spec. It is full audio file inside another file structure. Also has the usual: HEADER | DATA | INDEX (data and index can be in any order, there is also some metadata entries but they are optional).

One advantage of inheriting from GstElement is being able to go and parse the index when it is at the end just like qtdemux does when moov if after mdat. One can also seek without the index using estimatives for the audio frame sizes when VBR is used.

Comment on attachment 289335 [details] [review]
cafdemux: new element to demux Core Audio Format

Hi

> [..]
>+static gboolean
>+checkCAFFileHeader (GstCafDemux * caf, GstMapInfo * map)
>+{
>+  guint32 fileType = GST_READ_UINT32_LE (map->data);
>+  guint16 version = GST_READ_UINT16_BE (map->data + 4);
>+  guint16 flags = GST_READ_UINT16_BE (map->data + 6);
>+
>+  if (fileType != GST_MAKE_FOURCC ('c', 'a', 'f', 'f')) {
>+    GST_ELEMENT_ERROR (caf, STREAM, TYPE_NOT_FOUND, (NULL),
>+        ("Invalid CAF header: %" GST_FOURCC_FORMAT,
>+            GST_FOURCC_ARGS (fileType)));
>+    return FALSE;
>+  }
>+  GST_DEBUG_OBJECT (caf, "CAF version %d flags 0x%x", (gint) version,
>+      (guint) flags);
>+
>+  return TRUE;
>+}

Might make sense to at least issue a warning if version != 1.0?
I understand this is what we are absolutely* sure this code supports.

> [..]
>+static gboolean
>+initLPCM (GstCafDemux * caf, gdouble sampleRate, guint32 formatFlags,
>+    guint32 bytesPerPacket, guint32 framesPerPacket, guint32 channelsPerFrame,
>+    guint32 bitsPerChannel)
>+{
>+  const gboolean isFloat = (formatFlags & 0x1) != 0;
>+  const gboolean isLittleEndian = (formatFlags & 0x2) != 0;
>+  const guint rate = (gint) sampleRate;
>+  GstAudioFormat format;
>+  GstCaps *caps;
>+
>+  if (sampleRate <= .0 || rate < 0) {
>+    GST_DEBUG_OBJECT (caf, "Sample rate must be > 0 (not %lf -> %d)",
>+        sampleRate, rate);
>+    return FALSE;
>+  }

Please see bellow

>+
>+  if (framesPerPacket != 1) {
>+    GST_DEBUG_OBJECT (caf, "framesPerPacket must be 1 (not %d)",
>+        framesPerPacket);
>+    return FALSE;
>+  }
>+

Ditto

>+  if (channelsPerFrame <= 0) {
>+    GST_DEBUG_OBJECT (caf, "channelsPerFrame must be > 1 (not %d)",
>+        channelsPerFrame);
>+    return FALSE;
>+  }
>+

Ditto

>+  if (isFloat) {
>+    switch (bitsPerChannel) {
>+      case 64:
>+        format = isLittleEndian ? GST_AUDIO_FORMAT_F64LE :
>+            GST_AUDIO_FORMAT_F64BE;
>+        break;
>+      case 32:
>+        format = isLittleEndian ? GST_AUDIO_FORMAT_F32LE :
>+            GST_AUDIO_FORMAT_F32BE;
>+        break;
>+      default:
>+        GST_DEBUG_OBJECT (caf,
>+            "Only float samples with 32 or 64 bits are supported (not %d)",
>+            bitsPerChannel);
>+        return FALSE;

Ditto

>+    }
>+  } else {
>+    switch (bitsPerChannel) {
>+      case 16:
>+        format = isLittleEndian ? GST_AUDIO_FORMAT_S16LE :
>+            GST_AUDIO_FORMAT_S16BE;
>+        break;
>+      case 24:
>+        format = isLittleEndian ? GST_AUDIO_FORMAT_S24LE :
>+            GST_AUDIO_FORMAT_S24BE;
>+        break;
>+      case 32:
>+        format = isLittleEndian ? GST_AUDIO_FORMAT_S32LE :
>+            GST_AUDIO_FORMAT_S32BE;
>+        break;
>+      default:
>+        GST_DEBUG_OBJECT (caf,
>+            "Only int samples with 16, 24 or 32 bits are supported (not %d)",
>+            bitsPerChannel);
>+        return FALSE;

Ditto

> [..]
>+  /* TODO check bytesPerPacket */
>+  caf->frameSize = bytesPerPacket;
>+
>+  if (caf->channelMask == -1) {
>+    GST_DEBUG_OBJECT (caf, "Setting default channel-mask");
>+    if (channelsPerFrame == 2) {
>+      caf->channelMask = (1 << GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT) |
>+          (1 << GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT);
>+    } else {
>+      caf->channelMask = 0;
>+    }
>+  }
>+
>+  gst_audio_info_set_format (&caf->audioInfo, format, rate, channelsPerFrame,
>+      NULL);
>+  if (caf->channelMask == 0
>+      || !gst_audio_channel_positions_from_mask (channelsPerFrame,
>+          caf->channelMask, caf->audioInfo.position)) {
>+    int k;
>+    for (k = 0; k < 64; ++k) {
>+      GST_AUDIO_INFO_POSITION (&caf->audioInfo, k) =
>+          GST_AUDIO_CHANNEL_POSITION_NONE;
>+    }
>+    GST_INFO_OBJECT (caf, "Setting channel-mask to 0 (from 0x%"
>+        G_GINT64_MODIFIER "x)", caf->channelMask);
>+  }
>+
>+  caps = gst_audio_info_to_caps (&caf->audioInfo);
>+  GST_INFO_OBJECT (caf, "caps changed to %" GST_PTR_FORMAT, caps);
>+  gst_pad_push_event (GST_BASE_PARSE_SRC_PAD (caf), gst_event_new_caps (caps));
>+
>+  gst_caps_unref (caps);
>+
>+  return TRUE;
>+}

Looking at the error out path seems like pretty all the above
GST_DEBUG_OBJECT() messages should rather be WARNINGs. Unless
they catch known unfinished details on this new element. In
which case you should go with GST_FIXME_OBJECT() instead.

>+
>+static void
>+handleChannelBitmap (GstCafDemux * caf, guint32 channelBitmap)
>+{
>+  /* up to POSITION_REAR_CENTER the positions are in CAF the same  as in gst */

Drop the extra space before "as"

>+  if (channelBitmap < (1 << (GST_AUDIO_CHANNEL_POSITION_REAR_CENTER + 1)))
>+    caf->channelMask = channelBitmap;
>+  else {
>+    GST_WARNING_OBJECT (caf, "Channel bitmask %x not yet implemented",
>+        channelBitmap);

GST_FIXME_OBJECT()

> [..]
>+static gboolean
>+handleDescChunk (GstCafDemux * caf, const guint8 * buf)
>+{
>+  gdouble sampleRate = GST_READ_DOUBLE_BE (buf);
>+  guint32 formatID = GST_READ_UINT32_LE (buf + 8);
>+  guint32 formatFlags = GST_READ_UINT32_BE (buf + 12);
>+  guint32 bytesPerPacket = GST_READ_UINT32_BE (buf + 16);
>+  guint32 framesPerPacket = GST_READ_UINT32_BE (buf + 20);
>+  guint32 channelsPerFrame = GST_READ_UINT32_BE (buf + 24);
>+  guint32 bitsPerChannel = GST_READ_UINT32_BE (buf + 28);
>+
>+  GST_DEBUG_OBJECT (caf, "'desc': rate %lf, flags 0x%x, bytesPerPacket %d, "
>+      "framesPerPacket %d, channels %d, bitsPerChannel %d", sampleRate,
>+      formatFlags, bytesPerPacket, framesPerPacket, channelsPerFrame,
>+      bitsPerChannel);
>+  switch (formatID) {
>+    case GST_MAKE_FOURCC ('l', 'p', 'c', 'm'):
>+      return initLPCM (caf, sampleRate, formatFlags, bytesPerPacket,
>+          framesPerPacket, channelsPerFrame, bitsPerChannel);
>+    case GST_MAKE_FOURCC ('i', 'm', 'a', '4'):
>+    case GST_MAKE_FOURCC ('a', 'a', 'c', ' '):
>+    case GST_MAKE_FOURCC ('M', 'A', 'C', '3'):
>+    case GST_MAKE_FOURCC ('M', 'A', 'C', '6'):
>+    case GST_MAKE_FOURCC ('u', 'l', 'a', 'w'):
>+    case GST_MAKE_FOURCC ('a', 'l', 'a', 'w'):
>+    case GST_MAKE_FOURCC ('.', 'm', 'p', '1'):
>+    case GST_MAKE_FOURCC ('.', 'm', 'p', '2'):
>+    case GST_MAKE_FOURCC ('.', 'm', 'p', '3'):
>+    case GST_MAKE_FOURCC ('a', 'l', 'a', 'c'):
>+      GST_ELEMENT_ERROR (caf, STREAM, NOT_IMPLEMENTED, (NULL),
>+          ("format '%" GST_FOURCC_FORMAT "' not yet implemented",
>+              GST_FOURCC_ARGS (formatID)));
>+      return FALSE;

GST_FIXME_OBJECT() too.

> [..]
>+static GstFlowReturn
>+gst_caf_demux_handle_frame (GstBaseParse * demux, GstBaseParseFrame * frame,
>+    gint * skipsize)
>+{
>+  GstMapInfo map;
>+  GstFlowReturn ret = GST_FLOW_ERROR;
>+  GstCafDemux *caf = GST_CAF_DEMUX (demux);
>+
>+  if (G_LIKELY (caf->state == GST_CAF_STATE_GOT_DATA)) {
>+    gsize bufSize = gst_buffer_get_size (frame->buffer);
>+    gsize pushSize = MIN (caf->dataBytesLeft,
>+        (bufSize / caf->frameSize) * caf->frameSize);
>+
>+    GST_DEBUG_OBJECT (caf, "pushing data, bytes left=%" G_GUINT64_FORMAT
>+        " avail=%" G_GUINT64_FORMAT " frameSize:%d size:%" G_GUINT64_FORMAT,
>+        caf->dataBytesLeft, bufSize, caf->frameSize, pushSize);
>+    if (caf->dataBytesLeft == 0) {
>+      GST_DEBUG_OBJECT (caf, "Data chunk done");
>+      return GST_FLOW_OK;
>+    }
>+    caf->dataBytesLeft -= pushSize;
>+    return gst_base_parse_finish_frame (demux, frame, pushSize);
>+  }
>+
>+  gst_buffer_map (frame->buffer, &map, GST_MAP_READ);
>+
>+  g_assert (map.size >= 12);
>+
>+  if (G_UNLIKELY (caf->state == GST_CAF_STATE_NONE)) {
>+    if (!checkCAFFileHeader (caf, &map))
>+      goto fail;
>+    caf->state = GST_CAF_STATE_INITIALIZED;
>+    *skipsize = 8;
>+  } else {
>+    guint32 chunkType = GST_READ_UINT32_LE (map.data);
>+    gint64 chunkSize = GST_READ_UINT64_BE (map.data + 4);
>+
>+    g_assert (caf->state == GST_CAF_STATE_INITIALIZED);
>+
>+    switch (chunkType) {
>+      case GST_MAKE_FOURCC ('f', 'r', 'e', 'e'):
>+        *skipsize = chunkSize + 12;
>+        GST_DEBUG_OBJECT (caf, "Skipping 'free' chunk of size %"
>+            G_GINT64_FORMAT, chunkSize);
>+        break;
>+      case GST_MAKE_FOURCC ('c', 'h', 'a', 'n'):
>+        if (!fullChunkMapped (demux, chunkSize, &map, skipsize))
>+          break;
>+        handleChanChunk (caf, map.data + 12);
>+        *skipsize = chunkSize + 12;
>+        break;
>+      case GST_MAKE_FOURCC ('d', 'e', 's', 'c'):
>+        if (!fullChunkMapped (demux, chunkSize, &map, skipsize))
>+          break;
>+        if (!handleDescChunk (caf, map.data + 12))
>+          goto fail;
>+        *skipsize = chunkSize + 12;
>+        break;
>+      case GST_MAKE_FOURCC ('d', 'a', 't', 'a'):
>+        if (map.size < 16) {
>+          /* try until we have enough data for header */
>+          gst_base_parse_set_min_frame_size (demux, 16);
>+          *skipsize = 0;
>+          break;
>+        }
>+        {
>+          const guint32 editCount = GST_READ_UINT32_BE (map.data + 12);
>+          GST_DEBUG_OBJECT (caf, "editCount=%d", editCount);
>+          *skipsize = 16;
>+          caf->state = GST_CAF_STATE_GOT_DATA;
>+          caf->dataBytesLeft = chunkSize;
>+        }
>+        break;
>+      case GST_MAKE_FOURCC ('k', 'u', 'k', 'i'):
>+      case GST_MAKE_FOURCC ('i', 'n', 's', 't'):
>+      case GST_MAKE_FOURCC ('u', 'm', 'i', 'd'):
>+      case GST_MAKE_FOURCC ('u', 'u', 'i', 'd'):
>+        *skipsize = chunkSize + 12;
>+        GST_DEBUG_OBJECT (caf, "Chunk '%" GST_FOURCC_FORMAT
>+            "' not yet impelemented, skipping size %" G_GINT64_FORMAT,
>+            GST_FOURCC_ARGS (chunkType), chunkSize);

As above. Should be a GST_FIXME_OBJECT() instead.

>+        break;
>+      default:
>+        *skipsize = chunkSize + 12;
>+        GST_DEBUG_OBJECT (caf, "Skipping unknown chunk '%" GST_FOURCC_FORMAT
>+            "' size %" G_GINT64_FORMAT, GST_FOURCC_ARGS (chunkType), chunkSize);

_WARNING_OBJECT()

Also, please add the bug number in the commit message.

Created attachment 293196 [details] [review]
Patch to implement a CAF parser

Implemented reviewers comments
- Moved to gst-plugins-bad
- GST_FIXME_OBJECT

Review of attachment 293196 [details] [review]:

FYI: other similar elements wavparse, aifparse also subclass from GstElement.

::: gst/cafdemux/gstcafdemux.c
@@ +24,3 @@
+/**
+ * SECTION:element-cafdemux
+ * @see_also:

remove the empty @see_also:

@@ +143,3 @@
+  guint32 fileType = GST_READ_UINT32_LE (map->data);
+  guint16 version = GST_READ_UINT16_BE (map->data + 4);
+  guint16 flags = GST_READ_UINT16_BE (map->data + 6);

this will probably emit a "assigned, but unused variable warning" if debug is disabled. The spec states
"""
For CAF v1 files, must be set to 0. You should ignore any value of this field you don’t understand.
"""
So I'd add a
if (flags != 0) {
  GST_WARNING_OBJECT (caf, "Unhandled flags 0x%x", (guint) flags);
}

@@ +163,3 @@
+  const gboolean isFloat = (formatFlags & 0x1) != 0;
+  const gboolean isLittleEndian = (formatFlags & 0x2) != 0;
+  const guint rate = (gint) sampleRate;

(guint) sampleRate

@@ +167,3 @@
+  GstCaps *caps;
+
+  if (sampleRate <= .0 || rate < 0) {

rate is guint and can't be < 0

@@ +179,3 @@
+  }
+
+  if (channelsPerFrame <= 0) {

channelsPerFrame is unsigned, only check for != 0

@@ +202,3 @@
+    }
+  } else {
+    switch (bitsPerChannel) {

shouldn't we support 8: also as it is simple?

Peter, Stefan : any hopes of getting this mergeable in current -bad ?

-- 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-bad/issues/182.