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

[Alex Converse]

On Thu, Jan 15, 2009 at 1:30 PM, David DeHaven <dave at sagetv.com> wrote:
>
> On Jan 14, 2009, at 11:37 PM, 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.
>
>
> Can we do the same for FLAC please?
>
> Here's a hunk from my patch, modified for the new id3v2.c functions though
> the diff is against SVN 16617.

Thanks

> I haven't seen any with ID3v1 tags so I didn't bother checking.

ID3v1 sits at the end of th file so it should be a non issue.

--Alex Converse