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

[Michael Niedermayer]

On Tue, Jan 30, 2018 at 11:30:43AM -0300, James Almer 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).

I do disagree about this
Parsers where supposed to split data in frames but not discard any data. Yes 
we do have some code that breaks that rule and some of it like the one you
quoted earlier written by me (iam not really proud of that one but lucky its
very limited to what it can discard and where)

But the ideal behavior was to never discard data, include it in the next
frame, the previous or a seperate frame but not to discard.

Parsers are "mandatory" for some demuxers and if they discard data then that
data is lost and the user app would not even know that there was something that
was discarded. A decoder may very well be able to recover something from the
data.



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

This is a good idea but i dont think thats a plan that was discussed or
agreed on for FFmpeg. Design changes like this should be discussed on
ffmpeg-devel. FFmpegs design should be directed by the FFmpeg community

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180130/6d2370b7/attachment.sig>