[FFmpeg-devel] [PATCH] avcodec[/format]/webpenc: use WebPAnimEncoder API to generate animated WebP

Urvang Joshi urvang at google.com
Thu Apr 30 01:53:40 CEST 2015


On Mon, Apr 27, 2015 at 5:03 PM Michael Niedermayer <michaelni at gmx.at>
wrote:

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

In theory, here are some of the optimizations that AnimEncoder API (if
available) does to be more efficient than native muxer:
- Pick the best dispose/blend method combination for each frame.
- Based on the dispose/blend method, encode only a sub-rectangle of the
frame that has changed from the previous (disposed) frame.
- If some pixels in current frame match the corresponding pixels in the
previous frame, possibly turn them into transparent pixels (so that they
see through the previous frame's pixel), if that improves compression.
- Optionally, it can also insert keyframes at regular intervals (not used
in this patch; but I plan to add a command-line option to allow this in
future).

In practice, here is some data to compare the new AnimEncoder with ffmpeg's
native muxer --

I took ~7000 animated GIFs off the web and converted them to animated WebP
using the native muxer as well as the libwebpmux (both in "-lossless 1" and
"-lossless 0" modes). Here are the total output filesizes:

Lossless:
Native muxer: 2187508560 bytes
AnimEncoder: 1449909662 bytes
(33.7% saving)

Lossy:
Native muxer: 872878478 bytes
AnimEncoder: 645406034 bytes
(26% saving)

So, there is a clear benefit of using AnimEncoder when it's available.


>
> And assuming we do want that (which iam not sure we do)
> why are there changes in the encoder ?
>

AnimEncoder API used in the patch is a combination of encoder (WebPEncode)
and muxer (WebPMux).

So, when this new API is used:
- In the encoder layer: we use WebPAnimEncoderAdd() instead of WebPEncode().
- The muxer layer: has to work like a raw muxer.

On the other hand, when new API isn't available, the old code is used as it
is:
- In the codec layer: WebPEncode is used to encode each frame
- In the muxer layer:  ffmpeg muxer is used


> and independant of above, changes to codec and muxer layer should be
> seperate patches


As explained above, the changes to the codec and muxer layers are dependent
in this case; so they cannot be split.


> and with clear commit messages
>

Indeed, the commit message should have been clearer and more detailed.
Here's a patch with a re-written commit message.

Thanks,
Urvang


>
>
> >
> > >
> > >
> > > [...]
> > > > 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: no_whitespace.ffmpeg_animated_webp.patch
Type: application/octet-stream
Size: 9647 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150429/58d86de2/attachment.obj>


More information about the ffmpeg-devel mailing list