[FFmpeg-devel] [PATCH] avcodec[/format]/webpenc: use WebPAnimEncoder API to generate animated WebP
Urvang Joshi
urvang at google.com
Fri May 22 19:28:06 CEST 2015
On Thu, May 21, 2015 at 6:48 PM Michael Niedermayer <michaelni at gmx.at>
wrote:
> On Wed, May 20, 2015 at 01:54:59AM +0000, Urvang Joshi wrote:
> > On Thu, May 14, 2015 at 7:21 PM Michael Niedermayer <michaelni at gmx.at>
> > wrote:
> >
> > > On Fri, May 15, 2015 at 01:29:19AM +0000, Urvang Joshi wrote:
> > > > On Fri, May 8, 2015 at 8:22 AM Michael Niedermayer <michaelni at gmx.at
> >
> > > wrote:
> > > >
> > > > > On Thu, May 07, 2015 at 09:28:43PM +0000, Urvang Joshi wrote:
> > > > > > On Thu, Apr 30, 2015 at 12:05 PM Michael Niedermayer <
> > > michaelni at gmx.at>
> > > > > > wrote:
> > > > > >
> > > > > > > On Wed, Apr 29, 2015 at 11:53:40PM +0000, Urvang Joshi wrote:
> > > > > > > > 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).
> > > > > > >
> > > > > > > lets try to agree on terminology first:
> > > > > > > Code which compresses that rectangular area of pixels called
> > > > > > > a picture or frame into a compressed bitstream is called an
> encoder
> > > > > > > deciding how to encode a frame, frame area or pixel is all
> encoder
> > > > > > > side.
> > > > > > > In that light comparing "the native muxer" against AnimEncoder
> API
> > > > > > > which does all kinds of smart encoder steps is very odd thats
> like
> > > > > > > comparing debian against the linux kernel aka it makes no
> sense at
> > > all
> > > > > > >
> > > > > >
> > > > > > Sorry if this wasn't clear.
> > > > > > To be precise, I was comparing "WebPEncode + Native muxer" to
> > > > > "AnimEncoder"
> > > > > > (which is nothing but WebPEncode + libwebpmux).
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > 2nd our encoder supports reusing pixels from the previous
> frame.
> > > > > > > AnimEncoder might be better at it or it might be worse but its
> not
> > > a
> > > > > > > feature that isnt already supported
> > > > > > > the feature is tuned by the cr_threshold / cr_size options as i
> > > > > > > said previously.
> > > > > > >
> > > > > > > now if AnimEncoder is better than what we have ATM, i _do_
> want it
> > > > > > > supported but do NOT add this code with #ifdefs in the existing
> > > > > > > encoder.
> > > > > > > create a new file, and move the code into it.
> > > > > >
> > > > > >
> > > > > > I can investigate how feasible the splitting is. My concerns with
> > > > > splitting
> > > > > > into two files are:
> > > > >
> > > > > > - There is some common code which will be duplicated in the two
> > > files.
> > > > >
> > > > > common parts could be put in a common file and/or shared
> > > > >
> > > >
> > > > Just split the two encoders as well as muxers in separate files (with
> > > > common code in "_common.[hc]" files).
> > > >
> > > > Please take another look.
> > >
> > > its better but you are ignoring several things that where said
> > >
> > >
> > > [...]
> > >
> > > > Changelog | 1
> > > > configure | 5
> > > > libavcodec/Makefile | 3
> > > > libavcodec/libwebpenc.c | 281
> > > +-----------------------------------
> > > > libavcodec/libwebpenc_animencoder.c | 146 ++++++++++++++++++
> > > > libavcodec/libwebpenc_common.c | 261
> > > +++++++++++++++++++++++++++++++++
> > > > libavcodec/libwebpenc_common.h | 98 ++++++++++++
> > > > libavformat/Makefile | 3
> > > > libavformat/webpenc.c | 58 ++-----
> > > > libavformat/webpenc_animencoder.c | 68 ++++++++
> > > > libavformat/webpenc_common.c | 38 ++++
> > > > libavformat/webpenc_common.h | 61 +++++++
> > > > 12 files changed, 710 insertions(+), 313 deletions(-)
> > > > 210cc12f7bd486f8ca4e0ae3a799b27acfc0369f
> > > split_files.ffmpeg_animated_webp.patch
> > > > From 745a2735420271b287a0ed3a1d58b06d1bc36749 Mon Sep 17 00:00:00
> 2001
> > > > From: Urvang Joshi <urvang at google.com>
> > > > Date: Thu, 16 Apr 2015 15:21:59 -0700
> > > > Subject: [PATCH] WebP encoder and muxer: use WebPAnimEncoder API when
> > > > available.
> > >
> > > The moving of code needs to be split out into a seperate patch
> > >
> >
> > Good point! I'll separate out this patch into the following patches:
> > 1. Supporting animated WebP bitstream in on muxer side.
> > 2. Moving of some methods from libavcodec/libwebpenc.c to
> > 'libwebpenc_common.[hc]'
> > and
> > 3. Using WebPAnimEncoder API when available on codec side.
> >
> > (To be applied in that order).
> >
> > Sending first two patches on separate threads, and 3rd patch is attached
> > here.
> >
> >
> > > the changes to libavformat need to be put into a patch seperate
> > > of the libavcodec changes
> >
> >
> > > [...]
> > >
> > > > @@ -782,7 +782,8 @@ OBJS-$(CONFIG_LIBVPX_VP8_ENCODER) +=
> > > libvpxenc.o
> > > > OBJS-$(CONFIG_LIBVPX_VP9_DECODER) += libvpxdec.o libvpx.o
> > > > OBJS-$(CONFIG_LIBVPX_VP9_ENCODER) += libvpxenc.o libvpx.o
> > > > OBJS-$(CONFIG_LIBWAVPACK_ENCODER) += libwavpackenc.o
> > > > -OBJS-$(CONFIG_LIBWEBP_ENCODER) += libwebpenc.o
> > >
> > > > +OBJS-$(CONFIG_LIBWEBP_ENCODER) += libwebpenc_common.o
> > > > +OBJS-$(CONFIG_LIBWEBP_ENCODER) += libwebpenc.o
> > > libwebpenc_animencoder.o
> > >
> > > each encoder should use a seperate CONFIG_ identifer
> > >
> >
> > Done. Pls take another look at this part.
> >
> >
> > >
> > >
> > > > OBJS-$(CONFIG_LIBX264_ENCODER) += libx264.o
> > > > OBJS-$(CONFIG_LIBX265_ENCODER) += libx265.o
> > > > OBJS-$(CONFIG_LIBXAVS_ENCODER) += libxavs.o
> > > > diff --git a/libavcodec/libwebpenc.c b/libavcodec/libwebpenc.c
> > > > index 95d56ac..5bfceb9 100644
> > > > --- a/libavcodec/libwebpenc.c
> > > > +++ b/libavcodec/libwebpenc.c
> > > > @@ -21,254 +21,31 @@
> > > >
> > > > /**
> > > > * @file
> > > > - * WebP encoder using libwebp
> > > > + * WebP encoder using libwebp (WebPEncode API)
> > > > */
> > > >
> > > > -#include <webp/encode.h>
> > > > +#include "libwebpenc_common.h"
> > > >
> > > > -#include "libavutil/common.h"
> > > > -#include "libavutil/frame.h"
> > > > -#include "libavutil/imgutils.h"
> > > > -#include "libavutil/opt.h"
> > > > -#include "avcodec.h"
> > > > -#include "internal.h"
> > > > +#ifndef USE_WEBP_ANIMENCODER
> > > [...]
> > > > +
> > > > +#endif // USE_WEBP_ANIMENCODER
> > > > diff --git a/libavcodec/libwebpenc_animencoder.c
> > > b/libavcodec/libwebpenc_animencoder.c
> > >
> > > conditional compilation of files should be done at the
> > > Makefile/configure level not by wraping the whole file in a ifdef
> > >
> >
> > Done. I'm now letting the registration order of codecs determine which of
> > the two will be picked.
> >
> >
> > >
> > >
> > > [...]
> > > > diff --git a/libavformat/webpenc.c b/libavformat/webpenc.c
> > > > index ee110de..29f718e 100644
> > > > --- a/libavformat/webpenc.c
> > > > +++ b/libavformat/webpenc.c
> > > > @@ -1,5 +1,5 @@
> > > > /*
> > > > - * webp muxer
> > > > + * webp muxer (native)
> > > > * Copyright (c) 2014 Michael Niedermayer
> > > > *
> > > > * This file is part of FFmpeg.
> > > > @@ -19,36 +19,24 @@
> > > > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > > 02110-1301 USA
> > > > */
> > > >
> > > > -#include "libavutil/intreadwrite.h"
> > > > -#include "libavutil/opt.h"
> > > > -#include "avformat.h"
> > > > -#include "internal.h"
> > > > +#include "webpenc_common.h"
> > > > +
> > > > +#ifndef USE_WEBP_ANIMENCODER
> > > [...]
> > > > +#endif // USE_WEBP_ANIMENCODER
> > > > diff --git a/libavformat/webpenc_animencoder.c
> > > b/libavformat/webpenc_animencoder.c
> > > > new file mode 100644
> > > > index 0000000..a3cf856
> > > > --- /dev/null
> > > > +++ b/libavformat/webpenc_animencoder.c
> > > > @@ -0,0 +1,68 @@
> > > > +/*
> > > > + * webp muxer using libwebp (AnimEncoder API)
> > > > + *
> > > > + * Copyright (c) 2014 Michael Niedermayer
> > > > + *
> > > > + * This file is part of FFmpeg.
> > > > + *
> > > > + * FFmpeg is free software; you can redistribute it and/or
> > > > + * modify it under the terms of the GNU Lesser General Public
> > > > + * License as published by the Free Software Foundation; either
> > > > + * version 2.1 of the License, or (at your option) any later
> version.
> > > > + *
> > > > + * FFmpeg is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > > + * Lesser General Public License for more details.
> > > > + *
> > > > + * You should have received a copy of the GNU Lesser General Public
> > > > + * License along with FFmpeg; if not, write to the Free Software
> > > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > > 02110-1301 USA
> > > > + */
> > > > +
> > > > +#include "webpenc_common.h"
> > > > +
> > > > +#ifdef USE_WEBP_ANIMENCODER
> > > [...]
> > > > +#endif // USE_WEBP_ANIMENCODER
> > > > diff --git a/libavformat/webpenc_common.c
> b/libavformat/webpenc_common.c
> > >
> > > the code in libavformat needs to identify its input based on the data
> > > it receives not on the environment during build
> > >
> >
> > Done. PTAL.
> >
> >
> > >
> > > [...]
> > >
> > > --
> > > 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
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel at ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
>
> > Changelog | 1
> > configure | 9 +-
> > libavcodec/Makefile | 1
> > libavcodec/allcodecs.c | 1
> > libavcodec/libwebpenc_animencoder.c | 146
> ++++++++++++++++++++++++++++++++++++
> > 5 files changed, 157 insertions(+), 1 deletion(-)
> > 5b970a06f1685006bd7fe9d255ed137327940bf6
> 3.WebP-encoder-use-WebPAnimEncoder-API-when-available.patch
> > From 3f2231bf554f23962d4590665b4318062685c2d9 Mon Sep 17 00:00:00 2001
> > From: Urvang Joshi <urvang at google.com>
> > Date: Tue, 19 May 2015 18:04:07 -0700
> > Subject: [PATCH] WebP encoder: use WebPAnimEncoder API when available.
> >
> > WebPAnimEncoder API is a combination of encoder (WebPEncoder) and muxer
> > (WebPMux). It performs several optimizations to make it more efficient
> > than the combination of WebPEncode() and native ffmpeg muxer.
>
> doesnt seem to apply cleanly:
>
> Applying: WebP encoder: use WebPAnimEncoder API when available.
> fatal: sha1 information is lacking or useless (libavcodec/Makefile).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 WebP encoder: use WebPAnimEncoder API when available.
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
>
>
Rebased the patch, so shloud apply now.
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The bravest are surely those who have the clearest vision
> of what is before them, glory and danger alike, and yet
> notwithstanding go out to meet it. -- Thucydides
> _______________________________________________
> 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: 3.WebP-encoder-use-WebPAnimEncoder-API-when-available.patch
Type: text/x-patch
Size: 10163 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150522/9ec0351b/attachment.bin>
More information about the ffmpeg-devel
mailing list