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

[Zdenek Kabelac]

2009/4/30 Michael Niedermayer <michaelni at gmx.at>:
> 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

My main concern was about different result from parsing .avi streams -
but in this case ffmpeg uses mpegaudio_parser.c to parse mp3 stream,
thus if you know the format which is accessing directly codec and
doesn't go through this parser file - I'll be probably able to create
test file for this case - otherwise this issue could be only triggered
by directly calling libavcodec/mpegaudio_dec.c decoder_frame with
inappropriate buffer.

>
>
>>
>> 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've created wish list tracker event:

https://roundup.ffmpeg.org/roundup/ffmpeg/issue1044

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

Please see the wish list.
If it's still unclear - I'll happily answers them


Zdenek