isomp4: Handle mp4s subpicture streams better.

Clean up the handling of mp4s streams. Use the generic esds
descriptor function to extract the palette, instead of hard coding
a wrong magic offset.

Add some more size safety checks when parsing ES descriptors, and
replace magic numbers with the descriptive constants that are already
defined.

Enhance dump output for stsd atoms.

Streams from both bug 712643 and historic bug 568278 now both work
correctly.

Fixes: #712643

gst/isomp4/qtdemux.c

@@ -58,6 +58,7 @@
 #include "qtdemux_types.h"
 #include "qtdemux_dump.h"
 #include "fourcc.h"
+#include "descriptors.h"
 #include "qtdemux_lang.h"
 #include "qtdemux.h"
 #include "qtpalette.h"
@@ -5372,6 +5373,7 @@ qtdemux_parse_node (GstQTDemux * qtdemux, GNode * node, const guint8 * buffer,
         }
         GST_DEBUG_OBJECT (qtdemux,
             "parsing stsd (sample table, sample description) atom");
+        /* Skip over 8 byte atom hdr + 1 byte version, 3 bytes flags, 4 byte num_entries */
         qtdemux_parse_container (qtdemux, node, buffer + 16, end);
         break;
       }
@@ -5519,6 +5521,13 @@ qtdemux_parse_node (GstQTDemux * qtdemux, GNode * node, const guint8 * buffer,
         qtdemux_parse_container (qtdemux, node, buffer + 12, end);
         break;
       }
+      case FOURCC_mp4s:
+      {
+        GST_MEMDUMP_OBJECT (qtdemux, "mp4s", buffer, end - buffer);
+        /* Skip 8 byte header, plus 8 byte version + flags + entry_count */
+        qtdemux_parse_container (qtdemux, node, buffer + 16, end);
+        break;
+      }
       case FOURCC_XiTh:
       {
         guint32 version;
@@ -8360,46 +8369,20 @@ qtdemux_parse_trak (GstQTDemux * qtdemux, GNode * trak)
     switch (fourcc) {
       case FOURCC_mp4s:
       {
-        guint len;
+        GNode *mp4s = NULL;
-        const guint8 *data;
+        GNode *esds = NULL;
 
-        /* look for palette */
+        /* look for palette in a stsd->mp4s->esds sub-atom */
-        /* target mp4s atom */
+        mp4s = qtdemux_tree_get_child_by_type (stsd, FOURCC_mp4s);
-        len = QT_UINT32 (stsd_data + offset);
+        if (mp4s)
-        data = stsd_data + offset;
+          esds = qtdemux_tree_get_child_by_type (mp4s, FOURCC_esds);
-        /* verify sufficient length,
+        if (esds == NULL) {
-         * and esds present with decConfigDescr of expected size and position */
+          /* Invalid STSD */
-        if ((len >= 106 + 8)
+          GST_LOG_OBJECT (qtdemux, "Skipping invalid stsd: no esds child");
-            && (QT_FOURCC (data + 8 + 8 + 4) == FOURCC_esds)
+          break;
-            && (QT_UINT16 (data + 8 + 40) == 0x0540)) {
-          GstStructure *s;
-          guint32 clut[16];
-          gint i;
-
-          /* move to decConfigDescr data */
-          data = data + 8 + 42;
-          for (i = 0; i < 16; i++) {
-            clut[i] = QT_UINT32 (data);
-            data += 4;
-          }
-
-          s = gst_structure_new ("application/x-gst-dvd", "event",
-              G_TYPE_STRING, "dvd-spu-clut-change",
-              "clut00", G_TYPE_INT, clut[0], "clut01", G_TYPE_INT, clut[1],
-              "clut02", G_TYPE_INT, clut[2], "clut03", G_TYPE_INT, clut[3],
-              "clut04", G_TYPE_INT, clut[4], "clut05", G_TYPE_INT, clut[5],
-              "clut06", G_TYPE_INT, clut[6], "clut07", G_TYPE_INT, clut[7],
-              "clut08", G_TYPE_INT, clut[8], "clut09", G_TYPE_INT, clut[9],
-              "clut10", G_TYPE_INT, clut[10], "clut11", G_TYPE_INT, clut[11],
-              "clut12", G_TYPE_INT, clut[12], "clut13", G_TYPE_INT, clut[13],
-              "clut14", G_TYPE_INT, clut[14], "clut15", G_TYPE_INT, clut[15],
-              NULL);
-
-          /* store event and trigger custom processing */
-          stream->pending_event =
-              gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM, s);
-          stream->need_process = TRUE;
         }
+
+        gst_qtdemux_handle_esds (qtdemux, stream, esds, list);
         break;
       }
       default:
@@ -8410,7 +8393,6 @@ qtdemux_parse_trak (GstQTDemux * qtdemux, GNode * trak)
     GST_INFO_OBJECT (qtdemux,
         "type %" GST_FOURCC_FORMAT " caps %" GST_PTR_FORMAT,
         GST_FOURCC_ARGS (fourcc), stream->caps);
-
   } else {
     /* everything in 1 sample */
     stream->sampled = TRUE;
@@ -10063,22 +10045,24 @@ qtdemux_parse_tree (GstQTDemux * qtdemux)
 }
 
 /* taken from ffmpeg */
-static unsigned int
+static int
-get_size (guint8 * ptr, guint8 ** end)
+read_descr_size (guint8 * ptr, guint8 * end, guint8 ** end_out)
 {
   int count = 4;
   int len = 0;
 
   while (count--) {
-    int c = *ptr;
+    int c;
 
-    ptr++;
+    if (ptr >= end)
+      return -1;
+
+    c = *ptr++;
     len = (len << 7) | (c & 0x7f);
     if (!(c & 0x80))
       break;
   }
-  if (end)
+  *end_out = ptr;
-    *end = ptr;
   return len;
 }
 
@@ -10101,20 +10085,24 @@ gst_qtdemux_handle_esds (GstQTDemux * qtdemux, QtDemuxStream * stream,
   ptr += 8;
   GST_DEBUG_OBJECT (qtdemux, "version/flags = %08x", QT_UINT32 (ptr));
   ptr += 4;
-  while (ptr < end) {
+  while (ptr + 1 < end) {
     tag = QT_UINT8 (ptr);
     GST_DEBUG_OBJECT (qtdemux, "tag = %02x", tag);
     ptr++;
-    len = get_size (ptr, &ptr);
+    len = read_descr_size (ptr, end, &ptr);
     GST_DEBUG_OBJECT (qtdemux, "len = %d", len);
 
+    /* Check the stated amount of data is available for reading */
+    if (len < 0 || ptr + len > end)
+      break;
+
     switch (tag) {
-      case 0x03:
+      case ES_DESCRIPTOR_TAG:
         GST_DEBUG_OBJECT (qtdemux, "ID %04x", QT_UINT16 (ptr));
         GST_DEBUG_OBJECT (qtdemux, "priority %04x", QT_UINT8 (ptr + 2));
         ptr += 3;
         break;
-      case 0x04:{
+      case DECODER_CONFIG_DESC_TAG:{
         guint max_bitrate, avg_bitrate;
 
         object_type_id = QT_UINT8 (ptr);
@@ -10136,18 +10124,56 @@ gst_qtdemux_handle_esds (GstQTDemux * qtdemux, QtDemuxStream * stream,
         ptr += 13;
         break;
       }
-      case 0x05:
+      case DECODER_SPECIFIC_INFO_TAG:
         GST_MEMDUMP_OBJECT (qtdemux, "data", ptr, len);
-        data_ptr = ptr;
+        if (object_type_id == 0xe0 && len == 0x40) {
-        data_len = len;
+          guint8 *data;
+          GstStructure *s;
+          guint32 clut[16];
+          gint i;
+
+          GST_DEBUG_OBJECT (qtdemux,
+              "Have VOBSUB palette. Creating palette event");
+          /* move to decConfigDescr data and read palette */
+          data = ptr;
+          for (i = 0; i < 16; i++) {
+            clut[i] = QT_UINT32 (data);
+            data += 4;
+          }
+
+          s = gst_structure_new ("application/x-gst-dvd", "event",
+              G_TYPE_STRING, "dvd-spu-clut-change",
+              "clut00", G_TYPE_INT, clut[0], "clut01", G_TYPE_INT, clut[1],
+              "clut02", G_TYPE_INT, clut[2], "clut03", G_TYPE_INT, clut[3],
+              "clut04", G_TYPE_INT, clut[4], "clut05", G_TYPE_INT, clut[5],
+              "clut06", G_TYPE_INT, clut[6], "clut07", G_TYPE_INT, clut[7],
+              "clut08", G_TYPE_INT, clut[8], "clut09", G_TYPE_INT, clut[9],
+              "clut10", G_TYPE_INT, clut[10], "clut11", G_TYPE_INT, clut[11],
+              "clut12", G_TYPE_INT, clut[12], "clut13", G_TYPE_INT, clut[13],
+              "clut14", G_TYPE_INT, clut[14], "clut15", G_TYPE_INT, clut[15],
+              NULL);
+
+          /* store event and trigger custom processing */
+          stream->pending_event =
+              gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM, s);
+          stream->need_process = TRUE;
+        } else {
+          /* Generic codec_data handler puts it on the caps */
+          data_ptr = ptr;
+          data_len = len;
+        }
+
         ptr += len;
         break;
-      case 0x06:
+      case SL_CONFIG_DESC_TAG:
         GST_DEBUG_OBJECT (qtdemux, "data %02x", QT_UINT8 (ptr));
         ptr += 1;
         break;
       default:
-        GST_ERROR_OBJECT (qtdemux, "parse error");
+        GST_DEBUG_OBJECT (qtdemux, "Unknown/unhandled descriptor tag %02x",
+            tag);
+        GST_MEMDUMP_OBJECT (qtdemux, "descriptor data", ptr, len);
+        ptr += len;
         break;
     }
   }

gst/isomp4/qtdemux_dump.c

@@ -254,6 +254,52 @@ qtdemux_dump_dref (GstQTDemux * qtdemux, GstByteReader * data, int depth)
   return TRUE;
 }
 
+static gboolean
+qtdemux_dump_stsd_avc1 (GstQTDemux * qtdemux, GstByteReader * data, guint size,
+    int depth)
+{
+  guint32 fourcc;
+
+  /* Size of avc1 = 78 bytes */
+  if (size < (6 + 2 + 4 + 4 + 4 + 4 + 2 + 2 + 4 + 4 + 4 + 2 + 1 + 31 + 2 + 2))
+    return FALSE;
+
+  gst_byte_reader_skip_unchecked (data, 6);
+  GST_LOG_OBJECT (qtdemux, "%*s    data reference:%d", depth, "",
+      GET_UINT16 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    version/rev.:  %08x", depth, "",
+      GET_UINT32 (data));
+  fourcc = GET_FOURCC (data);
+  GST_LOG_OBJECT (qtdemux, "%*s    vendor:        %" GST_FOURCC_FORMAT, depth,
+      "", GST_FOURCC_ARGS (fourcc));
+  GST_LOG_OBJECT (qtdemux, "%*s    temporal qual: %u", depth, "",
+      GET_UINT32 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    spatial qual:  %u", depth, "",
+      GET_UINT32 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    width:         %u", depth, "",
+      GET_UINT16 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    height:        %u", depth, "",
+      GET_UINT16 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    horiz. resol:  %g", depth, "",
+      GET_FP32 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    vert. resol.:  %g", depth, "",
+      GET_FP32 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    data size:     %u", depth, "",
+      GET_UINT32 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    frame count:   %u", depth, "",
+      GET_UINT16 (data));
+  /* something is not right with this, it's supposed to be a string but it's
+   * not apparently, so just skip this for now */
+  gst_byte_reader_skip_unchecked (data, 1 + 31);
+  GST_LOG_OBJECT (qtdemux, "%*s    compressor:    (skipped)", depth, "");
+  GST_LOG_OBJECT (qtdemux, "%*s    depth:         %u", depth, "",
+      GET_UINT16 (data));
+  GST_LOG_OBJECT (qtdemux, "%*s    color table ID:%u", depth, "",
+      GET_UINT16 (data));
+
+  return TRUE;
+}
+
 gboolean
 qtdemux_dump_stsd (GstQTDemux * qtdemux, GstByteReader * data, int depth)
 {
@@ -268,40 +314,46 @@ qtdemux_dump_stsd (GstQTDemux * qtdemux, GstByteReader * data, int depth)
 
   for (i = 0; i < num_entries; i++) {
     GstByteReader sub;
-    guint32 size = 0, fourcc;
+    guint32 size, remain;
+    guint32 fourcc;
 
     if (!gst_byte_reader_get_uint32_be (data, &size) ||
         !qt_atom_parser_get_fourcc (data, &fourcc))
       return FALSE;
 
-    GST_LOG ("%*s    size:          %u", depth, "", size);
+    GST_LOG_OBJECT (qtdemux, "%*s    size:          %u", depth, "", size);
-    GST_LOG ("%*s    type:          %" GST_FOURCC_FORMAT, depth, "",
+    GST_LOG_OBJECT (qtdemux, "%*s    type:          %" GST_FOURCC_FORMAT, depth,
-        GST_FOURCC_ARGS (fourcc));
+        "", GST_FOURCC_ARGS (fourcc));
 
-    if (size < (6 + 2 + 4 + 4 + 4 + 4 + 2 + 2 + 4 + 4 + 4 + 2 + 1 + 31 + 2 + 2))
+    remain = gst_byte_reader_get_remaining (data);
+    /* Size includes the 8 bytes we just read: len & fourcc, then 8 bytes
+     * version, flags, entries_count */
+    if (size > remain + 8) {
+      GST_LOG_OBJECT (qtdemux,
+          "Not enough data left for this atom (have %u need %u)", remain, size);
       return FALSE;
+    }
+
+    qt_atom_parser_peek_sub (data, 0, size, &sub);
+    switch (fourcc) {
+      case FOURCC_avc1:
+        if (!qtdemux_dump_stsd_avc1 (qtdemux, &sub, size, depth + 1))
+          return FALSE;
+        break;
+      case FOURCC_mp4s:
+        if (!gst_byte_reader_get_uint32_be (&sub, &ver_flags) ||
+            !gst_byte_reader_get_uint32_be (&sub, &num_entries))
+          return FALSE;
+        if (!qtdemux_dump_unknown (qtdemux, &sub, depth + 1))
+          return FALSE;
+        break;
+      default:
+        /* Unknown stsd data, dump the bytes */
+        if (!qtdemux_dump_unknown (qtdemux, &sub, depth + 1))
+          return FALSE;
+        break;
+    }
 
-    qt_atom_parser_peek_sub (data, 0, 78, &sub);
-    gst_byte_reader_skip_unchecked (&sub, 6);
-    GST_LOG ("%*s    data reference:%d", depth, "", GET_UINT16 (&sub));
-    GST_LOG ("%*s    version/rev.:  %08x", depth, "", GET_UINT32 (&sub));
-    fourcc = GET_FOURCC (&sub);
-    GST_LOG ("%*s    vendor:        %" GST_FOURCC_FORMAT, depth, "",
-        GST_FOURCC_ARGS (fourcc));
-    GST_LOG ("%*s    temporal qual: %u", depth, "", GET_UINT32 (&sub));
-    GST_LOG ("%*s    spatial qual:  %u", depth, "", GET_UINT32 (&sub));
-    GST_LOG ("%*s    width:         %u", depth, "", GET_UINT16 (&sub));
-    GST_LOG ("%*s    height:        %u", depth, "", GET_UINT16 (&sub));
-    GST_LOG ("%*s    horiz. resol:  %g", depth, "", GET_FP32 (&sub));
-    GST_LOG ("%*s    vert. resol.:  %g", depth, "", GET_FP32 (&sub));
-    GST_LOG ("%*s    data size:     %u", depth, "", GET_UINT32 (&sub));
-    GST_LOG ("%*s    frame count:   %u", depth, "", GET_UINT16 (&sub));
-    /* something is not right with this, it's supposed to be a string but it's
-     * not apparently, so just skip this for now */
-    gst_byte_reader_skip_unchecked (&sub, 1 + 31);
-    GST_LOG ("%*s    compressor:    (skipped)", depth, "");
-    GST_LOG ("%*s    depth:         %u", depth, "", GET_UINT16 (&sub));
-    GST_LOG ("%*s    color table ID:%u", depth, "", GET_UINT16 (&sub));
     if (!gst_byte_reader_skip (data, size - (4 + 4)))
       return FALSE;
   }

gst/isomp4/qtdemux_types.c

@@ -174,6 +174,7 @@ static const QtNodeType qt_node_types[] = {
   {FOURCC_avcC, "AV codec configuration container", 0},
   {FOURCC_avc1, "AV codec configuration v1", 0},
   {FOURCC_avc3, "AV codec configuration v3", 0},
+  {FOURCC_mp4s, "VOBSUB codec configuration", 0},
   {FOURCC_hvc1, "HEVC codec configuration", 0},
   {FOURCC_hev1, "HEVC codec configuration", 0},
   {FOURCC_hvcC, "HEVC codec configuration container", 0},