[FFmpeg-devel] [PATCH] Google WebP support

[Pascal Massimino]

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


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


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


>
> 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
 (albeit discarding metadata). I'd just want './ffmpeg -i exif.jpg -y
foo.webp' to work flat out.

skal


>
>
> >
> > I'm not saying i can do that with a separate muxer/demuxer (yet), but i
> see
> > img2.c as being an even harder way. No?
>
>
>