[FFmpeg-devel] [PATCH] fixes in mpeg audio parsing

[Yoav Steinberg]

Michael Niedermayer wrote:
> On Thu, Nov 13, 2008 at 09:10:41PM +0200, Yoav Steinberg wrote:
>>
>> Michael Niedermayer wrote:
>>> On Thu, Nov 13, 2008 at 06:59:17PM +0200, Yoav Steinberg wrote:
>>> [...]
>>>>> One certainly could have concatenated 2 mp3 files with a id3v1 ending in
>>>>> the middle it would be nice if the parser & decoder would skip/ignore this
>>>>> trash in the middle.
>>>>> To me it seem that it shouldnt be too hard to make the parser skip
>>>>> this, unless a valid id3v1 tag can also be a valid mp3 frame.
>>>>>
>>>> The current parser will, in most cases, skip id3v1 tags in the stream 
>>>> which is good. But in one case the parser does actually think the tag is 
>>>> part of a frame and does not skip it. This is when the tag was put 
>>>> inside (overwriting) a frame's data. This isn't strictly valid, but it 
>>>> turns out there are such files out there.
>>> so, fix the parser
>> I don't see how the parser can detect such a case. If the tag data is 
>> inside the frame's data the parser will see a valid frame.
> 
> now i do not know if such case exists (maybe you could upload a sample?)

Not sure how you'd like to get the samples. I've put them on a web 
server, if that's good for you (I'll remove them in 24h):
http://steinberg.monfort.co.il/temp/mp3samples/00000073.mp3
http://steinberg.monfort.co.il/temp/mp3samples/00000091.mp3
http://steinberg.monfort.co.il/temp/mp3samples/00000127.mp3
These are 3 such cases I've come across running on part of my sample 
base, it _might_ not be possible for me to find out how they were 
encoded, since they are part of a large collection gathered from many 
sources. I don't know if you're interested in "the chance" of finding 
such a file, I've tested 142 files from my collection and found these 3.

> but assuming one exists then removing the last 128 byte will give you
> a truncated frame. That does not appear more correct than leaving a
> valid frame there.

Truncated frames are handled by the parser - actually since the input 
ends before the frame ends then the parser will never get to finish the 
frame so it won't be fully parsed / decoded. Not to mention that these 
frames really are truncated frames, because some software truncated them 
when it put the id3v1 tag in place.
To me this seems better than decoding an _invalid_ frame. True, there's 
always the chance this is a valid frame where 'TAG' just happens to 
appear in its data, but that means we probably messed up earlier when we 
read the id3v1 tag when we were supposed to ignore it because it was 
part of a frame.
Another thing to think of is what happens if someone changes the id3v1 
tag in such files. With the current implementation it would result in a 
change (though slight) in the audio output, that doesn't seem right.