[FFmpeg-devel] [PATCH] Google WebP support

[Pascal Massimino]

Aurelien,

On Sun, Oct 3, 2010 at 1:32 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:

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


thanks, all done hopefully in $attached

skal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_webp_ffmpeg.diff
Type: text/x-patch
Size: 9051 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101005/629d2bc7/attachment.bin>