[Ffmpeg-devel] [PATCH] allow ffmpeg to read mp3s beginning with partial frames

[Victor Voros]

Michael - thank you for the feedback. I will post a revised patch today
or tomorrow with the suggestions. I assume you're in agreement with the
cosmetic changes. (constants bad!). 

I believe the false positive issue can be resolved with

-if (mp3_is_frame_header(p->buf+d)) return AVPROBE_SCORE_MAX;
+if (ff_mpa_check_header(p->buf+d/*or however its called*/)) return
d==0?AVPROBE_SCORE_MAX:SOME_SMALL_NON_ZERO_NUMBER;

Wrt to the posted mp3s if there is no change to mp3_read_header but
mp3_read_probe is changed, then ffmpeg goes seriously pear-shaped (ie
codec thinks the random first few bytes of the file as valid sampling
frequencies etc).

It would make sense to move the partial frame skipping code (factored!)
from mp3_read_header to the libavcodec as presumably if something is
generating dodgy mp3 files it stands to reason something is able to mux
said dodgy files into a dodgy .mpg. I've seen in the past ffmpeg read
mpgs and get random data stats for the audio (tho don't have any
examples to hand) its possible that this sort of situation is the
reason.    

Victor Voros

-----Original Message-----
From: ffmpeg-devel-bounces at mplayerhq.hu
[mailto:ffmpeg-devel-bounces at mplayerhq.hu] On Behalf Of Michael
Niedermayer
Sent: 07 September 2006 23:37
To: FFMpeg development discussions and patches
Subject: Re: [Ffmpeg-devel] [PATCH] allow ffmpeg to read mp3s beginning
with partial frames

Hi

On Thu, Sep 07, 2006 at 11:08:15PM +0100, Victor Voros wrote:
[...] 
> > 
> > Attached is a patch to libavformat\mp3.c which addresses this in 
> > diff
> -u
> > format from head (6180)
> > 
> > Simply scans the file looking for a sync frame and starts there if 
> > found.
> 
> rejected, this does not belong in libavformat, demuxers should output 
> whats in the file, not do codec specific parsing and discarding of 
> stuff such code should be in the parser or decoder
> 
> mp3.c as I see it serves 2 purposes, 1) to determine whether the file 
> is in fact an mp3 file (mp3_Read_probe) and 2) to prime the file so 
> its pointing at the start of the first frame, (currently skipping a 
> possible
> id3 tag) ready for the codec to do its work(mp3_read_header). The file

> already has code to search for a valid mpeg frame sync, the change 
> expands on this by not assuming the frame sync is at the very start of

> the file. No extra codec 'knowledge' has been added to this file.
> 
> In any case, mp3_read_probe is the routine rejecting the said files. 
> One way or another this has to change to accommodate the files.

indeed mp3_read_probe() needs to be fixed, but that is the only function
in mp3.c which should be changed

[...]

> +#define MP3_PACKET_SIZE 1024
> +
> +static int mp3_is_frame_header(uint8_t *p) {
> +	uint8_t d;
> +
> +	if (*p != 0xff) return 0;
> +
> +	d = p[1];
> +	if((d & 0xe0) != 0xe0 || ((d & 0x18) == 0x08 || (d & 0x06) ==
0))
> +		return 0;
> +
> +	d = p[2];
> +	if((d & 0xf0) == 0xf0 || (d & 0x0c) == 0x0c)
> +		return 0;
> +
> +	return 1;
> +
> +}

looks like a duplication of ff_mpa_check_header()


> +
>  /* mp3 read */
>  
>  static int mp3_read_probe(AVProbeData *p)  {
>      int d;
>  
> -    if(p->buf_size < 4)
> -        return 0;
> -
> -    if(p->buf[0] == 'I' && p->buf[1] == 'D' && p->buf[2] == '3' &&
> -       p->buf[3] < 5)
> +    if((p->buf_size >= ID3_TAG_SIZE) && id3_match(p->buf))
>          return AVPROBE_SCORE_MAX;
>  
> -    if(p->buf[0] != 0xff)
> -        return 0;
> +	// handle buggy mp3 converter programs that have a habit of
starting mid-frame
> +	// and find the first frame. if that sounds ott Windows Media
player and itunes
> +	// handle these files.
>  
> -    d = p->buf[1];
> -    if((d & 0xe0) != 0xe0 || ((d & 0x18) == 0x08 || (d & 0x06) == 0))
> -        return 0;
> -
> -    d = p->buf[2];
> -    if((d & 0xf0) == 0xf0 || (d & 0x0c) == 0x0c)
> -        return 0;
> +    for (d=0;d<FFMIN(MP3_PACKET_SIZE,p->buf_size-4);d++) {
>  
> -    return AVPROBE_SCORE_MAX;
> +        if (mp3_is_frame_header(p->buf+d)) return AVPROBE_SCORE_MAX;

not ok due to false positive detection (mp3 in avi, mp3 in mov, mp3 in
mpeg-ps and many others)


[...]
>  }
>  
> +
>  static int mp3_read_header(AVFormatContext *s,

cosmetic change


[changes outside mp3_read_probe()]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel at mplayerhq.hu
http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel


------------------------------------------------------------------------------
This e-mail and its contents are intended for the use of the addressee(s) and may be confidential/privileged. No-one else may review, copy, disclose or otherwise use it or its contents. If you receive this e-mail in error, please contact the originator and delete it as soon as possible.
Benfield Limited is authorised by the Financial Services Authority under the reference number 311884. Registered in England no 1170753. Registered office 55 Bishopsgate.

Please refer to Benfield Limited's terms and conditions (www.benfieldgroup.com/terms) for a description of our services, duties and points of contact. Please review these terms and conditions at inception and renewal of all reinsurance and insurance contracts.  

Please note that our terms and conditions and interests in other companies may change over time and these changes will be reflected on the Benfield Limited website. The latest version of our terms and conditions supersedes all previous versions.
==============================================================================