[FFmpeg-devel] [PATCH] avcodec[/format]/webpenc: use WebPAnimEncoder API to generate animated WebP
Michael Niedermayer
michaelni at gmx.at
Tue Apr 28 02:02:41 CEST 2015
On Mon, Apr 27, 2015 at 10:18:52PM +0000, Urvang Joshi wrote:
> On Thu, Apr 23, 2015 at 2:51 PM Michael Niedermayer <michaelni at gmx.at>
> wrote:
>
> > On Thu, Apr 16, 2015 at 10:40:14PM +0000, Urvang Joshi wrote:
> > > On Thu, Apr 16, 2015 at 3:09 PM James Almer <jamrial at gmail.com> wrote:
> > >
> > > > On 16/04/15 4:18 PM, Urvang Joshi wrote:
> > > > > Hi,
> > > > > Here's the patch without whitespace changes.
> > > > >
> > > > > Thanks,
> > > > > Urvang
> > > >
> > > > This patch doesn't apply cleanly. Looks like something weird with the
> > > > indentation still.
> > > > Was this patch handmade? It says the hash for libwebpenc.c is 95d56ac
> > > > (same as git head),
> > > > but the contents of the patch don't match.
> > > >
> > >
> > > Sorry, I should have mentioned that it was created with
> > > "--ignore-all-space" option, so using the same option when applying the
> > > patch would have worked.
> > >
> > > But to avoid any confusion, here's the re-created patch, that should
> > apply
> > > cleanly with just 'git am'.
> > >
> > >
> > > >
> > > > After fixing the conflicts and compiling the patch seems to work, but
> > the
> > > > resulting
> > > > animated webp files are smaller than those using the native muxer using
> > > > the default
> > > > encoding and muxing settings.
> > > > Is this because the muxing done by libwebpmux is different, or are the
> > > > quality defaults
> > > > changed in any way when using this codepath? If the former then that's
> > > > pretty good, but
> > > > if the latter then it should probably be fixed.
> > > >
> > >
> > > Short answer: muxing done by libwebpmux is different, so it's expected
> > that
> > > it generates smaller WebP files.
> > >
> > > Detailed answer:
> > > The native muxer is naive, and it always uses X offset and Y offset of 0
> > > for all frames. This means the full width x height of all frames are
> > > encoded.
> >
> > > libwebpmux muxer is smart on the other hand: for example, it only encodes
> > > the part of the frame which has changed from previous frame.
> > > This and other optimizations result in smaller WebP files.
> >
> > can you show some PSNR vs filesize information ?
> >
>
> As explained below, we aren't changing the underlying encoder -- only the
> muxer is being changed.
>
>
> >
> >
> > >
> > > Thanks,
> > > Urvang
> > >
> > >
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel at ffmpeg.org
> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> >
> > > configure | 5 ++
> > > libavcodec/libwebpenc.c | 93
> > +++++++++++++++++++++++++++++++++++++++++++-----
> > > libavformat/webpenc.c | 44 ++++++++++++++++++++++
> >
> > why are there changes to libavformat in a patch changing an encoder?
> >
>
> Note that WebPAnimEncoder API used now internally uses WebP encoder and
> WebP muxer.
> Earlier, ffmpeg was using WebP encoder with its native muxer.
>
> So, we are only changing the muxer here, the underlying encoder is still
> the WebPEncoder as earlier.
Ok, so the patch adds many #ifs to both the muxer and
encoder, and there are more changes in the encoder than the muxer
the commit message which is 1 single line only speaks about the encoder
and the patch is only about the muxer.
Did i understand it correctly this time ?
assuming iam not entirely wrong here.
First question is what does the patch actually try to achive ?
replace a native muxer by a new external dependancy ?
if so, why would we want that ?
And assuming we do want that (which iam not sure we do)
why are there changes in the encoder ?
and independant of above, changes to codec and muxer layer should be
seperate patches and with clear commit messages
>
> >
> >
> > [...]
> > > index ee110de..0162eff 100644
> > > --- a/libavformat/webpenc.c
> > > +++ b/libavformat/webpenc.c
> > > @@ -22,12 +22,25 @@
> > > #include "libavutil/intreadwrite.h"
> > > #include "libavutil/opt.h"
> > > #include "avformat.h"
> > > +#include "config.h"
> > > #include "internal.h"
> > >
> > > +#if CONFIG_LIBWEBP
> > > +#include <webp/encode.h>
> > > +#if HAVE_WEBP_MUX_H
> > > +#include <webp/mux.h>
> > > +#if (WEBP_MUX_ABI_VERSION >= 0x0105)
> > > +#define USE_WEBP_ANIMENCODER
> > > +#endif
> > > +#endif
> > > +#endif
> > > +
> > > typedef struct WebpContext{
> > > AVClass *class;
> > > int frame_count;
> > > +#ifndef USE_WEBP_ANIMENCODER
> > > AVPacket last_pkt;
> > > +#endif
> > > int loop;
> > > } WebpContext;
> > >
> > > @@ -44,13 +57,40 @@ static int webp_write_header(AVFormatContext *s)
> > > av_log(s, AV_LOG_ERROR, "Only WebP is supported\n");
> > > return AVERROR(EINVAL);
> > > }
> > > - avpriv_set_pts_info(st, 24, 1, 1000);
> > >
> > > +#ifndef USE_WEBP_ANIMENCODER
> > > + avpriv_set_pts_info(st, 24, 1, 1000);
> > > avio_write(s->pb, "RIFF\0\0\0\0WEBP", 12);
> > > +#endif
> > > +
> > > + return 0;
> > > +}
> >
> > this removes the timebase setting, that looks wrong
> >
>
> Earlier, "AVCodec ff_libwebp_encoder" was NOT using "CODEC_CAP_DELAY"
> capability, so "avpkt->pts" was computed automatically:
> https://www.ffmpeg.org/doxygen/2.5/libavcodec_2utils_8c_source.html#l02123
>
> And the same was used to compute the duration of the current frame:
> http://ffmpeg.org/doxygen/trunk/webpenc_8c_source.html#l00103
>
> But, the new code DOES use "CODEC_CAP_DELAY" capability, and computes the
> timestamp of a frame explicitly as "avctx->time_base.num * frame->pts *
> 1000 / avctx->time_base.den;". So, IIUC, it doesn't need to call
> avpriv_set_pts_info().
>
> Am I missing anything?
You remove avpriv_set_pts_info() from the muxer, i dont see how
your explanation is related to the muxer
I mean there is the codec layer, and the muxer layer and between
them there is a interface made of AVPackets and various fields of
various structs so a change to the codec layer can really not
directly affect the muxer layer or obsolete anything in it.
it would be needed to change the interface, the format and fields by
which webp packets are interchanged. a change at thet level would
need to be documented and it would need to be ensured that it does
not break compatibility with anything
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150428/25d63628/attachment.asc>
More information about the ffmpeg-devel
mailing list