| Message ID | CAPUDrwf67aJdaVha8_oFX2KcZFpzGpODFLWzUQDnPPnNEmidKg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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@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 [...]
On 1/29/2018 10:24 PM, Michael Niedermayer 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@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 patches duplicates existing code for id3v1 added by you in 89a420b71b5. I don't know if the rationale mentioned in the above commit is still valid today, and if it can be applied for apetag as intended by this patch, but there's at least a precedent for this.
On Mon, Jan 29, 2018 at 10:29:04PM -0300, James Almer wrote: > On 1/29/2018 10:24 PM, Michael Niedermayer 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@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 patches duplicates existing code for id3v1 added by you in 89a420b71b5. > I don't know if the rationale mentioned in the above commit is still > valid today, and if it can be applied for apetag as intended by this > patch, but there's at least a precedent for this. yes, i have forgotten/missed this. i ll apply the patch, as we already skip tags there ... maybe this could be moved into a auto inserted bitstream filter or something later. This was harder to do when the id3 skip code was added [...]
On Tue, 30 Jan 2018 02:24:29 +0100 Michael Niedermayer <michael@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@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), 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...)
On 1/30/2018 2:45 AM, wm4 wrote: > On Tue, 30 Jan 2018 02:24:29 +0100 > Michael Niedermayer <michael@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@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
On Tue, 30 Jan 2018 11:30:43 -0300 James Almer <jamrial@gmail.com> wrote: > On 1/30/2018 2:45 AM, wm4 wrote: > > On Tue, 30 Jan 2018 02:24:29 +0100 > > Michael Niedermayer <michael@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@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?
On Tue, Jan 30, 2018 at 6:44 AM, wm4 <nfxjfg@googlemail.com> wrote: > 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. > > +1 to both of these points. To add in my 2 cents, consumption of metadata (APE, ID3, etc) packets seems like something that should be part of the core avformat routines. I.e. they should transparently feed into the AVFormatContext metadata structure. Chrome doesn't use them, so we're fine with discarding them like I did in my patch, but forwarding them to clients which have been told these are packets for codec X when in reality they are metadata packets feels unfortunate. - dale
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@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@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 [...]
2018-01-30 15:30 GMT+01:00 James Almer <jamrial@gmail.com>: > 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. Tickets #6634 and #6900. Carl Eugen
From 05f5e837862a958fb8791668ab3cdf8fc7766fe5 Mon Sep 17 00:00:00 2001 From: Dale Curtis <dalecurtis@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; + } + *poutbuf = buf; *poutbuf_size = buf_size; return next; -- 2.16.0.rc1.238.g530d649a79-goog