[FFmpeg-devel] [PATCH] Google WebP support

[Aurelien Jacobs]

On Sun, Oct 03, 2010 at 02:46:57AM -0700, Pascal Massimino wrote:
> Diego,
> 
> On Sat, Oct 2, 2010 at 8:32 AM, Diego Biurrun <diego at biurrun.de> wrote:
> 
> > On Thu, Sep 30, 2010 at 03:55:11PM -0700, Pascal Massimino wrote:
> > > On Thu, Sep 30, 2010 at 3:53 PM, Pascal Massimino <
> > > pascal.massimino at gmail.com> wrote:
> > >
> > > > On Thu, Sep 30, 2010 at 3:52 PM, Mike Melanson <mike at multimedia.cx>
> > wrote:
> > > >
> > > >> Google released a new image format called WebP. It's basically a VP8
> > > >> golden frame wrapped with a 20-byte header. Attached is a first pass
> > at
> > > >> support. Here is a ready-made sample:
> > > >>
> > > >> http://samples.mplayerhq.hu/image-samples/webp/
> > > >>
> > > > what about  http://pastebin.com/FbUvG77M
> > > >
> > > attached, too. That's easier
> > >
> > > --- libavformat/Makefile      (revision 25217)
> > > +++ libavformat/Makefile      (working copy)
> > > @@ -277,6 +277,7 @@
> > > +OBJS-$(CONFIG_WEBP_MUXER)                += webp.o
> >
> > Since this is compiled conditionally...
> >
> > > --- libavformat/webp.c        (revision 0)
> > > +++ libavformat/webp.c        (revision 0)
> > > @@ -0,0 +1,217 @@
> > > +
> > > +#include "avformat.h"
> > > +#include "riff.h"
> > > +
> > > +#ifdef CONFIG_WEBP_MUXER
> >
> > ...this #ifdef is unnecessary.
> >
> > > +    if (s->nb_streams != 1)
> >
> > if (...) {
> >
> > same below, more nits for you to notice on your own ;)
> >
> > > +#endif  /* CONFIG_WEBP_MUXER */
> > > +
> > > +#if CONFIG_WEBP_DEMUXER
> >
> > Hmm, it seems that the Makefile entry is incomplete..
> >
> 
>  should be fixed in attached patch...
> 
> skal

> Index: libavformat/Makefile
> ===================================================================
> --- libavformat/Makefile	(revision 25320)
> +++ libavformat/Makefile	(working copy)
> @@ -278,6 +278,8 @@
>  OBJS-$(CONFIG_WEBM_MUXER)                += matroskaenc.o matroska.o \
>                                              riff.o isom.o avc.o \
>                                              flacenc_header.o
> +OBJS-$(CONFIG_WEBP_MUXER)                += webp.o
> +OBJS-$(CONFIG_WEBP_DEMUXER)              += webp.o

You should split muxer and demuxer in 2 files (webpenc.c and webpdec.c).
They don't even share any code...
This will also avoid the #if CONFIG_WEBP_MUXER and so on.

> +static int webp_write_trailer(AVFormatContext *s)
> +{
> +    ByteIOContext *pb = s->pb;
> +    WEBPContext *webp = s->priv_data;
> +    put_metadata(s, "artist", "IART");
> +    put_metadata(s, "copyright", "ICOP");
> +    put_metadata(s, "title", "INAM");
> +    put_metadata(s, "comment", "ICMT");

This could use some alignment.

> +AVOutputFormat webp_muxer = {
> +    "webp",
> +    NULL_IF_CONFIG_SMALL("WebP"),
> +    "image/webp",
> +    "webp",
> +    sizeof(WEBPContext),
> +    CODEC_ID_NONE,
> +    CODEC_ID_VP8,
> +    webp_write_header,
> +    webp_write_packet,
> +    webp_write_trailer,
> +};

Please use named initializer in new code:
  .name      = "webp",
  .long_name = NULL_IF_CONFIG_SMALL("WebP"),
  ...

> +static int read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> +    AVStream *st;
> +    uint32_t riff_size;
> +
> +    if (get_le32(s->pb) != MKTAG('R', 'I', 'F', 'F'))

Here and in many other places, you could use AV_RL32("RIFF") for better
readability and searchability.

> +AVInputFormat webp_demuxer = {
> +    "webp",
> +    NULL_IF_CONFIG_SMALL("WebP"),
> +    0,
> +    probe,
> +    read_header,
> +    read_packet,
> +    .flags= AVFMT_GENERIC_INDEX,
> +    .extensions = "webp",
> +    .value = CODEC_ID_VP8,
> +    .codec_tag = (const AVCodecTag*[]){webp_codec_tags, 0},
> +};

Named initializers please.

Aurel