[FFmpeg-devel] [PATCH] Google WebP support

[Michael Niedermayer]

On Sat, Oct 09, 2010 at 09:10:35PM -0700, Pascal Massimino wrote:
> 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?

image formats are implemented over libavformat/img2.c
if you dont want to use this id like to first hear why it cant be used


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- 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/20101010/90aa9599/attachment.pgp>