[FFmpeg-devel] [PATCH 2/4] id3v2: merge TYER/TDAT/TIME to date tag

[Anton Khirnov]

On Sun, Oct 31, 2010 at 06:39:58PM +0100, Reimar D?ffinger wrote:
> On Sun, Oct 31, 2010 at 06:11:59PM +0100, Anton Khirnov wrote:
> > +static int is_number(const char *p)
> > +{
> > +    while (*p)
> > +        if (!isdigit(*p++))
> 
> Is digit depends on locale, are you sure this correct in this context?
> 
fixed
> > +static void merge_date(AVMetadata **m)
> > +{
> > +    const char    *keys[]  = {"TYER", "TDAT", "TIME"};
> 
> Why not "static const"? gcc usually will convert it on its own,
> but you risk it being pedantic and copying the data onto the stack
> pointlessly.
> Also storing pointers instead of the strings directly cost space and
> performance.
> 
fixed
> > +    AVMetadataTag *tags[3] = {0};
> 
> I think it is preferable to use NULL instead of 0 for pointers.
> Also you could just write all elements in this case.
> 
fixed
> > +    for (i = 0; i < sizeof(keys); i++) {
> > +        AVMetadataTag *t = av_metadata_get(*m, keys[i], NULL, AV_METADATA_MATCH_CASE);
> > +        if (!t || strlen(t->value) != 4 || !is_number(t->value))
> > +            break;
> > +        tags[i] = t;
> > +    }
> > +
> > +    if      (tags[2]) len = sizeof("YYYY-MM-DD HH:MM");
> > +    else if (tags[1]) len = sizeof("YYYY-MM-DD");
> > +    else if (tags[0]) len = sizeof("YYYY");
> > +    else return;
> > +
> > +    if (!(date = av_malloc(len)))
> > +        return;
> > +    snprintf(date, len, "%.4s-%.2s-%.2s %.2s:%.2s",
> > +             tags[0] ? tags[0]->value     : "",         // year
> > +             tags[1] ? tags[1]->value + 2 : "",         // month
> > +             tags[1] ? tags[1]->value     : "",         // day
> > +             tags[2] ? tags[2]->value     : "",         // hour
> > +             tags[2] ? tags[2]->value + 2 : "");        // minute
> 
> I suspect this could profit hugely from some restructuring, but at the very
> least I'd suggest adding some variables and doing
> year = tags[0] ? tags[0]->value : "";
> ....
> 
i fail to see what that would accomplish other than making it longer
> > +    for (i = 0; i < sizeof(keys); i++)
> 
> Here and above: certainly not, FF_ARRAY_ELEMS or what it is called could
> be used.
fixed

-- 
Anton Khirnov
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-id3v2-merge-TYER-TDAT-TIME-to-date-tag.patch
Type: text/x-diff
Size: 2450 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101031/a2034ba0/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101031/a2034ba0/attachment.pgp>