[FFmpeg-devel] [PATCH] Google WebP support
Aurelien Jacobs
aurel
Tue Oct 5 21:45:11 CEST 2010
On Tue, Oct 05, 2010 at 10:54:47AM -0700, Pascal Massimino wrote:
> 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
>
> [...]
>
> +static int write_header(AVFormatContext *s)
> +{
> + ByteIOContext *pb = s->pb;
> + WEBPContext *webp = s->priv_data;
> + AVStream *stream;
> + AVCodecContext *codec;
> +
> + if (s->nb_streams != 1)
> + {
> + av_log(s, AV_LOG_ERROR, "muxer only support 1 video stream.");
> + return -1;
> + }
Put the { on the same line as the if(). eg:
if (...) {
...
}
> + stream= s->streams[0];
> + codec= stream->codec;
> + if (codec->codec_type != AVMEDIA_TYPE_VIDEO)
> + return -1;
> + if (codec->codec_id != CODEC_ID_VP8)
> + return -1;
You should find an appropriate AVERROR code instead of -1.
> [...]
> +static int write_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> + AVStream *stream= s->streams[pkt->stream_index];
> + AVCodecContext *codec = stream->codec;
> + ByteIOContext *pb = s->pb;
> + uint64_t vp8_start;
> +
> + if (codec->codec_id == CODEC_ID_VP8)
> + {
> + vp8_start= ff_start_tag(pb, "VP8 ");
> + } else
> + {
> + av_log(s, AV_LOG_ERROR, "muxer only supports VP8 codec.");
> + return -1;
> + }
Here you could write
if (codec->codec_id != CODEC_ID_VP8) {
instead, and get ride of the else.
(don't forget to put the { on the same line as the if)
And you could probably probably return an error code more useful than
-1. Maybe AVERROR_NOTSUPP ?
> [...]
> +static void put_metadata(AVFormatContext *s,
> + const char* key, const char tag[4])
> +{
> + ByteIOContext *pb = s->pb;
> + AVMetadataTag *t;
> + uint64_t pos;
> + int len;
> + t = av_metadata_get(s->metadata, key, NULL, 0);
> + if (!t) return;
> + pos= ff_start_tag(pb, tag);
> + len= strlen(t->value) + 1;
Space before the = on those 2 lines.
> [...]
> +AVOutputFormat webp_muxer = {
> + .name = "webp",
> + .long_name = NULL_IF_CONFIG_SMALL("WebP"),
> + .mime_type = "image/webp",
> + .extensions = "webp",
> + .priv_data_size = sizeof(WEBPContext),
> + .audio_codec = CODEC_ID_NONE,
> + .video_codec = CODEC_ID_VP8,
> + .write_header = write_header,
> + .write_packet = write_packet,
> + .write_trailer = write_trailer,
> +};
Please, vertical alignment, as I showed in my example... (and same for
the other file).
> [...]
> +static int read_header(AVFormatContext *s, AVFormatParameters *ap)
> +{
> + AVStream *st;
> + uint32_t riff_size;
> +
> + if (get_le32(s->pb) != AV_RL32("RIFF"))
> + return AVERROR(EINVAL);
> + riff_size= get_le32(s->pb);
> + if (get_le32(s->pb) != AV_RL32("WEBP"))
> + return AVERROR(EINVAL);
> +
> + st = av_new_stream(s, 0);
> + if (!st)
> + return AVERROR(ENOMEM);
> +
> + st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
> + st->codec->codec_tag = MKTAG('V', 'P', '8', ' ');
AV_RL32("VP8 ") ?
Aurel
More information about the ffmpeg-devel
mailing list