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

[Michael Niedermayer]

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
B. there is a file for which behavior changes

which is it?

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

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090421/868d608d/attachment.pgp>