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

Zdenek Kabelac zdenek.kabelac
Thu Apr 23 10:27:58 CEST 2009


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

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.
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.
And I want to point out that there are API inconsistencies.

I've proposed several solution how to make the API look more
consistent - 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.

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

Zdenek



More information about the ffmpeg-devel mailing list