[FFmpeg-devel] [PATCH] Google WebP support
Pascal Massimino
pascal.massimino
Mon Oct 11 07:34:56 CEST 2010
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).
* how to handle alpha as a separate layer (that is: alpha is present in
another chunk)?
* 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...)
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
>
> Let us carefully observe those good qualities wherein our enemies excel us
> and endeavor to excel them, by avoiding what is faulty, and imitating what
> is excellent in them. -- Plutarch
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAkyxtD8ACgkQYR7HhwQLD6trxQCeKJcpGNhNARW6vIoxwIaXkJ7a
> H08AnArF6kR26d+DbXzC+/0QWG81Lyyj
> =Wrnx
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list