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

Thu Apr 30 21:05:14 CEST 2015

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

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. we also dont have the
xvid encoder in our mpeg4 encoder with #ifdefs

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

The standard procedure is to use raw RGB or raw YUV input
material
and for comparing lossy compression its needed to draw a quality vs
bitrate graph.
that is one lossy compressor cant be compared to another just by
filesize, the quality matters too

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

like i said above, please put the new codec layer stuff in a new file
dont intermingle it into an existing one with ifdefs

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

iam sure the muxer can read the chunk type to determine based on
that if its feeded with a RIFF webp file or the bitstream of frames
from inside a webp.
And when it can do so it can switch between the 2 muxing modes
so the changes not only can be split but its even trivial to do so

of course this still introduces a different format for the new
encoders AVPackets, i dont know if thats a good idea but its
something the community has to decide thats not for me to do
but if noone complains then i guess noone minds

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.
-------------- 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/20150430/eee6cc86/attachment.asc>