Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1962563002:
Fix ImageDecoder::frameIsCompleteAtIndex - fully received instead of decoded. (Closed)
|
Created:
4 years, 7 months ago by aleksandar.stojiljkovic Modified:
3 years, 5 months ago CC:
chromium-reviews, blink-reviews, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit ImageDecoder::frameIsCompleteAtIndex and frameIsFullyReceivedAtIndex
For ImageDecoder::frameIsCompleteAtIndex comment states: // Whether or not
the frame is fully received.
Implementation in ImageDecoder::frameIsCompleteAtIndex checked if decoded.
GIF and WEBP decoder have it properly implemented - parsing if frame data
is fully received.
JPEG and PNG decoder used this method to check if frame decoding is done.
Making it consistent to comment and usage in GIF & WEBP - return if frame
data is fully received.
History:
@hclam intention [1] was to have this API used only for multi-frame images.
[1]
https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
BUG=
Patch Set 1 #
Total comments: 1
Patch Set 2 : extending the scope with naming suggested by @scroggo #
Total comments: 24
Patch Set 3 : partial, still work in progress... #Patch Set 4 : PNG alpha & partial decode fixes. #
Total comments: 14
Patch Set 5 : BMPDecoder + remove partial #
Total comments: 14
Patch Set 6 : revert hasSeenAlpha approach in webp & png. bytebybyte unit tests. fixes. #
Total comments: 7
Patch Set 7 : bounds check in ImageDecoder #
Total comments: 2
Patch Set 8 : Review #23 fix - comments. cleanup. Thanks Leon. #
Total comments: 24
Patch Set 9 : review #27 fixes. Thanks pkasting@ #Patch Set 10 : ASSERT if frameCount not called to parse before calling... #
Total comments: 6
Patch Set 11 : review #35 fixes. #Patch Set 12 : Deferred decoding for ICO #
Total comments: 6
Patch Set 13 : Revert patchset 12: Deferred decoding for ICO #Patch Set 14 : better check in unit test #
Total comments: 11
Patch Set 15 : rebase #Messages
Total messages: 47 (4 generated)
aleksandar.stojiljkovic@intel.com changed reviewers: + pkasting@chromium.org, scroggo@chromium.org
Related to discussion in https://codereview.chromium.org/1925533003/diff/80001/third_party/WebKit/Sour... (High CPU and increased memory usage fix for high-res (GIF, WEBP...) animations)
https://codereview.chromium.org/1962563002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return m_isAllDataReceived; Although this seems to be truer to the initial intent of the method, I do not like the name. frameIsCompleteAtIndex really sounds like it reports whether the Frame's Status is FrameComplete. A better name might be frameIsFullyReceivedAtIndex. I also think the thread you're pulling goes deeper - which is to say that I think more needs to change to make this correct. Do all the callers really want to know whether all the data is received? Just looking at the method above this, it looks pretty clear to me that the caller actually wants to know whether the frame has been completely decoded. (On that note, that method is only called by DeferredImageDecoder::frameHasAlphaAtIndex [1], where I don't think it ever returns false. Even if frameIsCompleteAtIndex returns true, we'll then return whether the ImageFrame in m_actualDecoder has alpha. But that ImageFrame should never be decoded, so it should never have alpha. We have a bug around this - crbug.com/593430 (in which I had already shown my confusion around frameIsCompleteAtIndex).) If some callers do want to know whether the Frame is FrameComplete, I think it would make sense to have a non-virtual method that computes what the base version used to do. (We might want to change the name just to be safe) [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2016/05/09 15:41:45, scroggo_chromium wrote: > https://codereview.chromium.org/1962563002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): > > https://codereview.chromium.org/1962563002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return > m_isAllDataReceived; > Although this seems to be truer to the initial intent of the method, I do not > like the name. frameIsCompleteAtIndex really sounds like it reports whether the > Frame's Status is FrameComplete. A better name might be > frameIsFullyReceivedAtIndex. > > I also think the thread you're pulling goes deeper - which is to say that I > think more needs to change to make this correct. Do all the callers really want > to know whether all the data is received? Just looking at the method above this, > it looks pretty clear to me that the caller actually wants to know whether the > frame has been completely decoded. > Thanks, missed it. How about frameIsReceivedAtIndex? Not called on too many places, so let me continue refactoring and check the DeferredImageDecoder. > (On that note, that method is only called by > DeferredImageDecoder::frameHasAlphaAtIndex [1], where I don't think it ever > returns false. Even if frameIsCompleteAtIndex returns true, we'll then return > whether the ImageFrame in m_actualDecoder has alpha. But that ImageFrame should > never be decoded, so it should never have alpha. We have a bug around this - > crbug.com/593430 (in which I had already shown my confusion around > frameIsCompleteAtIndex).) > > If some callers do want to know whether the Frame is FrameComplete, I think it > would make sense to have a non-virtual method that computes what the base > version used to do. (We might want to change the name just to be safe) I have put isComplete() there but not checking index. Maybe frameIsDecodedAtIndex(size_t)... > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Patch set #2 - extending the scope with naming suggested by @scroggo. frameIsCompleteAtIndex added to base class as it is needed in multiple decoders.
Description was changed from ========== Fix ImageDecoder::frameIsCompleteAtIndex - fully received instead of decoded. Header comment states: // Whether or not the frame is fully received. Implementation in ImageDecoder::frameIsCompleteAtIndex checked if decoded. GIF and WEBP decoder have it properly implemented - parsing if frame data is fully received. JPEG and PNG decoder used this method to check if frame decoding is done. Making it consistent to comment and usage in GIF & WEBP - return if frame data is fully received. History: @hclam intention [1] was to have this API used only for multi-frame images. [1] https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image... BUG= ========== to ========== Split ImageDecoder::frameIsCompleteAtIndex and frameIsFullyReceivedAtIndex For ImageDecoder::frameIsCompleteAtIndex comment states: // Whether or not the frame is fully received. Implementation in ImageDecoder::frameIsCompleteAtIndex checked if decoded. GIF and WEBP decoder have it properly implemented - parsing if frame data is fully received. JPEG and PNG decoder used this method to check if frame decoding is done. Making it consistent to comment and usage in GIF & WEBP - return if frame data is fully received. History: @hclam intention [1] was to have this API used only for multi-frame images. [1] https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image... BUG= ==========
Some small complaints but I like the direction of this CL. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:232: // NOTE: Don't call frameIsFullyReceivedAtIndex() here, that will try to Is this comment true? https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h:80: bool frameIsFullyReceivedAtIndex(size_t) const override This looks to be doing the wrong thing. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:157: inline bool frameIsCompleteAtIndex(size_t frameIndex) const Should there be tests for this new method? Also, I think it's currently only called by subclasses (sorry if I missed one!) - can we mark it as protected? (If I am mistaken, I'm fine to leave it public.)
https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:166: m_frames[index].m_isComplete = m_source.frameIsFullyReceivedAtIndex(index); This field should be renamed m_isFullyReceived. ...although it's not clear to me if we even need this. Does just always calling the underlying oracle function slow things appreciably or cause the wrong answer anywhere? https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:232: // NOTE: Don't call frameIsFullyReceivedAtIndex() here, that will try to On 2016/05/09 21:54:33, scroggo_chromium wrote: > Is this comment true? I don't think so. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:149: return index >= m_frameBufferCache.size() || m_frameBufferCache[index].hasAlpha(); Will hasAlpha() return true on partially-decoded frames that don't have alpha in the decoded part? If not seems like this is less correct than before. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:88: return m_reader && (index < m_reader->imagesCount()) && m_reader->frameContext(index)->isComplete(); Hmm. This will always return false if decoding fails (i.e. if setFailed() returns true). Maybe it should always return true in that case instead? https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:168: bool WEBPImageDecoder::frameIsFullyReceivedAtIndex(size_t index) const I'm not convinced this function is correct for frameIsFullyReceivedAtIndex(). It looks like for animated images it implements something closer to frameIsCompleteAtIndex() instead.
https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:149: return index >= m_frameBufferCache.size() || m_frameBufferCache[index].hasAlpha(); On 2016/05/10 00:00:13, Peter Kasting wrote: > Will hasAlpha() return true on partially-decoded frames that don't have alpha in > the decoded part? I think that is the intent. When we create a new ImageFrame, we set m_hasAlpha (which this method returns) to true [1]. Some of our decoders (BMP, PNG) setHasAlpha(false) immediately after that [2][3]. BMP has comments that it will set it back to true if it sees a transparent pixel [4], but it is not obvious to me that either one sets it back to true in the case of an incomplete image. My understanding is that JPEG does the right thing - it sets hasAlpha to true initially [5] (unnecessarily, though, since setSize already did it) and then sets it to false on completion [6]. GIF [7] and WEBP [8] also seem to do the right thing. I think PNG and BMP actually have bugs here, which get exposed by this change. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [2] BMP: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [3] PNG: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [4] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [5] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [6] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [7]https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp&l=195&rcl=1462864689 / https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [8] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > If not seems like this is less correct than before.
https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:166: m_frames[index].m_isComplete = m_source.frameIsFullyReceivedAtIndex(index); On 2016/05/10 00:00:13, Peter Kasting wrote: > This field should be renamed m_isFullyReceived. > > ...although it's not clear to me if we even need this. Does just always calling > the underlying oracle function slow things appreciably or cause the wrong answer > anywhere? The reason I left the name is because of work ongoing in the patch (to minimize merging issues). Anyway, not an issue so renamed it. Removing FrameData usage in BitmapImage I'm also planning to do after both patches land - need more time to check the risks; performance and functionality. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:232: // NOTE: Don't call frameIsFullyReceivedAtIndex() here, that will try to On 2016/05/09 21:54:33, scroggo_chromium wrote: > Is this comment true? Removed. Seems that it was once the case and that could explain caching metadata in BitmapImage and ImageSource. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h:80: bool frameIsFullyReceivedAtIndex(size_t) const override On 2016/05/09 21:54:33, scroggo_chromium wrote: > This looks to be doing the wrong thing. Done. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:149: return index >= m_frameBufferCache.size() || m_frameBufferCache[index].hasAlpha(); On 2016/05/10 14:14:37, scroggo_chromium wrote: > On 2016/05/10 00:00:13, Peter Kasting wrote: > > Will hasAlpha() return true on partially-decoded frames that don't have alpha > in > > the decoded part? > > I think that is the intent. When we create a new ImageFrame, we set m_hasAlpha > (which this method returns) to true [1]. Some of our decoders (BMP, PNG) > setHasAlpha(false) immediately after that [2][3]. BMP has comments that it will > set it back to true if it sees a transparent pixel [4], but it is not obvious to > me that either one sets it back to true in the case of an incomplete image. > > My understanding is that JPEG does the right thing - it sets hasAlpha to true > initially [5] (unnecessarily, though, since setSize already did it) and then > sets it to false on completion [6]. GIF [7] and WEBP [8] also seem to do the > right thing. > > I think PNG and BMP actually have bugs here, which get exposed by this change. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [2] BMP: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [3] PNG: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [4] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [5] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [6] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [7]https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp&l=195&rcl=1462864689 > / > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [8] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > If not seems like this is less correct than before. > I disliked that we are handling "decoding partial received content has transparent background" feature here. Anyway, it doesn't look OK to use neither of new methods; frameIsFullyReceivedAtIndex nor frameIsCompleteAtIndex. frameIsFullyReceivedAtIndex has problem with PNG when partially downloaded - (PNG sets hasAlpha = false). frameIsCompleteAtIndex doesnt let cached hasAlpha from m_frameBufferCache for "cleared pixeldata" frames. So, need to check PNG & BMP decoders. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:157: inline bool frameIsCompleteAtIndex(size_t frameIndex) const On 2016/05/09 21:54:33, scroggo_chromium wrote: > Should there be tests for this new method? > > Also, I think it's currently only called by subclasses (sorry if I missed one!) > - can we mark it as protected? (If I am mistaken, I'm fine to leave it public.) It is public because it is called from PNGImageReader::decode. About tests - it is called on multiple places that are already tested in decoder tests I didn't see it necessary to test this method. Don't want to use MockImageDecoder for it, need to think to which test to add it. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:88: return m_reader && (index < m_reader->imagesCount()) && m_reader->frameContext(index)->isComplete(); On 2016/05/10 00:00:13, Peter Kasting wrote: > Hmm. This will always return false if decoding fails (i.e. if setFailed() > returns true). Maybe it should always return true in that case instead? What would be the usage of returning "frame is fully received" in case of failure? https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:168: bool WEBPImageDecoder::frameIsFullyReceivedAtIndex(size_t index) const On 2016/05/10 00:00:13, Peter Kasting wrote: > I'm not convinced this function is correct for frameIsFullyReceivedAtIndex(). > It looks like for animated images it implements something closer to > frameIsCompleteAtIndex() instead. I briefly checked WEBPImageDecoder::decodeFrameCount() and updateDemuxer. decodeFrameCount() is used to update m_frameBufferCache.size()and decodeFrameCount is based on frames that are already received. Don't know - it looks correct to me but will debug it first. In this change I don't think it matters - WEBP is multiframe decoder as GIF and frameIsCompleteAtIndex is for the same use cases just renamed to frameIsFullyReceivedAtIndex (it is renamed but functionality is the same).
https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:149: return index >= m_frameBufferCache.size() || m_frameBufferCache[index].hasAlpha(); On 2016/05/10 21:59:31, aleksandar.stojiljkovic wrote: > I disliked that we are handling "decoding partial received content has > transparent background" feature here. Me too, frankly. I don't think that's really something the image decoder itself should deal with. To me, whether the image has alpha should reflect whether the decoded image data has any partly-transparent pixels; separately, this code, or some other code that's not specific to each individual decoder, could check if the frame status is not yet FrameComplete, and if it's not, assume that the drawn frame will always have alpha (in the non-decoded portions). https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:88: return m_reader && (index < m_reader->imagesCount()) && m_reader->frameContext(index)->isComplete(); On 2016/05/10 21:59:31, aleksandar.stojiljkovic wrote: > On 2016/05/10 00:00:13, Peter Kasting wrote: > > Hmm. This will always return false if decoding fails (i.e. if setFailed() > > returns true). Maybe it should always return true in that case instead? > > What would be the usage of returning "frame is fully received" in case of > failure? We know we're not going to do any further updating/decoding, so anyone wanting to know "is the image finalizable" should probably get back "yes".
patch #4 https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:88: return m_reader && (index < m_reader->imagesCount()) && m_reader->frameContext(index)->isComplete(); On 2016/05/11 00:33:02, Peter Kasting wrote: > On 2016/05/10 21:59:31, aleksandar.stojiljkovic wrote: > > On 2016/05/10 00:00:13, Peter Kasting wrote: > > > Hmm. This will always return false if decoding fails (i.e. if setFailed() > > > returns true). Maybe it should always return true in that case instead? > > > > What would be the usage of returning "frame is fully received" in case of > > failure? > > We know we're not going to do any further updating/decoding, so anyone wanting > to know "is the image finalizable" should probably get back "yes". The usage of this is in [1] - GraphicsContext::computeFilterQuality. If image is not fully received, then lower quality interpolation is used. So for case of failure we don't use high quality sampling. I think it is better to leave it as it was. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:150: || m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial As I still didn't fix BMPImageDecoder alpha handling leaving m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial here - to cover previous functionality. https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (left): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:317: buffer.setHasAlpha(false); The code was like this in all of the decoders [1] but meanwhile partially decoded got alpha apparently and it was fixed in other decoders except PNG and BMP. PNG should be OK now. [1] @pkasting's patch at 2009 https://chromium.googlesource.com/chromium/src/+/f09863db44eed54d690e52815adb...
https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:149: return index >= m_frameBufferCache.size() || m_frameBufferCache[index].hasAlpha(); On 2016/05/11 00:33:02, Peter Kasting wrote: > On 2016/05/10 21:59:31, aleksandar.stojiljkovic wrote: > > I disliked that we are handling "decoding partial received content has > > transparent background" feature here. > > Me too, frankly. I don't think that's really something the image decoder itself > should deal with. To me, whether the image has alpha should reflect whether the > decoded image data has any partly-transparent pixels; separately, this code, or > some other code that's not specific to each individual decoder, could check if > the frame status is not yet FrameComplete, and if it's not, assume that the > drawn frame will always have alpha (in the non-decoded portions). Is there an existing caller that needs to know whether the decoded portion has alpha (as opposed to the whole bitmap)? It seems like the client needs to know whether there is alpha anywhere in the bitmap, since that affects blending/elements behind it showing etc. FWIW, I actually proposed that the image decoder report the alpha in the decoded portion in crbug.com/593430. In order for that to work properly, the client needs to only draw the portion of the bitmap that has been decoded. The deferred decoding pipeline is not set up to do that. Which reminds me - the use of this method is already pretty broken. It is only called by DeferredImageDecoder, which queries m_actualDecoder, which will never return false (since m_actualDecoder is not used for decoding). (DeferredImageDecoder's version can return false once all data is received and we throw away m_actualDecoder, so it will check with m_frameGenerator for a multi frame image.) In principle, I'm fine with making the ImageDecoder keep track of whether it's seen alpha*, although I would argue for a more specific name (instead of hasAlpha(), something like "hasSeenAlpha()" or "decodedPortionHasAlpha()"). We also need to add tests that verify that we actually get that behavior and state clearly that it's what we want. As aleksandar points out, we *used to* behave that way, but some decoders switched. * Assuming there's a way for the caller to determine whether the frame is complete. DeferredImageDecoder, for example, does not know. It could guess based on how much data was received, but that's no guarantee. https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:88: return m_reader && (index < m_reader->imagesCount()) && m_reader->frameContext(index)->isComplete(); On 2016/05/11 11:05:15, aleksandar.stojiljkovic wrote: > On 2016/05/11 00:33:02, Peter Kasting wrote: > > On 2016/05/10 21:59:31, aleksandar.stojiljkovic wrote: > > > On 2016/05/10 00:00:13, Peter Kasting wrote: > > > > Hmm. This will always return false if decoding fails (i.e. if setFailed() > > > > returns true). Maybe it should always return true in that case instead? > > > > > > What would be the usage of returning "frame is fully received" in case of > > > failure? > > > > We know we're not going to do any further updating/decoding, so anyone wanting > > to know "is the image finalizable" should probably get back "yes". > > The usage of this is in [1] - GraphicsContext::computeFilterQuality. > If image is not fully received, then lower quality interpolation is used. So for > case of failure we don't use high quality sampling. I think it is better to > leave it as it was. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But it also will be used in crrev.com/1925533003 to state that we should use the same genID so we'll use the same image from the cache, right? https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:168: bool WEBPImageDecoder::frameIsFullyReceivedAtIndex(size_t index) const On 2016/05/10 21:59:31, aleksandar.stojiljkovic wrote: > On 2016/05/10 00:00:13, Peter Kasting wrote: > > I'm not convinced this function is correct for frameIsFullyReceivedAtIndex(). > > It looks like for animated images it implements something closer to > > frameIsCompleteAtIndex() instead. > > I briefly checked WEBPImageDecoder::decodeFrameCount() and updateDemuxer. > decodeFrameCount() is used to update m_frameBufferCache.size()and > decodeFrameCount is based on frames that are already received. Don't know - it > looks correct to me but will debug it first. > > In this change I don't think it matters - WEBP is multiframe decoder as GIF and > frameIsCompleteAtIndex is for the same use cases just renamed to > frameIsFullyReceivedAtIndex (it is renamed but functionality is the same). As I understand it, GIF's version of frameIsFullyReceivedAtIndex tells us that we have received all the bytes for a frame (assuming it's valid). So if we decode that frame (maybe we haven't yet) it will end up as FrameComplete without providing more data (again, assuming it's valid). Looking at WEBP's decodeFrameCount, it is not obvious that we have received enough data to mark the frame FrameComplete after a decode. https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:150: || m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial On 2016/05/11 11:05:16, aleksandar.stojiljkovic wrote: > As I still didn't fix BMPImageDecoder alpha handling leaving > m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial here - to > cover previous functionality. FWIW, you didn't fix PNGImageDecoder (or, you fixed it the other direction), and you left the others unchanged. I think BMPImageDecoder matches the behavior you and pkasting@ proposed. I also think it's awkward here that frameHasAlphaAtIndex(i) may return a different value from another way a client could attempt to find this info: frameBufferAtIndex(i).hasAlpha() https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:405: m_hasPixelsWithAlpha = true; So now it will have alpha until fully decoded (when it may change to opaque)? I thought you and pkasting@ agreed that was the wrong behavior? And why did you leave BMP alone but "fix" PNG? https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:415: m_frameBufferCache[0].setHasAlpha(m_reader->hasAlpha() && m_hasPixelsWithAlpha); I think it's redundant here to check m_reader->hasAlpha() https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:426: m_hasPixelsWithAlpha = false; Why did you need to set this to false? Isn't it set to false in the constructor?
https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:88: return m_reader && (index < m_reader->imagesCount()) && m_reader->frameContext(index)->isComplete(); On 2016/05/11 11:05:15, aleksandar.stojiljkovic wrote: > On 2016/05/11 00:33:02, Peter Kasting wrote: > > On 2016/05/10 21:59:31, aleksandar.stojiljkovic wrote: > > > On 2016/05/10 00:00:13, Peter Kasting wrote: > > > > Hmm. This will always return false if decoding fails (i.e. if setFailed() > > > > returns true). Maybe it should always return true in that case instead? > > > > > > What would be the usage of returning "frame is fully received" in case of > > > failure? > > > > We know we're not going to do any further updating/decoding, so anyone wanting > > to know "is the image finalizable" should probably get back "yes". > > The usage of this is in [1] - GraphicsContext::computeFilterQuality. > If image is not fully received, then lower quality interpolation is used. So for > case of failure we don't use high quality sampling. The only reason we ever use low-quality filtering is so we don't spend too much time filtering an image which is going to be erased momentarily and redrawn in a different state. Once failure occurs the image is finalized, we don't redraw it, and thus we should use high-quality filtering, even on failed images. https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:150: || m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial On 2016/05/11 15:17:34, scroggo_chromium wrote: > On 2016/05/11 11:05:16, aleksandar.stojiljkovic wrote: > > As I still didn't fix BMPImageDecoder alpha handling leaving > > m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial here - to > > cover previous functionality. I think you want "!= ImageFrame::FrameComplete", because the frame has alpha when empty as well. > I also think it's awkward here that frameHasAlphaAtIndex(i) may return a > different value from another way a client could attempt to find this info: > frameBufferAtIndex(i).hasAlpha() Meaning instead of doing this either here or in the decoders, we should do it in ImageFrame::hasAlpha()? I'd be OK with that. As long as the individual decoders aren't accounting for this, any solution is OK by me.
https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:88: return m_reader && (index < m_reader->imagesCount()) && m_reader->frameContext(index)->isComplete(); On 2016/05/11 18:29:34, Peter Kasting wrote: > On 2016/05/11 11:05:15, aleksandar.stojiljkovic wrote: > > On 2016/05/11 00:33:02, Peter Kasting wrote: > > > On 2016/05/10 21:59:31, aleksandar.stojiljkovic wrote: > > > > On 2016/05/10 00:00:13, Peter Kasting wrote: > > > > > Hmm. This will always return false if decoding fails (i.e. if > setFailed() > > > > > returns true). Maybe it should always return true in that case instead? > > > > > > > > What would be the usage of returning "frame is fully received" in case of > > > > failure? > > > > > > We know we're not going to do any further updating/decoding, so anyone > wanting > > > to know "is the image finalizable" should probably get back "yes". > > > > The usage of this is in [1] - GraphicsContext::computeFilterQuality. > > If image is not fully received, then lower quality interpolation is used. So > for > > case of failure we don't use high quality sampling. > > The only reason we ever use low-quality filtering is so we don't spend too much > time filtering an image which is going to be erased momentarily and redrawn in a > different state. Once failure occurs the image is finalized, we don't redraw > it, and thus we should use high-quality filtering, even on failed images. Done. Clear now; failed() added to "fully received" computation. https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:150: || m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial On 2016/05/11 18:29:34, Peter Kasting wrote: > On 2016/05/11 15:17:34, scroggo_chromium wrote: > > On 2016/05/11 11:05:16, aleksandar.stojiljkovic wrote: > > > As I still didn't fix BMPImageDecoder alpha handling leaving > > > m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial here - to > > > cover previous functionality. > > I think you want "!= ImageFrame::FrameComplete", because the frame has alpha > when empty as well. Now the patch fixed the former PNG and BMP decoder behavior (to match others): on start, all frames are transparent. When the frame decoding is complete, mark ImageFrame::setHasAlpha(seenPixelsWithAlpha) (in every decoder separately for now). > > > I also think it's awkward here that frameHasAlphaAtIndex(i) may return a > > different value from another way a client could attempt to find this info: > > frameBufferAtIndex(i).hasAlpha() > > Meaning instead of doing this either here or in the decoders, we should do it in > ImageFrame::hasAlpha()? I'd be OK with that. As long as the individual > decoders aren't accounting for this, any solution is OK by me. Complete/Partial is now removed from this check. Still the decoders are calculating "has seen alpha" and then keeping ImageFrame::hasAlpha up to date. If I understand correctly, you'd prefer to get rid of ImageDecoder::frameHasAlphaAtIndex and always get this from ImageFrame? If so, I could do this as part of another patch, maybe [1] bellow. After scroggo@ pointed that frameHasAlphaAtIndex has been returning true we should check this: 1) What benefit is the "has seen alpha" approach, where decoder checks every pixel, bringing now? 2) On a related note, I have started approach here [1] to early return alpha (before decoding). Maybe alternative approach to one we have so far would be to remove all "has seen alpha" calculation and return only the calculation from [1] always. [1] https://crrev.com/1972683002: Early reporting of opaqueness by decoders. https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:405: m_hasPixelsWithAlpha = true; On 2016/05/11 15:17:34, scroggo_chromium wrote: > So now it will have alpha until fully decoded (when it may change to opaque)? Seemed like a bug, that the state in ImageFrame is not consistent for partially decoded content - so attempting to fix it here. About handling alpha state, I'll answer to original pkasting@'s comment. > I > thought you and pkasting@ agreed that was the wrong behavior? And why did you > leave BMP alone but "fix" PNG? I just didn't do BMP part then yet - fixed in this patch. https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:415: m_frameBufferCache[0].setHasAlpha(m_reader->hasAlpha() && m_hasPixelsWithAlpha); On 2016/05/11 15:17:34, scroggo_chromium wrote: > I think it's redundant here to check m_reader->hasAlpha() Done. https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:426: m_hasPixelsWithAlpha = false; On 2016/05/11 15:17:34, scroggo_chromium wrote: > Why did you need to set this to false? Isn't it set to false in the constructor? I thought it was safer to reset it for every new reader (in case it was reused for different data) but now saw it is not reused in ICO decoder either. Thanks. Removed.
https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:150: || m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial On 2016/05/16 13:09:49, aleksandar.stojiljkovic wrote: > On 2016/05/11 18:29:34, Peter Kasting wrote: > > On 2016/05/11 15:17:34, scroggo_chromium wrote: > > > On 2016/05/11 11:05:16, aleksandar.stojiljkovic wrote: > > > > As I still didn't fix BMPImageDecoder alpha handling leaving > > > > m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial here - > to > > > > cover previous functionality. > > > > I think you want "!= ImageFrame::FrameComplete", because the frame has alpha > > when empty as well. > > Now the patch fixed the former PNG and BMP decoder behavior (to match others): > on start, all frames are > transparent. When the frame decoding is complete, mark > ImageFrame::setHasAlpha(seenPixelsWithAlpha) (in every decoder separately for > now). > > > > > > I also think it's awkward here that frameHasAlphaAtIndex(i) may return a > > > different value from another way a client could attempt to find this info: > > > frameBufferAtIndex(i).hasAlpha() > > > > Meaning instead of doing this either here or in the decoders, we should do it > in > > ImageFrame::hasAlpha()? I'd be OK with that. As long as the individual > > decoders aren't accounting for this, any solution is OK by me. > > > Complete/Partial is now removed from this check. Still the decoders are > calculating "has seen alpha" and then keeping ImageFrame::hasAlpha up to date. > If I understand correctly, you'd prefer to get rid of > ImageDecoder::frameHasAlphaAtIndex and always get this from ImageFrame? If so, I > could do this as part of another patch, maybe [1] bellow. My preference is that *if* we have two separate methods, they both return the same answer. That can be accomplished by having the impl for frameHasAlphaAtIndex look like this: return index >= m_frameBufferCache.size() || m_frameBufferCache[index].hasAlpha(); > > After scroggo@ pointed that frameHasAlphaAtIndex has been returning true we > should check this: > > 1) What benefit is the "has seen alpha" approach, where decoder checks every > pixel, bringing now? Well, the general system (for getting the alpha state back to the client) is broken, so I would guess none. My real question is whether switching to what you and pkasting@ thought made sense (i.e. decoder does not worry about whether the frame is complete) would give us benefit. I think we'd need to update more code before we can see a benefit. > 2) On a related note, I have started approach here [1] to early return alpha > (before decoding). > > Maybe alternative approach to one we have so far would be to remove all "has > seen alpha" calculation and return only the calculation from [1] always. > > > [1] https://crrev.com/1972683002: Early reporting of opaqueness by decoders. > https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h:98: // For the last frame use bse class implementation. nit: base* https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:122: m_buffer->setStatus(ImageFrame::FramePartial); Previously, this set the alpha to false for a partially decoded BMP. Please add a test for this behavior change. https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h:360: // saw pixels that are transparent. I think this comment is confusing. It describes the variables above (although better explanations exist elsewhere) and then partially describes m_hasPixelsWithAlpha. I think it would be clearer to drop the first sentence and instead say something like: // Whether this image is considered to have transparent pixels, after // taking m_seenNonZeroAlphaPixel and m_seenZeroAlphaPixel into // consideration. https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:317: buffer.setStatus(ImageFrame::FramePartial); Previously, a partially decoded PNG file will report false for hasAlpha. With this CL, it will return true. Please add a test for that behavior. https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:424: if (!m_reader) { This change is unnecessary. https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:173: return ((m_formatFlags & ANIMATION_FLAG) && index < m_frameBufferCache.size()) || ImageDecoder::frameIsFullyReceivedAtIndex(index); I brought this up previously, but I don't think you've addressed it. Here is my previous comment: """ As I understand it, GIF's version of frameIsFullyReceivedAtIndex tells us that we have received all the bytes for a frame (assuming it's valid). So if we decode that frame (maybe we haven't yet) it will end up as FrameComplete without providing more data (again, assuming it's valid). Looking at WEBP's decodeFrameCount, it is not obvious that we have received enough data to mark the frame FrameComplete after a decode. """ Said a different way, if we have enough data to decode part of a frame (FramePartial), WEBP's implementation looks like it will return true, but GIF's implementation looks like it will return false.
One more large comment - patch set 5 includes a rebase in addition to your other changes, making it harder to see the (relevant) differences between patch sets. It's easier on your reviewers to either do a separate upload for a rebase or just wait to rebase until the patch set is ready.
https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:150: || m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial On 2016/05/16 20:32:08, scroggo_chromium wrote: > On 2016/05/16 13:09:49, aleksandar.stojiljkovic wrote: > > On 2016/05/11 18:29:34, Peter Kasting wrote: > > > On 2016/05/11 15:17:34, scroggo_chromium wrote: > > > > On 2016/05/11 11:05:16, aleksandar.stojiljkovic wrote: > > > > > As I still didn't fix BMPImageDecoder alpha handling leaving > > > > > m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial here - > > to > > > > > cover previous functionality. > > > > > > I think you want "!= ImageFrame::FrameComplete", because the frame has alpha > > > when empty as well. > > > > Now the patch fixed the former PNG and BMP decoder behavior (to match others): > > on start, all frames are > > transparent. When the frame decoding is complete, mark > > ImageFrame::setHasAlpha(seenPixelsWithAlpha) (in every decoder separately for > > now). > > > > > > > > > I also think it's awkward here that frameHasAlphaAtIndex(i) may return a > > > > different value from another way a client could attempt to find this info: > > > > frameBufferAtIndex(i).hasAlpha() > > > > > > Meaning instead of doing this either here or in the decoders, we should do > it > > in > > > ImageFrame::hasAlpha()? I'd be OK with that. As long as the individual > > > decoders aren't accounting for this, any solution is OK by me. > > > > > > Complete/Partial is now removed from this check. Still the decoders are > > calculating "has seen alpha" and then keeping ImageFrame::hasAlpha up to date. > > If I understand correctly, you'd prefer to get rid of > > ImageDecoder::frameHasAlphaAtIndex and always get this from ImageFrame? If so, > I > > could do this as part of another patch, maybe [1] bellow. > > My preference is that *if* we have two separate methods, they both return the > same answer. That can be accomplished by having the impl for > frameHasAlphaAtIndex look like this: > > return index >= m_frameBufferCache.size() || > m_frameBufferCache[index].hasAlpha(); What I want is for the following two things to be considered completely distinct: (1) Whether the decoded pixels in the image have alpha (2) Whether there is undecoded region within the image Keeping these separate means the decoders can consider only (1), making them simpler to understand and less error-prone. But the drawing subsystem that wants to know if the bitmap is fully opaque has to know about both. Perhaps the way to achieve this while still respecting Leon's concerns is to name the functions which provide these functions differently, e.g.: (1) ImageDecoder::DecodedPixelsHaveAlpha() (2) ImageFrame::IsFullyOpaque() Or something to that effect.
https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:150: || m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial On 2016/05/16 20:35:53, Peter Kasting wrote: > On 2016/05/16 20:32:08, scroggo_chromium wrote: > > On 2016/05/16 13:09:49, aleksandar.stojiljkovic wrote: > > > On 2016/05/11 18:29:34, Peter Kasting wrote: > > > > On 2016/05/11 15:17:34, scroggo_chromium wrote: > > > > > On 2016/05/11 11:05:16, aleksandar.stojiljkovic wrote: > > > > > > As I still didn't fix BMPImageDecoder alpha handling leaving > > > > > > m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial here > - > > > to > > > > > > cover previous functionality. > > > > > > > > I think you want "!= ImageFrame::FrameComplete", because the frame has > alpha > > > > when empty as well. > > > > > > Now the patch fixed the former PNG and BMP decoder behavior (to match > others): > > > on start, all frames are > > > transparent. When the frame decoding is complete, mark > > > ImageFrame::setHasAlpha(seenPixelsWithAlpha) (in every decoder separately > for > > > now). > > > > > > > > > > > > I also think it's awkward here that frameHasAlphaAtIndex(i) may return a > > > > > different value from another way a client could attempt to find this > info: > > > > > frameBufferAtIndex(i).hasAlpha() > > > > > > > > Meaning instead of doing this either here or in the decoders, we should do > > it > > > in > > > > ImageFrame::hasAlpha()? I'd be OK with that. As long as the individual > > > > decoders aren't accounting for this, any solution is OK by me. > > > > > > > > > Complete/Partial is now removed from this check. Still the decoders are > > > calculating "has seen alpha" and then keeping ImageFrame::hasAlpha up to > date. > > > If I understand correctly, you'd prefer to get rid of > > > ImageDecoder::frameHasAlphaAtIndex and always get this from ImageFrame? If > so, > > I > > > could do this as part of another patch, maybe [1] bellow. > > > > My preference is that *if* we have two separate methods, they both return the > > same answer. That can be accomplished by having the impl for > > frameHasAlphaAtIndex look like this: > > > > return index >= m_frameBufferCache.size() || > > m_frameBufferCache[index].hasAlpha(); > > What I want is for the following two things to be considered completely > distinct: > > (1) Whether the decoded pixels in the image have alpha > (2) Whether there is undecoded region within the image > > Keeping these separate means the decoders can consider only (1), making them > simpler to understand and less error-prone. But the drawing subsystem that > wants to know if the bitmap is fully opaque has to know about both. > > Perhaps the way to achieve this while still respecting Leon's concerns is to > name the functions which provide these functions differently, e.g.: > > (1) ImageDecoder::DecodedPixelsHaveAlpha() > (2) ImageFrame::IsFullyOpaque() > > Or something to that effect. Yes, that sounds good to me.
Patch #5 added - Revert hasSeenAlpha approach in webp & png. Bytebybyte unit tests. Fixes. On 2016/05/16 20:35:53, Peter Kasting wrote: > ... > Perhaps the way to achieve this while still respecting Leon's concerns is to > name the functions which provide these functions differently, e.g.: > > (1) ImageDecoder::DecodedPixelsHaveAlpha() > (2) ImageFrame::IsFullyOpaque() > > Or something to that effect. I'm pulling back from changing alpha reporting code in this patch. Reverted bmp and png changed from patch #4. About alpha reporting fixes - plan to continue work on Issue 593430 in separate patches. I need to check first what the alpha info is used for. About frameIsFullyReceivedAtIndex - added some unit testing and worked on review comments in patch #5. https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h:98: // For the last frame use bse class implementation. On 2016/05/16 20:32:08, scroggo_chromium wrote: > nit: base* Done. https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.h:360: // saw pixels that are transparent. On 2016/05/16 20:32:08, scroggo_chromium wrote: > I think this comment is confusing. It describes the variables above (although > better explanations exist elsewhere) and then partially describes > m_hasPixelsWithAlpha. > > I think it would be clearer to drop the first sentence and instead say something > like: > > // Whether this image is considered to have transparent pixels, after > // taking m_seenNonZeroAlphaPixel and m_seenZeroAlphaPixel into > // consideration. reverted the code. https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:424: if (!m_reader) { On 2016/05/16 20:32:08, scroggo_chromium wrote: > This change is unnecessary. Done. https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:173: return ((m_formatFlags & ANIMATION_FLAG) && index < m_frameBufferCache.size()) || ImageDecoder::frameIsFullyReceivedAtIndex(index); On 2016/05/16 20:32:08, scroggo_chromium wrote: > I brought this up previously, but I don't think you've addressed it. Here is my > previous comment: > > """ > As I understand it, GIF's version of frameIsFullyReceivedAtIndex tells us that > we have received all the bytes for a frame (assuming it's valid). So if we > decode that frame (maybe we haven't yet) it will end up as FrameComplete without > providing more data (again, assuming it's valid). > > Looking at WEBP's decodeFrameCount, it is not obvious that we have received > enough data to mark the frame FrameComplete after a decode. > """ > > Said a different way, if we have enough data to decode part of a frame > (FramePartial), WEBP's implementation looks like it will return true, but GIF's > implementation looks like it will return false. I refactored it a bit, but it is still based on index < m_frameBufferCache.size(). For multiframe webp, unless there is complete data for the frame, an entry for it is not added to m_frameBufferCache. https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:91: if (decoder->filenameExtension() != "webp" || frameIsFullyReceived) The check is conservative - when frameIsFullyReceivedAtIndex returns true, there is enough data to get the frame completely decoded. WebP decoder could rarely return false for frameIsCompleteAtIndex although there is enough data to get the frame fully decoded. This happens only for single frame WebP images - frame gets completely decoded 2 bytes before the end of file and WebP demux reports WebPIterator::complete true 2 bytes later or after frame decoding is completed. Multiframe WebPs used in tests don't expose that type of "byte padding" issue.
https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:173: return ((m_formatFlags & ANIMATION_FLAG) && index < m_frameBufferCache.size()) || ImageDecoder::frameIsFullyReceivedAtIndex(index); On 2016/05/22 15:41:53, aleksandar.stojiljkovic wrote: > On 2016/05/16 20:32:08, scroggo_chromium wrote: > > I brought this up previously, but I don't think you've addressed it. Here is > my > > previous comment: > > > > """ > > As I understand it, GIF's version of frameIsFullyReceivedAtIndex tells us that > > we have received all the bytes for a frame (assuming it's valid). So if we > > decode that frame (maybe we haven't yet) it will end up as FrameComplete > without > > providing more data (again, assuming it's valid). > > > > Looking at WEBP's decodeFrameCount, it is not obvious that we have received > > enough data to mark the frame FrameComplete after a decode. > > """ > > > > Said a different way, if we have enough data to decode part of a frame > > (FramePartial), WEBP's implementation looks like it will return true, but > GIF's > > implementation looks like it will return false. > > I refactored it a bit, but it is still based on index < > m_frameBufferCache.size(). > For multiframe webp, unless there is complete data for the frame, an entry for > it is not added to m_frameBufferCache. That's not obvious to me. An entry is created based on WebPDemuxGetI(WEBP_FF_FRAME_COUNT), which returns WebPDemuxer.num_frames_ [1]. That value is incremented if AddFrame returns true [2]. The comment above [3] states "Store a frame only if the animation flag is set there is *some* data for this frame is available." (emphasis mine) The comment for AddFrame [4] says it "ensur[es] the last frame is complete", but based on the code below, it seems to mean the last one prior to adding. So as far as I can tell, having a frame in m_frameBufferCache does not mean that the frame is complete. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... [3] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... [4] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:91: if (decoder->filenameExtension() != "webp" || frameIsFullyReceived) On 2016/05/22 15:41:53, aleksandar.stojiljkovic wrote: > The check is conservative - when frameIsFullyReceivedAtIndex returns true, there > is enough data to get the frame completely decoded. I'm still not convinced that that is the case. See my other comments about this. > > WebP decoder could rarely return false for frameIsCompleteAtIndex although there > is enough data to get the frame fully decoded. > This happens only for single frame WebP images - frame gets completely decoded 2 > bytes before the end of file and WebP demux reports WebPIterator::complete true > 2 bytes later or after frame decoding is completed. Should we fix that? > Multiframe WebPs used in tests don't expose that type of "byte padding" issue. But it sounds like that issue might be present in other WebPs? https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:89: || ImageDecoder::frameIsFullyReceivedAtIndex(index); This will incorporate m_failed, as Peter suggested in [1]. But it seems that it could return true if there are e.g. 2 frames, and a client calls frameIsFullyReceivedAtIndex(<some number greater than 2>). Is that okay? [1] https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour...
https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:173: return ((m_formatFlags & ANIMATION_FLAG) && index < m_frameBufferCache.size()) || ImageDecoder::frameIsFullyReceivedAtIndex(index); On 2016/05/23 16:40:42, scroggo_chromium wrote: > On 2016/05/22 15:41:53, aleksandar.stojiljkovic wrote: > > On 2016/05/16 20:32:08, scroggo_chromium wrote: > > > I brought this up previously, but I don't think you've addressed it. Here is > > my > > > previous comment: > > > > > > """ > > > As I understand it, GIF's version of frameIsFullyReceivedAtIndex tells us > that > > > we have received all the bytes for a frame (assuming it's valid). So if we > > > decode that frame (maybe we haven't yet) it will end up as FrameComplete > > without > > > providing more data (again, assuming it's valid). > > > > > > Looking at WEBP's decodeFrameCount, it is not obvious that we have received > > > enough data to mark the frame FrameComplete after a decode. > > > """ > > > > > > Said a different way, if we have enough data to decode part of a frame > > > (FramePartial), WEBP's implementation looks like it will return true, but > > GIF's > > > implementation looks like it will return false. > > > > I refactored it a bit, but it is still based on index < > > m_frameBufferCache.size(). > > For multiframe webp, unless there is complete data for the frame, an entry for > > it is not added to m_frameBufferCache. > > That's not obvious to me. An entry is created based on > WebPDemuxGetI(WEBP_FF_FRAME_COUNT), which returns WebPDemuxer.num_frames_ [1]. > That value is incremented if AddFrame returns true [2]. The comment above [3] > states > > "Store a frame only if the animation flag is set there is *some* data for this > frame is available." (emphasis mine) > > The comment for AddFrame [4] says it "ensur[es] the last frame is complete", but > based on the code below, it seems to mean the last one prior to adding. > > So as far as I can tell, having a frame in m_frameBufferCache does not mean that > the frame is complete. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > [4] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... This confused me too. Notice in WEBPImageDecoder::initializeNewFrame [1] assert stating that new frame added is always complete. WebPDemuxGetFrame is 1-based since 0-frame relates to last frame. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... void WEBPImageDecoder::initializeNewFrame(size_t index) { if (!(m_formatFlags & ANIMATION_FLAG)) { ASSERT(!index); return; } WebPIterator animatedFrame; WebPDemuxGetFrame(m_demux, index + 1, &animatedFrame); ASSERT(animatedFrame.complete == 1); Debugged the test and StoreFrame [2] sets complete only for frames where status == PARSE_OK. StoreFrame handling of frame_num ( line with frame->frame_num_ = frame_num; ) is a bit confusing as frame->frame_num > 0 is checked before call to AddFrame() [3] Of course, to make this code more robust, we should probably just call WebPDemuxGetFrame and return complete but I was considering that the before-mentioned assert is robust enough check (although only in debug mode). [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... [3] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:91: if (decoder->filenameExtension() != "webp" || frameIsFullyReceived) On 2016/05/23 16:40:42, scroggo_chromium wrote: > On 2016/05/22 15:41:53, aleksandar.stojiljkovic wrote: > > The check is conservative - when frameIsFullyReceivedAtIndex returns true, > there > > is enough data to get the frame completely decoded. > > I'm still not convinced that that is the case. See my other comments about this. > > > > > WebP decoder could rarely return false for frameIsCompleteAtIndex although > there > > is enough data to get the frame fully decoded. > > This happens only for single frame WebP images - frame gets completely decoded > 2 > > bytes before the end of file and WebP demux reports WebPIterator::complete > true > > 2 bytes later or after frame decoding is completed. > > Should we fix that? Looks like the fix would go to libwebp. I'll check with authors. Anyway, it should not affect this patch as it is not regression. Once fixed, the test could be fixed too. The test pass here for used single and multiframe webps. When true, "fully received" consistent with produced complete frame in decoding. > > > Multiframe WebPs used in tests don't expose that type of "byte padding" issue. > > But it sounds like that issue might be present in other WebPs? > I could try with other webp files. Let me check other files and analyze code deeper to see what is the size of this padding. https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:89: || ImageDecoder::frameIsFullyReceivedAtIndex(index); On 2016/05/23 16:40:42, scroggo_chromium wrote: > This will incorporate m_failed, as Peter suggested in [1]. But it seems that it > could return true if there are e.g. 2 frames, and a client calls > frameIsFullyReceivedAtIndex(<some number greater than 2>). Is that okay? > > [1] > https://codereview.chromium.org/1962563002/diff/20001/third_party/WebKit/Sour... Thanks. Bounds check was missing in ImageDecoder::frameIsFullyReceivedAtIndex - it wasn't OK to return only m_isAllDataReceived. Related to question here, yes makes sense to check on bounds also for failed() - as frame count was or wasn't calculated properly before failed.
https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:173: return ((m_formatFlags & ANIMATION_FLAG) && index < m_frameBufferCache.size()) || ImageDecoder::frameIsFullyReceivedAtIndex(index); On 2016/05/24 11:08:24, aleksandar.stojiljkovic wrote: > On 2016/05/23 16:40:42, scroggo_chromium wrote: > > On 2016/05/22 15:41:53, aleksandar.stojiljkovic wrote: > > > On 2016/05/16 20:32:08, scroggo_chromium wrote: > > > > I brought this up previously, but I don't think you've addressed it. Here > is > > > my > > > > previous comment: > > > > > > > > """ > > > > As I understand it, GIF's version of frameIsFullyReceivedAtIndex tells us > > that > > > > we have received all the bytes for a frame (assuming it's valid). So if we > > > > decode that frame (maybe we haven't yet) it will end up as FrameComplete > > > without > > > > providing more data (again, assuming it's valid). > > > > > > > > Looking at WEBP's decodeFrameCount, it is not obvious that we have > received > > > > enough data to mark the frame FrameComplete after a decode. > > > > """ > > > > > > > > Said a different way, if we have enough data to decode part of a frame > > > > (FramePartial), WEBP's implementation looks like it will return true, but > > > GIF's > > > > implementation looks like it will return false. > > > > > > I refactored it a bit, but it is still based on index < > > > m_frameBufferCache.size(). > > > For multiframe webp, unless there is complete data for the frame, an entry > for > > > it is not added to m_frameBufferCache. > > > > That's not obvious to me. An entry is created based on > > WebPDemuxGetI(WEBP_FF_FRAME_COUNT), which returns WebPDemuxer.num_frames_ [1]. > > That value is incremented if AddFrame returns true [2]. The comment above [3] > > states > > > > "Store a frame only if the animation flag is set there is *some* data for this > > frame is available." (emphasis mine) > > > > The comment for AddFrame [4] says it "ensur[es] the last frame is complete", > but > > based on the code below, it seems to mean the last one prior to adding. > > > > So as far as I can tell, having a frame in m_frameBufferCache does not mean > that > > the frame is complete. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > [3] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > [4] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > This confused me too. Notice in WEBPImageDecoder::initializeNewFrame [1] assert > stating that new frame added is always complete. WebPDemuxGetFrame is 1-based > since 0-frame relates to last frame. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > void WEBPImageDecoder::initializeNewFrame(size_t index) > { > if (!(m_formatFlags & ANIMATION_FLAG)) { > ASSERT(!index); > return; > } > WebPIterator animatedFrame; > WebPDemuxGetFrame(m_demux, index + 1, &animatedFrame); > ASSERT(animatedFrame.complete == 1); > > Debugged the test and StoreFrame [2] sets complete only for frames where status > == PARSE_OK. > StoreFrame handling of frame_num ( line with frame->frame_num_ = frame_num; ) is > a bit confusing as frame->frame_num > 0 is checked before call to AddFrame() [3] > > > Of course, to make this code more robust, we should probably just call > WebPDemuxGetFrame and return complete but I was considering that the > before-mentioned assert is robust enough check (although only in debug mode). > > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... Very confusing. Can you add a comment? https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:91: if (decoder->filenameExtension() != "webp" || frameIsFullyReceived) On 2016/05/24 11:08:24, aleksandar.stojiljkovic wrote: > On 2016/05/23 16:40:42, scroggo_chromium wrote: > > On 2016/05/22 15:41:53, aleksandar.stojiljkovic wrote: > > > The check is conservative - when frameIsFullyReceivedAtIndex returns true, > > there > > > is enough data to get the frame completely decoded. > > > > I'm still not convinced that that is the case. See my other comments about > this. > > > > > > > > WebP decoder could rarely return false for frameIsCompleteAtIndex although > > there > > > is enough data to get the frame fully decoded. > > > This happens only for single frame WebP images - frame gets completely > decoded > > 2 > > > bytes before the end of file and WebP demux reports WebPIterator::complete > > true > > > 2 bytes later or after frame decoding is completed. > > > > Should we fix that? > Looks like the fix would go to libwebp. I'll check with authors. > Anyway, it should not affect this patch as it is not regression. Once fixed, the > test could be fixed too. > > The test pass here for used single and multiframe webps. When true, "fully > received" consistent with produced complete frame in decoding. > > > > > Multiframe WebPs used in tests don't expose that type of "byte padding" > issue. > > > > But it sounds like that issue might be present in other WebPs? > > > > I could try with other webp files. > Let me check other files and analyze code deeper to see what is the size of this > padding. Please add a comment explaining why you need to special case WebP. https://codereview.chromium.org/1962563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:172: return ImageDecoder::frameIsFullyReceivedAtIndex(index) || frameIsCompleteAtIndex(index); Do you need "|| frameIsCompleteAtIndex(index)"? Is it possible that the frame is marked as complete if frameIsFullyReceivedAtIndex returned false? Is this the case where the frame got marked complete before we received the last two bytes? Another question that might be relevant - does the caller need to know frameIsCompleteAtIndex? The main caller of (the newly named) frameIsFullyReceivedAtIndex is DeferredImageDecoder, which calls it on m_actualDecoder, where frameIsCompleteAtIndex will never be true. But I think that's fine, because it really wants to know whether the frame is fully received.
https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:173: return ((m_formatFlags & ANIMATION_FLAG) && index < m_frameBufferCache.size()) || ImageDecoder::frameIsFullyReceivedAtIndex(index); On 2016/05/24 13:55:14, scroggo_chromium wrote: > On 2016/05/24 11:08:24, aleksandar.stojiljkovic wrote: > > On 2016/05/23 16:40:42, scroggo_chromium wrote: > > > On 2016/05/22 15:41:53, aleksandar.stojiljkovic wrote: > > > > On 2016/05/16 20:32:08, scroggo_chromium wrote: > > > > > I brought this up previously, but I don't think you've addressed it. > Here > > is > > > > my > > > > > previous comment: > > > > > > > > > > """ > > > > > As I understand it, GIF's version of frameIsFullyReceivedAtIndex tells > us > > > that > > > > > we have received all the bytes for a frame (assuming it's valid). So if > we > > > > > decode that frame (maybe we haven't yet) it will end up as FrameComplete > > > > without > > > > > providing more data (again, assuming it's valid). > > > > > > > > > > Looking at WEBP's decodeFrameCount, it is not obvious that we have > > received > > > > > enough data to mark the frame FrameComplete after a decode. > > > > > """ > > > > > > > > > > Said a different way, if we have enough data to decode part of a frame > > > > > (FramePartial), WEBP's implementation looks like it will return true, > but > > > > GIF's > > > > > implementation looks like it will return false. > > > > > > > > I refactored it a bit, but it is still based on index < > > > > m_frameBufferCache.size(). > > > > For multiframe webp, unless there is complete data for the frame, an entry > > for > > > > it is not added to m_frameBufferCache. > > > > > > That's not obvious to me. An entry is created based on > > > WebPDemuxGetI(WEBP_FF_FRAME_COUNT), which returns WebPDemuxer.num_frames_ > [1]. > > > That value is incremented if AddFrame returns true [2]. The comment above > [3] > > > states > > > > > > "Store a frame only if the animation flag is set there is *some* data for > this > > > frame is available." (emphasis mine) > > > > > > The comment for AddFrame [4] says it "ensur[es] the last frame is complete", > > but > > > based on the code below, it seems to mean the last one prior to adding. > > > > > > So as far as I can tell, having a frame in m_frameBufferCache does not mean > > that > > > the frame is complete. > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > > [2] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > > [3] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > > [4] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > > > This confused me too. Notice in WEBPImageDecoder::initializeNewFrame [1] > assert > > stating that new frame added is always complete. WebPDemuxGetFrame is 1-based > > since 0-frame relates to last frame. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > void WEBPImageDecoder::initializeNewFrame(size_t index) > > { > > if (!(m_formatFlags & ANIMATION_FLAG)) { > > ASSERT(!index); > > return; > > } > > WebPIterator animatedFrame; > > WebPDemuxGetFrame(m_demux, index + 1, &animatedFrame); > > ASSERT(animatedFrame.complete == 1); > > > > Debugged the test and StoreFrame [2] sets complete only for frames where > status > > == PARSE_OK. > > StoreFrame handling of frame_num ( line with frame->frame_num_ = frame_num; ) > is > > a bit confusing as frame->frame_num > 0 is checked before call to AddFrame() > [3] > > > > > > Of course, to make this code more robust, we should probably just call > > WebPDemuxGetFrame and return complete but I was considering that the > > before-mentioned assert is robust enough check (although only in debug mode). > > > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > [3] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb... > > Very confusing. Can you add a comment? Done. https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:91: if (decoder->filenameExtension() != "webp" || frameIsFullyReceived) On 2016/05/24 13:55:14, scroggo_chromium wrote: > On 2016/05/24 11:08:24, aleksandar.stojiljkovic wrote: > > On 2016/05/23 16:40:42, scroggo_chromium wrote: > > > On 2016/05/22 15:41:53, aleksandar.stojiljkovic wrote: > > > > The check is conservative - when frameIsFullyReceivedAtIndex returns true, > > > there > > > > is enough data to get the frame completely decoded. > > > > > > I'm still not convinced that that is the case. See my other comments about > > this. > > > > > > > > > > > WebP decoder could rarely return false for frameIsCompleteAtIndex although > > > there > > > > is enough data to get the frame fully decoded. > > > > This happens only for single frame WebP images - frame gets completely > > decoded > > > 2 > > > > bytes before the end of file and WebP demux reports WebPIterator::complete > > > true > > > > 2 bytes later or after frame decoding is completed. > > > > > > Should we fix that? > > Looks like the fix would go to libwebp. I'll check with authors. > > Anyway, it should not affect this patch as it is not regression. Once fixed, > the > > test could be fixed too. > > > > The test pass here for used single and multiframe webps. When true, "fully > > received" consistent with produced complete frame in decoding. > > > > > > > Multiframe WebPs used in tests don't expose that type of "byte padding" > > issue. > > > > > > But it sounds like that issue might be present in other WebPs? > > > > > > > I could try with other webp files. > > Let me check other files and analyze code deeper to see what is the size of > this > > padding. > > Please add a comment explaining why you need to special case WebP. Done. https://codereview.chromium.org/1962563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:172: return ImageDecoder::frameIsFullyReceivedAtIndex(index) || frameIsCompleteAtIndex(index); On 2016/05/24 13:55:14, scroggo_chromium wrote: > Do you need "|| frameIsCompleteAtIndex(index)"? Is it possible that the frame is > > Is this the case where the frame got marked complete before we received the last > two bytes? Yes, that is exactly the case. WebP parsing doesn't mark the frame as complete but when we decode it we get complete frame. WebP parsing marks it as complete 2 bytes later. So I added the "|| frameIsCompleteAtIndex(index);" to cover it. As it can happen only in tests, no point in having the check. Removed it. > > Another question that might be relevant - does the caller need to know > frameIsCompleteAtIndex? The main caller of (the newly named) > frameIsFullyReceivedAtIndex is DeferredImageDecoder, which calls it on > m_actualDecoder, where frameIsCompleteAtIndex will never be true. But I think > that's fine, because it really wants to know whether the frame is fully > received.
LGTM. Thanks! https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:92: // FIXME: libwebp parsing of crbug.364830.webp doesn't mark the frame We know the file name. Should we special case further to the file?
On 2016/05/24 19:34:11, scroggo_chromium wrote: > LGTM. Thanks! > > https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp > (right): > > https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:92: > // FIXME: libwebp parsing of crbug.364830.webp doesn't mark the frame > We know the file name. Should we special case further to the file? Thanks. I would like to leave it like this - plan to get it fixed in libwebp and then I would update this part. Bug 364830 looks somewhat related to this - early reporting of end of file.
https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h:99: if (index < const_cast<MockImageDecoder*>(this)->frameCount() - 1) Casting away const is really unpleasant. frameCount() could change a variety of things about this object. Can we implement this without doing that somehow? Maybe always call through to the base class? Maybe keep enough data in this class to answer the query without calling frameCount()? https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); This implementation worries me. (1) If we call this without having first called frameCount() to update the frame buffer cache size, then this might return false if |index| is too large, even when it should actually return true. (2) If the image is a multi-frame ICO, you don't override this method in the subclass decoder, and so I think this can return false even when it should return true, as long as we haven't received _all_ the data yet. (2) is probably fixable by implementing an ICO-specific version that checks if the data length is large enough to cover the desired frame's data region (the ICO has a directory that should provide this info). Of course, to know that, we need to have decoded at least the directory. This basically reduces to case (1). This means that in general this method is probably unsafe to call unless either it does partial image decoding, or all callers decode before calling it. That seems like a critical problem in the API, as I think part of the goal of this method was to be useful in cases where the caller didn't do, or want, any decoding. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:96: EXPECT_EQ(frameIsFullyReceived, decoder->frameIsCompleteAtIndex(frameCount - 1) || decoder->failed()); Isn't this test fundamentally incorrect? It's checking that frameIsCompleteAtIndex() changes value at the same time as frameIsFullyReceivedAtIndex(). But it's possible for a frame to be complete before it's fully received, if there's garbage data after the frame that the decoder doesn't know in advance isn't needed to decode the frame. The only way we could actually do this check, I think, is for frameIsFullyReceivedAtIndex() to try to decode the frame and return true if the status was FrameComplete. Which I don't think you want to do, so I'd remove this. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:175: // initializeNewFrame() only when the frame data is fully received. Really? That's not what it looks like to me. m_frameBufferCache is resized by ImageDecoder::frameCount(), which calls decodeFrameCount(), which can return a value larger than the number of frames we have complete data for, AFAICT.
Patch Set 9 : review #27 fixes. Thanks pkasting@ scroggo@, there are some changes related to pkasting@'s comment about ICO/direct decoding, so you might need to check it. Thanks. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h:99: if (index < const_cast<MockImageDecoder*>(this)->frameCount() - 1) On 2016/05/25 01:05:23, Peter Kasting wrote: > Casting away const is really unpleasant. frameCount() could change a variety of > things about this object. > > Can we implement this without doing that somehow? Maybe always call through to > the base class? Maybe keep enough data in this class to answer the query > without calling frameCount()? Done. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); On 2016/05/25 01:05:23, Peter Kasting wrote: > This implementation worries me. > > (1) If we call this without having first called frameCount() to update the frame > buffer cache size, then this might return false if |index| is too large, even > when it should actually return true. The pattern of calling frameCount() looks like the design choice here - to split the logic between clients (ImageSource, DeferredImageDecoder) and ImageDecoder::frameCount(). Parsing for frame completeness happens under frameCount()(which is invoking virtual decodeFrameCount). In case of frameBufferAtImdex, frameCount is called on start of API call. In all other cases, as clients are calling APIs after frameCount() there is no need to call it from within the ImageDecoder API calls. > (2) If the image is a multi-frame ICO, you don't override this method in the > subclass decoder, and so I think this can return false even when it should > return true, as long as we haven't received _all_ the data yet. As ICOs are opted out [1] of deferred image decoding, it is important to put back original condition: m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete. scroggo@, we have discussed about this related to WebP in previous patch - here the change is for different reason (direct decoding). [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > (2) is probably fixable by implementing an ICO-specific version that checks if > the data length is large enough to cover the desired frame's data region (the > ICO has a directory that should provide this info). Of course, to know that, we > need to have decoded at least the directory. This basically reduces to case > (1). As ICOs are opted out, this is related to direct decoding and restoring the condition existing in original code should make things right (assuming we don't want to implement deferred decoding support for ICO). > > This means that in general this method is probably unsafe to call unless either > it does partial image decoding, or all callers decode before calling it. That > seems like a critical problem in the API, as I think part of the goal of this > method was to be useful in cases where the caller didn't do, or want, any > decoding. Not sure if you are referring to ICO case/direct (non-deferred) decoding here or to all decoders implementation. Under frameCount -> virtual decodeFrameCount call, WebP and GIF are doing parsing and there is no need for decoding before frameIsFullyReceivedAtIndex(size_t index). https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:96: EXPECT_EQ(frameIsFullyReceived, decoder->frameIsCompleteAtIndex(frameCount - 1) || decoder->failed()); On 2016/05/25 01:05:23, Peter Kasting wrote: > Isn't this test fundamentally incorrect? It's checking that > frameIsCompleteAtIndex() changes value at the same time as > frameIsFullyReceivedAtIndex(). But it's possible for a frame to be complete > before it's fully received, if there's garbage data after the frame that the > decoder doesn't know in advance isn't needed to decode the frame. > > The only way we could actually do this check, I think, is for > frameIsFullyReceivedAtIndex() to try to decode the frame and return true if the > status was FrameComplete. Which I don't think you want to do, so I'd remove > this. This test checks that frameIsFullyReceivedAtIndex before calling decode frame (i.e. decoder->frameBufferAtIndex) returns the same value as frameIsCompleteAtIndex() after decode. It was missing before and it is important as it is verifying that clients get valid results for (frameCount + frameIsFullyReceivedAtIndex) before decoding frames. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:175: // initializeNewFrame() only when the frame data is fully received. On 2016/05/25 01:05:23, Peter Kasting wrote: > Really? That's not what it looks like to me. m_frameBufferCache is resized by > ImageDecoder::frameCount(), which calls decodeFrameCount(), which can return a > value larger than the number of frames we have complete data for, AFAICT. Yes, it is like that. I could work on documenting this further if needed. You could see it from frameCount [1]: frameCount() also calls WEBPImageDecoder::initializeNewFrame [2] only for frames which are completely available and it verifies it by ASSERT(animatedFrame.complete == 1); This functionality is specific to multiframe WebP only - that m_frameBufferCache size is equal to number of completely received frames. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > On 2016/05/25 01:05:23, Peter Kasting wrote: > > This implementation worries me. > > > > (1) If we call this without having first called frameCount() to update the > frame > > buffer cache size, then this might return false if |index| is too large, even > > when it should actually return true. > > The pattern of calling frameCount() looks like the design choice here - to split > the logic between clients (ImageSource, DeferredImageDecoder) and > ImageDecoder::frameCount(). > Parsing for frame completeness happens under frameCount()(which is invoking > virtual decodeFrameCount). > In case of frameBufferAtImdex, frameCount is called on start of API call. > In all other cases, as clients are calling APIs after frameCount() there is no > need to call it from within the ImageDecoder API calls. I guess what I'm saying is that it's dangerous to implement an API that is callable in a variety of cases but happens to be silently incorrect in some of them. Just because we happen to meet the right criteria today doesn't prevent us from refactoring later and goofing this up. The bare minimum would be some kind of comment about the prerequisites of the API. Better would be some kind of set of assertions that would prevent incorrect call order, and better still might be some system that transparently ensures all prerequisite decoding etc. has happened, without unnecessarily duplicating effort. > > (2) If the image is a multi-frame ICO, you don't override this method in the > > subclass decoder, and so I think this can return false even when it should > > return true, as long as we haven't received _all_ the data yet. > > As ICOs are opted out [1] of deferred image decoding, it is important to put > back original condition: m_frameBufferCache[index].getStatus() == > ImageFrame::FrameComplete. > scroggo@, we have discussed about this related to WebP in previous patch - here > the change is for different reason (direct decoding). > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... That's bizarre and seems wrong. Why are ICOs excluded here? We should fix that, unless there's some really good reason I'm missing.
Oops, hit send too early. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:96: EXPECT_EQ(frameIsFullyReceived, decoder->frameIsCompleteAtIndex(frameCount - 1) || decoder->failed()); On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > On 2016/05/25 01:05:23, Peter Kasting wrote: > > Isn't this test fundamentally incorrect? It's checking that > > frameIsCompleteAtIndex() changes value at the same time as > > frameIsFullyReceivedAtIndex(). But it's possible for a frame to be complete > > before it's fully received, if there's garbage data after the frame that the > > decoder doesn't know in advance isn't needed to decode the frame. > > > > The only way we could actually do this check, I think, is for > > frameIsFullyReceivedAtIndex() to try to decode the frame and return true if > the > > status was FrameComplete. Which I don't think you want to do, so I'd remove > > this. > > This test checks that frameIsFullyReceivedAtIndex before calling decode frame > (i.e. decoder->frameBufferAtIndex) returns the same value as > frameIsCompleteAtIndex() after decode. > It was missing before and it is important as it is verifying that clients get > valid results for (frameCount + frameIsFullyReceivedAtIndex) before decoding > frames. If you want to check that, isn't the correct test simply: if (frameIsFullyReceived) EXPECT_TRUE(decoder->frameIsCompleteAtIndex(frameCount - 1) || decoder->failed()); The remainder of the check is incorrect in the case I mentioned, or if decoding were to fail before the frame is fully received. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:175: // initializeNewFrame() only when the frame data is fully received. On 2016/05/30 16:12:20, aleksandar.stojiljkovic wrote: > On 2016/05/25 01:05:23, Peter Kasting wrote: > > Really? That's not what it looks like to me. m_frameBufferCache is resized > by > > ImageDecoder::frameCount(), which calls decodeFrameCount(), which can return a > > value larger than the number of frames we have complete data for, AFAICT. > > Yes, it is like that. I could work on documenting this further if needed. > You could see it from frameCount [1]: > frameCount() also calls WEBPImageDecoder::initializeNewFrame [2] only for frames > which are completely available and it verifies it by > ASSERT(animatedFrame.complete == 1); > > This functionality is specific to multiframe WebP only - that m_frameBufferCache > size is equal to number of completely received frames. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... That looks to me like a bug in the webp decoder, honestly. Doesn't it have the full projected frame count available near the beginning of the image? If so I think we should fix it to return that number as soon as it's available -- as all the other decoders here do -- and not simply return the number of frames that have been fully received. Not only is this more consistent and seems more in keeping with the spirit of "what is this image's frame count", I would worry about someone getting a nonzero number from this call and assuming that's the final number. I don't know that any code does this today -- I think if we get new data we may assume all bets are off, including the existing frame count -- but it wouldn't be hard for someone to make that assumption later.
https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); On 2016/05/31 03:28:26, Peter Kasting wrote: > On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > This implementation worries me. > > > > > > (1) If we call this without having first called frameCount() to update the > > frame > > > buffer cache size, then this might return false if |index| is too large, > even > > > when it should actually return true. > > > > The pattern of calling frameCount() looks like the design choice here - to > split > > the logic between clients (ImageSource, DeferredImageDecoder) and > > ImageDecoder::frameCount(). > > Parsing for frame completeness happens under frameCount()(which is invoking > > virtual decodeFrameCount). > > In case of frameBufferAtImdex, frameCount is called on start of API call. > > In all other cases, as clients are calling APIs after frameCount() there is no > > need to call it from within the ImageDecoder API calls. > > I guess what I'm saying is that it's dangerous to implement an API that is > callable in a variety of cases but happens to be silently incorrect in some of > them. Just because we happen to meet the right criteria today doesn't prevent > us from refactoring later and goofing this up. The bare minimum would be some > kind of comment about the prerequisites of the API. Better would be some kind > of set of assertions that would prevent incorrect call order, and better still > might be some system that transparently ensures all prerequisite decoding etc. > has happened, without unnecessarily duplicating effort. > > > > (2) If the image is a multi-frame ICO, you don't override this method in the > > > subclass decoder, and so I think this can return false even when it should > > > return true, as long as we haven't received _all_ the data yet. > > > > As ICOs are opted out [1] of deferred image decoding, it is important to put > > back original condition: m_frameBufferCache[index].getStatus() == > > ImageFrame::FrameComplete. > > scroggo@, we have discussed about this related to WebP in previous patch - > here > > the change is for different reason (direct decoding). > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > That's bizarre and seems wrong. Why are ICOs excluded here? We should fix > that, unless there's some really good reason I'm missing. We have a bug filed for the ICO exception: crbug.com/513306. It appears that ICO is excluded just because no one has added it. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:175: // initializeNewFrame() only when the frame data is fully received. On 2016/05/31 03:46:42, Peter Kasting wrote: > On 2016/05/30 16:12:20, aleksandar.stojiljkovic wrote: > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > Really? That's not what it looks like to me. m_frameBufferCache is resized > > by > > > ImageDecoder::frameCount(), which calls decodeFrameCount(), which can return > a > > > value larger than the number of frames we have complete data for, AFAICT. > > > > Yes, it is like that. I could work on documenting this further if needed. > > You could see it from frameCount [1]: > > frameCount() also calls WEBPImageDecoder::initializeNewFrame [2] only for > frames > > which are completely available and it verifies it by > > ASSERT(animatedFrame.complete == 1); > > > > This functionality is specific to multiframe WebP only - that > m_frameBufferCache > > size is equal to number of completely received frames. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > That looks to me like a bug in the webp decoder, honestly. Doesn't it have the > full projected frame count available near the beginning of the image? If so I > think we should fix it to return that number as soon as it's available -- as all > the other decoders here do -- and not simply return the number of frames that > have been fully received. Not only is this more consistent and seems more in > keeping with the spirit of "what is this image's frame count", AFAICT, the GIF decoder does not know its frame count up front either. When it receives data for another frame it increases its frame count [1]. There is an inconsistency, though - GIF may have partially received frames, but it appears that WEBP does not. FWIW, even if the projected frame count *is* at the beginning of the image, what do we do if it doesn't match the actual number of frames? - Ignore any extra frames? That seems wrong to me. - Stop (and loop, if looping is appropriate) if we come up short? (This is what we do if we do not trust the projected frame count.) Or insert empty frames? Either way trusting the projected frame count seems wrong to me. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > I would worry > about someone getting a nonzero number from this call and assuming that's the > final number. I don't know that any code does this today -- I think if we get > new data we may assume all bets are off, including the existing frame count -- > but it wouldn't be hard for someone to make that assumption later.
https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); On 2016/06/01 15:11:25, scroggo_chromium wrote: > On 2016/05/31 03:28:26, Peter Kasting wrote: > > On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > > This implementation worries me. > > > > > > > > (1) If we call this without having first called frameCount() to update the > > > frame > > > > buffer cache size, then this might return false if |index| is too large, > > even > > > > when it should actually return true. > > > > > > The pattern of calling frameCount() looks like the design choice here - to > > split > > > the logic between clients (ImageSource, DeferredImageDecoder) and > > > ImageDecoder::frameCount(). > > > Parsing for frame completeness happens under frameCount()(which is invoking > > > virtual decodeFrameCount). > > > In case of frameBufferAtImdex, frameCount is called on start of API call. > > > In all other cases, as clients are calling APIs after frameCount() there is > no > > > need to call it from within the ImageDecoder API calls. > > > > I guess what I'm saying is that it's dangerous to implement an API that is > > callable in a variety of cases but happens to be silently incorrect in some of > > them. Just because we happen to meet the right criteria today doesn't prevent > > us from refactoring later and goofing this up. The bare minimum would be some > > kind of comment about the prerequisites of the API. Better would be some kind > > of set of assertions that would prevent incorrect call order, and better still > > might be some system that transparently ensures all prerequisite decoding etc. > > has happened, without unnecessarily duplicating effort. > > > > > > (2) If the image is a multi-frame ICO, you don't override this method in > the > > > > subclass decoder, and so I think this can return false even when it should > > > > return true, as long as we haven't received _all_ the data yet. > > > > > > As ICOs are opted out [1] of deferred image decoding, it is important to put > > > back original condition: m_frameBufferCache[index].getStatus() == > > > ImageFrame::FrameComplete. > > > scroggo@, we have discussed about this related to WebP in previous patch - > > here > > > the change is for different reason (direct decoding). > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > That's bizarre and seems wrong. Why are ICOs excluded here? We should fix > > that, unless there's some really good reason I'm missing. > > We have a bug filed for the ICO exception: crbug.com/513306. It appears that ICO > is excluded just because no one has added it. Good find. It would be nice to get that fixed. Failing that, though, I would write this patch assuming that that will eventually be fixed, and so being prepared for the potential consequences. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:175: // initializeNewFrame() only when the frame data is fully received. On 2016/06/01 15:11:25, scroggo_chromium wrote: > On 2016/05/31 03:46:42, Peter Kasting wrote: > > On 2016/05/30 16:12:20, aleksandar.stojiljkovic wrote: > > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > > Really? That's not what it looks like to me. m_frameBufferCache is > resized > > > by > > > > ImageDecoder::frameCount(), which calls decodeFrameCount(), which can > return > > a > > > > value larger than the number of frames we have complete data for, AFAICT. > > > > > > Yes, it is like that. I could work on documenting this further if needed. > > > You could see it from frameCount [1]: > > > frameCount() also calls WEBPImageDecoder::initializeNewFrame [2] only for > > frames > > > which are completely available and it verifies it by > > > ASSERT(animatedFrame.complete == 1); > > > > > > This functionality is specific to multiframe WebP only - that > > m_frameBufferCache > > > size is equal to number of completely received frames. > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > [2] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > That looks to me like a bug in the webp decoder, honestly. Doesn't it have > the > > full projected frame count available near the beginning of the image? If so I > > think we should fix it to return that number as soon as it's available -- as > all > > the other decoders here do -- and not simply return the number of frames that > > have been fully received. Not only is this more consistent and seems more in > > keeping with the spirit of "what is this image's frame count", > > AFAICT, the GIF decoder does not know its frame count up front either. When it > receives data for another frame it increases its frame count [1]. Huh. I was misled by the way there's a separate frame vector for the reader versus decoder, and things like a "GIF frame count query" enum value, but it turns out that actually maps to "just decode normally". I think you're correct. > There is an inconsistency, though - GIF may have partially received frames, but > it appears that WEBP does not. Yeah. I'm not sure why that difference exists. We try to avoid animating to an incomplete frame so I don't think it should cause any visible effects, but from a code perspective it would be nice to be consistent. Perhaps GIF should do what WebP does, and we would be able to remove the animation code that avoids animating to an incomplete frame? Admittedly that's like one line, but whatever. > FWIW, even if the projected frame count *is* at the beginning of the image, what > do we do if it doesn't match the actual number of frames? > > - Ignore any extra frames? That seems wrong to me. > - Stop (and loop, if looping is appropriate) if we come up short? (This is what > we do if we do not trust the projected frame count.) Or insert empty frames? > > Either way trusting the projected frame count seems wrong to me. I agree.
https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); On 2016/06/01 15:23:49, Peter Kasting wrote: > On 2016/06/01 15:11:25, scroggo_chromium wrote: > > On 2016/05/31 03:28:26, Peter Kasting wrote: > > > On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > > > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > > > This implementation worries me. > > > > > > > > > > (1) If we call this without having first called frameCount() to update > the > > > > frame > > > > > buffer cache size, then this might return false if |index| is too large, > > > even > > > > > when it should actually return true. > > > > > > > > The pattern of calling frameCount() looks like the design choice here - to > > > split > > > > the logic between clients (ImageSource, DeferredImageDecoder) and > > > > ImageDecoder::frameCount(). > > > > Parsing for frame completeness happens under frameCount()(which is > invoking > > > > virtual decodeFrameCount). > > > > In case of frameBufferAtImdex, frameCount is called on start of API call. > > > > In all other cases, as clients are calling APIs after frameCount() there > is > > no > > > > need to call it from within the ImageDecoder API calls. > > > > > > I guess what I'm saying is that it's dangerous to implement an API that is > > > callable in a variety of cases but happens to be silently incorrect in some > of > > > them. Just because we happen to meet the right criteria today doesn't > prevent > > > us from refactoring later and goofing this up. The bare minimum would be > some > > > kind of comment about the prerequisites of the API. Better would be some > kind > > > of set of assertions that would prevent incorrect call order, and better > still > > > might be some system that transparently ensures all prerequisite decoding > etc. > > > has happened, without unnecessarily duplicating effort. > > > > > > > > (2) If the image is a multi-frame ICO, you don't override this method in > > the > > > > > subclass decoder, and so I think this can return false even when it > should > > > > > return true, as long as we haven't received _all_ the data yet. > > > > > > > > As ICOs are opted out [1] of deferred image decoding, it is important to > put > > > > back original condition: m_frameBufferCache[index].getStatus() == > > > > ImageFrame::FrameComplete. > > > > scroggo@, we have discussed about this related to WebP in previous patch - > > > here > > > > the change is for different reason (direct decoding). > > > > > > > > [1] > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > That's bizarre and seems wrong. Why are ICOs excluded here? We should fix > > > that, unless there's some really good reason I'm missing. > > > > We have a bug filed for the ICO exception: crbug.com/513306. It appears that > ICO > > is excluded just because no one has added it. > > Good find. > > It would be nice to get that fixed. Failing that, though, I would write this > patch assuming that that will eventually be fixed, and so being prepared for the > potential consequences. I'll take the ICO case separately - starting work on it. Suggest to scope this patch here to refactoring only and add ICO deferred decoding support including all the tests (e.g. bytebybyte parse and decode) in separate patch. Tests are missing for ICO - doing any functionality change would be risky. I understand that we might disagree that this patch actually changes functionality already but with current set of tests I see that covered. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); On 2016/05/31 03:28:26, Peter Kasting wrote: > On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > This implementation worries me. > > > > > > (1) If we call this without having first called frameCount() to update the > > frame > > > buffer cache size, then this might return false if |index| is too large, > even > > > when it should actually return true. > > > > The pattern of calling frameCount() looks like the design choice here - to > split > > the logic between clients (ImageSource, DeferredImageDecoder) and > > ImageDecoder::frameCount(). > > Parsing for frame completeness happens under frameCount()(which is invoking > > virtual decodeFrameCount). > > In case of frameBufferAtImdex, frameCount is called on start of API call. > > In all other cases, as clients are calling APIs after frameCount() there is no > > need to call it from within the ImageDecoder API calls. > > I guess what I'm saying is that it's dangerous to implement an API that is > callable in a variety of cases but happens to be silently incorrect in some of > them. Just because we happen to meet the right criteria today doesn't prevent > us from refactoring later and goofing this up. The bare minimum would be some > kind of comment about the prerequisites of the API. Better would be some kind > of set of assertions that would prevent incorrect call order, and better still > might be some system that transparently ensures all prerequisite decoding etc. > has happened, without unnecessarily duplicating effort. > > > > (2) If the image is a multi-frame ICO, you don't override this method in the > > > subclass decoder, and so I think this can return false even when it should > > > return true, as long as we haven't received _all_ the data yet. > > > > As ICOs are opted out [1] of deferred image decoding, it is important to put > > back original condition: m_frameBufferCache[index].getStatus() == > > ImageFrame::FrameComplete. > > scroggo@, we have discussed about this related to WebP in previous patch - > here > > the change is for different reason (direct decoding). > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > That's bizarre and seems wrong. Why are ICOs excluded here? We should fix > that, unless there's some really good reason I'm missing. I spent some time thinking about this after initial opinion how I wouldn't like to change it. Makes sense to do it - we could flag "the change happened" on ImageDecoder::setData and then call frameCount on start of frameIsFullyReceivedAtIndex (early exit if setData wasn't called). This way we would make WebPImageDecoder::m_haveAlreadyParsedThisData functionality common for all the decoders.
Patch Set 10 : ASSERT if frameCount not called to parse before calling... https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); On 2016/05/31 03:28:26, Peter Kasting wrote: > On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > This implementation worries me. > > > > > > (1) If we call this without having first called frameCount() to update the > > frame > > > buffer cache size, then this might return false if |index| is too large, > even > > > when it should actually return true. > > > > The pattern of calling frameCount() looks like the design choice here - to > split > > the logic between clients (ImageSource, DeferredImageDecoder) and > > ImageDecoder::frameCount(). > > Parsing for frame completeness happens under frameCount()(which is invoking > > virtual decodeFrameCount). > > In case of frameBufferAtImdex, frameCount is called on start of API call. > > In all other cases, as clients are calling APIs after frameCount() there is no > > need to call it from within the ImageDecoder API calls. > > I guess what I'm saying is that it's dangerous to implement an API that is > callable in a variety of cases but happens to be silently incorrect in some of > them. Just because we happen to meet the right criteria today doesn't prevent > us from refactoring later and goofing this up. The bare minimum would be some > kind of comment about the prerequisites of the API. Better would be some kind > of set of assertions that would prevent incorrect call order, and better still > might be some system that transparently ensures all prerequisite decoding etc. > has happened, without unnecessarily duplicating effort. Done - assertion added. Calling non const (e.g.frameCount) internally lead to making all of the methods non const and as an overhead. On the other side, frameCount is documented as non const that is parsing and decoding frame count so it looked OK. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:96: EXPECT_EQ(frameIsFullyReceived, decoder->frameIsCompleteAtIndex(frameCount - 1) || decoder->failed()); On 2016/05/31 03:46:42, Peter Kasting wrote: > On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > Isn't this test fundamentally incorrect? It's checking that > > > frameIsCompleteAtIndex() changes value at the same time as > > > frameIsFullyReceivedAtIndex(). But it's possible for a frame to be complete > > > before it's fully received, if there's garbage data after the frame that the > > > decoder doesn't know in advance isn't needed to decode the frame. > > > > > > The only way we could actually do this check, I think, is for > > > frameIsFullyReceivedAtIndex() to try to decode the frame and return true if > > the > > > status was FrameComplete. Which I don't think you want to do, so I'd remove > > > this. > > > > This test checks that frameIsFullyReceivedAtIndex before calling decode frame > > (i.e. decoder->frameBufferAtIndex) returns the same value as > > frameIsCompleteAtIndex() after decode. > > It was missing before and it is important as it is verifying that clients get > > valid results for (frameCount + frameIsFullyReceivedAtIndex) before decoding > > frames. > > If you want to check that, isn't the correct test simply: > > if (frameIsFullyReceived) > EXPECT_TRUE(decoder->frameIsCompleteAtIndex(frameCount - 1) || > decoder->failed()); > > The remainder of the check is incorrect in the case I mentioned, or if decoding > were to fail before the frame is fully received. For other than WebP it is important to check that also false means that there is not enough data to produce complete frame on decoding. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:96: EXPECT_EQ(frameIsFullyReceived, decoder->frameIsCompleteAtIndex(frameCount - 1) || decoder->failed()); On 2016/05/25 01:05:23, Peter Kasting wrote: > Isn't this test fundamentally incorrect? It's checking that > frameIsCompleteAtIndex() changes value at the same time as > frameIsFullyReceivedAtIndex(). But it's possible for a frame to be complete > before it's fully received, if there's garbage data after the frame that the > decoder doesn't know in advance isn't needed to decode the frame. Decoder get to know this in setData() which is called before decode. And after setData, frameCount is called to parse. I don't know how it is possible that the frame is complete before it is fully received (except if there was a failure). > > The only way we could actually do this check, I think, is for > frameIsFullyReceivedAtIndex() to try to decode the frame and return true if the > status was FrameComplete. Which I don't think you want to do, so I'd remove > this.
https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:343: bool m_haveUpdatedFrameCount; According to the style guide (https://www.chromium.org/blink/coding-style), member variables should be private. You can make this private and add protected: bool haveUpdatedFrameCount() const { return m_haveUpdatedFrameCount; } https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:300: { Maybe assert here that we've updated the frame count, since you've removed the call to parse? https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:182: return false; Don't you want to return if (haveUpdatedFrameCount)? Or does that never happen?
Patch Set 11 : review #35 fixes. https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:343: bool m_haveUpdatedFrameCount; On 2016/06/02 21:00:14, scroggo_chromium wrote: > According to the style guide (https://www.chromium.org/blink/coding-style), > member variables should be private. You can make this private and add > > protected: > bool haveUpdatedFrameCount() const { return m_haveUpdatedFrameCount; } Thanks. Done. https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:300: { On 2016/06/02 21:00:14, scroggo_chromium wrote: > Maybe assert here that we've updated the frame count, since you've removed the > call to parse? Done. https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:182: return false; On 2016/06/02 21:00:14, scroggo_chromium wrote: > Don't you want to return if (haveUpdatedFrameCount)? Or does that never happen? Reverted this code - having ImageDecoder::haveUpdatedSize() and ImageDecoder::haveUpdatedFrameCount() and using them in different ways in decoders didn't make the code simpler.
Description was changed from ========== Split ImageDecoder::frameIsCompleteAtIndex and frameIsFullyReceivedAtIndex For ImageDecoder::frameIsCompleteAtIndex comment states: // Whether or not the frame is fully received. Implementation in ImageDecoder::frameIsCompleteAtIndex checked if decoded. GIF and WEBP decoder have it properly implemented - parsing if frame data is fully received. JPEG and PNG decoder used this method to check if frame decoding is done. Making it consistent to comment and usage in GIF & WEBP - return if frame data is fully received. History: @hclam intention [1] was to have this API used only for multi-frame images. [1] https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image... BUG= ========== to ========== ImageDecoder::frameIsFullyReceivedAtIndex and deferred decoding for ICO 1) ICO was opted out of lazy (deferred) image decoding BUG=513306 2) frameIsFullyReceivedAtIndex fix: Method frameIsCompletedAtIndex was somewhere used with intent of "fully received" and on the other places as "completely decoded". Comment next to frameIsCompleteAtIndex states: // Whether or not the frame is fully received. Implementation in ImageDecoder::frameIsCompleteAtIndex checked if decoded. GIF and WEBP decoder have it properly implemented - parsing if frame data is fully received. JPEG and PNG decoder used this method to check if frame decoding is done. Making it consistent to comment and usage in GIF & WEBP - return if frame data is fully received. History: @hclam intention [1] was to have this API used only for multi-frame images. [1] https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image... BUG= ==========
Patch 12# - Deferred decoding for ICO. Tests pass locally but it feels too early for thorough review - wanted to doublecheck some of the design ideas. Thanks. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:154: return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size(); On 2016/06/01 16:21:33, aleksandar.stojiljkovic wrote: > On 2016/06/01 15:23:49, Peter Kasting wrote: > > On 2016/06/01 15:11:25, scroggo_chromium wrote: > > > On 2016/05/31 03:28:26, Peter Kasting wrote: > > > > On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > > > > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > > > > This implementation worries me. > > > > > > > > > > > > (1) If we call this without having first called frameCount() to update > > the > > > > > frame > > > > > > buffer cache size, then this might return false if |index| is too > large, > > > > even > > > > > > when it should actually return true. > > > > > > > > > > The pattern of calling frameCount() looks like the design choice here - > to > > > > split > > > > > the logic between clients (ImageSource, DeferredImageDecoder) and > > > > > ImageDecoder::frameCount(). > > > > > Parsing for frame completeness happens under frameCount()(which is > > invoking > > > > > virtual decodeFrameCount). > > > > > In case of frameBufferAtImdex, frameCount is called on start of API > call. > > > > > In all other cases, as clients are calling APIs after frameCount() there > > is > > > no > > > > > need to call it from within the ImageDecoder API calls. > > > > > > > > I guess what I'm saying is that it's dangerous to implement an API that is > > > > callable in a variety of cases but happens to be silently incorrect in > some > > of > > > > them. Just because we happen to meet the right criteria today doesn't > > prevent > > > > us from refactoring later and goofing this up. The bare minimum would be > > some > > > > kind of comment about the prerequisites of the API. Better would be some > > kind > > > > of set of assertions that would prevent incorrect call order, and better > > still > > > > might be some system that transparently ensures all prerequisite decoding > > etc. > > > > has happened, without unnecessarily duplicating effort. > > > > > > > > > > (2) If the image is a multi-frame ICO, you don't override this method > in > > > the > > > > > > subclass decoder, and so I think this can return false even when it > > should > > > > > > return true, as long as we haven't received _all_ the data yet. > > > > > > > > > > As ICOs are opted out [1] of deferred image decoding, it is important to > > put > > > > > back original condition: m_frameBufferCache[index].getStatus() == > > > > > ImageFrame::FrameComplete. > > > > > scroggo@, we have discussed about this related to WebP in previous patch > - > > > > here > > > > > the change is for different reason (direct decoding). > > > > > > > > > > [1] > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > That's bizarre and seems wrong. Why are ICOs excluded here? We should > fix > > > > that, unless there's some really good reason I'm missing. > > > > > > We have a bug filed for the ICO exception: crbug.com/513306. It appears that > > ICO > > > is excluded just because no one has added it. > > > > Good find. > > > > It would be nice to get that fixed. Failing that, though, I would write this > > patch assuming that that will eventually be fixed, and so being prepared for > the > > potential consequences. > > I'll take the ICO case separately - starting work on it. > Suggest to scope this patch here to refactoring only and add ICO deferred > decoding support including all the tests (e.g. bytebybyte parse and decode) in > separate patch. Tests are missing for ICO - doing any functionality change would > be risky. I understand that we might disagree that this patch actually changes > functionality already but with current set of tests I see that covered. Changed my mind - an attempt to implement ICO support is in Patch 12# Deferred decoding for ICO. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:92: // FIXME: libwebp parsing of crbug.364830.webp doesn't mark the frame On 2016/05/24 19:34:11, scroggo_chromium wrote: > We know the file name. Should we special case further to the file? Written the check differently. https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:96: EXPECT_EQ(frameIsFullyReceived, decoder->frameIsCompleteAtIndex(frameCount - 1) || decoder->failed()); On 2016/05/31 03:46:42, Peter Kasting wrote: > On 2016/05/30 16:12:19, aleksandar.stojiljkovic wrote: > > On 2016/05/25 01:05:23, Peter Kasting wrote: > > > Isn't this test fundamentally incorrect? It's checking that > > > frameIsCompleteAtIndex() changes value at the same time as > > > frameIsFullyReceivedAtIndex(). But it's possible for a frame to be complete > > > before it's fully received, if there's garbage data after the frame that the > > > decoder doesn't know in advance isn't needed to decode the frame. > > > > > > The only way we could actually do this check, I think, is for > > > frameIsFullyReceivedAtIndex() to try to decode the frame and return true if > > the > > > status was FrameComplete. Which I don't think you want to do, so I'd remove > > > this. > > > > This test checks that frameIsFullyReceivedAtIndex before calling decode frame > > (i.e. decoder->frameBufferAtIndex) returns the same value as > > frameIsCompleteAtIndex() after decode. > > It was missing before and it is important as it is verifying that clients get > > valid results for (frameCount + frameIsFullyReceivedAtIndex) before decoding > > frames. > > If you want to check that, isn't the correct test simply: > > if (frameIsFullyReceived) > EXPECT_TRUE(decoder->frameIsCompleteAtIndex(frameCount - 1) || > decoder->failed()); > > The remainder of the check is incorrect in the case I mentioned, or if decoding > were to fail before the frame is fully received. You have the point here. Written the the check like this - my argument how this was useful for GIF was wrong since GIFImageDecoderTest is having separate byteByByteDecode implementation. https://codereview.chromium.org/1962563002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:89: bool frameIsFullyReceived = decoder->frameIsFullyReceivedAtIndex(framesDecoded); The change here is that we decode frames one after another, not only frameCount -1 frame. Note the change in behavior for webP which is returning only partially received frames. Might make sense to decode both here. For ICO frames there is a difference comparet to all other decoders that memory layout is different from frame order. Memory layout could be like this e.g. |<frame1><frame0>| and framecount will return 1 frame until receiving full file (when framecount would return 2). At the end of file we don't want to count full decode for only for frame1 but first completely decode frame0 here and then frame1 in loop bellow. https://codereview.chromium.org/1962563002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:98: if (!decoder->frameIsCompleteAtIndex(framesDecoded) && !decoder->failed()) Broken logic here - fullyReceived = trye for partially decoded frame would trigger assert above. Removing this check and assert in next patch. https://codereview.chromium.org/1962563002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:116: if (frame && frame->getStatus() == ImageFrame::FrameComplete) "&& framesDecoded < frameCount" was reequired for WebP as framecount is only returning fullyreceived frames. New implementation tries to decode partially received frames (outside of frameCount for webp) https://codereview.chromium.org/1962563002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:120: Removed this newline. https://codereview.chromium.org/1962563002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:134: if ((dirEntry.m_imageOffset + dirEntry.m_byteSize) > m_data->size()) frameCount should include only fully received frames. The last frame could be partially received. https://codereview.chromium.org/1962563002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:188: ASSERT(m_frameBufferCache.size() == decodeFrameCount()); this assert is copied from previous code but is not very useful (it is just verifying that decodeAtIndex) is not called without decodeFrameCount. decodeFrameCount gets called from frameBufferAtIndex which calls decodeFrameCount.
I think ICO should be converted to deferred decoding in a separate patch.
Description was changed from ========== ImageDecoder::frameIsFullyReceivedAtIndex and deferred decoding for ICO 1) ICO was opted out of lazy (deferred) image decoding BUG=513306 2) frameIsFullyReceivedAtIndex fix: Method frameIsCompletedAtIndex was somewhere used with intent of "fully received" and on the other places as "completely decoded". Comment next to frameIsCompleteAtIndex states: // Whether or not the frame is fully received. Implementation in ImageDecoder::frameIsCompleteAtIndex checked if decoded. GIF and WEBP decoder have it properly implemented - parsing if frame data is fully received. JPEG and PNG decoder used this method to check if frame decoding is done. Making it consistent to comment and usage in GIF & WEBP - return if frame data is fully received. History: @hclam intention [1] was to have this API used only for multi-frame images. [1] https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image... BUG= ========== to ========== Split ImageDecoder::frameIsCompleteAtIndex and frameIsFullyReceivedAtIndex For ImageDecoder::frameIsCompleteAtIndex comment states: // Whether or not the frame is fully received. Implementation in ImageDecoder::frameIsCompleteAtIndex checked if decoded. GIF and WEBP decoder have it properly implemented - parsing if frame data is fully received. JPEG and PNG decoder used this method to check if frame decoding is done. Making it consistent to comment and usage in GIF & WEBP - return if frame data is fully received. History: @hclam intention [1] was to have this API used only for multi-frame images. [1] https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image... BUG= ==========
patchset 13 - reverted ICO support. On 2016/06/06 15:27:28, scroggo_chromium wrote: > I think ICO should be converted to deferred decoding in a separate patch. Thanks. Sorry for complicating the review. Reverted in patch set 13. Patch 14 contains changes related to this patch without ICO support. https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:95: for (size_t i = 0; i < framesDecoded; ++i) { This is partly addressed in DeferredImageDecoderTest but here it is doing it byte by byte. It is related to the behavior expected in DeferredImageDecoder::prepareLazyDecodedFrames - all the frames before partially available frame are expected to be fully received.
Stepping back a bit, what precise bug are we fixing here? THe description has no BUG= link and mostly describes the "what" rather than the "why". I'm not really clear on what we need this for. https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h:101: return ImageDecoder::frameIsFullyReceivedAtIndex(index); Nit: Simpler: return (index < m_frameBufferCache.size() - 1) || ImageDecoder::frameIsFullyReceivedAtIndex(index); https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:153: && (m_isAllDataReceived || failed() || m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete); If we know all decoders report only fully-received frames in the frame count, we could simplify this function down to just checking if the frame buffer cache was large enough. If we don't know that, then this implementation basically works, but risks false negatives if we've decoded enough to get an updated frame count, but haven't actually decoded the data of some of those new frames. There's no way around this unless you want to call frameBufferAtIndex() to force a decode. It's not obvious to me which one callers would want. We should strive to make sure these sorts of caveats are very clear in the API documentation. https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:179: return !frameIsFullyReceivedAtIndex(index) || m_frameBufferCache[index].hasAlpha(); This implementation confuses me. I don't understand why we could ever rely on frameIsFullyReceivedAtIndex() instead of FrameIsCompleteAtIndex(). For example, if the last frame is corrupt, "fully received" will return true due to the failed() check, but the frame is incomplete, and we should always report it as having alpha even if the decoding code didn't see alpha up to that point. So it seems like the right thing here is return (index < m_frameBufferCache.size()) && (!frameIsCompleteAtIndex(index) || m_frameBufferCache[index].hasAlpha()); (The first line could be omitted if we were OK with returning true for frames past the end, but it seems like the convention is to return false from all these oracles in this case.) https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:342: // Whether frameCount() was called to update frame count after setData(). Nit: update -> update the https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:95: for (size_t i = 0; i < framesDecoded; ++i) { On 2016/06/06 16:36:38, aleksandar.stojiljkovic wrote: > This is partly addressed in DeferredImageDecoderTest but here it is doing it > byte by byte. > It is related to the behavior expected in > DeferredImageDecoder::prepareLazyDecodedFrames - all the frames before partially > available frame are expected to be fully received. Is that guaranteed for things like ICO where later frames may appear before earlier ones? In theory, we could get enough data to completely decode ICO frome 2 before frame 1. Or does this never happen, because of your changes (in your other CL) to make ICO report that only the first n frames are received?
On 2016/06/10 00:10:02, Peter Kasting wrote: > Stepping back a bit, what precise bug are we fixing here? THe description has > no BUG= link and mostly describes the "what" rather than the "why". I'm not > really clear on what we need this for. > There is no bug. It is exploratory refactoring attempting to make the code clearer. Started from the notice that frameIsCompleteAtIndex has different meaning in different implementations. https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:153: && (m_isAllDataReceived || failed() || m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete); On 2016/06/10 00:10:02, Peter Kasting wrote: > If we know all decoders report only fully-received frames in the frame count, we > could simplify this function down to just checking if the frame buffer cache was > large enough. For GIF and single frame (e.g. JPEG) we return partial (first frame of animation) in frame count. > > If we don't know that, then this implementation basically works, but risks false > negatives if we've decoded enough to get an updated frame count, but haven't > actually decoded the data of some of those new frames. There's no way around > this unless you want to call frameBufferAtIndex() to force a decode. It's not > obvious to me which one callers would want. We should strive to make sure these > sorts of caveats are very clear in the API documentation. False negatives are expected and "|| m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete" is just a best effort in case decoding happened. On the other hand, id the frame was cleared from cache after this, it is possible that for the same index we return both true and false. So, to keep it consistant it is better to remove "getStatus() == ImageFrame::FrameComplete" for multiframe decoders - single frame decoders don't remove frames from cache. https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:179: return !frameIsFullyReceivedAtIndex(index) || m_frameBufferCache[index].hasAlpha(); On 2016/06/10 00:10:02, Peter Kasting wrote: > This implementation confuses me. I don't understand why we could ever rely on > frameIsFullyReceivedAtIndex() instead of FrameIsCompleteAtIndex(). For example, > if the last frame is corrupt, "fully received" will return true due to the > failed() check, but the frame is incomplete, and we should always report it as > having alpha even if the decoding code didn't see alpha up to that point. > > So it seems like the right thing here is > > return (index < m_frameBufferCache.size()) && > (!frameIsCompleteAtIndex(index) || > m_frameBufferCache[index].hasAlpha()); > > (The first line could be omitted if we were OK with returning true for frames > past the end, but it seems like the convention is to return false from all these > oracles in this case.) I think that adding failed() to fullyReceived is causing the inconsistency here. If it is fullyreceived (but not failed) then method is OK. Maybe we remove failed from frameIsFullyReceivedAtIndex and client calls both frameIsFullyReceivedAtIndex and failed() if needed to know that there is no more changes pending to frame encoded data. https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:95: for (size_t i = 0; i < framesDecoded; ++i) { On 2016/06/10 00:10:02, Peter Kasting wrote: > On 2016/06/06 16:36:38, aleksandar.stojiljkovic wrote: > > This is partly addressed in DeferredImageDecoderTest but here it is doing it > > byte by byte. > > It is related to the behavior expected in > > DeferredImageDecoder::prepareLazyDecodedFrames - all the frames before > partially > > available frame are expected to be fully received. > > Is that guaranteed for things like ICO where later frames may appear before > earlier ones? In theory, we could get enough data to completely decode ICO > frome 2 before frame 1. Or does this never happen, because of your changes (in > your other CL) to make ICO report that only the first n frames are received? This never happens now after the other CL.
https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:153: && (m_isAllDataReceived || failed() || m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete); On 2016/06/10 21:51:08, aleksandar.stojiljkovic wrote: > On 2016/06/10 00:10:02, Peter Kasting wrote: > > If we know all decoders report only fully-received frames in the frame count, > we > > could simplify this function down to just checking if the frame buffer cache > was > > large enough. > > For GIF and single frame (e.g. JPEG) we return partial (first frame of > animation) in frame count. I know, but it sounded on another CL like there was a possibility of unifying all the behaviors here. (I think returning partial frames in the frame count is probably better, though, so that would likely be what I'd unify on.) > > If we don't know that, then this implementation basically works, but risks > false > > negatives if we've decoded enough to get an updated frame count, but haven't > > actually decoded the data of some of those new frames. There's no way around > > this unless you want to call frameBufferAtIndex() to force a decode. It's not > > obvious to me which one callers would want. We should strive to make sure > these > > sorts of caveats are very clear in the API documentation. > > False negatives are expected and "|| m_frameBufferCache[index].getStatus() == > ImageFrame::FrameComplete" is just a best effort in case decoding happened. APIs that are expected to be wrong make me worried. These lead to confusing, hard-to-understand code later because the code doesn't actually mean what it naively reads as meaning. This is why I've been looking so hard for ways to make these bulletproof, and add assertions around all the edges. If we can, I'd prefer to make the implementation "just work"; where that isn't the right thing to do, we have to make it extremely clear to callers what the caveats are. > On > the other hand, id the frame was cleared from cache after this, it is possible > that for the same index we return both true and false. Do we actually delete frames from the cache entirely? It seems like that's overkill; we could just drop the ref to the pixel data and keep the metadata. That would avoid this issue. > So, to keep it consistant > it is better to remove "getStatus() == ImageFrame::FrameComplete" for multiframe > decoders - single frame decoders don't remove frames from cache. Moving in the direction of even-less-accurate results seems like the wrong way to go... https://codereview.chromium.org/1962563002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:179: return !frameIsFullyReceivedAtIndex(index) || m_frameBufferCache[index].hasAlpha(); On 2016/06/10 21:51:08, aleksandar.stojiljkovic wrote: > On 2016/06/10 00:10:02, Peter Kasting wrote: > > This implementation confuses me. I don't understand why we could ever rely on > > frameIsFullyReceivedAtIndex() instead of FrameIsCompleteAtIndex(). For > example, > > if the last frame is corrupt, "fully received" will return true due to the > > failed() check, but the frame is incomplete, and we should always report it as > > having alpha even if the decoding code didn't see alpha up to that point. > > > > So it seems like the right thing here is > > > > return (index < m_frameBufferCache.size()) && > > (!frameIsCompleteAtIndex(index) || > > m_frameBufferCache[index].hasAlpha()); > > > > (The first line could be omitted if we were OK with returning true for frames > > past the end, but it seems like the convention is to return false from all > these > > oracles in this case.) > > I think that adding failed() to fullyReceived is causing the inconsistency here. I don't understand why we would ever want to toggle between Complete and FullyReceived, regardless of whether either one includes failed() in its check. What's wrong with my proposed implementation? > Maybe we remove > failed from frameIsFullyReceivedAtIndex and client calls both > frameIsFullyReceivedAtIndex and failed() if needed to know that there is no more > changes pending to frame encoded data. Once again, this makes the API weirder and harder to use and demands that clients "do it right". In general we should be doing our utmost to not do that.
What's the status here? We went round and round on this CL a lot and my last set of comments has no reply. Don't want to leave this hanging forever.
On 2016/08/20 02:47:20, Peter Kasting wrote: > What's the status here? We went round and round on this CL a lot and my last > set of comments has no reply. Don't want to leave this hanging forever. Short keep-alive here. Sorry for delay, had to focus on some other issues. This is refactoring aimed to clarify things but in the latest comments Peter raised the issues rendering the patch doing the exactly opposite - making the API less clear. I still need to resolve this and keeping the patch open. Thanks.
Already done in https://codereview.chromium.org/2879123003 |
