Animated WebP: Optimize decoding in case of seeking

Source/core/platform/image-decoders/ImageDecoder.h

Index: Source/core/platform/image-decoders/ImageDecoder.h
diff --git a/Source/core/platform/image-decoders/ImageDecoder.h b/Source/core/platform/image-decoders/ImageDecoder.h
index 3c4cc2dc31ae1c33b46c6102351b02b868203354..0d710e15012c66a04f9dd36fe356cd3ed14fd5f3 100644
--- a/Source/core/platform/image-decoders/ImageDecoder.h
+++ b/Source/core/platform/image-decoders/ImageDecoder.h
@@ -64,6 +64,17 @@ namespace WebCore {
             DisposeOverwritePrevious  // Clear frame to previous framebuffer
                                       // contents
         };
+        // Indicates how transparent pixels of the current frame rectangle are
+        // blended with those of the previous frame buffer.
+        // Notes:
+        // * GIF always uses 'BlendAtopPreviousFrame'.
+        // * WebP also gives 'BlendAtopBgcolor' option. This is useful for cases

[Peter Kasting, 2013/08/22 20:04:36]

Nit: So you never stated clearly in the private email thread whether WebP has an
actual bgcolor concept, or whether "clear to bgcolor" is always equivalent to
"clear to #00000000" or something.  If it's the latter, then you were probably
right that using "bgcolor" here can be misleading, and we should do something
like "Overwrite" or "DoNotBlend" instead.

[urvang (Google), 2013/08/22 23:25:14]

Short answer: WebP does have a background color concept:
https://gerrit.chromium.org/gerrit/gitweb?p=webm/libwebp.git;a=blob;f=doc/web...

Long answer: As noted there, renders like Chrome are allowed to ignore it and
use their own background color, which is 0x00000000.
GIF has a similar background color field (see the "background color index" here:
http://www.w3.org/Graphics/GIF/spec-gif89a.txt) and it is ignored by Chrome as
well.

Given this, which name sounds better?

[Peter Kasting, 2013/08/22 23:39:31]

Uggghhhh, I can't believe you guys put in something whose interpretation is
optional like this.  It should have either been mandated or dropped entirely. 
Its current state just leads to confusion; who is actually going to respect this
field?  What is the encoder going to assume the decode process will do?

I guess in this case you could say "BlendAtopBgcolor" and document it as "by
blending against transparent, i.e. simply overwriting the corresponding pixels"
or some such.  ("Blending against transparent" is chosen because you stuck with
AlphaBlendSource and for parallel structure with the comment above on
DisposeOverwriteBgcolor.)

[urvang (Google), 2013/08/23 00:13:35]

I completely understand your point. We would have liked to make it fixed either
way as well. But then the scenario is exactly same for GIF as well (even though
the spec doesn't mention it): the browsers always chose transparent background,
some image viewers choose transparent backgrounds, while a few viewers show the
background  color GIF specified. And we wanted to cover that particular use case
of GIF in WebP.
Updated comment.

[Peter Kasting, 2013/08/23 00:17:55]

"Behave inconsistently" is a bug, not a use case.  There's no use case for
people to make images intentionally behave differently on different viewers. 
And certainly anyone transcoding GIF to WebP can afford for WebP to lock this
sort of inconsistency into one behavior or the other.  After all, you didn't
preserve other parts of GIF people don't care about like the pixel aspect ratio
field or GIF Text Mode, did you?

Just nuke it from the WebP spec.  If the spec is locked, then document it as
deprecated, say that all viewers MUST display transparent, and note that its
value will never be set.  Seriously, I'm not kidding.  This is a really bad
thing to have put in the spec.

[urvang (Google), 2013/08/23 00:21:32]

Point taken.

The spec is, in fact, locked in terms of 'ANIM' chunk (which 'bgcolor' is a part
of). So, I'll discuss with team and possibly mark it as deprecated.

Thanks for the suggestion.

+        //   where one wants to transform a few opaque pixels of previous frame
+        //   into non-opaque pixels in the current frame.
+        enum AlphaBlendSourceForSubframe {

[Peter Kasting, 2013/08/22 20:04:36]

Nit: Since I was mistaken about thinking this was only necessary for cases where
you had a smaller subframe, and since the enums above both start "Frame", maybe
this should be "FrameCompositionMethod", or  "FrameCompositionSource", or
"FrameAlphaBlendSource".

Bonus points for optionally nuking the word "Frame" out of all three enums,
since they're already in "ImageFrame" and thus "Frame" is implied.

[urvang (Google), 2013/08/22 23:25:14]

Using the name 'AlphaBlendSource'.
Done.

+            BlendAtopPreviousFrame, // Compose the rectangle of current frame by blending it with corresponding pixels in the previous (disposed) framebuffer.

[Peter Kasting, 2013/08/22 20:04:36]

Nit: Put these comments above the enum values (and then wrap to 80 chars since
the rest of the comments here do so):

    enum X {
        // Blah blah
        X1,

        // Note blank line above for easier scanning
        X2,
    }

Also, maybe the comments should say "...with the corresponding pixels in the
initial buffer state (i.e. any previous frame buffer after having been properly
disposed)." or something (the current comment is a little confusing).

[urvang (Google), 2013/08/22 23:25:14]

Done.

+            BlendAtopBgcolor, // Compose the rectangle of current frame by over-writing the corresponding pixels in the previous (disposed) framebuffer.
+        };
         typedef uint32_t PixelData;
 
         ImageFrame();
@@ -114,6 +125,7 @@ namespace WebCore {
         FrameStatus status() const { return m_status; }
         unsigned duration() const { return m_duration; }
         FrameDisposalMethod disposalMethod() const { return m_disposalMethod; }
+        AlphaBlendSourceForSubframe alphaBlendSource() const { return m_alphaBlendSource; }
         bool premultiplyAlpha() const { return m_premultiplyAlpha; }
         SkBitmap::Allocator* allocator() const { return m_allocator; }
         const SkBitmap& getSkBitmap() const { return m_bitmap->bitmap(); }
@@ -131,6 +143,7 @@ namespace WebCore {
         void setStatus(FrameStatus status);
         void setDuration(unsigned duration) { m_duration = duration; }
         void setDisposalMethod(FrameDisposalMethod method) { m_disposalMethod = method; }
+        void setAlphaBlendSource(AlphaBlendSourceForSubframe source) { m_alphaBlendSource = source; }
         void setPremultiplyAlpha(bool premultiplyAlpha) { m_premultiplyAlpha = premultiplyAlpha; }
         void setMemoryAllocator(SkBitmap::Allocator* allocator) { m_allocator = allocator; }
         void setSkBitmap(const SkBitmap& bitmap) { m_bitmap = NativeImageSkia::create(bitmap); }
@@ -199,6 +212,7 @@ namespace WebCore {
         FrameStatus m_status;
         unsigned m_duration;
         FrameDisposalMethod m_disposalMethod;
+        AlphaBlendSourceForSubframe m_alphaBlendSource;
         bool m_premultiplyAlpha;
 
         // The frame that must be decoded before this frame can be decoded.
@@ -383,14 +397,16 @@ namespace WebCore {
             if (m_frameBufferCache.isEmpty()) {
                 m_frameBufferCache.resize(1);
                 m_frameBufferCache[0].setRequiredPreviousFrameIndex(
-                    findRequiredPreviousFrame(0));
+                    findRequiredPreviousFrame(0, false));
             }
             m_frameBufferCache[0].setMemoryAllocator(allocator);
         }
 
     protected:
         // Calculates the most recent frame whose image data may be needed in
-        // order to decode frame |frameIndex|, based on frame disposal methods.
+        // order to decode frame |frameIndex|, based on frame disposal methods
+        // and 'frameRectIsOpaque', where 'frameRectIsOpaque' signifies whether

[Peter Kasting, 2013/08/22 20:04:36]

Nit: Use || instead of '' to denote variable names

[urvang (Google), 2013/08/22 23:25:14]

Done.

+        // the rectangle of frame at 'frameIndex' is known to be opaque.
         // If no previous frame's data is required, returns WTF::notFound.
         //
         // This function requires that the previous frame's
@@ -404,7 +420,7 @@ namespace WebCore {
         // Image formats which do not use more than one frame do not need to
         // worry about this; see comments on
         // ImageFrame::m_requiredPreviousFrameIndex.
-        size_t findRequiredPreviousFrame(size_t frameIndex);
+        size_t findRequiredPreviousFrame(size_t frameIndex, bool frameRectIsOpaque);
 
         virtual void clearFrameBuffer(size_t frameIndex);