Pull up initFrameBuffer to ImageDecoder.

Pull up initFrameBuffer to ImageDecoder.

Just a refactor. No change in behavior, no new tests.

Pull up the initFrameBuffer method from WEBPImageDecoder and GIFImageDecoder to
ImageDecoder. The condition whether the previous frame's buffer could be taken
instead of copied is extracted into a virtual method. Both decoders have some
format-specific code, which is retained in the override method.

Committed: https://crrev.com/1b922661248b6684779a874883c5bf30d73bf692
Cr-Commit-Position: refs/heads/master@{#433282}

[joostouwerling, 2016-11-13 20:47:26]

Description was changed from

==========
Pull up initFrameBuffer to ImageDecoder.

Pull up the initFrameBuffer method from WEBPImageDecoder and GIFImageDecoder to
ImageDecoder. The condition whether the previous frame's buffer could be taken
instead of copied is extracted into a virtual method. Both decoders have some
format-specific code, which is retained in the override method.
==========

to

==========
Pull up initFrameBuffer to ImageDecoder.

Just a refactor. No change in behavior, no new tests.

Pull up the initFrameBuffer method from WEBPImageDecoder and GIFImageDecoder to
ImageDecoder. The condition whether the previous frame's buffer could be taken
instead of copied is extracted into a virtual method. Both decoders have some
format-specific code, which is retained in the override method.
==========

[joostouwerling, 2016-11-13 20:54:50]

joostouwerling@google.com changed reviewers:
+ scroggo@chromium.org, urvang@chromium.org

[scroggo_chromium, 2016-11-14 13:51:46]

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:307: // the
callee must verify that the frame exists.
caller*

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:312: virtual
bool canReusePreviousFrameBuffer(size_t) const { return false; }
This one doesn't actually need to be virtual. We never want to reuse for
DisposeOverwritePrevious or ImageFrame::BlendAtopPreviousFrame; it just so
happens that GIF doesn't use one, and WEBP doesn't use the other.

Once it's no longer virtual, it can be written in line or as a static method in
ImageDecoder.cpp

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:365:
if (!ImageDecoder::initFrameBuffer(frameIndex))
Since all of the overrides first call ImageDecoder::initFrameBuffer, you could
instead make a separate virtual method:

virtual void ImageDecoder::onInitFrameBuffer(size_t index)
{
  // empty. subclasses will do alpha stuff.
}

(non-virtual) bool ImageDecoder::initFrameBuffer(size_t index)
{
  // ... Same thing it does now
  // possibly return false in here somewhere

  onInitFrameBuffer(size_t frameIndex);
  return true;
}

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
(right):

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:86: //
for this frame. Before calling this method, the callee must verify that
caller*

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:93: //
callee must verify that the frame exists.
caller*

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h
(right):

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h:76: //
calling this method, the callee must verify that the frame exists.
caller*

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h:82: //
method, the callee must verify that the frame exists.
caller*

[joostouwerling, 2016-11-14 16:25:10]

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:307: // the
callee must verify that the frame exists.
On 2016/11/14 13:51:45, scroggo_chromium wrote:
> caller*

Done.

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:312: virtual
bool canReusePreviousFrameBuffer(size_t) const { return false; }
On 2016/11/14 13:51:45, scroggo_chromium wrote:
> This one doesn't actually need to be virtual. We never want to reuse for
> DisposeOverwritePrevious or ImageFrame::BlendAtopPreviousFrame; it just so
> happens that GIF doesn't use one, and WEBP doesn't use the other.
> 
> Once it's no longer virtual, it can be written in line or as a static method
in
> ImageDecoder.cpp

For only GIF and WebP, this would be fine, but for APNG we can reuse
BlendAtopPreviousFrame. So with that it in mind, I think it is better to keep it
like this. 

I made it private though, since there is no need for subclasses to call this
method.

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:365:
if (!ImageDecoder::initFrameBuffer(frameIndex))
On 2016/11/14 13:51:46, scroggo_chromium wrote:
> Since all of the overrides first call ImageDecoder::initFrameBuffer, you could
> instead make a separate virtual method:
> 
> virtual void ImageDecoder::onInitFrameBuffer(size_t index)
> {
>   // empty. subclasses will do alpha stuff.
> }
> 
> (non-virtual) bool ImageDecoder::initFrameBuffer(size_t index)
> {
>   // ... Same thing it does now
>   // possibly return false in here somewhere
> 
>   onInitFrameBuffer(size_t frameIndex);
>   return true;
> }

Good suggestion! Done.

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
(right):

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:86: //
for this frame. Before calling this method, the callee must verify that
On 2016/11/14 13:51:46, scroggo_chromium wrote:
> caller*

Done.

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:93: //
callee must verify that the frame exists.
On 2016/11/14 13:51:46, scroggo_chromium wrote:
> caller*

Done.

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h
(right):

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h:76: //
calling this method, the callee must verify that the frame exists.
On 2016/11/14 13:51:46, scroggo_chromium wrote:
> caller*

Done.

https://codereview.chromium.org/2495183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h:82: //
method, the callee must verify that the frame exists.
On 2016/11/14 13:51:46, scroggo_chromium wrote:
> caller*

Done.

[scroggo_chromium, 2016-11-14 16:44:12]

lgtm

https://codereview.chromium.org/2495183002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/2495183002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:356: virtual
bool canReusePreviousFrameBuffer(size_t) const { return false; }
I think this will only get called by WEBP, GIF, and PNG (once you add it), all
of which will override this. So a default implementation may not matter (we need
to pick one, since the others won't implement it), but my expectation would be
that the default returns true. (I guess false is safer, in some sense, so maybe
this is better as is.)

https://codereview.chromium.org/2495183002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/2495183002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:364:
// ImageDecoder::initFrameBuffer does a DCHECK if |frameIndex| exists.
This comment is no longer necessary.

[joostouwerling, 2016-11-14 16:54:13]

https://codereview.chromium.org/2495183002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/2495183002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:356: virtual
bool canReusePreviousFrameBuffer(size_t) const { return false; }
On 2016/11/14 16:44:12, scroggo_chromium wrote:
> I think this will only get called by WEBP, GIF, and PNG (once you add it), all
> of which will override this. So a default implementation may not matter (we
need
> to pick one, since the others won't implement it), but my expectation would be
> that the default returns true. (I guess false is safer, in some sense, so
maybe
> this is better as is.)

I picked false for the last reason you mentioned. We could pick the GIF
condition since it is shared by both GIF and APNG but I think it is better to
explicitly state the condition in the specific decoder, with a comment
explaining why.

https://codereview.chromium.org/2495183002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
(right):

https://codereview.chromium.org/2495183002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:364:
// ImageDecoder::initFrameBuffer does a DCHECK if |frameIndex| exists.
On 2016/11/14 16:44:12, scroggo_chromium wrote:
> This comment is no longer necessary.

Done.

[urvang, 2016-11-14 17:28:42]

lgtm

[joostouwerling, 2016-11-14 17:46:58]

The CQ bit was checked by joostouwerling@google.com

[joostouwerling, 2016-11-14 17:46:59]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org
Link to the patchset: https://codereview.chromium.org/2495183002/#ps80001
(title: "Remove unnecessary comment")

[commit-bot: I haz the power, 2016-11-14 17:47:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 23:45:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 23:45:42]

Failed to apply patch for
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:223
  error: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:
patch does not apply

Patch:       third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
Index: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
index
327db4bc499b48345e2c0633a66afe98dcc50e4a..4454453d584545882376bb974b620c40fbd7d846
100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
@@ -223,6 +223,53 @@ void ImageDecoder::clearFrameBuffer(size_t frameIndex) {
   m_frameBufferCache[frameIndex].clearPixelData();
 }
 
+bool ImageDecoder::initFrameBuffer(size_t frameIndex) {
+  DCHECK(frameIndex < m_frameBufferCache.size());
+
+  ImageFrame* const buffer = &m_frameBufferCache[frameIndex];
+
+  // If the frame is already initialized, return true.
+  if (buffer->getStatus() != ImageFrame::FrameEmpty)
+    return true;
+
+  size_t requiredPreviousFrameIndex = buffer->requiredPreviousFrameIndex();
+  if (requiredPreviousFrameIndex == kNotFound) {
+    // This frame doesn't rely on any previous data.
+    if (!buffer->setSizeAndColorSpace(size().width(), size().height(),
+                                      colorSpace())) {
+      return setFailed();
+    }
+  } else {
+    ImageFrame* const prevBuffer =
+        &m_frameBufferCache[requiredPreviousFrameIndex];
+    DCHECK(prevBuffer->getStatus() == ImageFrame::FrameComplete);
+
+    // We try to reuse |prevBuffer| as starting state to avoid copying.
+    // If canReusePreviousFrameBuffer returns false, we must copy the data
since
+    // |prevBuffer| is necessary to decode this or later frames. In that case,
+    // copy the data instead.
+    if ((!canReusePreviousFrameBuffer(frameIndex) ||
+         !buffer->takeBitmapDataIfWritable(prevBuffer)) &&
+        !buffer->copyBitmapData(*prevBuffer))
+      return setFailed();
+
+    if (prevBuffer->getDisposalMethod() ==
+        ImageFrame::DisposeOverwriteBgcolor) {
+      // We want to clear the previous frame to transparent, without
+      // affecting pixels in the image outside of the frame.
+      const IntRect& prevRect = prevBuffer->originalFrameRect();
+      DCHECK(!prevRect.contains(IntRect(IntPoint(), size())));
+      buffer->zeroFillFrameRect(prevRect);
+    }
+  }
+
+  // Update our status to be partially complete.
+  buffer->setStatus(ImageFrame::FramePartial);
+
+  onInitFrameBuffer(frameIndex);
+  return true;
+}
+
 void ImageDecoder::updateAggressivePurging(size_t index) {
   if (m_purgeAggressively)
     return;

[joostouwerling, 2016-11-15 15:30:16]

The CQ bit was checked by joostouwerling@google.com

[commit-bot: I haz the power, 2016-11-15 15:30:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 15:37:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 15:37:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[joostouwerling, 2016-11-18 16:23:09]

The CQ bit was checked by joostouwerling@google.com

[joostouwerling, 2016-11-18 16:23:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
scroggo@chromium.org, urvang@chromium.org
Link to the patchset: https://codereview.chromium.org/2495183002/#ps100001
(title: "Solved merge conflict with crrev.com/2494363002")

[commit-bot: I haz the power, 2016-11-18 16:23:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 20:15:39]

Description was changed from

==========
Pull up initFrameBuffer to ImageDecoder.

Just a refactor. No change in behavior, no new tests.

Pull up the initFrameBuffer method from WEBPImageDecoder and GIFImageDecoder to
ImageDecoder. The condition whether the previous frame's buffer could be taken
instead of copied is extracted into a virtual method. Both decoders have some
format-specific code, which is retained in the override method.
==========

to

==========
Pull up initFrameBuffer to ImageDecoder.

Just a refactor. No change in behavior, no new tests.

Pull up the initFrameBuffer method from WEBPImageDecoder and GIFImageDecoder to
ImageDecoder. The condition whether the previous frame's buffer could be taken
instead of copied is extracted into a virtual method. Both decoders have some
format-specific code, which is retained in the override method.
==========

[commit-bot: I haz the power, 2016-11-18 20:15:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-18 20:23:17]

Description was changed from

==========
Pull up initFrameBuffer to ImageDecoder.

Just a refactor. No change in behavior, no new tests.

Pull up the initFrameBuffer method from WEBPImageDecoder and GIFImageDecoder to
ImageDecoder. The condition whether the previous frame's buffer could be taken
instead of copied is extracted into a virtual method. Both decoders have some
format-specific code, which is retained in the override method.
==========

to

==========
Pull up initFrameBuffer to ImageDecoder.

Just a refactor. No change in behavior, no new tests.

Pull up the initFrameBuffer method from WEBPImageDecoder and GIFImageDecoder to
ImageDecoder. The condition whether the previous frame's buffer could be taken
instead of copied is extracted into a virtual method. Both decoders have some
format-specific code, which is retained in the override method.

Committed: https://crrev.com/1b922661248b6684779a874883c5bf30d73bf692
Cr-Commit-Position: refs/heads/master@{#433282}
==========

[commit-bot: I haz the power, 2016-11-18 20:23:18]

Patchset 6 (id:??) landed as
https://crrev.com/1b922661248b6684779a874883c5bf30d73bf692
Cr-Commit-Position: refs/heads/master@{#433282}