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

Michael Niedermayer michaelni
Thu Apr 30 00:22:42 CEST 2009


On Wed, Apr 29, 2009 at 11:24:56PM +0200, Zdenek Kabelac wrote:
> 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>:
[...]
> > 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 ?)

Because the order is
1. description of the problem
2. reproduceable test case
3. analysis of the problem, that is understanding what happens and what is
   wrong
4. finding a solution
5. implementing the solution

you present us with 5. and its up to us to interpolate 1-4 from that and
in addition you argue about weird missuses of the API that may or may not
be related to the whole.
My experience shows that patches that fix such mysterious bugs are most
of the time worse than the code before.


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

thats is nonsese, there are containers that do not use a parser for mp3
and this makes me feel another step more uneasy about the patch


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

Iam not applying your initial patch because i do not understand what you
say and there is no test case from which i could bypass your chatter to
see what goes wrong,
now without a patch its obvious that this case is closed because
for a feature request i first would have to understand what you really
talk about. And as i alraedy said i do not have a clue what you talk about
and you apparently out of principle provide no test cases ...

also id like to repeat that you should start a new thread for each seperate
issue. The problem is nothing you say makes sense, Interleaving chatter
about several totally unrelated issues does not make that easier.


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

Did you do this or did you imagine it?
if the later, the case is closed. If the first we need the file you created
and the copy and pasted command line as well as its output and a
description of the expected output.


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

what does the patch do?
does it fix a bug?
if so, could you describe the bug?


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

If there is anything that affects ffmpeg iam interrested to hear it

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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20090430/141d3eee/attachment.pgp>



More information about the ffmpeg-devel mailing list