[FFmpeg-devel] [PATCH] ADTS AAC with ID3v2

[David DeHaven]

On Jan 18, 2009, at 9:11 AM, Robert Swain wrote:
> 2009/1/16 Michael Niedermayer <michaelni at gmx.at>:
>> On Fri, Jan 16, 2009 at 11:57:52AM -0500, Alex Converse wrote:
>>> On Thu, Jan 15, 2009 at 5:31 PM, Michael Niedermayer <michaelni at gmx.at 
>>> > wrote:
>>>> On Thu, Jan 15, 2009 at 02:37:11AM -0500, Alex Converse wrote:
>>>>> On Wed, Jan 14, 2009 at 1:53 PM, Michael Niedermayer <michaelni at gmx.at 
>>>>> > wrote:
>>>>>>
>>>>>> On Wed, Jan 14, 2009 at 01:39:05PM -0500, Alex Converse wrote:
>>>>>>> On Wed, Jan 14, 2009 at 12:57 PM, Michael Niedermayer <michaelni at gmx.at 
>>>>>>> >wrote:
>>>>>>>
>>>>>>>> On Wed, Jan 14, 2009 at 10:17:33AM -0500, Alex Converse wrote:
>>>>>>>>> On Wed, Jan 14, 2009 at 10:10 AM, Michael Niedermayer <michaelni at gmx.at
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> On Wed, Jan 14, 2009 at 09:47:29AM -0500, Alex Converse  
>>>>>>>>>> wrote:
>>>>>>>>>>> On Wed, Jan 14, 2009 at 9:17 AM, Michael Niedermayer <
>>>>>>>> michaelni at gmx.at
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On Wed, Jan 14, 2009 at 12:23:00AM -0500, Alex Converse  
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> The attached patch adds support to the ADTS AAC probe to  
>>>>>>>>>>>>> detect
>>>>>>>> AAC
>>>>>>>>>> files
>>>>>>>>>>>>> with ID3v2 tags at a higher level than the MP3 probe and  
>>>>>>>>>>>>> the MPC
>>>>>>>>>> probe.
>>>>>>>>>>>> It
>>>>>>>>>>>>> refactors some id3 detection from mp3.c into id3v2.c.
>>>>>>>>>>>>>
>>>>>>>>>>>> [...]
>>>>>>>>>>>>
>>>>>>>>>>>>> #define ID3v1_TAG_SIZE 128
>>>>>>>>>>>>> -
>>>>>>>>>>>>> #define ID3v1_GENRE_MAX 125
>>>>>>>>>>>>>
>>>>>>>>>>>>> static const char * const  
>>>>>>>>>>>>> id3v1_genre_str[ID3v1_GENRE_MAX + 1] =
>>>>>>>> {
>>>>>>>>>>>>
>>>>>>>>>>>> cosmetic
>>>>>>>>>>>>
>>>>>>>>>>>> [...]
>>>>>>>>>>>>> @@ -487,7 +472,7 @@ static int  
>>>>>>>>>>>>> mp3_read_header(AVFormatContext
>>>>>>>> *s,
>>>>>>>>>>>>>     ret = get_buffer(s->pb, buf, ID3v2_HEADER_SIZE);
>>>>>>>>>>>>>     if (ret != ID3v2_HEADER_SIZE)
>>>>>>>>>>>>>         return -1;
>>>>>>>>>>>>> -    if (id3v2_match(buf)) {
>>>>>>>>>>>>> +    if (ff_id3v2_match(buf)) {
>>>>>>>>>>>>>         /* parse ID3v2 header */
>>>>>>>>>>>>>         len = ((buf[6] & 0x7f) << 21) |
>>>>>>>>>>>>>             ((buf[7] & 0x7f) << 14) |
>>>>>>>>>>>>
>>>>>>>>>>>> the split out of (ff_)id3v2_match is ok but should be a  
>>>>>>>>>>>> seperate
>>>>>>>> patch
>>>>>>>>>>>> and commit
>>>>>>>>>>>>
>>>>>>>>>>>> [...]
>>>>>>>>>>>>>     }
>>>>>>>>>>>>> -    if   (first_frames>=3) return AVPROBE_SCORE_MAX/2+1;
>>>>>>>>>>>>> +    if   (first_frames>=3) return AVPROBE_SCORE_MAX/2+2;
>>>>>>>>>>>>>     else if(max_frames>500)return AVPROBE_SCORE_MAX/2;
>>>>>>>>>>>>>     else if(max_frames>=3) return AVPROBE_SCORE_MAX/4;
>>>>>>>>>>>>>     else if(max_frames>=1) return 1;
>>>>>>>>>>>>
>>>>>>>>>>>> why?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Just having an ID3v2 tag gives a score of  
>>>>>>>>>>> AVPROBE_SCORE_MAX/2+1 for
>>>>>>>> mp3.
>>>>>>>>>>
>>>>>>>>>> That sounds like a bug ...
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The comment about mpeg-ps in mp3.c made me think it was a  
>>>>>>>>> hackish design
>>>>>>>>> decision, not a bug.
>>>>>>>>
>>>>>>>> well one could call it that way, still the existence of  
>>>>>>>> hackish code is no
>>>>>>>> argument to add more hackish code like the +1 -> +2
>>>>>>>>
>>>>>>>
>>>>>>> What would be the correct solution for MP3 then? skipping the  
>>>>>>> tag and trying
>>>>>>> again?
>>>>>>
>>>>>> i think so
>>>>>
>>>>> I fixed the MP3 hackishness in the attached patch. I also fixed  
>>>>> a typo
>>>>> in the old patch. Once I fixed MP3, musepack became the next  
>>>>> greedy
>>>>> culprit so to prevent MP3 regressions I had to fix him too. I  
>>>>> don't
>>>>> have any MPC files with ID3v2 so that portion of that patch is
>>>>> untested.
>>>>>
>>>>> In a related note, the tag skipper in mpc_read_header seems to not
>>>>> check for a footer, which I think could be trouble.
>>>> [...]
>>>>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
>>>>> index c27443b..649fafa 100644
>>>>> --- a/libavformat/id3v2.c
>>>>> +++ b/libavformat/id3v2.c
>>>>> @@ -33,3 +33,15 @@ int ff_id3v2_match(const uint8_t *buf)
>>>>>             (buf[8] & 0x80) == 0 &&
>>>>>             (buf[9] & 0x80) == 0;
>>>>> }
>>>>> +
>>>>> +int ff_id3v2_tag_len(const uint8_t * buf)
>>>>> +{
>>>>> +    int len = ((buf[6] & 0x7f) << 21) +
>>>>> +        ((buf[7] & 0x7f) << 14) +
>>>>> +        ((buf[8] & 0x7f) << 7) +
>>>>> +        (buf[9] & 0x7f) +
>>>>> +        ID3v2_HEADER_SIZE;
>>>>> +    if (buf[5] & 0x10)
>>>>> +        len += ID3v2_HEADER_SIZE;
>>>>> +    return len;
>>>>> +}
>>>>
>>>> is this correct?
>>>> i mean this does either X + ID3v2_HEADER_SIZE or X +  
>>>> 2*ID3v2_HEADER_SIZE
>>>>
>>>
>>> That is my understanding.
>>
>> then patches ok
>
> Hrm. I've already applied the one pertaining to AAC but now I see that
> there is overlap between that one and the other ones.
>
> Alex: Could you possibly provide another patch against trunk please?
> And if you have commit rights now, you may as well commit it yourself.
> :)

<parrot mode> FLAC! FLAC! </parrot mode>
This works on all the id3v2 infested flac files I have.

-DrD-
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flac_id3.patch
Type: application/octet-stream
Size: 1532 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090122/d1851c2c/attachment.obj>
-------------- next part --------------