[FFmpeg-devel] [PATCH] Google WebP support
Michael Niedermayer
michaelni
Mon Oct 11 13:59:33 CEST 2010
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
> * 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
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.
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
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.
>
> 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?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/20101011/c8294e6b/attachment.pgp>
More information about the ffmpeg-devel
mailing list