[FFmpeg-devel] [PATCH] Google WebP support

[Pascal Massimino]

Hi Aurelien,

On Thu, Oct 7, 2010 at 2:38 AM, Aurelien Jacobs <aurel at gnuage.org> wrote:

> 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.
>
>
all good suggestions! See $attached

skal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_webp_ffmpeg.4.diff
Type: application/octet-stream
Size: 11408 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101007/e89ceff9/attachment.obj>