On Fri, May 22, 2015 at 05:28:06PM +0000, Urvang Joshi wrote: > On Thu, May 21, 2015 at 6:48 PM Michael Niedermayer <michae...@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 <michae...@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 <michae...@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 < > > > > michae...@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 < > > > > > > michae...@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 < > > > > > > > > michae...@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 < > > > > > > jamr...@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@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 <urv...@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@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 <urv...@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.
applied i think the AVOption table and AVClass name could still be improved to match each encoder more individually, i dont think both support all options Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 1 "Used only once" - "Some unspecified defect prevented a second use" "In good condition" - "Can be repaird by experienced expert" "As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel