[FFmpeg-devel] [mp3] Skip APE tags when parsing mp3 packets.

[James Almer]

On 1/30/2018 2:45 AM, wm4 wrote:
> On Tue, 30 Jan 2018 02:24:29 +0100
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
>> On Mon, Jan 29, 2018 at 03:13:54PM -0800, Dale Curtis wrote:
>>> Otherwise the decoder will throw "Missing header" errors when the
>>> packets are sent for decoding.  
>>
>>>  mpegaudio_parser.c |    7 +++++++
>>>  1 file changed, 7 insertions(+)
>>> 2628fa8480b15237a528e94b1689da7321ce9440  skip-ape-tags.patch
>>> From 05f5e837862a958fb8791668ab3cdf8fc7766fe5 Mon Sep 17 00:00:00 2001
>>> From: Dale Curtis <dalecurtis at chromium.org>
>>> Date: Mon, 29 Jan 2018 15:10:26 -0800
>>> Subject: [PATCH] [mp3] Skip APE tags when parsing mp3 packets.
>>>
>>> Otherwise the decoder will throw "Missing header" errors when the
>>> packets are sent for decoding.
>>> ---
>>>  libavcodec/mpegaudio_parser.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/libavcodec/mpegaudio_parser.c b/libavcodec/mpegaudio_parser.c
>>> index 8c39825792..244281b56f 100644
>>> --- a/libavcodec/mpegaudio_parser.c
>>> +++ b/libavcodec/mpegaudio_parser.c
>>> @@ -23,6 +23,7 @@
>>>  #include "parser.h"
>>>  #include "mpegaudiodecheader.h"
>>>  #include "libavutil/common.h"
>>> +#include "libavformat/apetag.h" // for APE tag.
>>>  #include "libavformat/id3v1.h" // for ID3v1_TAG_SIZE
>>>  
>>>  typedef struct MpegAudioParseContext {
>>> @@ -120,6 +121,12 @@ static int mpegaudio_parse(AVCodecParserContext *s1,
>>>          return next;
>>>      }
>>>  
>>> +    if (flush && buf_size >= APE_TAG_FOOTER_BYTES && memcmp(buf, APE_TAG_PREAMBLE, 8) == 0) {
>>> +        *poutbuf = NULL;
>>> +        *poutbuf_size = 0;
>>> +        return next;
>>> +    }  
>>
>> This doesnt feel right
>>
>> Parsers should not discard data
>>
>> a bistream filter could discard data, so could a demuxer if thats how the
>> format should be interpreted. Or the decoder could simply detect this case
>> and not print an error/warning
> 
> This should obviously be done by the demuxer, unless I'm missing some
> other use cases. Should still be OK to skip in the parser. Tags have no
> business in a packet stream (they're not supposed to be there)

I recently changed the raw aac demuxer to stop propagating "junk" at the
beginning and end of the stream (things like id3 and ape tags) by
discarding anything that's not a complete frame, but the result was that
it kinda broke some fully playable files that had one or two damaged
frames. It was meant for codec copy cases, since the aac decoder dealt
with it just fine.

Skipping junk until a sync word is found then combining data until you
get a full frame is currently a valid usage for parsers (see mlp
parser). But i think the plan was to have them only analyze headers to
set relevant parameters in both frames and codec context, and leave the
task to produce full frames to bitstream filters.

, but
> given how broken mp3 data usually is, it's probably nice if the parser
> can filter them out. (Wouldn't be surprised if you find tags in muxed
> tracks in mkv or mp4...)

I've seen files with internal ffmpeg markers in the wild. I have no idea
how they even made it there, but some users seem to find ways to mux the
craziest shit :p