[FFmpeg-devel] [PATCH] Google WebP support

Michael Niedermayer michaelni
Tue Oct 12 23:39:26 CEST 2010


On Mon, Oct 11, 2010 at 11:27:08PM -0700, Pascal Massimino wrote:
> Michael,
> 
> On Mon, Oct 11, 2010 at 4:59 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > On Sun, Oct 10, 2010 at 10:34:56PM -0700, Pascal Massimino wrote:
> > > Michael,
> > >
> > > On Sun, Oct 10, 2010 at 5:40 AM, Michael Niedermayer <michaelni at gmx.at
> > >wrote:
> > >
> > > > 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
> > > >
> > >
> > > In decreasing order of concern:
> > >
> > > * i dont' see the handling / validation of metadata in img2 (which
> > > is the primary goal of wrapping vp8 in RIFF).
> >
> > metadata handling should be added/moved to lavcodec
> >
> >
> > > * how to handle alpha as a separate layer (that is: alpha is present in
> > > another chunk)?
> >
> > img2 passes the file to the decoder, the decoder can decode it to any
> > colorspace
> >
> >
> "the" decoder, assuming there's only one. Will i have to write always
> the same code to handle RIFF+metadata in, say, vp9.c, etc.?

you can put that in a libavcodec/webp.c


> 
> 
> > > * vp8 is not the end of it. How could i tell that several video_codec_id
> > are
> > > possibly found (i'm thinking adding a lossless variant for instance. Like
> > > ffv1 or ffv2 if it ever surfaces somewhere...)
> >
> > ffv1 is already supported in img2
> >
> >
> as a standalone format. Not something that would be a compression option
> of another one.
>
> 
> 
> > also if google designed a image format that cant be handled reasonable in
> > img2
> > while everything from png, jpeg, jpeg2000, ffv1, lossless jpeg, jpeg-ls,
> > tiff,
> > raw, bmp, pcx, and even mpeg1 to mpeg4 and h264 can be handled in img2
> > then honestly i think google made a mistake in that design somewhere
> >
> > The idea is that images are frames, a muxer is something wraping
> > such frames of several streams with surrounding structure to create a
> > multimedia file.
> >
> 
> my idea in this case was rather that a muxer combines several streams of
> information
> into a single transportable entity: keyframe bits + geo tag + aperture info
> + title = one image
> where every piece of info can be accessed separately.

google should have read http://www.exif.org/Exif2-2.PDF before designing webp
its how extra tags have been added to jpeg and it is alot more sane.

Thats just besides that geo tags and aperture you mention are frame properties
and not file properties, if you have a video they can change per image it thus
makes alot sense that they are added at a level where they can be stored
per frame in videos. adding them to the codec layer would have ensured that.


> 
> 
> > If for image files you need things that havnt been in the frames of videos
> > then the addition should be at the video codec level not the muxer level
> > because there is no need for a muxer level in images.
> > Thats just besides one might wish to store several such images in a video
> > aka
> > a slideshow and might wish to preserve that extra information, at the last
> > here
> > it should be obvious that any such extra information is frame not muxer
> > information.
> >
> > as is i guess webp support will require RIFF support in libavcodec/vp8
> >
> 
> no, not necessarily, hopefully. It would be like asking LZW to understand
> PNG headers for compression level >= 1.

that would be 3 steps worse than webp design


> 
> 
> >
> > there may be other ways to do it but this one seems simplest, other
> > suggestions
> > are welcome but numbered image support, per image metadata and alpha
> > obviously
> > should be possible in the design.
> >
> 
> really, i don't see what img2.c is bringing as value. Since i still will
> have to do the code
> for RIFF / metadata handling (somewhere else. Possibly multiple times.). And
> the

> use-case assumed isn't there. I don't want ./ffmpeg -i *.webp -y toto.avi to
> work

thats ok, but i want numbered images to work. And if your implementation cannot
handle that or more correctly noone knows how to even add it on top of that
implementation while a img2 based one could handle it, then there is a problem
in your implementation.

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20101012/6e0b7b16/attachment.pgp>



More information about the ffmpeg-devel mailing list