|
|
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. |
DescriptionAdd 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() #Messages
Total messages: 92 (0 generated)
Is this a web-exposed feature which should be announced on blink-dev, per: http://www.chromium.org/blink#new-features
On 2013/04/10 20:17:01, Eric Seidel (Google) wrote: > Is this a web-exposed feature which should be announced on blink-dev, per: > http://www.chromium.org/blink#new-features Hi Eric, The support for WebP has been there for a while. This change adds support for a recently introduced feature of WebP. So, not sure if it qualifies as a web-exposed feature. I'll let Noel comment on the same.
I think this counts as web-exposed and we should go through the new feature process. The process is designed to be lightweight, but we won't know if we don't try it out on some things.
I'm happy to provide my support when the feature email is sent. :) I see no reason why we shouldn't support animating webp.
I think layout test is a poor way to test image animations as they are time dependent. There is already support for image related unit tests I think we should add more tests there.
On 2013/04/15 21:18:41, Alpha wrote: > I think layout test is a poor way to test image animations as they are time > dependent. There is already support for image related unit tests I think we > should add more tests there. Indeed. Any pointers as to what is a good way to test animations pls?
You can reference this file: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Test the decoder like you normally would from a unit test.
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... > > Test the decoder like you normally would from a unit test. Thanks Alpha! Here's an updated patch with unit tests for animated WebP.
The tests look good. There are a couple things I want to note: 1. Please enclose this in a webcore feature such that this can be turned on by about:flags. 2. I'm making changes to ImageDecoder.h interface, WebP animation will not work after those changes. The changes I'm making is to move frameIsCompleteAtIndex() and frameDurationAtIndex() to ImageDecoder, such that we don't have to do a full decode to "check" if a frame is complete and get the duration. Change is underway with GIF decoder to do this, WebP decoder should have this feature as well.
On 2013/04/19 18:03:34, Alpha wrote: > The tests look good. > > There are a couple things I want to note: > 1. Please enclose this in a webcore feature such that this can be turned on by > about:flags. Done. Reorganized a code a bit to do this -- especially, separated out '#define WEBP_ICC_ANIM_SUPPORT' from '#define QCMS_WEBP_COLOR_CORRECTION'. > 2. I'm making changes to ImageDecoder.h interface, WebP animation will not work > after those changes. The changes I'm making is to move frameIsCompleteAtIndex() > and frameDurationAtIndex() to ImageDecoder, such that we don't have to do a full > decode to "check" if a frame is complete and get the duration. Change is > underway with GIF decoder to do this, WebP decoder should have this feature as > well. I believe you are referring to this change: https://codereview.chromium.org/14317005/ I'll keep an eye on it, and update the this patch once that change is in. In the meantime, could you review that all else is OK?
Sorry for the delay. A couple minor issues and please clarify comments. https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/RunAllTests.cpp (right): https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/RunAllTests.cpp:37: #include <base/test/test_suite.h> // FIXME: Avoid this source dependency on Chromium's base module. Is it important to reorder these includes? If not please don't reorder them. https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/RunAllTests.cpp:53: WebCore::RuntimeEnabledFeatures::setAnimatedWebPEnabled(true); I suggest doing this only in WebP test. You can defined TEST_F such that this is called for all the unit tests in that file. https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WEBPImageDecoderTest.cpp:82: TEST(WEBPImageDecoderTest, verifyAnimationParameters) You can have these as TEST_F such that enabling animated WebP is done for all tests in this file. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:248: frameRect.setWidth(size().width() - fIter.x_offset); What if frameRect.x() is always greater then size().width()? https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:250: frameRect.setHeight(size().height() - fIter.y_offset); Same here. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: // We need to restore transparent pixels to as they were just after initFrame() call. That is: This comment is confusing. Can you clarify? For example explain why this is necessary in addition to what the code is doing. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:449: if (!((pixel >> 24) & 0xff) && !prevRect.contains(IntPoint(canvasX, canvasY))) // Need to restore. Does this have alpha blending issue as well? Please add a comment if it does. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:37: #define WEBP_ICC_ANIM_SUPPORT Can you have this always defined such that animated WebP code is always compiled? What is the reason to have this define given we can turn this off in runtime.
Thanks Alpha! Please take another look. https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/RunAllTests.cpp (right): https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/RunAllTests.cpp:37: #include <base/test/test_suite.h> // FIXME: Avoid this source dependency on Chromium's base module. On 2013/04/25 22:07:12, Alpha wrote: > Is it important to reorder these includes? If not please don't reorder them. Yes, this was needed to make 'check-webkit-style' happy. Otherwise, it was complaining about 'include order problem'. Anyway, reverting this file as I'm moving this to the WebP test itself. https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/RunAllTests.cpp:53: WebCore::RuntimeEnabledFeatures::setAnimatedWebPEnabled(true); On 2013/04/25 22:07:12, Alpha wrote: > I suggest doing this only in WebP test. You can defined TEST_F such that this is > called for all the unit tests in that file. Done. https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/13980003/diff/16001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WEBPImageDecoderTest.cpp:82: TEST(WEBPImageDecoderTest, verifyAnimationParameters) On 2013/04/25 22:07:12, Alpha wrote: > You can have these as TEST_F such that enabling animated WebP is done for all > tests in this file. Done. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:248: frameRect.setWidth(size().width() - fIter.x_offset); On 2013/04/25 22:07:12, Alpha wrote: > What if frameRect.x() is always greater then size().width()? Note that I kept the initFrameBuffer() method here to be very similar to the same method in GIFImageDecoder.cpp. That way, it would be easy to refactor it later. My suggestion would be to not modify this logic before refactoring. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:250: frameRect.setHeight(size().height() - fIter.y_offset); On 2013/04/25 22:07:12, Alpha wrote: > Same here. same commment. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:440: // We need to restore transparent pixels to as they were just after initFrame() call. That is: On 2013/04/25 22:07:12, Alpha wrote: > This comment is confusing. Can you clarify? For example explain why this is > necessary in addition to what the code is doing. You are right. I added a comment above to explain why this 'restoration/correction' of transparent pixels is needed. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:449: if (!((pixel >> 24) & 0xff) && !prevRect.contains(IntPoint(canvasX, canvasY))) // Need to restore. On 2013/04/25 22:07:12, Alpha wrote: > Does this have alpha blending issue as well? Please add a comment if it does. Done. https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/16001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:37: #define WEBP_ICC_ANIM_SUPPORT On 2013/04/25 22:07:12, Alpha wrote: > Can you have this always defined such that animated WebP code is always > compiled? What is the reason to have this define given we can turn this off in > runtime. It's basically to support different versions of libwebp. This was useful in WebKit where different ports used different versions of libwebp, but probably not needed in Blink. I'll do a cleanup separately to remove the code for previous libwebp versions -- as there is more such code. Also, it will help migrating the patch to webkit.
LGTM. Please CC me with the refactoring as well. Thanks.
On 2013/04/26 20:49:59, Alpha wrote: > LGTM. > > Please CC me with the refactoring as well. Thanks. Thanks! As discussed, I'll wait for your change https://codereview.chromium.org/14317005/ to go in, make the necessary changes here and then commit this.
https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/i... File LayoutTests/fast/images/invalid-animated-webp.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/i... 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 the test should be a pixel test to check that we draw the broken-image icon. https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/w... File LayoutTests/fast/images/webp-animated-large.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/w... LayoutTests/fast/images/webp-animated-large.html:5: This test passes if no errors occur while decoding this large animation.<br> How does this test work? If there were decoding errors, I don't see how it detects those errors. https://codereview.chromium.org/13980003/diff/25001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/13980003/diff/25001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WEBPImageDecoderTest.cpp:92: TEST_F(WEBPImageDecoderTest, verifyAnimationParameters) I like that you added "Animation" to the test name. Should the other tests have "Animation"|"Animated" in their respective test names? For example, we might add some non-animated webp image format tests in this file in future. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:41: #else 41: Change it to #endif here. Then you could you move the ICC_FLAG and ALPHA_FLAG stuff down into the // Backward emulation section @52. @52 could then read as: #if (WEBP_DECODER_ABI_VERSION < 0x0163) // Backward emulation for versions earlier than 0.1.99. #define MODE_rgbA MODE_RGBA #define MODE_bgrA MODE_BGRA #define ALPHA_FLAG 0 #define ICCP_FLAG 0 #elif (WEBP_DECODER_ABI_VERSION < 0x0200) // Backward emulation for versions earlier than 0.2.0. #define ALPHA_FLAG 0x000010 #define ICCP_FLAG 0 #endif which is easier to read at least to my eyes. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:85: { One of our member variables (m_decoderBuffer) is not initialized. See also the comment @515. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:90: clearAll(); clear() here and elsewhere. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:149: if (frame.status() != ImageFrame::FrameComplete) { Prefer the early return in Blink. Write this as if (frame.status() == ImageFrame::FrameComplete) return &frame; (so the code that follows doesn't need to dance off the page to the right :) https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:159: if (!initFrameBuffer(fIter, index)) Combine this "if" with the one in the prior line? if ((m_formatFlags & ANIMATION_FLAG) && !initFrameBuffer(fIter, index)) return 0; https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:166: } else { WebPDemuxReleaseIterator(&fIter); return &frame; } and then we wouldn't need the "else {", nor lines 172-174. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:168: ASSERT(!index); This ASSERT is correct in that we should only decode non-animated images on this path. However if RuntimeEnabledFeatures::animatedWebPEnabled() is false, note that updateDemux() still expands the frame buffer cache to the number of frames in the animated image regardless. That number of frames is reported to BitmapImage via frameCount(). BitmapImage will subsequently try to decode each frame, so the ASSERT(!index) fails for a multi-frame animated image. (Did you test this code in a DEBUG compile?). I think updateDemux() needs the following code in its innards: m_formatFlags = WebPDemuxGetI(m_demux, WEBP_FF_FORMAT_FLAGS); bool hasAnimation = (m_formatFlags & ANIMATION_FLAG); if (!RuntimeEnabledFeatures::animatedWebPEnabled() && hasAnimation) return setFailed(); somewhere before updateDemux() tries to expand the frame buffer cache, maybe before line 209. That'll stop BitmapImage from trying to decode animated images when they are disabled via the runtime flag. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:195: if (!m_haveAlreadyParsedThisData) { Prefer the early return, please. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:196: WebPDemuxDelete(m_demux); Could you move these two lines (196-197) down to where you update the demux with WebPDemuxPartial(). Second, let's remove dataSize. Just use m_data->size() instead (it's const and inline). https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:202: return 0; // Wait for headers so that WebPDemuxPartial doesn't return null. return 0? return false; I think. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:205: m_demux = WebPDemuxPartial(&inputData, &m_demuxState); I assume WebPDemuxPartial() will correctly reset m_demuxState here? https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:208: if (m_demuxState >= WEBP_DEMUX_PARSED_HEADER) { Prefer the early return, but the current statement says greater than or equal to WEBP_DEMUX_PARSED_HEADER. Yet the libwebp code states: // Get the 'feature' value from the 'dmux'. // NOTE: values are only valid if WebPDemux() was used or WebPDemuxPartial() // returned a state > WEBP_DEMUX_PARSING_HEADER. WEBP_EXTERN(uint32_t) WebPDemuxGetI(...) Seems we could read data via WebPDemuxGetI() when m_demuxState == WEBP_DEMUX_PARSING_HEADER in this block, meaning we'd read invalid data? Is the documentation for WebPDemuxGetI wrong? https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:211: setFailed(); return setFailed(); https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:226: if (newFrameCount > m_frameBufferCache.size()) { See the comment for line 168. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:298: // Update frame status to be partially complete. Space before this line. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:302: void WEBPImageDecoder::clearFrameBufferCache(size_t clearBeforeFrame) Space before this line. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:386: int stride; stride is not used, what's it for? https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:398: if ((m_formatFlags & ICCP_FLAG) && !ignoresGammaAndColorProfile()) { Why all the #ifdef QCMS_WEBP_COLOR_CORRECTION's in the body of this if? https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:515: WebPInitDecBuffer(&m_decoderBuffer); Where is m_decoderBuffer released? Seems m_decoderBuffer is leaking memory unless there's a matching WebPFreeDecBuffer(&m_decoderBuffer) somewhere. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:37: #define WEBP_ICC_ANIM_SUPPORT (avoid abbrevs) Make it WEBP_ANIMIMATION or some such. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:84: bool initFrameBuffer(const WebPIterator& fIter, size_t frameIndex); s/ fIter// https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:102: void clearAll(); All what? clear() was fine.
Thanks Noel! PTAL. https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/i... File LayoutTests/fast/images/invalid-animated-webp.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/i... LayoutTests/fast/images/invalid-animated-webp.html:6: <img src="resources/webp-animated.webp"> On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > This test should load resources/invalid-animated-webp.webp, right? Also the > test should be a pixel test to check that we draw the broken-image icon. > I believe this layout test is no longer needed, as we have a unit test now: WEBPImageDecoderTest.cpp, which tests this. Removing. Let me know if we still want this. https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/w... File LayoutTests/fast/images/webp-animated-large.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/w... LayoutTests/fast/images/webp-animated-large.html:5: This test passes if no errors occur while decoding this large animation.<br> On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > How does this test work? If there were decoding errors, I don't see how it > detects those errors. I had added this test hoping that Layout tests may be designed to catch memory errors (for example, clearFrameBuffer() would be called only for large animations), but perhaps not. Now that I've added WEBPImageDecoderTest.cpp, which should be much more comprehensive, we could remove this if you suggest so. And I could add this image to be tested there. https://codereview.chromium.org/13980003/diff/25001/Source/WebKit/chromium/te... File Source/WebKit/chromium/tests/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/13980003/diff/25001/Source/WebKit/chromium/te... Source/WebKit/chromium/tests/WEBPImageDecoderTest.cpp:92: TEST_F(WEBPImageDecoderTest, verifyAnimationParameters) On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > I like that you added "Animation" to the test name. Should the other tests have > "Animation"|"Animated" in their respective test names? For example, we might > add some non-animated webp image format tests in this file in future. Changed the name of the fixture class itself to include 'animated'. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:41: #else On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > 41: Change it to #endif here. > > Then you could you move the ICC_FLAG and ALPHA_FLAG stuff down into the // > Backward emulation section @52. @52 could then read as: > > #if (WEBP_DECODER_ABI_VERSION < 0x0163) > // Backward emulation for versions earlier than 0.1.99. > #define MODE_rgbA MODE_RGBA > #define MODE_bgrA MODE_BGRA > #define ALPHA_FLAG 0 > #define ICCP_FLAG 0 > #elif (WEBP_DECODER_ABI_VERSION < 0x0200) > // Backward emulation for versions earlier than 0.2.0. > #define ALPHA_FLAG 0x000010 > #define ICCP_FLAG 0 > #endif > > which is easier to read at least to my eyes. Restuctured as per ur suggestion. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:85: { On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > One of our member variables (m_decoderBuffer) is not initialized. See also the > comment @515. I moved this initialization to 515, because decode buffer needs to be initialized for every frame now. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:90: clearAll(); On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > clear() here and elsewhere. Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:149: if (frame.status() != ImageFrame::FrameComplete) { On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Prefer the early return in Blink. Write this as > > if (frame.status() == ImageFrame::FrameComplete) > return &frame; > > (so the code that follows doesn't need to dance off the page to the right :) Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:159: if (!initFrameBuffer(fIter, index)) On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Combine this "if" with the one in the prior line? > > if ((m_formatFlags & ANIMATION_FLAG) && !initFrameBuffer(fIter, index)) > return 0; Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:166: } else { On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > WebPDemuxReleaseIterator(&fIter); > return &frame; > } > > and then we wouldn't need the "else {", nor lines 172-174. Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:168: ASSERT(!index); On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > This ASSERT is correct in that we should only decode non-animated images on this > path. > > However if RuntimeEnabledFeatures::animatedWebPEnabled() is false, note that > updateDemux() still expands the frame buffer cache to the number of frames in > the animated image regardless. That number of frames is reported to BitmapImage > via frameCount(). BitmapImage will subsequently try to decode each frame, so > the ASSERT(!index) fails for a multi-frame animated image. > > (Did you test this code in a DEBUG compile?). > > I think updateDemux() needs the following code in its innards: > > m_formatFlags = WebPDemuxGetI(m_demux, WEBP_FF_FORMAT_FLAGS); > bool hasAnimation = (m_formatFlags & ANIMATION_FLAG); > if (!RuntimeEnabledFeatures::animatedWebPEnabled() && hasAnimation) > return setFailed(); > > somewhere before updateDemux() tries to expand the frame buffer cache, maybe > before line 209. That'll stop BitmapImage from trying to decode animated images > when they are disabled via the runtime flag. > > Good catch! Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:195: if (!m_haveAlreadyParsedThisData) { On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Prefer the early return, please. Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:196: WebPDemuxDelete(m_demux); On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Could you move these two lines (196-197) down to where you update the demux with > WebPDemuxPartial(). > > Second, let's remove dataSize. Just use m_data->size() instead (it's const and > inline). Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:202: return 0; // Wait for headers so that WebPDemuxPartial doesn't return null. On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > return 0? return false; I think. Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:205: m_demux = WebPDemuxPartial(&inputData, &m_demuxState); On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > I assume WebPDemuxPartial() will correctly reset m_demuxState here? Yes, it will. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:208: if (m_demuxState >= WEBP_DEMUX_PARSED_HEADER) { On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Prefer the early return, Done. but the current statement says greater than or equal to > WEBP_DEMUX_PARSED_HEADER. Yet the libwebp code states: > > // Get the 'feature' value from the 'dmux'. > // NOTE: values are only valid if WebPDemux() was used or WebPDemuxPartial() > // returned a state > WEBP_DEMUX_PARSING_HEADER. > WEBP_EXTERN(uint32_t) WebPDemuxGetI(...) > > Seems we could read data via WebPDemuxGetI() when m_demuxState == > WEBP_DEMUX_PARSING_HEADER in this block, meaning we'd read invalid data? Is the > documentation for WebPDemuxGetI wrong? Not sure what the confusion is: "state > WEBP_DEMUX_PARSING_HEADER" and "state >= WEBP_DEMUX_PARSED_HEADER" are equivalent statements. Also, we are *not* calling GetI() when in PARSING_HEADER state. Anyway, using using "if (m_demuxState <= WEBP_DEMUX_PARSING_HEADER) return true;" to match the note in libwebp code. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:211: setFailed(); On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > return setFailed(); Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:226: if (newFrameCount > m_frameBufferCache.size()) { On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > See the comment for line 168. Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:298: // Update frame status to be partially complete. On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Space before this line. Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:302: void WEBPImageDecoder::clearFrameBufferCache(size_t clearBeforeFrame) On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Space before this line. Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:386: int stride; On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > stride is not used, what's it for? Good catch! Thanks. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:398: if ((m_formatFlags & ICCP_FLAG) && !ignoresGammaAndColorProfile()) { On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Why all the #ifdef QCMS_WEBP_COLOR_CORRECTION's in the body of this if? Cleaned up this code to have the whole 'if' surrounded by #ifdef QCMS_WEBP_COLOR_CORRECTION -- by choosing MODE_ARGB only when QCMS_WEBP_COLOR_CORRECTION is defined and color profile is present (see decode() below). https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:515: WebPInitDecBuffer(&m_decoderBuffer); On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > Where is m_decoderBuffer released? Seems m_decoderBuffer is leaking memory > unless there's a matching WebPFreeDecBuffer(&m_decoderBuffer) somewhere. WebPIDelete() calls WebPFreeDecBuffer() internally. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:37: #define WEBP_ICC_ANIM_SUPPORT On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > (avoid abbrevs) Make it WEBP_ANIMIMATION or some such. Done. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:84: bool initFrameBuffer(const WebPIterator& fIter, size_t frameIndex); On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > s/ fIter// What is special about this argument? Why should we omit the variable name only for this argument? https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:102: void clearAll(); On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > All what? clear() was fine. 'clear()' was actually renamed to 'clearDecoder()', which clears data for a single frame (in case of animation). So I called this 'clearAll()' (was supposed to mean clear data for all frames). But fine, clear() is also ok.
https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/i... File LayoutTests/fast/images/invalid-animated-webp.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/i... LayoutTests/fast/images/invalid-animated-webp.html:6: <img src="resources/webp-animated.webp"> On 2013/04/30 13:14:15, urvang wrote: > On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > > This test should load resources/invalid-animated-webp.webp, right? Also the > > test should be a pixel test to check that we draw the broken-image icon. > > > > > I believe this layout test is no longer needed, as we have a unit test now: > WEBPImageDecoderTest.cpp, which tests this. > Removing. Let me know if we still want this. See below. https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/w... File LayoutTests/fast/images/webp-animated-large.html (right): https://codereview.chromium.org/13980003/diff/25001/LayoutTests/fast/images/w... LayoutTests/fast/images/webp-animated-large.html:5: This test passes if no errors occur while decoding this large animation.<br> On 2013/04/30 13:14:15, urvang wrote: > On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > > How does this test work? If there were decoding errors, I don't see how it > > detects those errors. > > I had added this test hoping that Layout tests may be designed to catch memory > errors (for example, clearFrameBuffer() would be called only for large > animations), but perhaps not. > > Now that I've added WEBPImageDecoderTest.cpp, which should be much more > comprehensive, we could remove this if you suggest so. And I could add this > image to be tested there. The unit test is fine, but we also want a layout test since they get tested with the ASAN and TSAN bots that check for memory leaks and threading issues, respectively. Suggest you look at fast/images/webp-image-decoding.html and crib something similar for animated webp images. Find three images to test with, for example, the ones you added to fast/image/resources =) https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:208: if (m_demuxState >= WEBP_DEMUX_PARSED_HEADER) { > Anyway, using using "if (m_demuxState <= WEBP_DEMUX_PARSING_HEADER) return > true;" to match the note in libwebp code. Hard to distinquish WEBP_DEMUX_PARSING_HEADER and WEBP_DEMUX_PARSED_HEADER. Looks right now, I would "return false;" though for consistency. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:37: #define WEBP_ICC_ANIM_SUPPORT On 2013/04/30 13:14:15, urvang wrote: > On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > > (avoid abbrevs) Make it WEBP_ANIMIMATION or some such. > > Done. WEBP_ICC_ANIMIMATION_SUPPORT -> WEBP_ANIMIMATION_SUPPORT please. https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:84: bool initFrameBuffer(const WebPIterator& fIter, size_t frameIndex); On 2013/04/30 13:14:15, urvang wrote: > On 2013/04/29 18:41:57, Noel Gordon (Google) wrote: > > s/ fIter// > > What is special about this argument? Why should we omit the variable name only > for this argument? http://www.chromium.org/blink/coding-style Names 9. Leave meaningless variable names out of function declarations ... [names-variable-name-in-function-decl] Read Source/core/platform/graphics/GraphicsContext.h for good examples.
https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/w... File LayoutTests/fast/images/webp-animated-large.html (right): https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/w... LayoutTests/fast/images/webp-animated-large.html:6: <img src="resources/webp-animated-large.webp"> As suggested, crib your layout test from fast/images/webp-image-decoding.html to appease our ASAN/TSAN bots. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:48: #define ICCP_FLAG 0 Remove. ICCP_FLAG use is guarded in decode() now. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:52: #define ICCP_FLAG 0 Ditto. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:154: WebPIterator fIter; fIter again (abbrev). Perhaps call it buffer. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:202: m_haveAlreadyParsedThisData = true; Should this line be moved somewhere before the return @195 ? No matter how you early return from this routine, I'm sure we have already parsed this data. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:205: return true; // Not enough data for parsing canvas width/height yet. Return false; for consistency. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:213: const bool hasAnimation = (m_formatFlags & ANIMATION_FLAG); 213-215. Could we move these lines up so we exit early, before we even try to set the image size, and so on. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:217: if (hasAnimation && !m_haveReadAnimParams && (newFrameCount >= 1)) { /curious newFrameCount >= 1. Do single-frame images have loopCount and other animation parameters? https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:222: // Note: The following casts an 'unsigned int' to 'int'. But that is fine, because loop count is always <= 16 bits. How about turning the comment into code. int loopCount = WebPDemuxGetI(m_demux, WEBP_FF_LOOP_COUNT) & 0xffff; https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:223: m_repetitionCount = (!loopCount) ? cAnimationLoopInfinite : loopCount; ... = !loopCount ? cAnimationLoopInfinite ... https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:235: bool WEBPImageDecoder::initFrameBuffer(const WebPIterator& fIter, size_t frameIndex) fIter. Call it frame. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:256: buffer.setDisposalMethod(fIter.dispose_method == WEBP_MUX_DISPOSE_BACKGROUND ? ImageFrame::DisposeOverwriteBgcolor : ImageFrame::DisposeKeep); So note that the only disposals methods for a webp frame are: ImageFrame::DisposeOverwriteBgcolor ImageFrame::DisposeKeep ... https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:261: // This is the first frame, so we're not relying on any previous data. ... and that this if clause deals with !frameIndex case. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:271: if ((prevMethod == ImageFrame::DisposeKeep) || (prevMethod == ImageFrame::DisposeNotSpecified)) { Is prevMethod == ImageFrame::DisposeNotSpecified a possible webp disposal method? https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:275: } else { // prevMethod == ImageFrame::DisposeOverwriteBgcolor This comment could break, an ASSERT would not. Turn this comment into code. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:279: if (!frameIndex || prevRect.contains(IntRect(IntPoint(), scaledSize()))) { Given the comment @261, what value does !frameIndex always have here? https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:303: // We always preserve at least one frame. The question is why? The GIF decoder bails if m_frameBufferCache.isEmpty() here. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:390: ASSERT(width == scaledSize().width()); These ASSERTs are gonna hurt. The width here is that of the frame? Can the frame width be smaller than the image width returned by scaledSize().width() (the canvas width)? ASSERT(decodedHeight <= scaledSize().height()) is not tight enough. The frame height can be smaller than the canvas height? https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:396: // Color Profile. Redundant comment given the include guard right? https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:433: if (!((pixel >> 24) & 0xff)) { // Need to restore. Unless you want to break Android, write this as unsigned alpha = (pixel >> SK_A32_SHIFT) & 0xff; if (!alpha) { // Need to restore. ... SK_A32_SHIFT on Android differs from the value used for desktop Chrome. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:434: const ImageFrame::PixelData prevPixel = *prevBuffer.getAddr(canvasX, canvasY); s/const// https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:449: const ImageFrame::PixelData prevPixel = *prevBuffer.getAddr(canvasX, canvasY); Should prevPixel be defined inside the if clause where it is used similar to line 434 ? s/const// https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:451: if (!((pixel >> 24) & 0xff) && !prevRect.contains(IntPoint(canvasX, canvasY))) // Need to restore. Again, don't break the droids. unsigned alpha = (pixel >> SK_A32_SHIFT) & 0xff; if (!alpha && !prevRect.contains( ...
https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/25001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:37: #define WEBP_ICC_ANIM_SUPPORT > WEBP_ICC_ANIMIMATION_SUPPORT -> WEBP_ANIMIMATION_SUPPORT please. This define is to show that libwebp version in use supports ICC and animation, so I'd really like to have both in the name. The plan is to later remove these defines related to old libwebp versions from Blink (but keep them in Webkit). Note: This is different from QCMS_WEBP_COLOR_CORRECTION, as it specifies whether to apply color correction (based on whether libqcms is available). https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/w... File LayoutTests/fast/images/webp-animated-large.html (right): https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/w... LayoutTests/fast/images/webp-animated-large.html:6: <img src="resources/webp-animated-large.webp"> On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > As suggested, crib your layout test from fast/images/webp-image-decoding.html to > appease our ASAN/TSAN bots. Oh these run under ASAN/TSAN, good. However, just realized that these tests won't work as "RuntimeEnabledFeatures::animatedWebPEnabled()" is false by default. (Not sure how it was working so far). Question: Is there a way to enable this flag for layout tests (similar to the way it is done in unit tests)? Otherwise, I'm just putting test expectation to be 'failure' for now. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:48: #define ICCP_FLAG 0 On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Remove. ICCP_FLAG use is guarded in decode() now. Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:52: #define ICCP_FLAG 0 On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Ditto. Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:154: WebPIterator fIter; On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > fIter again (abbrev). Perhaps call it buffer. Calling it 'webpFrame' now. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:202: m_haveAlreadyParsedThisData = true; On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Should this line be moved somewhere before the return @195 ? No matter how you > early return from this routine, I'm sure we have already parsed this data. Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:205: return true; // Not enough data for parsing canvas width/height yet. On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Return false; for consistency. Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:213: const bool hasAnimation = (m_formatFlags & ANIMATION_FLAG); On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > 213-215. Could we move these lines up so we exit early, before we even try to > set the image size, and so on. Changed the code a bit to exit before setSize() call in such a case. See if this is better. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:217: if (hasAnimation && !m_haveReadAnimParams && (newFrameCount >= 1)) { On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > /curious newFrameCount >= 1. Do single-frame images have loopCount and other > animation parameters? Ideally, no. But theoretically, single-frame animated images are possible. The real reason: we are working on partial data. So, we check for 'newFrameCount >= 1' to see if we have gone past the ANIM chunk (as noted in the comment). https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:222: // Note: The following casts an 'unsigned int' to 'int'. But that is fine, because loop count is always <= 16 bits. On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > How about turning the comment into code. > > int loopCount = WebPDemuxGetI(m_demux, WEBP_FF_LOOP_COUNT) & 0xffff; > That seems a bit ugly. Let me put an assert instead. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:223: m_repetitionCount = (!loopCount) ? cAnimationLoopInfinite : loopCount; On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > ... = !loopCount ? cAnimationLoopInfinite ... Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:235: bool WEBPImageDecoder::initFrameBuffer(const WebPIterator& fIter, size_t frameIndex) On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > fIter. Call it frame. Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:256: buffer.setDisposalMethod(fIter.dispose_method == WEBP_MUX_DISPOSE_BACKGROUND ? ImageFrame::DisposeOverwriteBgcolor : ImageFrame::DisposeKeep); On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > So note that the only disposals methods for a webp frame are: > > ImageFrame::DisposeOverwriteBgcolor > ImageFrame::DisposeKeep > > ... see comment in 279 https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:261: // This is the first frame, so we're not relying on any previous data. On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > ... and that this if clause deals with !frameIndex case. see comment at 279 https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:271: if ((prevMethod == ImageFrame::DisposeKeep) || (prevMethod == ImageFrame::DisposeNotSpecified)) { On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Is prevMethod == ImageFrame::DisposeNotSpecified a possible webp disposal > method? It's not. I kept this code snippet as similar to GIF one as possible for a refactoring later. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:275: } else { // prevMethod == ImageFrame::DisposeOverwriteBgcolor On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > This comment could break, an ASSERT would not. Turn this comment into code. Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:279: if (!frameIndex || prevRect.contains(IntRect(IntPoint(), scaledSize()))) { On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Given the comment @261, what value does !frameIndex always have here? I see what you are getting at. But again, the idea is to keep this code snippet as similar to GIF one as possible for a refactoring later. See the FIXME before this method. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:303: // We always preserve at least one frame. On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > The question is why? The GIF decoder bails if m_frameBufferCache.isEmpty() > here. Note that GIF decoder never clears the 'end' (last frame) either. In WebP case, due to 'DisposePrevious' not being possible, the above two conditions can be merged to 'size() <= 1'. And, the reason why we need at least one frame, is to construct the next frame (as initFrame() needs previous frame). Added note. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:390: ASSERT(width == scaledSize().width()); On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > These ASSERTs are gonna hurt. The width here is that of the frame? Can the > frame width be smaller than the image width returned by scaledSize().width() > (the canvas width)? > > ASSERT(decodedHeight <= scaledSize().height()) is not tight enough. The frame > height can be smaller than the canvas height? > OK, to clear things up: - The frame width/height *stored* in the WebP file can be smaller than canvas width/height. - *But*, a renderer needs to recreate the whole *canvas* corresponding to each frame (due to disposal methods, etc). That's why the size of each frame is initialized to be the same as that of the canvas. See line#262. The same is true for GIF as well. - The frame sizes are stored in ImageFrames using 'setOriginalFrameRect', and we decode into that rectangle. I had thought that this was evident, but it seems not. Pls suggest where a comment might help. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:396: // Color Profile. On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Redundant comment given the include guard right? Maybe, yes. But, the include guard can be removed later, right? I would still keep the comment to be coherent -- one comment per each main task the method is doing. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:433: if (!((pixel >> 24) & 0xff)) { // Need to restore. On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Unless you want to break Android, write this as > > unsigned alpha = (pixel >> SK_A32_SHIFT) & 0xff; > if (!alpha) { // Need to restore. > ... > > SK_A32_SHIFT on Android differs from the value used for desktop Chrome. Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:434: const ImageFrame::PixelData prevPixel = *prevBuffer.getAddr(canvasX, canvasY); On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > s/const// I thought a const would be desirable? Anyway, removing this assuming it has something to do with Webkit style. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:449: const ImageFrame::PixelData prevPixel = *prevBuffer.getAddr(canvasX, canvasY); On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Should prevPixel be defined inside the if clause where it is used similar to > line 434 ? > > s/const// Done. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:451: if (!((pixel >> 24) & 0xff) && !prevRect.contains(IntPoint(canvasX, canvasY))) // Need to restore. On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > Again, don't break the droids. > > unsigned alpha = (pixel >> SK_A32_SHIFT) & 0xff; > if (!alpha && !prevRect.contains( ... Done.
https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/w... File LayoutTests/fast/images/webp-animated-large.html (right): https://codereview.chromium.org/13980003/diff/46001/LayoutTests/fast/images/w... 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 these run under ASAN/TSAN, good. http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN for example. > However, just realized that these tests won't work as > "RuntimeEnabledFeatures::animatedWebPEnabled()" is false by default. (Not sure > how it was working so far). > > Question: Is there a way to enable this flag for layout tests (similar to the > way it is done in unit tests)? Yes, and the enable will soon be auto-generated according to Eric. Failing that, you modify InternalSettings to allow a layout test to set a runtime flag. For example, % grep CSSVariables Source/core/testing/InternalSettings.* and see LayoutTests/fast/css/variables/invalid-font-reference.html > Otherwise, I'm just putting test expectation to be 'failure' for now. Seems fine for now. Expect news from Eric. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:222: // Note: The following casts an 'unsigned int' to 'int'. But that is fine, because loop count is always <= 16 bits. On 2013/05/01 22:25:16, urvang wrote: > On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > > How about turning the comment into code. > > > > int loopCount = WebPDemuxGetI(m_demux, WEBP_FF_LOOP_COUNT) & 0xffff; > > > That seems a bit ugly. Let me put an assert instead. Maybe loopCount is just unnecessary noise. m_repetitionCount = WebPDemuxGetI(m_demux, WEBP_FF_LOOP_COUNT) & 0xffff; if (!m_repetitionCount) m_repetitionCount = cAnimationLoopInfinite; https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:279: if (!frameIndex || prevRect.contains(IntRect(IntPoint(), scaledSize()))) { On 2013/05/01 22:25:16, urvang wrote: > On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > > Given the comment @261, what value does !frameIndex always have here? > > I see what you are getting at. > But again, the idea is to keep this code snippet as similar to GIF one as > possible for a refactoring later. See the FIXME before this method. Similar is fine: my view was a FIXME would also help the refactor. // FIXME: "First frame" yet !frameIndex is always false here. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:303: // We always preserve at least one frame. On 2013/05/01 22:25:16, urvang wrote: > On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > > The question is why? The GIF decoder bails if m_frameBufferCache.isEmpty() > > here. > > Note that GIF decoder never clears the 'end' (last frame) either. > In WebP case, due to 'DisposePrevious' not being possible, the above two > conditions can be merged to 'size() <= 1'. Right, merged. Thanks for the explanation. > And, the reason why we need at least one frame, is to construct the next frame > (as initFrame() needs previous frame). Added note. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:390: ASSERT(width == scaledSize().width()); On 2013/05/01 22:25:16, urvang wrote: > On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > > These ASSERTs are gonna hurt. The width here is that of the frame? Can the > > frame width be smaller than the image width returned by scaledSize().width() > > (the canvas width)? > > > > ASSERT(decodedHeight <= scaledSize().height()) is not tight enough. The frame > > height can be smaller than the canvas height? > > > > OK, to clear things up: > - The frame width/height *stored* in the WebP file can be smaller than canvas > width/height. So the frame width can be smaller than the canvas width. > - *But*, a renderer needs to recreate the whole *canvas* corresponding to each > frame (due to disposal methods, etc). That's why the size of each frame is > initialized to be the same as that of the canvas. See line#262. The same is true > for GIF as well. Yes I know how buffer.setSize() works: it creates a canvas-width/height buffer. > - The frame sizes are stored in ImageFrames using 'setOriginalFrameRect', and we > decode into that rectangle. So if that frame rectangle width is smaller than the canvas width say, my question was: what width does WebPIDecGetRGB(m_decoder, &decodedHeight, &width, 0, 0) return? The answer is either 1) the frame rectangle width, or 2) the canvas width. If it's 1), the width ASSERT can fail. If it's 2), I could construct a webp image with a frame rectangle that would cause the code the write off the end of the frame buffer into memory you don't own. https://codereview.chromium.org/13980003/diff/46001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:396: // Color Profile. On 2013/05/01 22:25:16, urvang wrote: > On 2013/05/01 19:11:41, Noel Gordon (Google) wrote: > > Redundant comment given the include guard right? > > Maybe, yes. But, the include guard can be removed later, right? I would still > keep the comment to be coherent -- one comment per each main task the method is > doing. Blink style would encourage coherency as follows. if ((m_formatFlags & ICCP_FLAG) && !ignoresGammaAndColorProfile()) applyColorProfile(buffer,...); The comment doesn't tell me anything that the code does not.
I believe it's mostly there. Thanks for working on it. Few minor things to clean up. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:235: // FIXME: This method is very similar to the one in GIFImageDecoder.cpp and should be refactored. I'm not sure we should hitch our wagon to the GIFImageDecoder.cpp. The GIF decoder supports image scaling, #if ENABLE(IMAGE_DECODER_DOWNSAMPLING) to be precise, and that feature has never been enabled in Chrome and it has no tests. The rest of WEBP decoder code doesn't support it, but the code here has a bunch of references to it because it was copied from the GIF decoder. We have better ways to handle the use-case, using compression proxies http://blog.chromium.org/2013/03/data-compression-in-chrome-beta-for.html for example. Moreover, if we ever wanted to support a client-side image down-scale, similar to IMAGE_DECODER_DOWNSAMPLING, then we'd most likely use WEBP's in-built scaling support. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:251: const int left = upperBoundScaledX(frameRect.x()); This block of code is an example of the down sampling enable. Since the WEBP decoder does not support that enable, its ImageDecoder::m_scaled is always false -- this whole block of left right top bottom goop could be replaced a single line: buffer.setOriginalFrameRect(frameRect); https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:263: if (!buffer.setSize(scaledSize().width(), scaledSize().height())) Here is another example: scaledSize().width() etc. Since we don't support the enable, it's the same as size().width(). And you would not be the first who wondered about the difference: why use scaledSize() vs size(), etc. Guess my point is the FIXME should be about removing the down sampling code, rather than refactoring with GIF, and the code here would be a lot simpler. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:394: ASSERT(width == buffer.originalFrameRect().width()); Space before this line. And the width assert is now correct. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:395: ASSERT(decodedHeight <= scaledSize().height()); The one could be better. scaledSize().height() = size().height() = canvas height. I think we should use the buffer.originalFrameRect().height() here? You should also use ASSERT_WITH_SECURITY_IMPLICATION, rather than simple ASSERT in both cases. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:77: // Recreates demux object when new data is available, and sets canvas size, format flags, You're trying to tell me again what I can read from the code :) And it would be a very long comment if you itemized all that this involved routine does, and it returns false as times. Maybe you could discuss it all in the change decription if there's something important we need to know about why this routine exists. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:83: // failure, this will mark the image as failed. s/this will mark/marks/ https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:85: // Apply color profile, and perform necessary post-processing for frame disposal. applyPostProcessing() tells me that some post processing is needed. But we don't always apply a color profile, or do frame disposal. Perhaps let the function name stand alone on its own merits, no comment needed. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:88: // Number of rows decoded and post-processed so far in the current frame. It I'd accept the first sentence only. Move this down, and group it with the other member vars.
All done. Thanks for the review! https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:235: // FIXME: This method is very similar to the one in GIFImageDecoder.cpp and should be refactored. On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > I'm not sure we should hitch our wagon to the GIFImageDecoder.cpp. > > The GIF decoder supports image scaling, #if ENABLE(IMAGE_DECODER_DOWNSAMPLING) > to be precise, and that feature has never been enabled in Chrome and it has no > tests. The rest of WEBP decoder code doesn't support it, but the code here has > a bunch of references to it because it was copied from the GIF decoder. We have > better ways to handle the use-case, using compression proxies > > http://blog.chromium.org/2013/03/data-compression-in-chrome-beta-for.html > > for example. Moreover, if we ever wanted to support a client-side image > down-scale, similar to IMAGE_DECODER_DOWNSAMPLING, then we'd most likely use > WEBP's in-built scaling support. Well, the scaledSize() returns size() essentially, if the decoder doesn't support scaling. But yes, removing scaling related code (and also the !frameIndex FIXME, while we're at it) would simplify the method, surely. Cleaned up the method in this regard, and let's forget about refactoring. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:251: const int left = upperBoundScaledX(frameRect.x()); On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > This block of code is an example of the down sampling enable. Since the WEBP > decoder does not support that enable, its ImageDecoder::m_scaled is always false > -- this whole block of left right top bottom goop could be replaced a single > line: > > buffer.setOriginalFrameRect(frameRect); Done. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:263: if (!buffer.setSize(scaledSize().width(), scaledSize().height())) On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > Here is another example: scaledSize().width() etc. Since we don't support the > enable, it's the same as size().width(). And you would not be the first who > wondered about the difference: why use scaledSize() vs size(), etc. > > Guess my point is the FIXME should be about removing the down sampling code, > rather than refactoring with GIF, and the code here would be a lot simpler. Just using size() now https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:394: ASSERT(width == buffer.originalFrameRect().width()); On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > Space before this line. And the width assert is now correct. Done. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:395: ASSERT(decodedHeight <= scaledSize().height()); On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > The one could be better. scaledSize().height() = size().height() = canvas > height. I think we should use the buffer.originalFrameRect().height() here? > > You should also use ASSERT_WITH_SECURITY_IMPLICATION, rather than simple ASSERT > in both cases. Done. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:77: // Recreates demux object when new data is available, and sets canvas size, format flags, On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > You're trying to tell me again what I can read from the code :) And it would be > a very long comment if you itemized all that this involved routine does, and it > returns false as times. Maybe you could discuss it all in the change decription > if there's something important we need to know about why this routine exists. I like to err on the side of explicit commenting in the code; Blink seems to err the other way ;) Anyway, removed the comment. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:83: // failure, this will mark the image as failed. On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > s/this will mark/marks/ Done. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:85: // Apply color profile, and perform necessary post-processing for frame disposal. On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > applyPostProcessing() tells me that some post processing is needed. But we > don't always apply a color profile, or do frame disposal. Perhaps let the > function name stand alone on its own merits, no comment needed. Done. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:88: // Number of rows decoded and post-processed so far in the current frame. It On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > I'd accept the first sentence only. Done. > Move this down, and group it with the other > member vars. Not sure what you meant. Do you want to to bring all member vars down (after all the method declarations)? I feel that this way is better maybe, to keep the number all #ifdefs under control :)
https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:395: ASSERT(decodedHeight <= scaledSize().height()); On 2013/05/03 21:55:20, urvang wrote: > On 2013/05/03 18:15:46, Noel Gordon (Google) wrote: > > The one could be better. scaledSize().height() = size().height() = canvas > > height. I think we should use the buffer.originalFrameRect().height() here? > > > > You should also use ASSERT_WITH_SECURITY_IMPLICATION, rather than simple > ASSERT > > in both cases. > > Done. I don't see the ASSERT_WITH_SECURITY_IMPLICATIONs. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:77: // Recreates demux object when new data is available, and sets canvas size, format flags, > I like to err on the side of explicit commenting in the code; Blink seems > to err the other way ;) Comments don't help if they contain factual errors, which they did in this case. Read the references about comments at the bottom of this page: http://www.chromium.org/blink/coding-style I saw no update to the change description. You could add something explicit there, discussing why this routine exists along with any other important aspects of this change you want to comment about ... Some recent change description examples https://chromium.googlesource.com/chromium/blink/+/eb044f7ad5a3561119266652f1... https://chromium.googlesource.com/chromium/blink/+/c385eb957f7bbb7d462d47db44... to get you going. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:88: // Number of rows decoded and post-processed so far in the current frame. It On 2013/05/03 21:55:20, urvang wrote: > Not sure what you meant. Do you want to to bring all member vars down (after all > the method declarations)? I feel that this way is better maybe, to keep the > number all #ifdefs under control :) Apologies. I meant group the members of this #ifdef group, like so #ifdef WEBP_ICC_ANIMATION_SUPPORT ... void applyPostProcessing(size_t frameIndex); WebPDemuxer* m_demux; WebPDemuxState m_demuxState; bool m_haveAlreadyParsedThisData; bool m_haveReadAnimParams; int m_repetitionCount; int m_decodedHeight; #endif
https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:77: , m_haveReadAnimParams(false) abbrv. https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:266: if ((prevMethod == ImageFrame::DisposeKeep) || (prevMethod == ImageFrame::DisposeNotSpecified)) { prevMethod == ImageFrame::DisposeNotSpecified ? you don't use/need that, right? prevMethod seems redundant is so, so you remove it and use prevBuffer.disposalMethod() in it's place. https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:274: // So, we copy the whole previous buffer, then clear just its frame. This comment is duplicated at line 281. Seems we have one too many. https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:276: // Clearing the first frame, or a frame the size of the whole You addressed the !frameIndex FIXME, but I still see "Clearing the first frame, ..." in this comment. https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:387: ASSERT(width == buffer.originalFrameRect().width()); Missing ASSERT_WITH_SECURITY_IMPLICATION, I think you want const IntRect& frameRect = buffer.originalFrameRect(); ASSERT_WITH_SECURITY_IMPLICATION(width == frameRect.width()); ASSERT_WITH_SECURITY_IMPLICATION(decodedHeight <= frameRect.height()); const int top = frameRect.y(); const int left = frameRect.x();
https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:77: , m_haveReadAnimParams(false) On 2013/05/04 17:43:50, Noel Gordon (Google) wrote: > abbrv. Renamed https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:266: if ((prevMethod == ImageFrame::DisposeKeep) || (prevMethod == ImageFrame::DisposeNotSpecified)) { On 2013/05/04 17:43:50, Noel Gordon (Google) wrote: > prevMethod == ImageFrame::DisposeNotSpecified ? you don't use/need that, right? > > prevMethod seems redundant is so, so you remove it and use > prevBuffer.disposalMethod() in it's place. Done. https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:274: // So, we copy the whole previous buffer, then clear just its frame. On 2013/05/04 17:43:50, Noel Gordon (Google) wrote: > This comment is duplicated at line 281. Seems we have one too many. Again, comes from GIF; but u r right. Removed comment at 281. https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:276: // Clearing the first frame, or a frame the size of the whole On 2013/05/04 17:43:50, Noel Gordon (Google) wrote: > You addressed the !frameIndex FIXME, but I still see "Clearing the first frame, > ..." in this comment. Removed https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:387: ASSERT(width == buffer.originalFrameRect().width()); On 2013/05/04 17:43:50, Noel Gordon (Google) wrote: > Missing ASSERT_WITH_SECURITY_IMPLICATION, I think you want > > const IntRect& frameRect = buffer.originalFrameRect(); > ASSERT_WITH_SECURITY_IMPLICATION(width == frameRect.width()); > ASSERT_WITH_SECURITY_IMPLICATION(decodedHeight <= frameRect.height()); > const int top = frameRect.y(); > const int left = frameRect.x(); > Done.
https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:274: // So, we copy the whole previous buffer, then clear just its frame. On 2013/05/06 19:10:36, urvang wrote: > On 2013/05/04 17:43:50, Noel Gordon (Google) wrote: > > This comment is duplicated at line 281. Seems we have one too many. > Again, comes from GIF; but u r right. Removed comment at 281. Should be the other way around. Remove the comment from here (274), and retain the comment at 281.
https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/69001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:274: // So, we copy the whole previous buffer, then clear just its frame. On 2013/05/07 02:37:17, Noel Gordon (Google) wrote: > Should be the other way around. Remove the comment from here (274), and retain > the comment at 281. This is subjective to be honest: I was thinking the comment here explains what the whole block does. But alright, moved to 281.
Nothing subjective here: we copy and clear the frame in one place only, exactly like the GIF decoder http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/image-decoder...
> Nothing subjective here: we copy and clear the frame in one place only, exactly > like the GIF decoder I meant something different, but it's gonna be hard explaining over mail. Anyway, new placement of comment is ok. P.S. Took care of a couple of previous comments as well. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:77: // Recreates demux object when new data is available, and sets canvas size, format flags, On 2013/05/04 17:23:43, Noel Gordon (Google) wrote: > I saw no update to the change description. You could add something explicit > there, discussing why this routine exists along with any other important aspects > of this change you want to comment about ... > > Some recent change description examples > > https://chromium.googlesource.com/chromium/blink/+/eb044f7ad5a3561119266652f1... > https://chromium.googlesource.com/chromium/blink/+/c385eb957f7bbb7d462d47db44... > > to get you going. I added notes about the major methods added in the change description. https://codereview.chromium.org/13980003/diff/64001/Source/core/platform/imag... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:88: // Number of rows decoded and post-processed so far in the current frame. It On 2013/05/04 17:23:43, Noel Gordon (Google) wrote: > On 2013/05/03 21:55:20, urvang wrote: > > > Not sure what you meant. Do you want to to bring all member vars down (after > all > > the method declarations)? I feel that this way is better maybe, to keep the > > number all #ifdefs under control :) > > Apologies. I meant group the members of this #ifdef group, like so > > #ifdef WEBP_ICC_ANIMATION_SUPPORT > ... > void applyPostProcessing(size_t frameIndex); > > WebPDemuxer* m_demux; > WebPDemuxState m_demuxState; > bool m_haveAlreadyParsedThisData; > bool m_haveReadAnimParams; > int m_repetitionCount; > int m_decodedHeight; > #endif Done.
On 2013/05/07 06:53:44, urvang wrote: > > P.S. Took care of a couple of previous comments as well. I was wondering about those, thanks for addressing them.
Hi Noel, Alpha, Implemented the two methods added recently in: https://chromiumcodereview.appspot.com/14317005/ Pls have another look.
On 2013/05/08 23:45:33, urvang wrote: > Hi Noel, Alpha, > Implemented the two methods added recently in: > https://chromiumcodereview.appspot.com/14317005/ I wondered if the default ImageDecoder::frameIsCompleteAtIndex() implementation be sufficent? Since Alpha changed the meaning of frameIsCompleteAtIndex(), maybe he tell us about the exact requirements here. I noticed that since Alpha'a change, then with this patch, some webp images no longer animate like their equivalent gif image, for example: ~urvang/anim2/5053654b78c67fb221c7dfa9439045f3.webp ~urvang/anim2/5053654b78c67fb221c7dfa9439045f3.gif Not sure why. They certainly did animate alike before the change.
frameIsCompleteAtIndex() now means that a frame is fully loaded (data complete). The default implementation checks for a fully decode frame which implies a frame is fully loaded. Even if frameIsCompleteAtIndex() returns false the file should still animate as long as the entire file fully downloaded. frameIsCompleteAtIndex() is only useful when the animation happens when downloading is in progress.
On Thu, May 9, 2013 at 11:08 AM, <hclam@chromium.org> wrote: > frameIsCompleteAtIndex() now means that a frame is fully loaded (data > complete). > The default implementation checks for a fully decode frame which implies a > frame > is fully loaded. > > Even if frameIsCompleteAtIndex() returns false the file should still > animate ase > long as the entire file fully downloaded. frameIsCompleteAtIndex() is only > useful when the animation happens when downloading is in progress. > Noel is right. To be precise: what I noticed is that for a brief period (maybe the first run of animation), the animation of WebP file behaves differently than the GIF one (it's either faster or slower). And later, it runs correctly. It seems more related to frame duration to me. @Alpha: Could you look at my frameIsCompleteAtIndex() and frameDurationAtIndex() implementations to see if I've missed something, or if I'm doing something that is different than what GIF does. > > https://codereview.chromium.**org/13980003/<https://codereview.chromium.org/1... >
After Alpha's change, frameIsCompleteAtIndex() and frameDurationAtIndex() don't decode() anymore (they are const). BitmapImage::startAnimation() uses those routines to kick off the initial animation timers. However, all the frame durations are 0 for the initial animation run, and are forced to 100ms duration in ImageSource::frameDurationAtIndex() as a result, because the frame durations are only known once a decode() happens via frameBufferAtIndex(). The fix is to record the frame durations earlier. Since the animation system always calls frameCount() first, we can use it to read the frame durations of loaded frames. frameCount() calls updateDemuxer() so that seems the place to do it: bool WEBPImageDecoder::updateDemuxer() { ... if (newFrameCount > m_frameBufferCache.size()) { m_frameBufferCache.resize(newFrameCount); for (size_t i = 0; i < newFrameCount; ++i) { m_frameBufferCache[i].setPremultiplyAlpha(m_premultiplyAlpha); if (!hasAnimation) continue; WebPIterator animatedFrame; bool frameIsLoadedAtIndex = false; if (WebPDemuxGetFrame(m_demux, i + 1, &animatedFrame)) frameIsLoadedAtIndex = animatedFrame.complete; if (frameIsLoadedAtIndex) m_frameBufferCache[i].setDuration(animatedFrame.duration); WebPDemuxReleaseIterator(&animatedFrame); } } return true; } frameIsCompleteAtIndex() has to 1) handle the non-animated image case and we should call the default implementation for that, and 2) for animated images, work out if the animated frame is loaded (data complete). Something like: bool WEBPImageDecoder::frameIsCompleteAtIndex(size_t index) const { if (!RuntimeEnabledFeatures::animatedWebPEnabled()) return ImageDecoder::frameIsCompleteAtIndex(index); if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER) return false; if (!(m_formatFlags & ANIMATION_FLAG)) return ImageDecoder::frameIsCompleteAtIndex(index); if (isAllDataReceived()) return ImageDecoder::frameIsCompleteAtIndex(index); bool frameIsLoadedAtIndex = false; if (index >= m_frameBufferCache.size()) return false; WebPIterator animatedFrame; if (WebPDemuxGetFrame(m_demux, index + 1, &animatedFrame)) frameIsLoadedAtIndex = animatedFrame.complete; WebPDemuxReleaseIterator(&animatedFrame); return frameIsLoadedAtIndex; } [Aside: it feels dodgy to me that frameIsCompleteAtIndex() can now return true for undecoded animated image frames. frameHasAlphaAtIndex() uses it and so does the meta-data caching system in BitmapImage ... ]. Finally, the animation system calls frameDurationAtIndex() for the loaded frames. Since the durations of loaded frames were stored in updateDemuxer(), we can just return the frame's duration: float WEBPImageDecoder::frameDurationAtIndex(size_t index) const { if (index < m_frameBufferCache.size()) return m_frameBufferCache[index].duration(); return 0; } Another thing. BitmapImage::repetitionCount() comments that decoders should return cAnimationLoopOnce when the decoder is uncertain about the repetition count. int WEBPImageDecoder::repetitionCount() const { if (failed() || !m_haveReadAnimParams) return cAnimationLoopOnce; return m_repetitionCount; }
Thanks for looking into this Noel! The updated patch works correctly. On 2013/05/13 23:31:01, Noel Gordon (Google) wrote: > After Alpha's change, frameIsCompleteAtIndex() and frameDurationAtIndex() don't > decode() anymore (they are const). BitmapImage::startAnimation() uses those > routines to kick off the initial animation timers. However, all the frame > durations are 0 for the initial animation run, and are forced to 100ms duration > in ImageSource::frameDurationAtIndex() as a result, because the frame durations > are only known once a decode() happens via frameBufferAtIndex(). > > The fix is to record the frame durations earlier. Since the animation system > always calls frameCount() first, we can use it to read the frame durations of > loaded frames. frameCount() calls updateDemuxer() so that seems the place to do > it: > > bool WEBPImageDecoder::updateDemuxer() > { > ... > > if (newFrameCount > m_frameBufferCache.size()) { > m_frameBufferCache.resize(newFrameCount); > for (size_t i = 0; i < newFrameCount; ++i) { > m_frameBufferCache[i].setPremultiplyAlpha(m_premultiplyAlpha); > if (!hasAnimation) > continue; > WebPIterator animatedFrame; > bool frameIsLoadedAtIndex = false; > if (WebPDemuxGetFrame(m_demux, i + 1, &animatedFrame)) > frameIsLoadedAtIndex = animatedFrame.complete; > if (frameIsLoadedAtIndex) > m_frameBufferCache[i].setDuration(animatedFrame.duration); > WebPDemuxReleaseIterator(&animatedFrame); > } > } > > return true; > } > Indeed, I was thinking about this as well. A slight concern was this duplicate call to WebPDemuxGetFrame(), but no issues there as this is lightweight -- just returns a reference to that frame data from demux. Done. > frameIsCompleteAtIndex() has to 1) handle the non-animated image case and we > should call the default implementation for that, and 2) for animated images, > work out if the animated frame is loaded (data complete). Something like: > > bool WEBPImageDecoder::frameIsCompleteAtIndex(size_t index) const > { > if (!RuntimeEnabledFeatures::animatedWebPEnabled()) > return ImageDecoder::frameIsCompleteAtIndex(index); > > if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER) > return false; > if (!(m_formatFlags & ANIMATION_FLAG)) > return ImageDecoder::frameIsCompleteAtIndex(index); > if (isAllDataReceived()) > return ImageDecoder::frameIsCompleteAtIndex(index); > > bool frameIsLoadedAtIndex = false; > if (index >= m_frameBufferCache.size()) > return false; > WebPIterator animatedFrame; > if (WebPDemuxGetFrame(m_demux, index + 1, &animatedFrame)) > frameIsLoadedAtIndex = animatedFrame.complete; > WebPDemuxReleaseIterator(&animatedFrame); > > return frameIsLoadedAtIndex; > } Modified this method as u suggested, with some differences: - We never call frameIsCompleteAtIndex() of the base class, as this default implementation is too strict (it doesn't say the frame is complete until it is fully decoded). We just use GetFrame() instead. - Even if animatedWebPEnabled() is false, GetFrame() method works fine (as UpdateDemux() would have set the m_frameBuffer.size() correctly). > > [Aside: it feels dodgy to me that frameIsCompleteAtIndex() can now return true > for undecoded animated image frames. frameHasAlphaAtIndex() uses it and so does > the meta-data caching system in BitmapImage ... ]. I'm also setting buffer.setHasAlpha() whenever we resize m_frameBufferCache, so that frameHasAlphaAtIndex() returns the right value. > > Finally, the animation system calls frameDurationAtIndex() for the loaded > frames. Since the durations of loaded frames were stored in updateDemuxer(), we > can just return the frame's duration: > > float WEBPImageDecoder::frameDurationAtIndex(size_t index) const > { > if (index < m_frameBufferCache.size()) > return m_frameBufferCache[index].duration(); > return 0; > } Done. > > Another thing. BitmapImage::repetitionCount() comments that decoders should > return cAnimationLoopOnce when the decoder is uncertain about the repetition > count. > > int WEBPImageDecoder::repetitionCount() const { > if (failed() || !m_haveReadAnimParams) > return cAnimationLoopOnce; > return m_repetitionCount; > } Note that m_repetitionCount is initialized to cAnimationLoopOnce already, so we don't need to do this handling inside repetitionCount().
> > int WEBPImageDecoder::repetitionCount() const { > > if (failed() || !m_haveReadAnimParams) > > return cAnimationLoopOnce; > > return m_repetitionCount; > > } > > Note that m_repetitionCount is initialized to cAnimationLoopOnce already, so we > don't need to do this handling inside repetitionCount(). Did you read GIFImageDecoder::repetitionCount() at all? Can enter the failed() state at any time after reading the repetition count?
On 2013/05/14 06:20:55, Noel Gordon (Google) wrote: > > > int WEBPImageDecoder::repetitionCount() const { > > > if (failed() || !m_haveReadAnimParams) > > > return cAnimationLoopOnce; > > > return m_repetitionCount; > > > } > > > > Note that m_repetitionCount is initialized to cAnimationLoopOnce already, so > we > > don't need to do this handling inside repetitionCount(). > > Did you read GIFImageDecoder::repetitionCount() at all? Can enter the failed() > state at any time after reading the repetition count? OK, added the special case of failed() state. I'm still a little perplexed though: why would repetitionCount() be called at all (or any other decoder method, for that matter) once a decoder enters a failed state? As to the '!m_haveReadAnimationParameters' part above, we don't need that special case as m_repetitionCount and m_haveReadAnimationParameters are set at the same time (and have proper default values before).
On 2013/05/14 10:26:18, urvang wrote: > OK, added the special case of failed() state. I'm still a little perplexed > though: why would repetitionCount() be called at all (or any other decoder > method, for that matter) once a decoder enters a failed state? https://bugs.webkit.org/show_bug.cgi?id=111144 maybe has the answer?
> > Noel wrote: > > frameIsCompleteAtIndex() has to 1) handle the non-animated image case and we > > should call the default implementation for that, and 2) for animated images, > > work out if the animated frame is loaded (data complete). Something like: > > [snip] > Urvang wrote: > Modified this method as u suggested, with some differences: > > - We never call frameIsCompleteAtIndex() of the base class, as this default > implementation is too strict (it doesn't say the frame is complete until it is > fully decoded). We just use GetFrame() instead. > - Even if animatedWebPEnabled() is false, GetFrame() method works fine (as > UpdateDemux() would have set the m_frameBuffer.size() correctly). frameIsCompleteAtIndex() is also used outside the animation system, with non-animated images. And the default worked for non-animated webp images before, it was not broken, right? Now you say it's "too strict" and have changed behavior for non-animated webp images. Why was this change needed? How have you tested this change? What are the implications of this change on the behavior of the ImageSource and BitmapImage classes?
message: On 2013/05/14 11:50:56, Noel Gordon (Google) wrote: > > > Noel wrote: > > > frameIsCompleteAtIndex() has to 1) handle the non-animated image case and we > > > should call the default implementation for that, and 2) for animated images, > > > work out if the animated frame is loaded (data complete). Something like: > > > [snip] > > > Urvang wrote: > > Modified this method as u suggested, with some differences: > > > > - We never call frameIsCompleteAtIndex() of the base class, as this default > > implementation is too strict (it doesn't say the frame is complete until it is > > fully decoded). We just use GetFrame() instead. > > - Even if animatedWebPEnabled() is false, GetFrame() method works fine (as > > UpdateDemux() would have set the m_frameBuffer.size() correctly). > > frameIsCompleteAtIndex() is also used outside the animation system, with > non-animated images. I had a look at all the places where ImageDecoder::frameIsCompleteAtIndex() is used in a *non-animation context*, and here's what I found just this one: In BitmapImage, it is used to set 'm_frames[index].m_isComplete'; and thenin dataChanged(), m_isComplete is used to clear the partial frames. > And the default worked for non-animated webp images > before, it was not broken, right? Yes it was working earlier too, because "new frameIsCompleteAtIndex() returns true" implies that the "default frameIsCompleteAtIndex() would also return true". See next comment for more elaboration. > Now you say it's "too strict" and have > changed behavior for non-animated webp images. Why was this change needed? Let me elaborate on what I meant by 'too strict': - The requirement of the method is to return true if the frame is fully *available*. - Now, because the default implementation doesn't know how to infer this, it keeps returning false until the frame is fully *decoded* (which implies that the frame must be fully available too). - Now, imagine that some function (currently or in future) performs some optimization based on whether the frame is fully available. A more precise return value by WEBPImageDecoder::frameIsCompleteAtIndex() might make that function more efficient. > How > have you tested this change? What are the implications of this change on the > behavior of the ImageSource and BitmapImage classes? See my comment above about where this method is used. As to the testing: - frameIsCompleteAtIndex() _is_ called in the overall decoding flow. So, the expected behavior for non-animated WebP images would be tested by existing layout tests. - The expected behavior in case of animated WebP is tested by the unit test. To summarize, One _can_ potentially keep the existing behavior for non-animated images, but I feel that this new implementation is more precise.
Implementation of frameIsCompleteAtIndex() and frameDurationAtIndex() looks good to me.
https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/page/... File Source/core/page/RuntimeEnabledFeatures.in (right): https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/page/... Source/core/page/RuntimeEnabledFeatures.in:68: AnimatedWebP Alphabetize, and add a status=something https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/platf... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/platf... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:115: if (!updateDemuxer()) A simple call to updateDemuxer() should work here right? viz., there's no real need for the if test if I'm reading correctly. https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/platf... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:160: WebPDemuxReleaseIterator(&webpFrame); This iterator release api seems strange. You release here, but have returns above this line that don't call the release api. https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/platf... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:193: if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER || index >= m_frameBufferCache.size()) Seems we have no choice but to override this function based on the testing of the kasse.webp image, but that means we then have to handle the animated and non-animated cases with care. For the non-animated image case, m_demuxState == WEBP_DEMUX_DONE means we have received enough to decode, but it doesn't really mean frameIsCompleteAtIndex because decoding could fail. Perhaps handle the non-animated case first as follows: if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER) return false; if (!(m_formatFlags & ANIMATION_FLAG)) return ImageDecoder::frameIsCompleteAtIndex(index); viz., use the default frameIsCompleteAtIndex impl for non-animated images. Then finish off the routine with the animated image case, perhaps something like: bool frameIsLoadedAtIndex = index < m_frameBufferCache.size(); return frameIsLoadedAtIndex; https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/platf... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:235: if (!RuntimeEnabledFeatures::animatedWebPEnabled() && hasAnimation) if (hasAnimation && !RuntimeEnabledFeatures::animatedWebPEnabled()) https://chromiumcodereview.appspot.com/13980003/diff/125001/Source/core/platf... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:260: ASSERT(getSuccess == 1 && animatedFrame.complete == 1); // Always true for animated case. Would ASSERT(animatedFrame.complete); be sufficient here?
Apart from incorporating the suggestions, there are some major changes in this new patch to adapt to this recent change in anim code: https://chromiumcodereview.appspot.com/15969015 PTAL. https://codereview.chromium.org/13980003/diff/125001/Source/core/page/Runtime... File Source/core/page/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/13980003/diff/125001/Source/core/page/Runtime... Source/core/page/RuntimeEnabledFeatures.in:68: AnimatedWebP On 2013/05/29 16:43:29, Noel Gordon (Google) wrote: > Alphabetize, and add a status=something Done. https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:115: if (!updateDemuxer()) On 2013/05/29 16:43:29, Noel Gordon (Google) wrote: > A simple call to updateDemuxer() should work here right? viz., there's no real > need for the if test if I'm reading correctly. One would like isSizeAvailable() call to be as lightweight as possible. So, we don't want to call updateDemuxer() unless we need to. https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:160: WebPDemuxReleaseIterator(&webpFrame); On 2013/05/29 16:43:29, Noel Gordon (Google) wrote: > This iterator release api seems strange. You release here, but have returns > above this line that don't call the release api. You are right, we need to call release before return at line#156. Done. (Note: the same is not needed at line#154 though, as a failed GetFrame() wouldn't assign anything to webpFrame. Another note: for now, this release API is just a placeholder. Nothing really needs to be deallocated there as of now. But we need to be future proof). https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:193: if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER || index >= m_frameBufferCache.size()) On 2013/05/29 16:43:29, Noel Gordon (Google) wrote: > Seems we have no choice but to override this function based on the testing of > the kasse.webp image, but that means we then have to handle the animated and > non-animated cases with care. > > For the non-animated image case, m_demuxState == WEBP_DEMUX_DONE means we have > received enough to decode, but it doesn't really mean frameIsCompleteAtIndex > because decoding could fail. Perhaps handle the non-animated case first as > follows: > > if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER) > return false; > if (!(m_formatFlags & ANIMATION_FLAG)) > return ImageDecoder::frameIsCompleteAtIndex(index); > > viz., use the default frameIsCompleteAtIndex impl for non-animated images. Then > finish off the routine with the animated image case, perhaps something like: > > bool frameIsLoadedAtIndex = index < m_frameBufferCache.size(); > return frameIsLoadedAtIndex; Done. https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:235: if (!RuntimeEnabledFeatures::animatedWebPEnabled() && hasAnimation) On 2013/05/29 16:43:29, Noel Gordon (Google) wrote: > if (hasAnimation && !RuntimeEnabledFeatures::animatedWebPEnabled()) Done. https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:260: ASSERT(getSuccess == 1 && animatedFrame.complete == 1); // Always true for animated case. On 2013/05/29 16:43:29, Noel Gordon (Google) wrote: > Would ASSERT(animatedFrame.complete); be sufficient here? We don't really have any initialization for 'animatedFrame', so I'd play safe and check return value too.
https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:115: if (!updateDemuxer()) On 2013/06/06 20:20:29, urvang wrote: > On 2013/05/29 16:43:29, Noel Gordon (Google) wrote: > > A simple call to updateDemuxer() should work here right? viz., there's no real > > need for the if test if I'm reading correctly. > > One would like isSizeAvailable() call to be as lightweight as possible. So, we > don't want to call updateDemuxer() unless we need to. But you do call updateDemuxer() here and test it's return value in an If. My suggestion was the if (test) is not needed ... if (!ImageDecoder::isSizeAvailable()) #ifdef WEBP_ICC_ANIMATION_SUPPORT updateDemuxer(); #else decode(reinterpret_cast<const uint8_t*>(m_data->data()), ... #endif return ImageDecoder::isSizeAvailable(); so that this function really does tell the caller if the image size is available. https://codereview.chromium.org/13980003/diff/125001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:260: ASSERT(getSuccess == 1 && animatedFrame.complete == 1); // Always true for animated case. On 2013/06/06 20:20:29, urvang wrote: > On 2013/05/29 16:43:29, Noel Gordon (Google) wrote: > > Would ASSERT(animatedFrame.complete); be sufficient here? > > We don't really have any initialization for 'animatedFrame', so I'd play safe > and check return value too. The code sweepers come though our code-base and find that getSuccess is an unused variable. Can we get rid of it?
https://codereview.chromium.org/13980003/diff/141001/Source/WebKit/chromium/p... File Source/WebKit/chromium/public/WebRuntimeFeatures.h (right): https://codereview.chromium.org/13980003/diff/141001/Source/WebKit/chromium/p... Source/WebKit/chromium/public/WebRuntimeFeatures.h:1: /* Looks to me like this file was re-written by you. Not sure this file needs to be changed.
/code-review tool borken 211 int WEBPImageDecoder::repetitionCount() const 212 { 213 return failed() ? cAnimationLoopOnce : m_repetitionCount; 214 } This was copied from GIF decoder, and there was subsequent discussion about it causing bugs in the GIF decoder. Alpha would know and should advise you about what to do here. For example, should we ditch this test and just return m_repetitionCount given all the changes to BitmapImage and the animation system?
216 bool WEBPImageDecoder::frameIsCompleteAtIndex(size_t index) const 217 { 218 if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER) 219 return false; This code is used without the the enable flag too. Let's be clear about that; start this function with ... if (!RuntimeEnabledFeatures::animatedWebPEnabled()) return ImageDecoder::frameIsCompleteAtIndex(index); if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER) ... 222 // In case of animation, number of complete frames is same as m_frameBufferCache.size() (see updateDemuxer()). This comment has me wondering what does "complete frames" mean anymore. PS. since the review tool is gone, I'll continue when it comes back Monday (next week).
On 2013/06/07 08:19:09, Noel Gordon (Google) wrote: > /code-review tool borken > > 211 int WEBPImageDecoder::repetitionCount() const > 212 { > 213 return failed() ? cAnimationLoopOnce : m_repetitionCount; > 214 } > > This was copied from GIF decoder, and there was subsequent discussion about it > causing bugs in the GIF decoder. Alpha would know and should advise you about > what to do here. For example, should we ditch this test and just return > m_repetitionCount given all the changes to BitmapImage and the animation system? I just talked to Urvang, the code here is correct. The bug was caused by a self-inflicted error in the animation system and is fixed.
Thanks Noel & Alpha! Took care of the suggestions. One comment inline. > 222 // In case of animation, number of complete frames is same as > m_frameBufferCache.size() (see updateDemuxer()). > This comment has me wondering what does "complete frames" mean anymore. A complete frame means that (compressed) data for the whole frame is available. If you meant to ask "when can a frame be NOT complete", the answer is: in case of non-animated WebP. In such a case, GetFrame() can return a partial image as it would make incremental decoding possible later. On 2013/06/07 19:00:22, Alpha wrote: > On 2013/06/07 08:19:09, Noel Gordon (Google) wrote: > > /code-review tool borken > > > > 211 int WEBPImageDecoder::repetitionCount() const > > 212 { > > 213 return failed() ? cAnimationLoopOnce : m_repetitionCount; > > 214 } > > > > This was copied from GIF decoder, and there was subsequent discussion about it > > causing bugs in the GIF decoder. Alpha would know and should advise you about > > what to do here. For example, should we ditch this test and just return > > m_repetitionCount given all the changes to BitmapImage and the animation > system? > > I just talked to Urvang, the code here is correct. The bug was caused by a > self-inflicted error in the animation system and is fixed. Ack.
https://codereview.chromium.org/13980003/diff/141001/Source/WebKit/chromium/p... File Source/WebKit/chromium/public/WebRuntimeFeatures.h (right): https://codereview.chromium.org/13980003/diff/141001/Source/WebKit/chromium/p... Source/WebKit/chromium/public/WebRuntimeFeatures.h:1: /* On 2013/06/07 08:06:21, Noel Gordon (Google) wrote: > Looks to me like this file was re-written by you. Not sure this file needs to > be changed. Ohh, I thought I had take care of this. Fixed now.
> On 2013/06/07 08:19:09, Noel Gordon (Google) wrote: On 2013/06/10 22:11:36, urvang wrote: > > 222 // In case of animation, number of complete frames is same as m_frameBufferCache.size() (see updateDemuxer()). > > > This comment has me wondering what does "complete frames" mean anymore. > > A complete frame means that (compressed) data for the whole frame is available does it? > If you meant to ask "when can a frame be NOT complete", the answer is: in case > of non-animated WebP. In such a case, GetFrame() can return a partial image as > it would make incremental decoding possible later. The function is called frameIsCompleteAtIndex() -- it tells us if a frame is complete, meaning that the frame has been fully decoded. At least it always did until Alpha changed its meaning for animated image frames only to return if an animated frame was "loaded at index". Your comment should probably not say "complete frames" for animated images. That's confusing in the context of this function, eg., does it mean decoded or loaded, etc? bool frameIsLoadedAtIndex = index < m_frameBufferCache.size(); ... See https://codereview.chromium.org/13980003/#msg44
https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:152: frameToDecode = m_frameBufferCache[frameToDecode].requiredPreviousFrameIndex(); In general, you should have qinmin to look over all areas of your change that deal with the new code he added. Still, I recall that non-animated images come through this do loop too. So what prevents the requiredPreviousFrameIndex() ASSERT about m_requiredPreviousFrameIndexValid in the non-animated case? https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:161: for (Vector<size_t>::const_reverse_iterator iter = framesToDecode.rbegin(); iter != rend; ++iter) { 156-161: I wonder if a reverse index over the vector save us from all this trouble ... for (size_t i = framesToDecode.size(); i > 0; --i) { size_t frameIndex = framesToDecode[i - 1]; ... https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:356: // Clear the decoder state so that image can be decoded again when requested. Sentences: "that image"? Did you mean to say "the image" or "the frame" ? https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:477: // Note: if the requiredPreviousFrameIndex is |notFound|, there's nothing to do. The previous frame buffer will be combined with the current frame buffer in the for-loop below. I assume that means the previous frame should already be decoded (frameComplete). If so, should we ASSERT that (we do at line 462)? https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:53: virtual ImageFrame* frameBufferAtIndex(size_t index) OVERRIDE; nit: s/index//
Reviewed code related to requiredPreviousFrameIndex. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:161: for (Vector<size_t>::const_reverse_iterator iter = framesToDecode.rbegin(); iter != rend; ++iter) { On 2013/06/13 15:50:17, Noel Gordon (Google) wrote: > 156-161: I wonder if a reverse index over the vector save us from all this > trouble ... > > for (size_t i = framesToDecode.size(); i > 0; --i) { > size_t frameIndex = framesToDecode[i - 1]; > ... Urvang, when you change this, please also change the similar code in GIFImageDecoder.cpp into this style. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:308: const size_t requiredPreviousFrameIndex = findRequiredPreviousFrame(frameIndex); Why not using buffer.requiredPreviousFrameIndex() here? https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:321: For GIF, if the previous frame has DisposeOverwriteBgcolor and it's rect doesn't fully cover the image, only the covered part will be cleared to background (GIFImageDecoder.cpp:350-360). Does WEBP work like this? https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:329: size_t WEBPImageDecoder::clearCacheExceptFrame(size_t clearExceptFrame) This method seems the same as the ImageDecoder::clearCacheExceptFrame().
Thanks Noel and Xianzhu! > Your comment should probably not say > "complete frames" for animated images. That's confusing in the context of > this function, eg., does it mean decoded or loaded, etc? > bool frameIsLoadedAtIndex = index < m_frameBufferCache.size(); > ... Humm, that clarifies the point. As discussed offline, I had a look at the "comments about comments" discussion; specifically how Blink style recommends replacing comments with code wherever possible. So, using 'frameIsLoadedAtIndex' as suggested. Also, got rid of some other trivial comments. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:152: frameToDecode = m_frameBufferCache[frameToDecode].requiredPreviousFrameIndex(); On 2013/06/13 15:50:17, Noel Gordon (Google) wrote: > In general, you should have qinmin to look over all areas of your change that > deal with the new code he added. I requested him to review the same (he has just commented). > > Still, I recall that non-animated images come through this do loop too. So what > prevents the requiredPreviousFrameIndex() ASSERT about > m_requiredPreviousFrameIndexValid in the non-animated case? > This will work correctly for non-animated case too. This is because 'requiredPreviousFrameIndex' for each frame is set by calling findRequiredPreviousFrame(). And, findRequiredPreviousFrame(index) returns 'notFound' for index == 0 (see: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:161: for (Vector<size_t>::const_reverse_iterator iter = framesToDecode.rbegin(); iter != rend; ++iter) { On 2013/06/13 15:50:17, Noel Gordon (Google) wrote: > 156-161: I wonder if a reverse index over the vector save us from all this > trouble ... > > for (size_t i = framesToDecode.size(); i > 0; --i) { > size_t frameIndex = framesToDecode[i - 1]; > ... Oh ya, I was wondering the same (had just copied this from GIF code). In fact, the Blink style guide also prefers indexes over iterators for vectors. So, simplified as suggested. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:161: for (Vector<size_t>::const_reverse_iterator iter = framesToDecode.rbegin(); iter != rend; ++iter) { On 2013/06/13 20:04:19, Xianzhu wrote: > On 2013/06/13 15:50:17, Noel Gordon (Google) wrote: > > 156-161: I wonder if a reverse index over the vector save us from all this > > trouble ... > > > > for (size_t i = framesToDecode.size(); i > 0; --i) { > > size_t frameIndex = framesToDecode[i - 1]; > > ... > > Urvang, when you change this, please also change the similar code in > GIFImageDecoder.cpp into this style. Done. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:308: const size_t requiredPreviousFrameIndex = findRequiredPreviousFrame(frameIndex); On 2013/06/13 20:04:19, Xianzhu wrote: > Why not using buffer.requiredPreviousFrameIndex() here? Oh yes, thanks! https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:321: On 2013/06/13 20:04:19, Xianzhu wrote: > For GIF, if the previous frame has DisposeOverwriteBgcolor and it's rect doesn't > fully cover the image, only the covered part will be cleared to background > (GIFImageDecoder.cpp:350-360). Does WEBP work like this? Oh yes, that's required here too (I did have the same in earlier patches). Bad copy-paste error on my part. Thanks for catching! https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:329: size_t WEBPImageDecoder::clearCacheExceptFrame(size_t clearExceptFrame) On 2013/06/13 20:04:19, Xianzhu wrote: > This method seems the same as the ImageDecoder::clearCacheExceptFrame(). There is one subtle difference, actually, due to a slight difference in GIF and WebP implementations. (See comment at #454 - 457). But you are right, this is worthy of a comment. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:356: // Clear the decoder state so that image can be decoded again when requested. On 2013/06/13 15:50:17, Noel Gordon (Google) wrote: > Sentences: "that image"? Did you mean to say "the image" or "the frame" ? "so that" is meant to be read together here. Tweaked this to "so that this partial frame ..." to be more precise. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:477: // Note: if the requiredPreviousFrameIndex is |notFound|, there's nothing to do. On 2013/06/13 15:50:17, Noel Gordon (Google) wrote: > The previous frame buffer will be combined with the current frame buffer in the > for-loop below. I assume that means the previous frame should already be > decoded (frameComplete). If so, should we ASSERT that (we do at line 462)? Indeed, added the ASSERT. https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:53: virtual ImageFrame* frameBufferAtIndex(size_t index) OVERRIDE; On 2013/06/13 15:50:17, Noel Gordon (Google) wrote: > nit: s/index// Done.
https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... 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: > On 2013/06/13 20:04:19, Xianzhu wrote: > > This method seems the same as the ImageDecoder::clearCacheExceptFrame(). > > There is one subtle difference, actually, due to a slight difference in GIF and > WebP implementations. (See comment at #454 - 457). > > But you are right, this is worthy of a comment. Thanks for the explanation. I put clearCacheExceptFrame() in ImageDecoder because I thought it would be common among different decoders, but now WEB is different. clearCacheExceptFrame() should be virtual. A question about the requirement chain. It seems that for WEBP, a frame can require multiple previous frames at the same time, right? For example, A <- B <- C ('<-' stands for the requiredPreviousFrameIndex relationship), C will depend on both A and B if B is partial, and on clearCacheExceptFrame(C) all A, B, C should be preserved, right? For GIFImageDecoder, if a frame requires a previous frame, it's initial state depends on only on that frame because there is no alpha blending, so at most only one frame will be preserved in clearCacheExceptFrame() (either the clearExceptFrame itself, or a previous non-empty frame if clearExceptFrame is empty).
Hi, On 2013/06/14 17:50:26, Xianzhu wrote: > https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... > File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): > > https://codereview.chromium.org/13980003/diff/148001/Source/core/platform/ima... > 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: > > On 2013/06/13 20:04:19, Xianzhu wrote: > > > This method seems the same as the ImageDecoder::clearCacheExceptFrame(). > > > > There is one subtle difference, actually, due to a slight difference in GIF > and > > WebP implementations. (See comment at #454 - 457). > > > > But you are right, this is worthy of a comment. > > Thanks for the explanation. I put clearCacheExceptFrame() in ImageDecoder > because I thought it would be common among different decoders, but now WEB is > different. > > clearCacheExceptFrame() should be virtual. Done. > > A question about the requirement chain. It seems that for WEBP, a frame can > require multiple previous frames at the same time, right? For example, A <- B <- > C ('<-' stands for the requiredPreviousFrameIndex relationship), C will depend > on both A and B if B is partial, and on clearCacheExceptFrame(C) all A, B, C > should be preserved, right? > > For GIFImageDecoder, if a frame requires a previous frame, it's initial state > depends on only on that frame because there is no alpha blending, so at most > only one frame will be preserved in clearCacheExceptFrame() (either the > clearExceptFrame itself, or a previous non-empty frame if clearExceptFrame is > empty). You are right about GIF: only one frame would be retained after every call to clearCacheExceptFrame() always. And to keep it simple, I wanted to keep similar behavior in WebP too: That is, always preserve only one frame after clearCacheExceptFrame(): - The clearExceptFrame itself if it is in status "FrameComplete" - A previous frame in status FrameComplete in the dependency chain otherwise So, for your example chain A <- B <- C, clearCacheExceptFrame(C) would retain only A (if C was not complete). In fact, the comments weren't reflecting this correctly. Reworded them.
The parts related to requiredPreviousFrameIndex look good to me.
On 2013/06/14 01:13:12, urvang wrote: > > > > Still, I recall that non-animated images come through this do loop too. So what > > prevents the requiredPreviousFrameIndex() ASSERT about > > m_requiredPreviousFrameIndexValid in the non-animated case? > > > > This will work correctly for non-animated case too. This is because > 'requiredPreviousFrameIndex' for each frame is set by calling > findRequiredPreviousFrame(). And, findRequiredPreviousFrame(index) returns > 'notFound' for index == 0. Where in WEBPImageDecoder.cpp does findRequiredPreviousFrame() get called for the non-animated case?
On 2013/06/15 02:45:36, Noel Gordon (Google) wrote: > On 2013/06/14 01:13:12, urvang wrote: > > > > > > Still, I recall that non-animated images come through this do loop too. So > what > > > prevents the requiredPreviousFrameIndex() ASSERT about > > > m_requiredPreviousFrameIndexValid in the non-animated case? > > > > > > > This will work correctly for non-animated case too. This is because > > 'requiredPreviousFrameIndex' for each frame is set by calling > > findRequiredPreviousFrame(). And, findRequiredPreviousFrame(index) returns > > 'notFound' for index == 0. > > Where in WEBPImageDecoder.cpp does findRequiredPreviousFrame() get called for > the non-animated case? Sorry, the correct answer is that 'requiredPreviousFrameIndex' is initialized to 'notFound' for every frame: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Yes, and m_requiredPreviousFrameIndexValid is initially false too: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Now call requiredPreviousFrameIndex(), is it the one here? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Sent from mobile. Excuse typos. On Jun 14, 2013 8:15 PM, <noel@chromium.org> wrote: > > Yes, and m_requiredPreviousFrameIndexValid is initially false too: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Now call requiredPreviousFrameIndex(), is it the one here? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Humm... So the it's going to fail in debug build. While at it, let me build and test the debug build for good. (Away from comp, will update soon) > > https://codereview.chromium.org/13980003/
On 2013/06/15 04:51:34, urvang wrote: > Sent from mobile. Excuse typos. > On Jun 14, 2013 8:15 PM, <mailto:noel@chromium.org> wrote: > > > > Yes, and m_requiredPreviousFrameIndexValid is initially false too: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Now call requiredPreviousFrameIndex(), is it the one here? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Humm... So the it's going to fail in debug build. > > While at it, let me build and test the debug build for good. > (Away from comp, will update soon) > Fixed this case and made sure all is well in the debug build. > > > > https://codereview.chromium.org/13980003/
Anything preventing you from sending this patch to the try servers? https://codereview.chromium.org/13980003/diff/192001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/192001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:217: const bool frameIsLoadedAtIndex = index < m_frameBufferCache.size(); You and constedness, s/const//
Hi Noel, Thanks! Applied the suggestions discussed offline. I'll run the trybots when I get the permission. https://codereview.chromium.org/13980003/diff/192001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/192001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:217: const bool frameIsLoadedAtIndex = index < m_frameBufferCache.size(); On 2013/06/18 06:26:35, Noel Gordon (Google) wrote: > You and constedness, s/const// Done.
> I'll run the trybots when I get the permission. Righto.
https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp:46: #include <base/basictypes.h> you shouldn't use chromium's base library in blink, including in unit tests.
#include "wtf/dtoa/utils.h" instead, and use its ARRAY_SIZE macro (in place of ARRAYSIZE_UNSAFE).
https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:50: virtual String filenameExtension() OVERRIDE const { return "webp"; } s/OVERRIDE//
Build fix for the missing webp include: jamesr@ suggested off-line that WebKit.gyp needs a libwebp dependency for the component build https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... So add the following line to that grouping: '<(DEPTH)/third_party/libwebp/libwebp.gyp:libwebp',
Fixed compile issues and added a unit test to test "frame doesn't have alpha, but frame background needs alpha" case. https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:50: virtual String filenameExtension() OVERRIDE const { return "webp"; } On 2013/06/21 05:36:26, Noel Gordon (Google) wrote: > s/OVERRIDE// Done. P.S. It's weird, because both release and debug builds are ok on my linux box. Also, GIF uses OVERRIDE for the same method, too. Anyway, as long as it works. https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp (right): https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoderTest.cpp:46: #include <base/basictypes.h> On 2013/06/21 04:45:30, jamesr wrote: > you shouldn't use chromium's base library in blink, including in unit tests. Thanks for pointing that out James! Using "wtf/dtoa/utils.h" as per the suggestion.
https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:50: virtual String filenameExtension() OVERRIDE const { return "webp"; } On 2013/06/24 11:02:03, urvang wrote: > On 2013/06/21 05:36:26, Noel Gordon (Google) wrote: > > s/OVERRIDE// > > Done. > > P.S. It's weird, because both release and debug builds are ok on my linux box. > Also, GIF uses OVERRIDE for the same method, too. > Anyway, as long as it works. OVERRIDE is defined to nothing for g++ build. It'll be defined and checked for clang build. Why not using OVERRIDE here?
Hi Xianzhu, On 2013/06/24 16:20:24, Xianzhu wrote: > https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... > File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): > > https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... > Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:50: virtual String > filenameExtension() OVERRIDE const { return "webp"; } > On 2013/06/24 11:02:03, urvang wrote: > > On 2013/06/21 05:36:26, Noel Gordon (Google) wrote: > > > s/OVERRIDE// > > > > Done. > > > > P.S. It's weird, because both release and debug builds are ok on my linux box. > > Also, GIF uses OVERRIDE for the same method, too. > > Anyway, as long as it works. > > OVERRIDE is defined to nothing for g++ build. It'll be defined and checked for > clang build. > > Why not using OVERRIDE here? OVERRIDE, when used for this particular method, was causing trouble for some builds. See: http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds... and http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/441... My guess is: this is because the base class method filenameExtension() is *pure* virtual. So technically, we are *implementing* it here, not *overriding* it.
On 2013/06/24 18:13:47, urvang wrote: > Hi Xianzhu, > > On 2013/06/24 16:20:24, Xianzhu wrote: > > > https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... > > File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): > > > > > https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... > > Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:50: virtual String > > filenameExtension() OVERRIDE const { return "webp"; } > > On 2013/06/24 11:02:03, urvang wrote: > > > On 2013/06/21 05:36:26, Noel Gordon (Google) wrote: > > > > s/OVERRIDE// > > > > > > Done. > > > > > > P.S. It's weird, because both release and debug builds are ok on my linux > box. > > > Also, GIF uses OVERRIDE for the same method, too. > > > Anyway, as long as it works. > > > > OVERRIDE is defined to nothing for g++ build. It'll be defined and checked for > > clang build. > > > > Why not using OVERRIDE here? > > OVERRIDE, when used for this particular method, was causing trouble for some > builds. > See: > http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds... > and > http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/441... > I found the reason of failure: s/OVERRIDE const/const OVERRIDE/ otherwise the compiler thinks the non-const version of the function is being overridden. > My guess is: this is because the base class method filenameExtension() is *pure* > virtual. So technically, we are *implementing* it here, not *overriding* it. There are many places that we are using OVERRIDE for new code in the case, for example WebViewImpl.h. I think implementing a pure virtual function is a kind of overriding, that is, calling the function will call the overriding one instead of the pure virtual one.
On 2013/06/24 18:22:36, Xianzhu wrote: > On 2013/06/24 18:13:47, urvang wrote: > > Hi Xianzhu, > > > > On 2013/06/24 16:20:24, Xianzhu wrote: > > > > > > https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... > > > File Source/core/platform/image-decoders/webp/WEBPImageDecoder.h (right): > > > > > > > > > https://codereview.chromium.org/13980003/diff/213003/Source/core/platform/ima... > > > Source/core/platform/image-decoders/webp/WEBPImageDecoder.h:50: virtual > String > > > filenameExtension() OVERRIDE const { return "webp"; } > > > On 2013/06/24 11:02:03, urvang wrote: > > > > On 2013/06/21 05:36:26, Noel Gordon (Google) wrote: > > > > > s/OVERRIDE// > > > > > > > > Done. > > > > > > > > P.S. It's weird, because both release and debug builds are ok on my linux > > box. > > > > Also, GIF uses OVERRIDE for the same method, too. > > > > Anyway, as long as it works. > > > > > > OVERRIDE is defined to nothing for g++ build. It'll be defined and checked > for > > > clang build. > > > > > > Why not using OVERRIDE here? > > > > OVERRIDE, when used for this particular method, was causing trouble for some > > builds. > > See: > > > http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds... > > and > > > http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/441... > > > > I found the reason of failure: > s/OVERRIDE const/const OVERRIDE/ > otherwise the compiler thinks the non-const version of the function is being > overridden. Ahh I see. That makes sense. I'll let the trybots confirm.
This patch has no layout tests. You should enable the animated webp tests in TestExpectations. https://codereview.chromium.org/13980003/diff/237001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/237001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:242: return setFailed(); // Must be a failure as we have at least 'minSizeForDemux' bytes. 'minSizeForDemux' ?
Enabled the layout tests. https://codereview.chromium.org/13980003/diff/237001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/237001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:242: return setFailed(); // Must be a failure as we have at least 'minSizeForDemux' bytes. On 2013/06/25 01:30:07, Noel Gordon (Google) wrote: > 'minSizeForDemux' ? Removed comment.
LGTM with nits. I still see 'minSizeForDemux' at line 242.
+jamesr and pkasting Could you guys give OWNER's approval pls?
I only gave a cursory review. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); I suspect there's a faster way of doing this, though it may require calling Skia functions directly. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:348: // Otherwise, we preserve a previous frame with status FrameComplete whose data is required to decode |clearExceptFrame, either in initFrameBuffer() or ApplyPostProcessing(). Nit: Missing pipe https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:350: while ((clearExceptFrame < m_frameBufferCache.size()) && (m_frameBufferCache[clearExceptFrame].status() != ImageFrame::FrameComplete)) This function differs only fractionally from the superclass method. Copy and pasting this much code is error-prone. Find a way to share most of the code. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:467: // frame and the previous frame buffer. Argh. It really sucks to do this as another pass after we've decoded. Why can't we integrate this directly into the decode pass? That should be both faster and easier. This would also eliminate the need to override clearCacheExceptFrame().
Thanks Peter! PTAL. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/26 00:18:17, Peter Kasting wrote: > I suspect there's a faster way of doing this, though it may require calling Skia > functions directly. GIFDecoder (https://chromium.googlesource.com/chromium/blink/+/master/Source/core/platfor...) uses the same method for clearing pixels to transparent; so I'm guessing this is the best way. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:348: // Otherwise, we preserve a previous frame with status FrameComplete whose data is required to decode |clearExceptFrame, either in initFrameBuffer() or ApplyPostProcessing(). On 2013/06/26 00:18:17, Peter Kasting wrote: > Nit: Missing pipe Done. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:350: while ((clearExceptFrame < m_frameBufferCache.size()) && (m_frameBufferCache[clearExceptFrame].status() != ImageFrame::FrameComplete)) On 2013/06/26 00:18:17, Peter Kasting wrote: > This function differs only fractionally from the superclass method. Copy and > pasting this much code is error-prone. Find a way to share most of the code. Done. Now, the logic of finding which frame to preserve is part of GIF/WebP decoders. And base class just performs clearing (except the special case of 0 or 1 frames). https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:467: // frame and the previous frame buffer. On 2013/06/26 00:18:17, Peter Kasting wrote: > Argh. It really sucks to do this as another pass after we've decoded. Why > can't we integrate this directly into the decode pass? That should be both > faster and easier. > > This would also eliminate the need to override clearCacheExceptFrame(). Sorry, I should have added you earlier to the review, when this question was answered. To repeat, this is because of the difference between WebP and GIF decode: In case of GIF, here's what happens: - It first decodes current frame into its own buffer. - Then, copies pixel-by-pixel to the ImageFrame buffer -- either the previous frame pixel or current frame pixel -- as per the 'alpha' value. In case of WebP: - It decodes directly into the ImageFrame buffer. (No extra memory is used). [One reason for this is that Decode Row callback is very hard to implement in case of WebP]. - Only when prev frame and current frame have some common area, only that common area would need to be re-looked to see if we should restore pixels to those in prev frame. So, technically, WebP takes less memory (no extra decode buffer) and "should I restore to previous pixel" check happens for smaller area of the frame. I feel these benefits compensate for the complexity.
https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/26 22:12:47, urvang wrote: > On 2013/06/26 00:18:17, Peter Kasting wrote: > > I suspect there's a faster way of doing this, though it may require calling > Skia > > functions directly. > > GIFDecoder > (https://chromium.googlesource.com/chromium/blink/+/master/Source/core/platfor...) > uses the same method for clearing pixels to transparent; so I'm guessing this is > the best way. That's only because before we forked Blink, the GIFDecoder was using generic non-Skia-specific ImageDecoder functions. If we can do better now, we should. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:467: // frame and the previous frame buffer. On 2013/06/26 22:12:47, urvang wrote: > Sorry, I should have added you earlier to the review, when this question was > answered. To repeat, this is because of the difference between WebP and GIF > decode: > > In case of GIF, here's what happens: > - It first decodes current frame into its own buffer. > - Then, copies pixel-by-pixel to the ImageFrame buffer -- either the previous > frame pixel or current frame pixel -- as per the 'alpha' value. > > In case of WebP: > - It decodes directly into the ImageFrame buffer. (No extra memory is used). > [One reason for this is that Decode Row callback is very hard to implement in > case of WebP]. > - Only when prev frame and current frame have some common area, only that common > area would need to be re-looked to see if we should restore pixels to those in > prev frame. > > So, technically, WebP takes less memory (no extra decode buffer) and "should I > restore to previous pixel" check happens for smaller area of the frame. > I feel these benefits compensate for the complexity. I would imagine the common case is that the previous frame and the current frame are both full frames, so we're running the check over the whole frame every frame. That's certainly the common case with GIF. I think we can do better than this. If the WebP decode function can decode directly to an ImageFrame, then also hand it a pointer to the previous ImageFrame (or frame data for it or whatever), so that when it hits a transparent pixel it can do the blend as part of the decode. The GIF decoder could be improved to do this as well by having it decode directly to an ImageFrame instead of using the HaveDecodedRow callback implementation.
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/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/26 22:58:23, Peter Kasting wrote: > On 2013/06/26 22:12:47, urvang wrote: > > On 2013/06/26 00:18:17, Peter Kasting wrote: > > > I suspect there's a faster way of doing this, though it may require calling > > Skia > > > functions directly. > > > > GIFDecoder > > > (https://chromium.googlesource.com/chromium/blink/+/master/Source/core/platfor...) > > uses the same method for clearing pixels to transparent; so I'm guessing this > is > > the best way. > > That's only because before we forked Blink, the GIFDecoder was using generic > non-Skia-specific ImageDecoder functions. If we can do better now, we should. Let me ask this question: do you see any method that can do this better? I did't see any in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Perhaps we could create a method, which, given a frame rectangle, clears all pixels in there to (0, 0, 0, 0) -- using memset(). But, I feel that should belong to its own patch, no? https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:467: // frame and the previous frame buffer. On 2013/06/26 22:58:23, Peter Kasting wrote: > On 2013/06/26 22:12:47, urvang wrote: > > Sorry, I should have added you earlier to the review, when this question was > > answered. To repeat, this is because of the difference between WebP and GIF > > decode: > > > > In case of GIF, here's what happens: > > - It first decodes current frame into its own buffer. > > - Then, copies pixel-by-pixel to the ImageFrame buffer -- either the previous > > frame pixel or current frame pixel -- as per the 'alpha' value. > > > > In case of WebP: > > - It decodes directly into the ImageFrame buffer. (No extra memory is used). > > [One reason for this is that Decode Row callback is very hard to implement in > > case of WebP]. > > - Only when prev frame and current frame have some common area, only that > common > > area would need to be re-looked to see if we should restore pixels to those in > > prev frame. > > > > So, technically, WebP takes less memory (no extra decode buffer) and "should I > > restore to previous pixel" check happens for smaller area of the frame. > > I feel these benefits compensate for the complexity. > > I would imagine the common case is that the previous frame and the current frame > are both full frames, so we're running the check over the whole frame every > frame. That's certainly the common case with GIF. > > I think we can do better than this. If the WebP decode function can decode > directly to an ImageFrame, then also hand it a pointer to the previous > ImageFrame (or frame data for it or whatever), so that when it hits a > transparent pixel it can do the blend as part of the decode. Well, that would be desirable, but it is also non-trivial. Consider what all the decoder will have to take care of (in addition to alpha-blending): - Using disposal method to find out previous required frames. - The frame rectangle of current and previous required frame(s). - Using color profile (dependency on QCMS, not available for all platforms) - With all these, he library would need to work on all platforms. This is almost duplicating what should be the job of a renderer. It would also blow up the size of the library and dependencies. And Given the complexities, I could think about having this support in libwebp in future, but not for now. > > The GIF decoder could be improved to do this as well by having it decode > directly to an ImageFrame instead of using the HaveDecodedRow callback > implementation.
https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/27 01:27:37, urvang wrote: > On 2013/06/26 22:58:23, Peter Kasting wrote: > > On 2013/06/26 22:12:47, urvang wrote: > > > On 2013/06/26 00:18:17, Peter Kasting wrote: > > > > I suspect there's a faster way of doing this, though it may require > calling > > > Skia > > > > functions directly. > > > > > > GIFDecoder > > > > > > (https://chromium.googlesource.com/chromium/blink/+/master/Source/core/platfor...) > > > uses the same method for clearing pixels to transparent; so I'm guessing > this > > is > > > the best way. > > > > That's only because before we forked Blink, the GIFDecoder was using generic > > non-Skia-specific ImageDecoder functions. If we can do better now, we should. > > Let me ask this question: do you see any method that can do this better? I did't > see any in > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Perhaps we could create a method, which, given a frame rectangle, clears all > pixels in there to (0, 0, 0, 0) -- using memset(). > But, I feel that should belong to its own patch, no? In the WebP decoder, we grab the pointer directly from ImageFrame and write directly into memory: (see WebPImageDecoder.cpp:193) uint8_t* dst = reinterpret_cast<uint8_t*>(buffer.getAddr(prevRect.x(), prevRect.y())); memset(dst, 0, (prevRect.maxX() - prevRect.x()) * sizeof(ImageFrame::PixelData)); would work, i think.
https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/27 07:22:27, skal wrote: > On 2013/06/27 01:27:37, urvang wrote: > > Let me ask this question: do you see any method that can do this better? I > did't > > see any in > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Right, that's what I'm saying: up until the fork, that file had to be cross-platform and have no Skia references. Whereas if we have a fast method to clear a rectangle out of an image, we'll need to look in Skia itself, and then we'll want to make that available to both the WebP and GIF decoders. > uint8_t* dst = reinterpret_cast<uint8_t*>(buffer.getAddr(prevRect.x(), > prevRect.y())); > memset(dst, 0, (prevRect.maxX() - prevRect.x()) * > sizeof(ImageFrame::PixelData)); > > would work, i think. It would probably be an improvement. What I was really hoping for in asking this question was for someone to contact Mike Reed (or another Skia wizard) and ask what the fastest method to clear a rectangle out of an image is. https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:467: // frame and the previous frame buffer. On 2013/06/27 01:27:37, urvang wrote: > On 2013/06/26 22:58:23, Peter Kasting wrote: > > I think we can do better than this. If the WebP decode function can decode > > directly to an ImageFrame, then also hand it a pointer to the previous > > ImageFrame (or frame data for it or whatever), so that when it hits a > > transparent pixel it can do the blend as part of the decode. > > Well, that would be desirable, but it is also non-trivial. Consider what all the > decoder will have to take care of (in addition to alpha-blending): > - Using disposal method to find out previous required frames. Not actually necessary. The decoder assumes the embedder passes in the correct image data to composite this frame against. It's the embedder's job to figure out which frame this is, ensure it has the right data, and provide it to the decoder. > - The frame rectangle of current and previous required frame(s). The decoder already has to know the current frame rect in order to write to the correct coordinates within the image. It doesn't actually need to know the previous frame rect, it just composites against the same coordinates in the previous frame. > - Using color profile (dependency on QCMS, not available for all platforms) Ultimately, this seems like the sort of thing the decoder should take care of. However, for now, it seems like this could be implemented as a callout hook to the embedder, basically "here's the raw row data, please do any necessary color correction". The decoder would then call this before compositing. This should be approximately the same speed as your current implementation, I think?
https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... File Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:327: buffer.setRGBA(x, y, 0, 0, 0, 0); On 2013/06/27 17:40:18, Peter Kasting wrote: > On 2013/06/27 07:22:27, skal wrote: > > On 2013/06/27 01:27:37, urvang wrote: > > > Let me ask this question: do you see any method that can do this better? I > > did't > > > see any in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Right, that's what I'm saying: up until the fork, that file had to be > cross-platform and have no Skia references. Whereas if we have a fast method to > clear a rectangle out of an image, we'll need to look in Skia itself, and then > we'll want to make that available to both the WebP and GIF decoders. > > > uint8_t* dst = reinterpret_cast<uint8_t*>(buffer.getAddr(prevRect.x(), > > prevRect.y())); > > memset(dst, 0, (prevRect.maxX() - prevRect.x()) * > > sizeof(ImageFrame::PixelData)); > > > > would work, i think. > > It would probably be an improvement. What I was really hoping for in asking > this question was for someone to contact Mike Reed (or another Skia wizard) and > ask what the fastest method to clear a rectangle out of an image is. As discussed offline, I added a method to zero-fill a frame rectangle using the memset. Contacting Mike offline to find out if there is a better way to do this (separate patch). https://codereview.chromium.org/13980003/diff/249001/Source/core/platform/ima... Source/core/platform/image-decoders/webp/WEBPImageDecoder.cpp:467: // frame and the previous frame buffer. On 2013/06/27 17:40:18, Peter Kasting wrote: > On 2013/06/27 01:27:37, urvang wrote: > > On 2013/06/26 22:58:23, Peter Kasting wrote: > > > I think we can do better than this. If the WebP decode function can decode > > > directly to an ImageFrame, then also hand it a pointer to the previous > > > ImageFrame (or frame data for it or whatever), so that when it hits a > > > transparent pixel it can do the blend as part of the decode. > > > > Well, that would be desirable, but it is also non-trivial. Consider what all > the > > decoder will have to take care of (in addition to alpha-blending): > > - Using disposal method to find out previous required frames. > > Not actually necessary. The decoder assumes the embedder passes in the correct > image data to composite this frame against. It's the embedder's job to figure > out which frame this is, ensure it has the right data, and provide it to the > decoder. That could probably be made to work; some details will have to be worked out though: e.g. The code which outputs the pixels is deep inside the codebase -- which would affect lossy & lossless, incremental and non-incremental decoding. Will have to be written carefully and well-tested. > > > - The frame rectangle of current and previous required frame(s). > > The decoder already has to know the current frame rect in order to write to the > correct coordinates within the image. It doesn't actually need to know the > previous frame rect, it just composites against the same coordinates in the > previous frame. > > > - Using color profile (dependency on QCMS, not available for all platforms) > > Ultimately, this seems like the sort of thing the decoder should take care of. > However, for now, it seems like this could be implemented as a callout hook to > the embedder, basically "here's the raw row data, please do any necessary color > correction". The decoder would then call this before compositing. This should > be approximately the same speed as your current implementation, I think? Actually, the decoder isn't aware of the frame rectangle. It just writes into the output buffer given to it. But, that shouldn't be a problem, as the renderer can always pass the appropriate output buffer (pointer to the top-left pixel, width, height and stride) for prev and current frame. To summarize: It's a good suggestion and adding such an API could be considered for the next major libwebp release. Until that API comes up, we'll have to make do with the current approach. As discussed offline, putting a FIXME to take this up later.
LGTM. As previously noted, I did a cursory rather than exhaustive review, mostly because noel has presumably already done a much more thorough review. If you want me to review exhaustively, let me know.
Source/WebKit lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/urvang@google.com/13980003/284001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/urvang@google.com/13980003/284001
Message was sent while issue was closed.
Change committed as 153187 |