[FFmpeg-devel] [PATCH] Google WebP support
Pascal Massimino
pascal.massimino
Tue Oct 5 22:49:06 CEST 2010
Aurelien,
another iteration:
On Tue, Oct 5, 2010 at 12:45 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
> 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 (...) {
> ...
> }
>
done
>
> > + 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.
>
>
indeed, fixed a few of these 'return -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)
>
>
done
> 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.
>
>
changed everywhere
> > [...]
> > +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).
>
>
all done
> > [...]
> > +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 ") ?
done (but one need to keep the MKTAG in the static initializer though...)
see attached revision
skal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch_webp_ffmpeg.2.diff
Type: text/x-patch
Size: 9215 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101005/e70ee007/attachment.bin>
More information about the ffmpeg-devel
mailing list