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

[David DeHaven]

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.
I haven't seen any with ID3v1 tags so I didn't bother checking.

-DrD-

-------------- next part --------------
A non-text attachment was scrubbed...
Name: flac_id3v2.patch
Type: application/octet-stream
Size: 702 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090115/9252c86a/attachment.obj>
-------------- next part --------------