[FFmpeg-devel] [PATCH] Google WebP support

Aurelien Jacobs aurel
Thu Oct 7 11:38:31 CEST 2010


On Wed, Oct 06, 2010 at 07:24:11PM -0700, Pascal Massimino wrote:
> Aurelien, Stefano,
> 
> On Wed, Oct 6, 2010 at 2:01 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
> 
> > On Wed, Oct 06, 2010 at 08:37:23PM +0200, Stefano Sabatini wrote:
> > > On date Tuesday 2010-10-05 13:49:06 -0700, Pascal Massimino encoded:
> > > > Aurelien,
> > > >
> > > > another iteration:
> > > >
> > > > On Tue, Oct 5, 2010 at 12:45 PM, Aurelien Jacobs <aurel at gnuage.org>
> > wrote:
> > > [...]
> > > > Index: libavformat/webpdec.c
> > > > ===================================================================
> > > > --- libavformat/webpdec.c   (revision 0)
> > > > +++ libavformat/webpdec.c   (revision 0)
> > > > @@ -0,0 +1,120 @@
> > > [...]
> > > > +static int read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > +{
> > > > +    AVStream *stream = s->streams[pkt->stream_index];
> > > > +    uint32_t tag = get_le32(s->pb);
> > > > +    uint32_t chunk_size = get_le32(s->pb);
> > > > +    int ret = -1;
> > > > +    if (tag == stream->codec->codec_tag)
> > > > +        ret = av_get_packet(s->pb, pkt, chunk_size);
> > >
> > > > +    else if (tag == AV_RL32("IART"))
> > > > +        ret = get_metadata(s->pb, stream, "artist", chunk_size);
> > > > +    else if (tag == AV_RL32("ICOP"))
> > > > +        ret = get_metadata(s->pb, stream, "copyright", chunk_size);
> > > > +    else if (tag == AV_RL32("INAM"))
> > > > +        ret = get_metadata(s->pb, stream, "title", chunk_size);
> > > > +    else if (tag == AV_RL32("ICMT"))
> > > > +        ret = get_metadata(s->pb, stream, "comment", chunk_size);
> > >
> > > This can be factorized:
> > > ret = get_metadata(s->pb, stream, tag == AV_RL32("IART") ? "artist"    :
> > >                                   tag == AV_RL32("ICOP") ? "copyright" :
> > >                                   ...
> >
> > Actually you shouldn't use "artist", "title" and other generic strings
> > as keys. Instead you should use "IART", "INAM" and others directly, and
> > just set an appropriate conversion table in AVInputFormat.metadata_conv.
> > Same applies to the muxer.
> >
> 
> much nicer indeed! See $attached.

Indeed !

> [...]
> Index: webpdec.c
> ===================================================================
> --- webpdec.c	(revision 0)
> +++ webpdec.c	(revision 0)
> [...]
> +const AVMetadataConv ff_webp_metadata_conv[] = {
> +    { "IART", "artist"},
> +    { "ICOP", "copyright"},
> +    { "INAM", "title"},
> +    { "ICMT", "comment"},
> +    { 0 }
> +};

This must be moved to a shared file (webp.c and its header webp.h) as it
is shared by the muxer and demuxer (and that the compilation of one of
them might be disabled).
Also you may want to add a space before each '}' (but that's really
nitpicking).

> [...]
> +static int read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    int ret = -1;
> +    AVStream *stream = s->streams[pkt->stream_index];
> +    uint32_t tag = get_le32(s->pb);
> +    uint32_t chunk_size = get_le32(s->pb);
> +    if (tag == stream->codec->codec_tag)
> +        ret = av_get_packet(s->pb, pkt, chunk_size);
> +    else {
> +        int i;
> +        for (i = 0; ff_webp_metadata_conv[i].native; ++i) {
> +            const char* metadata_tag = ff_webp_metadata_conv[i].native;
> +            if (tag == AV_RL32(metadata_tag)) {
> +                ret = get_metadata(s->pb, stream, metadata_tag, chunk_size);
> +                break;
> +            }
> +        }
> +    }
> +    if (ret >= 0) {
> +      pkt->flags |= AV_PKT_FLAG_KEY;  /* must be set _after_ av_get_packet() */

It would probably be cleaner to set this right after the av_get_packet()
call, inside the if(). There is no reason to set this when reading a
metadata tag...
Also the comment looks useless to me, but I don't really mind.

Aurel



More information about the ffmpeg-devel mailing list