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

[Zdenek Kabelac]

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)

> You might get away with some luck and some hacks without that but its
> not a patch that "should be probably commited into svn as well" even
> less so in a api example that people might use as basis for their
> application.

anyway I think you should assume I know what is the purpose of api-example.c....
(and I've also used it as an example ;))

>
> why does mp3 need a demuxer?! id3 and i hate that as well, if you happen
> to have a time machine contact me in private ;)

Well all the time I'm  talking about broken mp3 stream - not mp3
stream with tags....
(i.e. my patch is not for the musical mp3 stream with id3 tags - I'm
only pointing to the problem of mp3
inside avi container - and ability to catch as much as possible of the
original sound that
could be found in the stream - in this case it's 2 frame difference...

Zdenek