[FFmpeg-devel] [PATCH] ADTS AAC with ID3v2
Robert Swain
robert.swain
Mon Jan 19 00:33:25 CET 2009
2009/1/18 Alex Converse <alex.converse at gmail.com>:
> On Sun, Jan 18, 2009 at 12:11 PM, Robert Swain <robert.swain at gmail.com> 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?
>
> id3v2-all.diff still seems to apply cleanly. But it was missing a
> dependency on id3v2.o for the mpc demuxer. So I've added that.
Feel free to commit it if you now have commit rights.
Regards,
Rob
More information about the ffmpeg-devel
mailing list