id3demux: fix parsing of unsync'ed ID3 v2.4 tags and frames

Reversing the unsynchronisation seems to work slightly differently
for ID3 v2.3 tags and v2.4 tags: v2.3 tags don't have syncsafe frame
sizes in the frame header, so the unsynchronisation is applied to
the whole frame data including all the frame headers. v2.4 frames
have sync-safe sizes, however, so the unsynchronisation only needs
to be applied to the actual frame data, and it seems that's what's
being done as well. So we need to undo the unsynchronisation on a
per-frame basis for v2.4 tags for things to work properly.

Fixes extraction of coverart/images from APIC frames in ID3 v2.4
tags (#588148).

Add unit test for this as well.

6 files changed, 80 insertions, 12 deletions

diff --git a/gst/id3demux/id3tags.c b/gst/id3demux/id3tags.c
index 1d511f336..96d25521d 100644
--- a/gst/id3demux/id3tags.c
+++ b/gst/id3demux/id3tags.c
@@ -97,7 +97,7 @@ id3demux_calc_id3v2_tag_size (GstBuffer * buf)
   return size;
 }
 
-static guint8 *
+guint8 *
 id3demux_ununsync_data (const guint8 * unsync_data, guint32 * size)
 {
   const guint8 *end;
@@ -195,7 +195,10 @@ id3demux_read_id3v2_tag (GstBuffer * buffer, guint * id3v2_size,
   else
     work.hdr.frame_data_size = read_size - ID3V2_HDR_SIZE;
 
-  if ((flags & ID3V2_HDR_FLAG_UNSYNC)) {
+  /* in v2.3 the frame sizes are not syncsafe, so the entire tag had to be
+   * unsynced. In v2.4 the frame sizes are syncsafe so it's just the frame
+   * data that needs un-unsyncing, but not the frame headers. */
+  if ((flags & ID3V2_HDR_FLAG_UNSYNC) != 0 && ID3V2_VER_MAJOR (version) <= 3) {
     GST_DEBUG ("Un-unsyncing entire tag");
     uu_data = id3demux_ununsync_data (work.hdr.frame_data,
         &work.hdr.frame_data_size);
diff --git a/gst/id3demux/id3tags.h b/gst/id3demux/id3tags.h
index b5bc950dd..14a42de38 100644
--- a/gst/id3demux/id3tags.h
+++ b/gst/id3demux/id3tags.h
@@ -115,6 +115,8 @@ enum {
 /* From id3v2frames.c */
 gboolean id3demux_id3v2_parse_frame (ID3TagsWorking *work);
 
+guint8 * id3demux_ununsync_data (const guint8 * unsync_data, guint32 * size);
+
 G_END_DECLS
 
 #endif
diff --git a/gst/id3demux/id3v2frames.c b/gst/id3demux/id3v2frames.c
index 377d84cf5..a0331ef38 100644
--- a/gst/id3demux/id3v2frames.c
+++ b/gst/id3demux/id3v2frames.c
@@ -73,6 +73,7 @@ id3demux_id3v2_parse_frame (ID3TagsWorking * work)
   guint frame_data_size = work->cur_frame_size;
   gchar *tag_str = NULL;
   GArray *tag_fields = NULL;
+  guint8 *uu_data = NULL;
 
 #ifdef HAVE_ZLIB
   guint8 *uncompressed_data = NULL;
@@ -86,17 +87,14 @@ id3demux_id3v2_parse_frame (ID3TagsWorking * work)
     }
   }
 
-  /* Can't handle encrypted frames right now */
+  /* Can't handle encrypted frames right now (in case we ever do, we'll have
+   * to do the decryption after the un-unsynchronisation and decompression,
+   * not here) */
   if (work->frame_flags & ID3V2_FRAME_FORMAT_ENCRYPTION) {
     GST_WARNING ("Encrypted frames are not supported");
     return FALSE;
   }
 
-  if (work->frame_flags & ID3V2_FRAME_FORMAT_UNSYNCHRONISATION) {
-    GST_WARNING ("ID3v2 frame with unsupported unsynchronisation applied. "
-        "May fail badly");
-  }
-
   tag_name = gst_tag_from_id3_tag (work->frame_id);
   if (tag_name == NULL &&
       strncmp (work->frame_id, "RVA2", 4) != 0 &&
@@ -120,6 +118,19 @@ id3demux_id3v2_parse_frame (ID3TagsWorking * work)
     }
   }
 
+  /* in v2.3 the frame sizes are not syncsafe, so the entire tag had to be
+   * unsynced. In v2.4 the frame sizes are syncsafe so it's just the frame
+   * data that needs un-unsyncing, but not the frame headers. */
+  if (ID3V2_VER_MAJOR (work->hdr.version) == 4) {
+    if ((work->hdr.flags & ID3V2_HDR_FLAG_UNSYNC) != 0 ||
+        ((work->frame_flags & ID3V2_FRAME_FORMAT_UNSYNCHRONISATION) != 0)) {
+      GST_DEBUG ("Un-unsyncing frame %s", work->frame_id);
+      uu_data = id3demux_ununsync_data (frame_data, &frame_data_size);
+      frame_data = uu_data;
+      GST_MEMDUMP ("ID3v2 frame (un-unsyced)", frame_data, frame_data_size);
+    }
+  }
+
   work->parse_size = frame_data_size;
 
   if (work->frame_flags & ID3V2_FRAME_FORMAT_COMPRESSION) {
@@ -134,6 +145,7 @@ id3demux_id3v2_parse_frame (ID3TagsWorking * work)
 
     if (uncompress (dest, &destSize, src, frame_data_size) != Z_OK) {
       g_free (uncompressed_data);
+      g_free (uu_data);
       return FALSE;
     }
     if (destSize != work->parse_size) {
@@ -141,12 +153,14 @@ id3demux_id3v2_parse_frame (ID3TagsWorking * work)
           ("Decompressing ID3v2 frame %s did not produce expected size %d bytes (got %lu)",
           tag_name, work->parse_size, destSize);
       g_free (uncompressed_data);
+      g_free (uu_data);
       return FALSE;
     }
     work->parse_data = uncompressed_data;
 #else
     GST_WARNING ("Compressed ID3v2 tag frame could not be decompressed"
         " because gstid3demux was compiled without zlib support");
+    g_free (uu_data);
     return FALSE;
 #endif
   } else {
@@ -209,6 +223,8 @@ id3demux_id3v2_parse_frame (ID3TagsWorking * work)
     free_tag_strings (tag_fields);
   }
 
+  g_free (uu_data);
+
   return result;
 }
 
diff --git a/tests/check/elements/id3demux.c b/tests/check/elements/id3demux.c
index ef1d45381..724461bd1 100644
--- a/tests/check/elements/id3demux.c
+++ b/tests/check/elements/id3demux.c
@@ -198,7 +198,7 @@ GST_START_TEST (test_wcop)
 GST_END_TEST;
 
 static void
-check_unsync (const GstTagList * tags, const gchar * file)
+check_unsync_v23 (const GstTagList * tags, const gchar * file)
 {
   gchar *album = NULL;
   gchar *title = NULL;
@@ -220,9 +220,54 @@ check_unsync (const GstTagList * tags, const gchar * file)
   g_free (artist);
 }
 
-GST_START_TEST (test_unsync)
+GST_START_TEST (test_unsync_v23)
 {
-  run_check_for_file ("id3-577468-unsynced-tag.tag", check_unsync);
+  run_check_for_file ("id3-577468-unsynced-tag.tag", check_unsync_v23);
+}
+
+GST_END_TEST;
+
+static void
+check_unsync_v24 (const GstTagList * tags, const gchar * file)
+{
+  const GValue *val;
+  GstBuffer *buf;
+  gchar *album = NULL;
+  gchar *title = NULL;
+  gchar *artist = NULL;
+
+  fail_unless (gst_tag_list_get_string (tags, GST_TAG_TITLE, &title));
+  fail_unless (title != NULL);
+  fail_unless_equals_string (title, "Starlight");
+  g_free (title);
+
+  fail_unless (gst_tag_list_get_string (tags, GST_TAG_ALBUM, &album));
+  fail_unless (album != NULL);
+  fail_unless_equals_string (album, "L'albumRockVol.4 CD1");
+  g_free (album);
+
+  fail_unless (gst_tag_list_get_string (tags, GST_TAG_ARTIST, &artist));
+  fail_unless (artist != NULL);
+  fail_unless_equals_string (artist, "Muse");
+  g_free (artist);
+
+  val = gst_tag_list_get_value_index (tags, GST_TAG_IMAGE, 0);
+  fail_unless (val != NULL);
+  fail_unless (GST_VALUE_HOLDS_BUFFER (val));
+  buf = gst_value_get_buffer (val);
+  fail_unless (buf != NULL);
+  fail_unless (GST_BUFFER_CAPS (buf) != NULL);
+  fail_unless_equals_int (GST_BUFFER_SIZE (buf), 38022);
+  /* check for jpeg start/end markers */
+  fail_unless_equals_int (GST_BUFFER_DATA (buf)[0], 0xff);
+  fail_unless_equals_int (GST_BUFFER_DATA (buf)[1], 0xd8);
+  fail_unless_equals_int (GST_BUFFER_DATA (buf)[38020], 0xff);
+  fail_unless_equals_int (GST_BUFFER_DATA (buf)[38021], 0xd9);
+}
+
+GST_START_TEST (test_unsync_v24)
+{
+  run_check_for_file ("id3-588148-unsynced-v24.tag", check_unsync_v24);
 }
 
 GST_END_TEST;
@@ -236,7 +281,8 @@ id3demux_suite (void)
   suite_add_tcase (s, tc_chain);
   tcase_add_test (tc_chain, test_tdat_tyer);
   tcase_add_test (tc_chain, test_wcop);
-  tcase_add_test (tc_chain, test_unsync);
+  tcase_add_test (tc_chain, test_unsync_v23);
+  tcase_add_test (tc_chain, test_unsync_v24);
 
   return s;
 }
diff --git a/tests/files/Makefile.am b/tests/files/Makefile.am
index 73ab4f2d5..a0a282937 100644
--- a/tests/files/Makefile.am
+++ b/tests/files/Makefile.am
@@ -5,6 +5,7 @@ EXTRA_DIST = \
 	id3-407349-2.tag \
 	id3-447000-wcop.tag \
 	id3-577468-unsynced-tag.tag \
+	id3-588148-unsynced-v24.tag \
 	pcm16sine.flv \
 	test-cert.pem \
 	test-key.pem
diff --git a/tests/files/id3-588148-unsynced-v24.tag b/tests/files/id3-588148-unsynced-v24.tag
new file mode 100644
index 000000000..099d930e7
--- /dev/null
+++ b/tests/files/id3-588148-unsynced-v24.tag