Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(224)

Issue 13980003: Add animation support for WebP images (Closed)

Created:
7 years, 8 months ago by urvang (Google)
Modified:
7 years, 5 months ago
CC:
blink-reviews, skal
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Add animation support for WebP images Animation support was added to WebP in v0.3.0. This patch adds the same support in Blink. Notes about major methods added to WEBPImageDecoder.h: updateDemuxer(): Re-creates the m_demux object when new data is available. It also updates the demux state, canvas width/height, format flags, animation parameters, repetition count etc as appropriate. Any further reading of image data, color profile etc happens from the m_demux object. initFrameBuffer(): Initializes the frame buffer at the given index based on the previous frame's disposal method, which can be DisposeKeep or DisposeRestoreBgcolor for WebP. It also initializes other properties of the frame such as its duration and its frame rectangle. applyPostProcessing(): (Formerly: 'applyColorProfile') In addition to applying the color profile, it also applies post-processing based on the disposal method in case of animation. clearCacheExceptFrame(): Contain the logic to determine whether the given frame or one needed by it should be preserved. It makes sure to preserve any frames that may be needed by initFrameBuffer() as well as applyPostProcessing() to facilitate the construction of next frames. Changes to ImageDecoder.h: clearCacheExceptFrame(): Modified to simply clear all frames except the given frame (unless frame buffer cache has only one frame). The logic of selecting the frame to retain is moved to derived class methods in GIFImageDecoder and WEBPImageDecoder. zeroFillFrameRect(): Clears the pixels of this ImageFrame in the given frame rectangle to (0, 0, 0, 0) and sets hasAlpha to true. BUG=229641 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153187

Patch Set 1 #

Patch Set 2 : Added unit tests #

Patch Set 3 : Refactored unit tests #

Patch Set 4 : Enclose under runtime flag #

Total comments: 16

Patch Set 5 : #

Total comments: 56

Patch Set 6 : #

Total comments: 52

Patch Set 7 : #

Patch Set 8 : comments + correct assert #

Total comments: 23

Patch Set 9 : some cleanup #

Total comments: 12

Patch Set 10 : ASSERT_WITH_SECURITY_IMPLICATION + comments #

Patch Set 11 : comment #

Patch Set 12 : variable grouping #

Patch Set 13 : Implement frameIsCompleteAtIndex and frameDurationAtIndex #

Patch Set 14 : UpdateDemuxer() also sets frame durations now #

Patch Set 15 : repetitionCount() is LoopOnce in failed state #

Patch Set 16 : 'hasalpha' should always be true while decode is in progress #

Patch Set 17 : simplification #

Total comments: 14

Patch Set 18 : Adapt to recent animation code changes #

Total comments: 2

Patch Set 19 : #

Total comments: 19

Patch Set 20 : initFrame correction + add/remove commeents #

Patch Set 21 : virtual clearCacheExceptFrame(); reword a comment #

Patch Set 22 : Debug fix #

Total comments: 2

Patch Set 23 : Final patch #

Patch Set 24 : Rebase + Handle transparent frame background #

Total comments: 5

Patch Set 25 : Unit tests for opaque frame(s) with transparent background #

Patch Set 26 : const OVERRIDE #

Total comments: 2

Patch Set 27 : Enable layout tests #

Patch Set 28 : Patch for landing #

Total comments: 17

Patch Set 29 : Refactor part of clearCacheExceptFrame #

Patch Set 30 : Create and use zeroFillPixelData() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+594 lines, -323 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +0 lines, -4 lines 0 comments Download
M Source/WebKit/chromium/WebKit.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +1 line, -0 lines 0 comments Download
M Source/WebKit/chromium/WebKitUnitTests.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/platform/image-decoders/ImageDecoder.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 4 chunks +12 lines, -13 lines 0 comments Download
M Source/core/platform/image-decoders/ImageDecoder.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +0 lines, -8 lines 0 comments Download
M Source/core/platform/image-decoders/ImageDecoderTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +0 lines, -28 lines 0 comments Download
M Source/core/platform/image-decoders/ImageFrame.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +13 lines, -0 lines 0 comments Download
M Source/core/platform/image-decoders/gif/GIFImageDecoder.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 3 chunks +15 lines, -6 lines 0 comments Download
M Source/core/platform/image-decoders/webp/WEBPImageDecoder.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +35 lines, -9 lines 0 comments Download
M Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 7 chunks +360 lines, -79 lines 0 comments Download
A + Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 11 chunks +155 lines, -176 lines 0 comments Download

Messages

Total messages: 92 (0 generated)
urvang (Google)
7 years, 8 months ago (2013-04-10 20:14:22 UTC) #1
eseidel
Is this a web-exposed feature which should be announced on blink-dev, per: http://www.chromium.org/blink#new-features
7 years, 8 months ago (2013-04-10 20:17:01 UTC) #2
urvang (Google)
On 2013/04/10 20:17:01, Eric Seidel (Google) wrote: > Is this a web-exposed feature which should ...
7 years, 8 months ago (2013-04-11 00:47:35 UTC) #3
jamesr
I think this counts as web-exposed and we should go through the new feature process. ...
7 years, 8 months ago (2013-04-11 03:10:28 UTC) #4
eseidel
I'm happy to provide my support when the feature email is sent. :) I see ...
7 years, 8 months ago (2013-04-11 03:34:12 UTC) #5
Alpha Left Google
I think layout test is a poor way to test image animations as they are ...
7 years, 8 months ago (2013-04-15 21:18:41 UTC) #6
urvang (Google)
On 2013/04/15 21:18:41, Alpha wrote: > I think layout test is a poor way to ...
7 years, 8 months ago (2013-04-15 21:20:48 UTC) #7
Alpha Left Google
You can reference this file: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp Test the decoder like you normally would from a ...
7 years, 8 months ago (2013-04-15 21:23:26 UTC) #8
urvang (Google)
On 2013/04/15 21:23:26, Alpha wrote: > You can reference this file: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp > ...
7 years, 8 months ago (2013-04-18 23:54:43 UTC) #9
Alpha Left Google
The tests look good. There are a couple things I want to note: 1. Please ...
7 years, 8 months ago (2013-04-19 18:03:34 UTC) #10
urvang (Google)
On 2013/04/19 18:03:34, Alpha wrote: > The tests look good. > > There are a ...
7 years, 8 months ago (2013-04-23 22:36:53 UTC) #11
Alpha Left Google
Sorry for the delay. A couple minor issues and please clarify comments. https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/tests/RunAllTests.cpp File Source/WebKit/chromium/tests/RunAllTests.cpp ...
7 years, 8 months ago (2013-04-25 22:07:12 UTC) #12
urvang (Google)
Thanks Alpha! Please take another look. https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/tests/RunAllTests.cpp File Source/WebKit/chromium/tests/RunAllTests.cpp (right): https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/tests/RunAllTests.cpp#newcode37 Source/WebKit/chromium/tests/RunAllTests.cpp:37: #include <base/test/test_suite.h> // ...
7 years, 8 months ago (2013-04-26 01:46:23 UTC) #13
Alpha Left Google
LGTM. Please CC me with the refactoring as well. Thanks.
7 years, 7 months ago (2013-04-26 20:49:59 UTC) #14
urvang (Google)
On 2013/04/26 20:49:59, Alpha wrote: > LGTM. > > Please CC me with the refactoring ...
7 years, 7 months ago (2013-04-26 21:26:21 UTC) #15
Noel Gordon
https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/invalid-animated-webp.html File LayoutTests/fast/images/invalid-animated-webp.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/invalid-animated-webp.html#newcode6 LayoutTests/fast/images/invalid-animated-webp.html:6: <img src="resources/webp-animated.webp"> This test should load resources/invalid-animated-webp.webp, right? Also ...
7 years, 7 months ago (2013-04-29 18:41:57 UTC) #16
urvang (Google)
Thanks Noel! PTAL. https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/invalid-animated-webp.html File LayoutTests/fast/images/invalid-animated-webp.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/invalid-animated-webp.html#newcode6 LayoutTests/fast/images/invalid-animated-webp.html:6: <img src="resources/webp-animated.webp"> On 2013/04/29 18:41:57, Noel ...
7 years, 7 months ago (2013-04-30 13:14:15 UTC) #17
Noel Gordon
https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/invalid-animated-webp.html File LayoutTests/fast/images/invalid-animated-webp.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/invalid-animated-webp.html#newcode6 LayoutTests/fast/images/invalid-animated-webp.html:6: <img src="resources/webp-animated.webp"> On 2013/04/30 13:14:15, urvang wrote: > On ...
7 years, 7 months ago (2013-05-01 17:55:28 UTC) #18
Noel Gordon
https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/webp-animated-large.html File LayoutTests/fast/images/webp-animated-large.html (right): https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/webp-animated-large.html#newcode6 LayoutTests/fast/images/webp-animated-large.html:6: <img src="resources/webp-animated-large.webp"> As suggested, crib your layout test from ...
7 years, 7 months ago (2013-05-01 19:11:41 UTC) #19
urvang (Google)
https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.h File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.h#newcode37 Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:37: #define WEBP_ICC_ANIM_SUPPORT > WEBP_ICC_ANIMIMATION_SUPPORT -> WEBP_ANIMIMATION_SUPPORT please. This define ...
7 years, 7 months ago (2013-05-01 22:25:16 UTC) #20
Noel Gordon
https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/webp-animated-large.html File LayoutTests/fast/images/webp-animated-large.html (right): https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/webp-animated-large.html#newcode6 LayoutTests/fast/images/webp-animated-large.html:6: <img src="resources/webp-animated-large.webp"> On 2013/05/01 22:25:16, urvang wrote: > Oh ...
7 years, 7 months ago (2013-05-02 18:00:26 UTC) #21
Noel Gordon
I believe it's mostly there. Thanks for working on it. Few minor things to clean ...
7 years, 7 months ago (2013-05-03 18:15:46 UTC) #22
urvang (Google)
All done. Thanks for the review! https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode235 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:235: // FIXME: This ...
7 years, 7 months ago (2013-05-03 21:55:20 UTC) #23
Noel Gordon
https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode395 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:395: ASSERT(decodedHeight <= scaledSize().height()); On 2013/05/03 21:55:20, urvang wrote: > ...
7 years, 7 months ago (2013-05-04 17:23:43 UTC) #24
Noel Gordon
https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode77 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:77: , m_haveReadAnimParams(false) abbrv. https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode266 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:266: if ((prevMethod == ImageFrame::DisposeKeep) ...
7 years, 7 months ago (2013-05-04 17:43:50 UTC) #25
urvang (Google)
https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode77 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:77: , m_haveReadAnimParams(false) On 2013/05/04 17:43:50, Noel Gordon (Google) wrote: ...
7 years, 7 months ago (2013-05-06 19:10:35 UTC) #26
Noel Gordon
https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode274 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:274: // So, we copy the whole previous buffer, then ...
7 years, 7 months ago (2013-05-07 02:37:17 UTC) #27
urvang (Google)
https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode274 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:274: // So, we copy the whole previous buffer, then ...
7 years, 7 months ago (2013-05-07 06:02:43 UTC) #28
Noel Gordon
Nothing subjective here: we copy and clear the frame in one place only, exactly like ...
7 years, 7 months ago (2013-05-07 06:29:25 UTC) #29
urvang (Google)
> Nothing subjective here: we copy and clear the frame in one place only, exactly ...
7 years, 7 months ago (2013-05-07 06:53:44 UTC) #30
Noel Gordon
On 2013/05/07 06:53:44, urvang wrote: > > P.S. Took care of a couple of previous ...
7 years, 7 months ago (2013-05-07 07:21:06 UTC) #31
urvang (Google)
Hi Noel, Alpha, Implemented the two methods added recently in: https://chromiumcodereview.appspot.com/14317005/ Pls have another look.
7 years, 7 months ago (2013-05-08 23:45:33 UTC) #32
Noel Gordon
On 2013/05/08 23:45:33, urvang wrote: > Hi Noel, Alpha, > Implemented the two methods added ...
7 years, 7 months ago (2013-05-09 16:45:48 UTC) #33
Alpha Left Google
frameIsCompleteAtIndex() now means that a frame is fully loaded (data complete). The default implementation checks ...
7 years, 7 months ago (2013-05-09 18:08:20 UTC) #34
urvang (Google)
On Thu, May 9, 2013 at 11:08 AM, <hclam@chromium.org> wrote: > frameIsCompleteAtIndex() now means that ...
7 years, 7 months ago (2013-05-10 09:16:33 UTC) #35
Noel Gordon
After Alpha's change, frameIsCompleteAtIndex() and frameDurationAtIndex() don't decode() anymore (they are const). BitmapImage::startAnimation() uses those ...
7 years, 7 months ago (2013-05-13 23:31:01 UTC) #36
urvang (Google)
Thanks for looking into this Noel! The updated patch works correctly. On 2013/05/13 23:31:01, Noel ...
7 years, 7 months ago (2013-05-14 04:05:43 UTC) #37
Noel Gordon
> > int WEBPImageDecoder::repetitionCount() const { > > if (failed() || !m_haveReadAnimParams) > > return ...
7 years, 7 months ago (2013-05-14 06:20:55 UTC) #38
urvang (Google)
On 2013/05/14 06:20:55, Noel Gordon (Google) wrote: > > > int WEBPImageDecoder::repetitionCount() const { > ...
7 years, 7 months ago (2013-05-14 10:26:18 UTC) #39
Noel Gordon
On 2013/05/14 10:26:18, urvang wrote: > OK, added the special case of failed() state. I'm ...
7 years, 7 months ago (2013-05-14 11:08:15 UTC) #40
Noel Gordon
> > Noel wrote: > > frameIsCompleteAtIndex() has to 1) handle the non-animated image case ...
7 years, 7 months ago (2013-05-14 11:50:56 UTC) #41
urvang (Google)
message: On 2013/05/14 11:50:56, Noel Gordon (Google) wrote: > > > Noel wrote: > > ...
7 years, 7 months ago (2013-05-14 21:49:04 UTC) #42
Alpha Left Google
Implementation of frameIsCompleteAtIndex() and frameDurationAtIndex() looks good to me.
7 years, 7 months ago (2013-05-15 22:29:36 UTC) #43
Noel Gordon
https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/page/RuntimeEnabledFeatures.in File Source/core/page/RuntimeEnabledFeatures.in (right): https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/page/RuntimeEnabledFeatures.in#newcode68 Source/core/page/RuntimeEnabledFeatures.in:68: AnimatedWebP Alphabetize, and add a status=something https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp ...
7 years, 6 months ago (2013-05-29 16:43:28 UTC) #44
urvang (Google)
Apart from incorporating the suggestions, there are some major changes in this new patch to ...
7 years, 6 months ago (2013-06-06 20:20:28 UTC) #45
Noel Gordon
https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode115 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:115: if (!updateDemuxer()) On 2013/06/06 20:20:29, urvang wrote: > On ...
7 years, 6 months ago (2013-06-07 08:02:35 UTC) #46
Noel Gordon
https://codereview.chromium.org/13980003/diff/141001/Source/WebKit/chromium/public/WebRuntimeFeatures.h File Source/WebKit/chromium/public/WebRuntimeFeatures.h (right): https://codereview.chromium.org/13980003/diff/141001/Source/WebKit/chromium/public/WebRuntimeFeatures.h#newcode1 Source/WebKit/chromium/public/WebRuntimeFeatures.h:1: /* Looks to me like this file was re-written ...
7 years, 6 months ago (2013-06-07 08:06:20 UTC) #47
Noel Gordon
/code-review tool borken 211 int WEBPImageDecoder::repetitionCount() const 212 { 213 return failed() ? cAnimationLoopOnce : ...
7 years, 6 months ago (2013-06-07 08:19:09 UTC) #48
Noel Gordon
216 bool WEBPImageDecoder::frameIsCompleteAtIndex(size_t index) const 217 { 218 if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER) 219 ...
7 years, 6 months ago (2013-06-07 08:28:14 UTC) #49
Alpha Left Google
On 2013/06/07 08:19:09, Noel Gordon (Google) wrote: > /code-review tool borken > > 211 int ...
7 years, 6 months ago (2013-06-07 19:00:22 UTC) #50
urvang (Google)
Thanks Noel & Alpha! Took care of the suggestions. One comment inline. > 222 // ...
7 years, 6 months ago (2013-06-10 22:11:36 UTC) #51
urvang (Google)
https://codereview.chromium.org/13980003/diff/141001/Source/WebKit/chromium/public/WebRuntimeFeatures.h File Source/WebKit/chromium/public/WebRuntimeFeatures.h (right): https://codereview.chromium.org/13980003/diff/141001/Source/WebKit/chromium/public/WebRuntimeFeatures.h#newcode1 Source/WebKit/chromium/public/WebRuntimeFeatures.h:1: /* On 2013/06/07 08:06:21, Noel Gordon (Google) wrote: > ...
7 years, 6 months ago (2013-06-10 22:11:59 UTC) #52
Noel Gordon
> On 2013/06/07 08:19:09, Noel Gordon (Google) wrote: On 2013/06/10 22:11:36, urvang wrote: > > ...
7 years, 6 months ago (2013-06-13 08:10:27 UTC) #53
Noel Gordon
https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode152 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:152: frameToDecode = m_frameBufferCache[frameToDecode].requiredPreviousFrameIndex(); In general, you should have qinmin ...
7 years, 6 months ago (2013-06-13 15:50:17 UTC) #54
Xianzhu
Reviewed code related to requiredPreviousFrameIndex. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode161 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:161: for (Vector<size_t>::const_reverse_iterator iter = ...
7 years, 6 months ago (2013-06-13 20:04:18 UTC) #55
urvang (Google)
Thanks Noel and Xianzhu! > Your comment should probably not say > "complete frames" for ...
7 years, 6 months ago (2013-06-14 01:13:12 UTC) #56
Xianzhu
https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode329 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:329: size_t WEBPImageDecoder::clearCacheExceptFrame(size_t clearExceptFrame) On 2013/06/14 01:13:13, urvang wrote: > ...
7 years, 6 months ago (2013-06-14 17:50:26 UTC) #57
urvang (Google)
Hi, On 2013/06/14 17:50:26, Xianzhu wrote: > https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp > File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): > > https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode329 ...
7 years, 6 months ago (2013-06-14 20:07:11 UTC) #58
Xianzhu
The parts related to requiredPreviousFrameIndex look good to me.
7 years, 6 months ago (2013-06-14 20:43:01 UTC) #59
Noel Gordon
On 2013/06/14 01:13:12, urvang wrote: > > > > Still, I recall that non-animated images ...
7 years, 6 months ago (2013-06-15 02:45:36 UTC) #60
urvang (Google)
On 2013/06/15 02:45:36, Noel Gordon (Google) wrote: > On 2013/06/14 01:13:12, urvang wrote: > > ...
7 years, 6 months ago (2013-06-15 03:05:32 UTC) #61
Noel Gordon
Yes, and m_requiredPreviousFrameIndexValid is initially false too: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/platform/image-decoders/skia/ImageDecoderSkia.cpp&sq=package:chromium&rcl=1371210824&l=45 Now call requiredPreviousFrameIndex(), is it the one ...
7 years, 6 months ago (2013-06-15 03:15:18 UTC) #62
urvang (Google)
Sent from mobile. Excuse typos. On Jun 14, 2013 8:15 PM, <noel@chromium.org> wrote: > > ...
7 years, 6 months ago (2013-06-15 04:51:34 UTC) #63
urvang (Google)
On 2013/06/15 04:51:34, urvang wrote: > Sent from mobile. Excuse typos. > On Jun 14, ...
7 years, 6 months ago (2013-06-16 08:26:46 UTC) #64
Noel Gordon
Anything preventing you from sending this patch to the try servers? https://codereview.chromium.org/13980003/diff/192001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): ...
7 years, 6 months ago (2013-06-18 06:26:34 UTC) #65
urvang (Google)
Hi Noel, Thanks! Applied the suggestions discussed offline. I'll run the trybots when I get ...
7 years, 6 months ago (2013-06-18 09:12:31 UTC) #66
Noel Gordon
> I'll run the trybots when I get the permission. Righto.
7 years, 6 months ago (2013-06-18 12:29:23 UTC) #67
jamesr
https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp#newcode46 Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp:46: #include <base/basictypes.h> you shouldn't use chromium's base library in ...
7 years, 6 months ago (2013-06-21 04:45:29 UTC) #68
Noel Gordon
#include "wtf/dtoa/utils.h" instead, and use its ARRAY_SIZE macro (in place of ARRAYSIZE_UNSAFE).
7 years, 6 months ago (2013-06-21 05:35:17 UTC) #69
Noel Gordon
https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/image-decoders/webp/WEBPImageDecoder.h File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/image-decoders/webp/WEBPImageDecoder.h#newcode50 Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:50: virtual String filenameExtension() OVERRIDE const { return "webp"; } ...
7 years, 6 months ago (2013-06-21 05:36:24 UTC) #70
Noel Gordon
Build fix for the missing webp include: jamesr@ suggested off-line that WebKit.gyp needs a libwebp ...
7 years, 6 months ago (2013-06-21 05:41:28 UTC) #71
urvang (Google)
Fixed compile issues and added a unit test to test "frame doesn't have alpha, but ...
7 years, 6 months ago (2013-06-24 11:02:02 UTC) #72
Xianzhu
https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/image-decoders/webp/WEBPImageDecoder.h File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/image-decoders/webp/WEBPImageDecoder.h#newcode50 Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:50: virtual String filenameExtension() OVERRIDE const { return "webp"; } ...
7 years, 6 months ago (2013-06-24 16:20:24 UTC) #73
urvang (Google)
Hi Xianzhu, On 2013/06/24 16:20:24, Xianzhu wrote: > https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/image-decoders/webp/WEBPImageDecoder.h > File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): > > ...
7 years, 6 months ago (2013-06-24 18:13:47 UTC) #74
Xianzhu
On 2013/06/24 18:13:47, urvang wrote: > Hi Xianzhu, > > On 2013/06/24 16:20:24, Xianzhu wrote: ...
7 years, 6 months ago (2013-06-24 18:22:36 UTC) #75
urvang (Google)
On 2013/06/24 18:22:36, Xianzhu wrote: > On 2013/06/24 18:13:47, urvang wrote: > > Hi Xianzhu, ...
7 years, 6 months ago (2013-06-24 18:37:05 UTC) #76
Noel Gordon
This patch has no layout tests. You should enable the animated webp tests in TestExpectations. ...
7 years, 6 months ago (2013-06-25 01:30:06 UTC) #77
urvang (Google)
Enabled the layout tests. https://codereview.chromium.org/13980003/diff/237001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/237001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode242 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:242: return setFailed(); // Must be ...
7 years, 6 months ago (2013-06-25 01:38:58 UTC) #78
Noel Gordon
LGTM with nits. I still see 'minSizeForDemux' at line 242.
7 years, 6 months ago (2013-06-25 03:50:15 UTC) #79
urvang (Google)
+jamesr and pkasting Could you guys give OWNER's approval pls?
7 years, 6 months ago (2013-06-25 04:19:08 UTC) #80
Peter Kasting
I only gave a cursory review. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode327 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, ...
7 years, 5 months ago (2013-06-26 00:18:16 UTC) #81
urvang (Google)
Thanks Peter! PTAL. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode327 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); ...
7 years, 5 months ago (2013-06-26 22:12:46 UTC) #82
Peter Kasting
https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode327 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/26 22:12:47, ...
7 years, 5 months ago (2013-06-26 22:58:22 UTC) #83
urvang (Google)
Hi Peter, Thanks! My comments inline. If further clarification is required, let's discuss offline. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp ...
7 years, 5 months ago (2013-06-27 01:27:36 UTC) #84
skal
https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode327 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/27 01:27:37, ...
7 years, 5 months ago (2013-06-27 07:22:26 UTC) #85
Peter Kasting
https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode327 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/27 07:22:27, ...
7 years, 5 months ago (2013-06-27 17:40:17 UTC) #86
urvang (Google)
https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp#newcode327 Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/27 17:40:18, ...
7 years, 5 months ago (2013-06-27 22:53:11 UTC) #87
Peter Kasting
LGTM. As previously noted, I did a cursory rather than exhaustive review, mostly because noel ...
7 years, 5 months ago (2013-06-27 22:55:11 UTC) #88
jamesr
Source/WebKit lgtm
7 years, 5 months ago (2013-06-27 22:57:15 UTC) #89
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/urvang@google.com/13980003/284001
7 years, 5 months ago (2013-06-27 22:59:12 UTC) #90
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/urvang@google.com/13980003/284001
7 years, 5 months ago (2013-06-27 23:35:14 UTC) #91
commit-bot: I haz the power
7 years, 5 months ago (2013-06-28 04:00:26 UTC) #92
Message was sent while issue was closed.
Change committed as 153187

Powered by Google App Engine
This is Rietveld 408576698