flacparse: fix broken reordering of flac metadata

Each FLAC metadata block starts with a flag denoting whether it is the
last metadata block. The existing flacparse code moves any existing
VORBISCOMMENT block to immediately follow the STREAMINFO block without
changing any block's last-metadata-block flag. If no VORBISCOMMENT block
exists, it created one with the last-metadata-block flag set to true.
This results in gstflacdec sometimes giving bad headers to libflac when
trying to play perfectly valid FLAC files depending on the file's
metadata ordering. Depending on the contents of the other metadata
blocks, current versions of libflac may or may not return
FLAC__STREAM_DECODER_ERROR_STATUS_BAD_HEADER when given this broken
metadata. This is most noticeable with files that have a large cover art
image attached where VORBISCOMMENT is the very last metadata block with
no PADDING afterwards.

This patch changes that behavior so that:

1. For FLAC files that already have a VORBISCOMMENT block, the metadata
   order is preserved.
2. For FLAC files that do not have a VORBISCOMMENT block, the generated
   dummy VORBISCOMMENT is placed immediately after STREAMINFO and
   inherits the last-metadata-block flag from STREAMINFO.

https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/484

1 files changed, 16 insertions, 12 deletions

diff --git a/gst/audioparsers/gstflacparse.c b/gst/audioparsers/gstflacparse.c
index 2758d4cfc..77999509e 100644
--- a/gst/audioparsers/gstflacparse.c
+++ b/gst/audioparsers/gstflacparse.c
@@ -184,7 +184,7 @@ static GstStaticPadTemplate sink_factory = GST_STATIC_PAD_TEMPLATE ("sink",
     );
 
 static GstBuffer *gst_flac_parse_generate_vorbiscomment (GstFlacParse *
-    flacparse);
+    flacparse, gboolean is_last);
 
 static inline void gst_flac_parse_reset_buffer_time_and_offset (GstBuffer *
     buffer);
@@ -1244,6 +1244,7 @@ gst_flac_parse_handle_headers (GstFlacParse * flacparse)
   GstCaps *caps;
   GList *l;
   GstFlowReturn res = GST_FLOW_OK;
+  gboolean is_streaminfo_last = FALSE;
 
   caps = gst_caps_new_simple ("audio/x-flac",
       "channels", G_TYPE_INT, flacparse->channels,
@@ -1265,6 +1266,7 @@ gst_flac_parse_handle_headers (GstFlacParse * flacparse)
       marker = header;
     } else if (map.size > 1 && (map.data[0] & 0x7f) == 0) {
       streaminfo = header;
+      is_streaminfo_last = (map.data[0] & 0x80) != 0;
     } else if (map.size > 1 && (map.data[0] & 0x7f) == 4) {
       vorbiscomment = header;
     }
@@ -1277,8 +1279,11 @@ gst_flac_parse_handle_headers (GstFlacParse * flacparse)
   if (vorbiscomment == NULL && streaminfo != NULL) {
     GST_DEBUG_OBJECT (flacparse,
         "missing vorbiscomment header; generating dummy");
-    vorbiscomment = gst_flac_parse_generate_vorbiscomment (flacparse);
-    flacparse->headers = g_list_insert (flacparse->headers, vorbiscomment,
+    /* this vorbiscomment header is inserted after streaminfo and inherits its last-metadata-block flag */
+    vorbiscomment =
+        gst_flac_parse_generate_vorbiscomment (flacparse, is_streaminfo_last);
+    flacparse->headers =
+        g_list_insert (flacparse->headers, vorbiscomment,
         g_list_index (flacparse->headers, streaminfo) + 1);
   }
 
@@ -1313,6 +1318,8 @@ gst_flac_parse_handle_headers (GstFlacParse * flacparse)
     writemap.data[8] = (num & 0x00FF) >> 0;
     memcpy (writemap.data + 9, "fLaC", 4);
     memcpy (writemap.data + 13, sinfomap.data, sinfomap.size);
+    /* clear the last-metadata-block flag because a VORBISCOMMENT always follows */
+    writemap.data[13] = 0x00;   /* is_last = 0; type = 0; */
     _value_array_append_buffer (&array, buf);
 
     gst_buffer_unmap (streaminfo, &sinfomap);
@@ -1320,14 +1327,10 @@ gst_flac_parse_handle_headers (GstFlacParse * flacparse)
     gst_buffer_unref (buf);
   }
 
-  /* add VORBISCOMMENT header */
-  _value_array_append_buffer (&array, vorbiscomment);
-
-  /* add other headers, if there are any */
+  /* add other headers, including VORBISCOMMENT */
   for (l = flacparse->headers; l; l = l->next) {
     if (GST_BUFFER_CAST (l->data) != marker &&
-        GST_BUFFER_CAST (l->data) != streaminfo &&
-        GST_BUFFER_CAST (l->data) != vorbiscomment) {
+        GST_BUFFER_CAST (l->data) != streaminfo) {
       _value_array_append_buffer (&array, GST_BUFFER_CAST (l->data));
     }
   }
@@ -1369,7 +1372,8 @@ push_headers:
 
 /* empty vorbiscomment */
 static GstBuffer *
-gst_flac_parse_generate_vorbiscomment (GstFlacParse * flacparse)
+gst_flac_parse_generate_vorbiscomment (GstFlacParse * flacparse,
+    gboolean is_last)
 {
   GstTagList *taglist = gst_tag_list_new_empty ();
   guchar header[4];
@@ -1377,7 +1381,7 @@ gst_flac_parse_generate_vorbiscomment (GstFlacParse * flacparse)
   GstBuffer *vorbiscomment;
   GstMapInfo map;
 
-  header[0] = 0x84;             /* is_last = 1; type = 4; */
+  header[0] = (is_last ? 0x80 : 0x00) | 0x04;   /* is_last may vary; type = 4; */
 
   vorbiscomment =
       gst_tag_list_to_vorbiscomment_buffer (taglist, header,
@@ -1476,7 +1480,7 @@ gst_flac_parse_generate_headers (GstFlacParse * flacparse)
   flacparse->headers = g_list_append (flacparse->headers, streaminfo);
 
   flacparse->headers = g_list_append (flacparse->headers,
-      gst_flac_parse_generate_vorbiscomment (flacparse));
+      gst_flac_parse_generate_vorbiscomment (flacparse, TRUE));
 
   return TRUE;
 }