[FFmpeg-devel] [PATCH] avformat/id3v2: support buggy id3v2.3 tag length in id3v2.4

[Benoit Fouet]

Hi,

----- Mail original -----
> On Thu, Oct 16, 2014 at 06:06:39PM +0200, Benoit Fouet wrote:
> > Hi,
> > 
> > On 16 October 2014 17:10:38 CEST, Michael Niedermayer
> > <michaelni at gmx.at> wrote:
> > >On Thu, Oct 16, 2014 at 11:44:47AM +0200, Benoit Fouet wrote:
> > >> Some encoders do not use syncsafe sizes in v2.4 id3 tags. Check
> > >> the
> > >next
> > >> tag to try to choose between the two.
> > >> 
> > >> Fixes ticket #4003
> > >> ---
> > >>  libavformat/id3v2.c | 42
> > >>  +++++++++++++++++++++++++++++++++++++++++-
> > >>  1 file changed, 41 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > >> index 5469e0a..3bccd76 100644
> > >> --- a/libavformat/id3v2.c
> > >> +++ b/libavformat/id3v2.c
> > >> @@ -170,6 +170,23 @@ static unsigned int get_size(AVIOContext
> > >> *s, int
> > >len)
> > >>      return v;
> > >>  }
> > >>  
> > >> +/* No real verification, only check that the tag consists of
> > >> + * a combination of capital alpha-numerical characters */
> > >> +static int is_tag(const char *buf, int len)
> > >> +{
> > >> +    if (!len)
> > >> +        return 0;
> > >> +
> > >> +    while (len--)
> > >> +        if ((buf[len] < 'A' ||
> > >> +             buf[len] > 'Z') &&
> > >> +            (buf[len] < '0' ||
> > >> +             buf[len] > '9'))
> > >> +            return 0;
> > >> +
> > >> +    return 1;
> > >> +}
> > >> +
> > >>  /**
> > >>   * Free GEOB type extra metadata.
> > >>   */
> > >> @@ -734,8 +751,31 @@ static void id3v2_parse(AVIOContext *pb,
> > >AVDictionary **metadata,
> > >>              tag[4] = 0;
> > >>              if (version == 3) {
> > >>                  tlen = avio_rb32(pb);
> > >> -            } else
> > >> +            } else {
> > >>                  tlen = get_size(pb, 4);
> > >> +                if (tlen > 0x7f) {
> > >
> > >i think that check isnt sufficient to detect that the 2 readings
> > >differ. For example 0xFF would be read as 0xFF by one and 0x7F by
> > >the other and would not trigger this
> > > 
> > 
> > True, although I guess that could be handled by just making this
> > test a >= instead of >.
> 
> that wouldnt work with 0x81
> 

Indeed... I'll modify get_size then, to have something more reliable.

> 
> > 
> > >also maybe len could be used to distingish a subset of cases
> > > 
> > 
> > Not sure I understand what you mean here... The tlen check seems to
> > be enough to me.
> 
> i was thinking of avoiding the seek for cases where one is clearly
> invalid like tlen > len IIRC the variable names
> 
> 

OK, I see what you mean. This is something that can be done in another patch though, as it is not related to the v2.3 vs. v2.4 lengths.

> > 
> > >and there is the problem with seekable=0 streams where seeking
> > >back might fail, not sure what to do in that case ...
> > > 
> > 
> > We could theoretically buffer the data instead of seeking back, as
> > the syncsafe size will always be smaller than the other one. Is
> > this something that we want?
> > I can work on something like that.
> 
> probably that is needed, also see ffio_ensure_seekback()
> 

OK, thanks.

-- 
Ben