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

wm4 nfxjfg at googlemail.com
Tue Jan 30 16:44:09 EET 2018


On Tue, 30 Jan 2018 11:30:43 -0300
James Almer <jamrial at gmail.com> wrote:

> 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.

In theory we could have a flag that controls this (I thought there
actually was such a flag, but didn't find anything). If we ever find
that discarding data is a problem, maybe such a flag could be added.

Also I still kind of hope someone will make a new parser API, since the
current one is clunky with its AVCodecContext use and its inability to
return errors.

Anyway, my point was that raw demuxers should strip tags like id3v2 and
APE before we add tag stripping to every parser that's used for raw
demuxing. (Still would be good to get info on what use case the patch
fixes.)

> , 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

The wonderful merged side data stuff?


More information about the ffmpeg-devel mailing list