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

[Yoav Steinberg]

Michael Niedermayer wrote:
> On Thu, Nov 13, 2008 at 09:16:24AM +0200, Yoav Steinberg wrote:
>>
>> Michael Niedermayer wrote:
>>> On Wed, Nov 12, 2008 at 07:02:55PM +0200, Yoav Steinberg wrote:
>>>> Hi,
>>>> Attached are patches witch make some improvements for mp3 parsing.
>>>> skip_vbrtag_and_id3v1.patch:
>>>> - When file contains ID3v1 tag at the end don't attempt to read it as 
>>>> part of a packet. In some cases there are mp3 files where the ID3v1 tag 
>>>> was stuck forcefully at the end of the file inside the last frame 
>>>> boundary. In those cases the patch eliminates attempts to parse/decode 
>>>> the ID3 tag.
>>>> - If we find a VBR tag at the beginning of the file don't attempt to 
>>>> parse it as a valid frame.
>>> These are 2 seperate things which must be in seperate patches.
>> Attached is a single patch for the prevention of reading ID3v1 as part of a 
>> packet.
>>
>>>> @@ -505,10 +515,20 @@
>>>>  static int mp3_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>>  {
>>>>      int ret, size;
>>>> +    int64_t size_left;
>>>>      //    AVStream *st = s->streams[0];
>>>>       size= MP3_PACKET_SIZE;
>>>>  +    /* if we're nearing the end of the input and there's an
>>>> +       ID3v1 tag then make sure we don't read it as a packet */
>>>> +    if (((MPAContext*)s->priv_data)->found_id3v1 && s->file_size && +    
>>>>     (size_left = s->file_size - url_ftell(s->pb) - ID3v1_TAG_SIZE) < 
>>>> size) {
>>>> +        size = size_left;
>>>> +        if (size <= 0)
>>>> +            return AVERROR(EIO);
>>>> +    }
>>>> +
>>>>      ret= av_get_packet(s->pb, pkt, size);
>>>>       pkt->stream_index = 0;
>>> file_size is not known for stdin/pipes
>> That's why I put the '&& s->file_size' so that this logic will be avoided 
>> when file size is not known. In any case the found_id3v1 will be false in 
>> such cases. But for future compatibility in case the tag will somehow be 
>> known but not the file size I added the '&& s->file_size' (which is set to 
>> 0 if file size is not known).
> 
> Fixing a bug just for seekable input _requires_ a proof that it cannot be
> fixed for non seekable or that a complete fix would be too messy.
> 

Not quite sure what you mean by proof. I'll explain why it cannot be 
handled nicely for non seekable, let me know what other info/proof you want.
The only way to detect ID3v1 tag is by probing the last 128 bytes of the 
stream. This is obviously not possible for non seekable streams.

It _might_ be possible to change read_packet to always buffer the last 
128 bytes of data and return them with a delay after more data is 
available. This way when we reach the end of stream we can check that 
the last 128 bytes don't include an ID3v1 tag before actually passing 
the data upwards. But:
1) This will be ugly since we delay data read from the stream and might 
produce some messy code with performance degrading memcpy's.
2) The bug isn't critical (worst case causes bad decoding of last frame, 
and more common case causes extra work in the parser searching for a 
frame for the last 128 bytes before reaching eof).
3) Anyone using this demuxer on a non seekable stream won't get ID3v1 
tag info anyway, so bugs produced by the presence of the tag can be 
avoided by not using such data sources without loosing out on any 
functionality they wouldn't loose out on anyway.

So it's probably not worth it to try fixing it on non seekable streams, 
what do you think?