Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2879123003:
blink: Fix frame completion querying for static images. (Closed)
Descriptionblink: Fix frame completion querying for static images.
ImageDecoder::FrameIsCompleteAtIndex is meant to return whether all
encoded data for the frame at the queried index has been received.
While it returns this value for animated images, for single frame
images, it returns true only if a cached decode for the complete image
is available. This discrepancy ends up having code in blink incorrectly
assuming that the image has only been partially loaded.
This change has the decoder return this status for static images based
on whether all data for an image has been received.
BUG=722168
Review-Url: https://codereview.chromium.org/2879123003
Cr-Commit-Position: refs/heads/master@{#472000}
Committed: https://chromium.googlesource.com/chromium/src/+/4d60bb8e8f149732f85716124e05c349b360c333
Patch Set 1 #
Total comments: 7
Patch Set 2 : test #
Total comments: 5
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== blink: Fix frame completion querying for static images. BUG= ========== to ========== blink: Fix frame completion querying for static images. ImageDecoder::FrameIsCompleteAtIndex[1] is meant to return whether all encoded data for the frame at the queried index has been received.While it returns this value for animated images, for single frame images, it returns true only if a cached decode for the complete image is available. This discrepancy ends up having code in blink::BitmapImage incorrectly assuming that the image has only been partially loaded[2]. BUG=722168 ==========
Description was changed from ========== blink: Fix frame completion querying for static images. ImageDecoder::FrameIsCompleteAtIndex[1] is meant to return whether all encoded data for the frame at the queried index has been received.While it returns this value for animated images, for single frame images, it returns true only if a cached decode for the complete image is available. This discrepancy ends up having code in blink::BitmapImage incorrectly assuming that the image has only been partially loaded[2]. BUG=722168 ========== to ========== blink: Fix frame completion querying for static images. ImageDecoder::FrameIsCompleteAtIndex is meant to return whether all encoded data for the frame at the queried index has been received. While it returns this value for animated images, for single frame images, it returns true only if a cached decode for the complete image is available. This discrepancy ends up having code in blink incorrectly assuming that the image has only been partially loaded. This change has the decoder return this status for static images based on whether all data for an image has been received. BUG=722168 ==========
khushalsagar@chromium.org changed reviewers: + urvang@chromium.org
PTAL. The only test that is failing is the PNGTest that is asserting the old behaviour for this function.
On 2017/05/15 03:36:28, Khushal wrote: > PTAL. The only test that is failing is the PNGTest that is asserting the old > behaviour for this function. Update the test then? https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:185: return index < frame_buffer_cache_.size() && Shouldn't you call "FrameIsDecodedAtIndex()" now instead? https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:192: return IsAllDataReceived(); I'm curious if you get a warning about "index" being unused here.
Thanks! Just wanted to confirm this was reasonable before updating the test. :) https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:185: return index < frame_buffer_cache_.size() && On 2017/05/15 17:33:07, urvang wrote: > Shouldn't you call "FrameIsDecodedAtIndex()" now instead? FrameIsDecodedAtIndex is true only if the decoded data for that frame is cached. For the cases where the decoder is used just for reading the metadata on the main thread, this will never be true. From a quick glance, it looks like the only code that uses it is that exact case in DeferredImageDecoder. I can just keep the current call FrameIsCompleteAtIndex here to avoid a change in behaviour? https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:192: return IsAllDataReceived(); On 2017/05/15 17:33:07, urvang wrote: > I'm curious if you get a warning about "index" being unused here. The bots don't seem to be complaining.
https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:185: return index < frame_buffer_cache_.size() && On 2017/05/15 17:55:17, Khushal wrote: > On 2017/05/15 17:33:07, urvang wrote: > > Shouldn't you call "FrameIsDecodedAtIndex()" now instead? > > FrameIsDecodedAtIndex is true only if the decoded data for that frame is cached. > For the cases where the decoder is used just for reading the metadata on the > main thread, this will never be true. From a quick glance, it looks like the > only code that uses it is that exact case in DeferredImageDecoder. > > I can just keep the current call FrameIsCompleteAtIndex here to avoid a change > in behaviour? Yes, if keeping the current call still works, you can do this change separately. https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:192: return IsAllDataReceived(); On 2017/05/15 17:55:17, Khushal wrote: > On 2017/05/15 17:33:07, urvang wrote: > > I'm curious if you get a warning about "index" being unused here. > > The bots don't seem to be complaining. Acknowledged.
The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run
Updated the test as well. https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2879123003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:185: return index < frame_buffer_cache_.size() && On 2017/05/15 18:02:10, urvang wrote: > On 2017/05/15 17:55:17, Khushal wrote: > > On 2017/05/15 17:33:07, urvang wrote: > > > Shouldn't you call "FrameIsDecodedAtIndex()" now instead? > > > > FrameIsDecodedAtIndex is true only if the decoded data for that frame is > cached. > > For the cases where the decoder is used just for reading the metadata on the > > main thread, this will never be true. From a quick glance, it looks like the > > only code that uses it is that exact case in DeferredImageDecoder. > > > > I can just keep the current call FrameIsCompleteAtIndex here to avoid a change > > in behaviour? > > Yes, if keeping the current call still works, you can do this change separately. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by khushalsagar@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494904128995880,
"parent_rev": "a1bef6ebf879776e004eb3d79b715d146a339bb0", "commit_rev":
"fa941e123a0af39e4f264ab26d48f4c792e30fad"}
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494904128995880,
"parent_rev": "a970533a51c5570e6ab2ab1fbb8a6d862e180fdb", "commit_rev":
"4d60bb8e8f149732f85716124e05c349b360c333"}
Message was sent while issue was closed.
Description was changed from ========== blink: Fix frame completion querying for static images. ImageDecoder::FrameIsCompleteAtIndex is meant to return whether all encoded data for the frame at the queried index has been received. While it returns this value for animated images, for single frame images, it returns true only if a cached decode for the complete image is available. This discrepancy ends up having code in blink incorrectly assuming that the image has only been partially loaded. This change has the decoder return this status for static images based on whether all data for an image has been received. BUG=722168 ========== to ========== blink: Fix frame completion querying for static images. ImageDecoder::FrameIsCompleteAtIndex is meant to return whether all encoded data for the frame at the queried index has been received. While it returns this value for animated images, for single frame images, it returns true only if a cached decode for the complete image is available. This discrepancy ends up having code in blink incorrectly assuming that the image has only been partially loaded. This change has the decoder return this status for static images based on whether all data for an image has been received. BUG=722168 Review-Url: https://codereview.chromium.org/2879123003 Cr-Commit-Position: refs/heads/master@{#472000} Committed: https://chromium.googlesource.com/chromium/src/+/4d60bb8e8f149732f85716124e05... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4d60bb8e8f149732f85716124e05...
Message was sent while issue was closed.
scroggo@chromium.org changed reviewers: + scroggo@chromium.org
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:192: return IsAllDataReceived(); Yay! I like that this method now means the same thing for animated images and non-animated images. https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:196: virtual bool FrameIsCompleteAtIndex(size_t) const; nit: As a follow-on, I think we should rename this to FrameIsReceivedAtIndex. This old name is confusing. https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:995: return decoder->FrameIsDecodedAtIndex(0); This is the only caller of the new API, and this method is in turn only called by JPEGImageDecoder, which has access to the internal info needed by this method. Maybe we should delete this method and have JPEGImageDecoder check directly? To keep the clarity of this inline method, we could make this a private method on JPEGImageDecoder: JPEGImageDecoder::IsComplete(bool only_size) const { if (HasImagePlanes() && !only_size) return true; return frame_buffer_cache_[0].GetStatus() == ImageFrame::kFrameComplete; }
Message was sent while issue was closed.
https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:196: virtual bool FrameIsCompleteAtIndex(size_t) const; On 2017/05/30 15:51:34, scroggo_chromium wrote: > nit: As a follow-on, I think we should rename this to FrameIsReceivedAtIndex. > This old name is confusing. Uploaded crrev.com/2982083002 https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/2879123003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:995: return decoder->FrameIsDecodedAtIndex(0); On 2017/05/30 15:51:34, scroggo_chromium wrote: > This is the only caller of the new API, and this method is in turn only called > by JPEGImageDecoder, which has access to the internal info needed by this > method. Maybe we should delete this method and have JPEGImageDecoder check > directly? To keep the clarity of this inline method, we could make this a > private method on JPEGImageDecoder: > > JPEGImageDecoder::IsComplete(bool only_size) const { > if (HasImagePlanes() && !only_size) > return true; > return frame_buffer_cache_[0].GetStatus() == ImageFrame::kFrameComplete; > } Uploaded crrev.com/2984463002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
