[FFmpeg-devel] [PATCH] fix parsing of broken mp3 streams

[Zdenek Kabelac]

2009/4/23 Michael Niedermayer <michaelni at gmx.at>:
> On Thu, Apr 23, 2009 at 10:27:58AM +0200, Zdenek Kabelac wrote:
>> 2009/4/22 Michael Niedermayer <michaelni at gmx.at>:
>> > On Wed, Apr 22, 2009 at 10:50:10AM +0200, Zdenek Kabelac wrote:
>> >> 2009/4/22 Michael Niedermayer <michaelni at gmx.at>:
>> >> > On Tue, Apr 21, 2009 at 02:31:59PM +0200, Zdenek Kabelac wrote:
>> >> >> 2009/4/21 Michael Niedermayer <michaelni at gmx.at>:
>> >> >> > On Tue, Apr 21, 2009 at 11:14:16AM +0200, Zdenek Kabelac wrote:
>> >> >> >> 2009/4/21 Michael Niedermayer <michaelni at gmx.at>:
>> >> >> >> > On Tue, Apr 21, 2009 at 01:01:04AM +0200, Zdenek Kabelac wrote:
>> >> >> >> >> 2009/4/20 Michael Niedermayer <michaelni at gmx.at>:
>> >> >> >> >> > On Mon, Apr 20, 2009 at 09:37:25PM +0200, Zdenek Kabelac wrote:
>> >> >> >> >> >> 2009/4/19 Michael Niedermayer <michaelni at gmx.at>:
>> >> >> >> >> >> > On Sun, Apr 19, 2009 at 11:18:06PM +0200, Zdenek Kabelac wrote:
>> >> >> >> >> >> >> Hi
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> Here is a small patch that fixes of running out-of-buffer in parsing
>> >> >> >> >> >> >> broken mp3 data stream.
>> >> >> >> >> >> >> This solution is rather a hotfix - better solution would be to check
>> >> >> >> >> >> >> at least one or two next mp3
>> >> >> >> >> >> >> frames in sequence whether they are part of the same audio stream or
>> >> >> >> >> >> >> some random junk
>> >> >> >> >> >> >> which has 0xfffx header inside. With this patch ugly noise could be
>> >> >> >> >> >> >> sometimes noticed.
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> Also questionable is whether it should return -1 if no header is found
>> >> >> >> >> >> >> or rather return skipped
>> >> >> >> >> >> >> bytes and out_size = 0 - as then usually such packet is rescaned
>> >> >> >> >> >> >> multiple times with
>> >> >> >> >> >> >> one-byte step forward...
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> Zdenek
>> >> >> >> >> >> >>
>> >> >> >> >> >> >> - Fix buffer overrun
>> >> >> >> >> >> >> - Properly return parsed bytes together with skipped bytes
>> >> >> >> >> >> >
>> >> >> >> >> >> > please provide a sample so we can confirm the bugfix, the patch
>> >> >> >> >> >> > looks mostly correct though
>> >> >> >> >> >> >
>> >> >> >> >> >>
>> >> >> >> >> >> I've upload just one mp3 dumped stream upload.ffmpeg.org as
>> >> >> >> >> >> junk_at_mp3stream ?directory - together with short text and two patch
>> >> >> >> >> >
>> >> >> >> >> >> - I'm attaching patch for api-example.c ?to easily compare results.
>> >> >> >> >> >
>> >> >> >> >> > i dont care what a modified tool does
>> >> >> >> >> > is there a problem that is reproduceable with ffmpeg or ffplay that
>> >> >> >> >> > your patch fixes?
>> >> >> >> >>
>> >> >> >> >> Patch is fixing mp3 decoder to skip only broken junk inside passed
>> >> >> >> >> data ?while decoding as much mp3 frames as possible and avoid buffer
>> >> >> >> >> over reading - don't ask me which tools are muxing avi streams with
>> >> >> >> >> junk in packets - obviously it some kind of re-synchronization from
>> >> >> >> >> splinting huge avi streams into small chunks....
>> >> >> >> >>
>> >> >> >> >> You could check for your self is to compare the result of extracted
>> >> >> >> >> wav size via api-example and then do
>> >> >> >> >> the same with ffmpeg -i junk.mp3 ?o.wav - you might observe small
>> >> >> >> >> difference 4027436 != 4018220
>> >> >> >> >> To do my homework and complete the list: mplayer -ao pcm:file=wav
>> >> >> >> >> junk.mp3 - creates 4022830 - but IMHO it decodes some broken packets
>> >> >> >> >> at the begining)
>> >> >> >> >>
>> >> >> >> >> (btw the patch for api-example should be probably commited into svn as well...)
>> >> >> >> >> Usually such badly muxed sample streams are way to small to notice
>> >> >> >> >> significant desynchronization.
>> >> >> >> >
>> >> >> >> > your original patch looked fine but after that you just talk nonsense
>> >> >> >> > apiexample is a example for codecs, not containers, mp3 must be passed
>> >> >> >> > through a demuxer and parser.
>> >> >> >>
>> >> >> >> I knew it would be hard - anyway I'll try once again - please check my
>> >> >> >> original patch
>> >> >> >> and see the mpegaudiodec.c code then please answer me following question:
>> >> >> >>
>> >> >> >> - What will stop parser from checking given buffer for mp3 header tag
>> >> >> >> after the buffer size
>> >> >> >> ?i.e. pass there zero memory area ?- I think decoder shouldn't run
>> >> >> >> behind the given buffer
>> >> >> >> even in the case it contains obviously wrong data - i.e. non-mp3 in this case.
>> >> >> >> (user would have to put false mp3 header after the passed buffer to
>> >> >> >> stop the parser)
>> >> >> >>
>> >> >> >> - If the mp3 packet is found within some offset from the beginning why
>> >> >> >> it should return
>> >> >> >> the size of parsed packed without the skipped bytes from the start of buffer.
>> >> >> >> (so next parsing will again start in the middle of previous mp3 packet)
>> >> >> >>
>> >> >> >> - Explain how the libavformat/mp3.c:mp3_read_packet() solve the problem?
>> >> >> >> (speaking of MP3_PACKET_SIZE - theoretical mythical max size of mp3
>> >> >> >> chunk is 1440)
>> >> >> >
>> >> >> > Iam not disputing that the original patch possibly fixes a issue, i
>> >> >> > am asking if you have a test case so we can test it.
>> >> >> >
>> >> >> > either
>> >> >> > A. the patch has no effect at all on ffmpeg & ffplay
>> >> >>
>> >> >> I think I've already shown that we could get a different amount of WAV
>> >> >> samples from particular mp3 audio stream - we might have a discussion
>> >> >> which number is correct - but IMHO ffmpeg tool ?should always try to
>> >> >> get as much as possible original samples from data stream - but I
>> >> >> could be alone...
>> >> >
>> >>
>> >> At the beginning I want to state that I always admire your work - and
>> >> I'm really sorry it takes me so much time to explain the problem here,
>> >> but as I think I'm right I'd prefer to not give up until we will
>> >> properly understand each other and of course if I'm wrong, I want to
>> >> understand why...
>> >>
>> >> > so does the stream you posted decode differently with unmodified ffmpeg
>> >> > vs. with the original patch?
>> >> > IIRC you only spoke of what the hacked apiexample does
>> >>
>> >> I think I've expressed few times already that ffplay skips full audio
>> >> chunk (ffplay.c line ~1593) when it sees broken chunk i.e. mp3 chunk
>> >> is crossing frame boundary. In my api-example.c change is modification
>> >> which shows how I'm seeing the proper way of decoding of the byte mp3
>> >> stream stored in .avi chunk - when it finds error it rolls forward in
>> >> this stream and it could find a mp3 frame ?which is currently lost by
>> >> plain full chunk skip. That is when the 'small' difference comes from.
>> >>
>> >> api-example.c should be showing the API usage to the user of FFmpeg
>> >> library - and if avcodec_decode_audio3() is supposed to return number
>> >> of consumed bytes from buffer - it should work this way in the
>> >> api-example code - Somehow I do not fully understand what makes you so
>> >> crazy about this as IMHO my small patch just do it in the way the API
>> >> is supposed to work ?
>> >
>> >>
>> >> The current logic mp3 decoder is actually doing also parsers' work to
>> >> match the beginning of mp3 chunk - and there is no mp3 parser for avi
>> >> chunks in use I think. (as the container is ?.avi stream - not .mp3
>> >> stream)
>> >
>> > avidec.c:
>> > ?case CODEC_TYPE_AUDIO:
>> > [...]
>> > ? ? ? ? ? ? ? ? ? ?/* Force parsing as several audio frames can be in
>> > ? ? ? ? ? ? ? ? ? ? * one packet and timestamps refer to packet start. */
>> > ? ? ? ? ? ? ? ? ? ?st->need_parsing = AVSTREAM_PARSE_TIMESTAMPS;
>> >
>> >
>> >>
>> >> So what I'm saying is - that mp3 is muxed into .avi as byte stream -
>> >> thus we should not easily give up full chunk if error is found - i.e.
>> >> there could be a person who will put all mp3 chunks into one .avi
>> >> frame - which is possible and perfectly valid
>> >
>> > no, its disallowed by the avi spec from MS but 99% of the
>> > muxers ignore it and then start talking about weird bugs in microsofts
>> > implementation of avi in relation to vbr mp3 (for CBR the specs allow
>> > choping up frames randomly into chunks)
>>
>> actually it's more complicated - you can legally put multiple VBR
>> frames into one .avi chunk ?-
>
> no you cannot do that, the spec does not allow it.
> "dwSampleSize
> ?...
> ?If it is zero, each sample of data (such as a video frame) must be in a separate chunk."
>
>
>> and actually it would be good for making
>> the file a bit smaller - and while combining 2-3 chunks together it
>> should be still pretty easy to maintain good lips synchronization.
>
> maybe it would work with tolerable AV sync errors, maybe not i dont know
> i dont have access to the source of all implementations of all avi demuxers.
> Thats why there are specs,
> instead of 50 muxers being designed to work with 50 demuxers, 50 muxers
> just follow the spec ... well ideally
>
>
>>
>> Here is Avery's text that explains in depth all the magic behind it:
>> http://www.virtualdub.org/blog/pivot/entry.php?id=27
>>
>
>> So if you put large enough nBlockAlign field to cover max size of
>> several combined frames - it will make a valid stream.
>
> it might (or might not) work with MS dshow demuxer but that doesnt make
> it a valid stream.
>
>
>> I'm not yet sure - why mencoder set this value to '1' - which probably
>> creates unplayable files at least on some Windows machines.
>>
>>
>> >> and actually saves some
>> >> space in the .avi tables, but for some players the stream is hardly
>> >> seekable then - and the first error detected in mp3 stream will cause
>> >> lost of full audio track if frame_size gets -1.
>> >
>> > the mp3 decoder is supposed to be feeded with individual mp3 frames
>> > It will even tell you that by:
>> > ? ?if(s->frame_size<=0 || s->frame_size > buf_size){
>> > ? ? ? ?av_log(avctx, AV_LOG_ERROR, "incomplete frame\n");
>> > ? ? ? ?return -1;
>> > ? ?}else if(s->frame_size < buf_size){
>> > ? ? ? ?av_log(avctx, AV_LOG_ERROR, "incorrect frame size\n");
>> > ? ? ? ?buf_size= s->frame_size;
>> > ? ?}
>> >
>> > now it might be that its able to handle the case of several frames in one
>> > piece but thats not to say this is how its supposed to be used
>>
>> I'm not solving 'ideal' world - when you put there 'valid' frame - I'm
>> talking about the case of parsing invalid frame.
>
> yes and?
>
>
>> And I want to point out that there are API inconsistencies.
>>
>> I've proposed several solution how to make the API look more
>> consistent -
>
> i dont remember but then i also dont remember you showing a inconsistency
> you mix so many issues now that i really think i should just
> ignore you.

Please do not - I really do not try to waste your time - but sometimes
it's hard to explain the problem.

> please start a new thread for each individual thing and keep on topic
> api inconsistencies, handling of multiple mp3 frames, avi vbr mp3 muxing,
> invalid mp3 frame handling, hacking apiexample, passing more than 1 frame
> to decoders, fixing a bug in the mp3 decoder, ......

I've found some time to look deeper into the issue of those missing mp3 packets.

My original patch was fixing decoder (and I still IMHO think it should
be applied - as there is no reason to keep unlimited buffer scanning -
just look into the code - it really is a bug to scan buffer and not
decrease the buffer size counter - I'm really puzzled why it takes so
much to fix such simple code ?)

The reason why ffmpeg doesn't see it as a problem is - its' using
mpegaudio_parser.c which I've missed to check - my fault.
Thus ffmpeg will not push broken data to decoder without a real mp3
packet inside - thus the problem has no chance to show in ffmpeg.

The reason why it's missing 2 mp3 packets per resynchronization is
that when mp3 stream contains junk inside - it will
skip 2 frames during next resynchronization (mpegaudio_parser.c - line
~130) - so I would probably suggest to switch  frame size to 0 and
decrease header_count only when the stream was not yet synchronized in
this case or when packet has different playback frequency - but I
assume it is something for error concealment options - but I'm really
afraid to develop myself some patch for this - as even very simple one
are hard to push in :(... - so I'll stay with description of symptoms
and leaving the rest to you.

i.e. - when you create stream with with few mp3 frames - then insert
zero data and again same packets - you will miss 2 packets on output
with the tool pktdumper - I've used it to get some raw mp3 packet -
then with dd created a short zero file and then I've joined them with
cat.

(btw I'm attaching short patch for pktdumper - in case you would be
interested - it releases allocated packets and closes input stream)

>
>> and hoping we will get to some conclusion. Also I think
>> decoder itself is autonomous unit - i.e. library user uses only
>> libavcodec and expect consistent behaviour. Now it looks like decoder
>> is also 'parsing' the stream - so either stream parsing is disabled -
>> i.e. returns -1 ?if the buffer does not begin with mp3 header - ?or
>> otherwise we should return amount of parsed bytes this decoder has
>> scanned and return 0 in the output size.
>
> The decoder is decoding mp3 frames, it is capable to deal with damaged
> mp3 frames, like flipped bits or a few extra bytes before or after the
> frame, thats not parsing.

Which still leads to my initial patch - decoder should never scan for
the mp3 header past the given buffer - current code could do that.

>
>
>>
>> >>
>> >> As I do not want to extended my theories further - I wait until we
>> >> agree here on some conclusion.
>> >>
>> >> There is number of solutions for this problem - I just wanted to go in
>> >> the way a smallest changes and I also said the fix in ffplay & ffmpeg
>> >> is not one-line change - but it's not so complicated either.
>> >>
>> >> And one note to VBR I've mentioned previously - you probably
>> >> misunderstand that I mean some problems with reading Xing headers from
>> >> music audio mp3 files - I've only meant playing VBR audio mixed in AVI
>> >> streams (which I probably didn't emphasize enough) - which is
>> >> currently also a bit buggy - check i.e. 13fantavsync.avi in mplayer
>> >> samples site. There are other issues but let's fix them one-by-one.
>> >>
>> >> >
>> >> >
>> >> >>
>> >> >> > B. there is a file for which behavior changes
>> >> >>
>> >> >> The fact that it's not running out of memory bounds when the mp3
>> >> >> header could not be found in the given buffer is probably because
>> >> >> usually lots of other mp3 frames are lying nearby in memory so it will
>> >> >> effective stop - and there are not too many heavily broken stream.
>> >> >
>> >> > maybe, maybe not
>> >> >
>> >> >
>> >> >>
>> >> >> So to answer your questions
>> >> >> A - currently my patch does not influence those tools as they discard
>> >> >> whole data chunk if the error is found.
>> >> >> B - artificial file could be probably created which will show problem
>> >> >> from scanning data past the buffer - and generate coredump - though
>> >> >> it's not probably so simple to ensure memory layout that no mp3 header
>> >> >> will not be found past the allocated header.
>> >> >
>> >> > so you claim 2 mutually exlusive theorems are true?
>> >> > sorry
>> >> > either ffmpeg does or does not change with some input
>> >>
>> >>
>> >> FFmpeg with my proposed patch doesn't change in the way, it produces
>> >> same amount of audio samples.
>> >> The only change is - it will not crash due to scanning past the input buffer
>> >>
>> >> I think I cannot say it more clearly ?
>> >
>> > so you have no sample that crashed ffmpeg nor that makes ffmpeg produce
>> > diferent output
>>
>> Speaking about samples - I've checked the test directory of ffmpeg
>> whether it contains some code for checking handling of broken files -
>> and haven't noticed any - is there any support for creating of
>> 'broken' files ?- i.e. create of .avi streams and put weird error
>> bytes/bits into some places in this stream and checking whether ffmpeg
>> will survive and produces expected amount of output ?
>
> tools/trasher
>

Yes - I've noticed this tool - but it's not designed to make explicit
bit modification of specifically created files. Anyway I've figured
out where is my problem.  If you are still interested more in this
topic - let me know - otherwise I'll not take you more time on this
and I'll probably implement this small fixes only for myself.

Zdenek

Here is promised short patch for pktdumper.c:

Index: tools/pktdumper.c
===================================================================
--- tools/pktdumper.c	(revision 18717)
+++ tools/pktdumper.c	(working copy)
@@ -110,8 +110,11 @@
         pktnum++;
         if (maxpkts && (pktnum >= maxpkts))
             break;
+        av_free_packet(&pkt);
     }

+    av_close_input_file(fctx);
+
     while (donotquit)
         sleep(60);