[FFmpeg-devel] [PATCH] Google WebP support

[Pascal Massimino]

Hi,

On Thu, Oct 7, 2010 at 2:25 PM, Pascal Massimino <pascal.massimino at gmail.com
> wrote:

> 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
>

any more suggestions? Good to go?

skal


>
> skal
>
>