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

[Michael Niedermayer]

On Thu, Nov 13, 2008 at 04:47:58PM +0200, Yoav Steinberg wrote:
> 
> 
> 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 mean proof in the mathematical sense, that is that no sequence of
instructions could detect it reliably for non seekable streams.


>  I'll explain why it cannot be 
> handled nicely for non seekable, let me know what other info/proof you want.

fine


> The only way to detect ID3v1 tag is by probing the last 128 bytes of the 
> stream. 

thats not an explanation, thats just the same claim in slightly different
words

[...]


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

Thats no "might", thats certainly possible. But thats not what i mean.
First why do you not detect the id3v1 tag? i mean check if there is a mp3
frame or a id3v1 tag by looking at the header.

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.

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

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- 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/20081113/a45a858f/attachment.pgp>