Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516593003:
Pull up clearCacheExceptFrame to ImageDecoder. (Closed)
|
Created:
4 years, 1 month ago by joostouwerling Modified:
4 years ago CC:
chromium-reviews, blink-reviews, jzern, skal, urvang Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPull up clearCacheExceptFrame to ImageDecoder.
Just a refactor, no changed behavior or new tests.
This is a follow-up on crrev.com/2468233002 (Pull up
clearCacheExceptFrame impl. to ImageDecoder), after a behavior change
for WebP's cache clearing has landed in crrev.com/2499633002 (Always
preserve provided frame in WebP::clearCacheExceptFrame).
Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and
GIFImageDecoder to ImageDecoder. This increases code reuse, especially since
PNGImageDecoder can use it for the Animated PNG implementation as well. Child
classes of ImageDecoder only need to override clearFrameBuffer() and / or
frameStatusIsSufficientForSuccessors(), if necessary.
The baseline idea of the implementation is that the provided frame is
always preserved, and that there are two cases in which a previous
frame is also preserved:
1. The provided frame is not yet sufficiently decoded to decode subsequent
frames, as indicated by frameStatusIsSufficientForSuccessors(). We need
to keep the required previous frame to sufficiently decode the provided
frame first.
2. The disposal method of the provided frame is DisposeOverwritePrevious. In
this case, we need to keep the required previous frame in cache to
prevent re-decoding that frame when the provided frame is disposed.
WebP does not know DisposeOverwritePrevious, so the new implementation
is effectively equivalent to the original method, because
frameStatusIsSufficientForSuccessors returns true iff the status is
FrameComplete.
Gif's original implementation is somewhat different, in the sense that
the roles of |clearExceptFrame| and |clearExceptFrame2| are
interchanged. Now, |clearExpectFrame| is always kept and
|clearExceptFrame2| contains any required previous frame. The resulting
behavior is not changed, though, given the following case analysis:
- The provided frame is FrameEmpty. The old implementation would only
keep the nearest non-empty frame in cache, since the first block is a
no-op due to the != FrameEmpty condition, and the second block would
search for the nearest non-empty frame in cache.
The new patch sets |clearExceptFrame2| to the required previous frame,
since !frameStatusIsSufficientForSuccessors evaluates to true. The
second block will find the nearest non-empty frame in cache.
|clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but
is ignored since that method checks for FrameEmpty frames.
- The provided frame is FrameComplete or FramePartial, and the disposal
method is DisposeOverwritePrevious. In the old implementation this
results in |clearExceptFrame| being set to the required previous
frame in the first block, and |clearExceptFrame2| is set to the
provided frame. The second block searches for the nearest non-empty
frame, starting at |clearExceptFrame|.
The new implementation stores the required previous frame in
|clearExceptFrame2| because of the disposal method, and similarly
searches for the nearest non-empty frame in the second block.
- The provided frame is FrameComplete or FramePartial, and the disposal
method is *not* DisposeOverwritePrevious. In this case, the old
implementation only preserves the provided frame since the first block
is a no-op due to the disposal condition, and the second block is a
no-op as well since the frames' status is not FrameEmpty.
The new implementation does not set |clearExceptFrame2| because of the
disposal condition in the first block, and thus only preserves the
provided frame.
Committed: https://crrev.com/e0df7a9b0f7bab12de923b1f58d047e0ff4f4de4
Cr-Commit-Position: refs/heads/master@{#435636}
Patch Set 1 #Patch Set 2 : Make clearCacheExceptFrame non-virtual #
Total comments: 2
Patch Set 3 : Reword comment describing clearExceptFrame2 #Patch Set 4 : Merge from master to avoid merge conflicts #Patch Set 5 : Make clearCacheExceptFrame virtual to allow MockImageDecoder override #
Total comments: 4
Patch Set 6 : Add todo for changing MockImageDecoder #
Messages
Total messages: 34 (18 generated)
Description was changed from ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always cleared and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. BUG= ========== to ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always cleared and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. BUG= ==========
Description was changed from ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always cleared and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. BUG= ========== to ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always cleared and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. ==========
joostouwerling@google.com changed reviewers: + cblume@chromium.org, scroggo@chromium.org, urvang@chromium.org
On 2016/11/18 21:49:29, joostouwerling wrote: non-owner lgtm
lgtm
lgtm > Gif's original implementation is somewhat different, in the sense that > the roles of |clearExceptFrame| and |clearExceptFrame2| are > interchanged. Now, |clearExpectFrame| is always cleared and I think you mean is always *kept*? > |clearExceptFrame2| contains any required previous frame. The resulting > behavior is not changed, though, given the following case analysis: https://codereview.chromium.org/2516593003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2516593003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:238: // Now |clearExceptFrame2| indicates the frame that future frames will depend I don't think this comment is quite correct. It's possible that the next frame/future frames depend on clearExceptFrame, but clearExceptFrame depends on clearExceptFrame2, right? e.g. in WebP if clearExceptFrame is FramePartial. (I suppose in that case, the next frame *also* depends on clearExceptFrame2, transitively, so maybe it's not too misleading, but it confused me a little...)
Description was changed from ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always cleared and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. ========== to ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always kept and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. ==========
On 2016/11/28 16:34:38, scroggo_chromium wrote: > lgtm > > > Gif's original implementation is somewhat different, in the sense that > > the roles of |clearExceptFrame| and |clearExceptFrame2| are > > interchanged. Now, |clearExpectFrame| is always cleared and > > I think you mean is always *kept*? Yes. Fixed. > > |clearExceptFrame2| contains any required previous frame. The resulting > > behavior is not changed, though, given the following case analysis: > > https://codereview.chromium.org/2516593003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): > > https://codereview.chromium.org/2516593003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:238: // Now > |clearExceptFrame2| indicates the frame that future frames will depend > I don't think this comment is quite correct. It's possible that the next > frame/future frames depend on clearExceptFrame, but clearExceptFrame depends on > clearExceptFrame2, right? e.g. in WebP if clearExceptFrame is FramePartial. (I > suppose in that case, the next frame *also* depends on clearExceptFrame2, > transitively, so maybe it's not too misleading, but it confused me a little...)
https://codereview.chromium.org/2516593003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2516593003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:238: // Now |clearExceptFrame2| indicates the frame that future frames will depend On 2016/11/28 16:34:38, scroggo_chromium wrote: > I don't think this comment is quite correct. It's possible that the next > frame/future frames depend on clearExceptFrame, but clearExceptFrame depends on > clearExceptFrame2, right? e.g. in WebP if clearExceptFrame is FramePartial. (I > suppose in that case, the next frame *also* depends on clearExceptFrame2, > transitively, so maybe it's not too misleading, but it confused me a little...) I don't think it was inherently wrong, but I clarified it to make sure no confusion is left :)
The CQ bit was checked by joostouwerling@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org, cblume@chromium.org, urvang@chromium.org Link to the patchset: https://codereview.chromium.org/2516593003/#ps40001 (title: "Reword comment describing clearExceptFrame2")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by joostouwerling@google.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2516593003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2516593003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:253: // order to run the test ImageFrameGenerator::clearMultiFrameDecode. Instead of making this virtual, how about making MockImageDecoder override clearFrameBuffer? MockImageDecoder will have to be a little more complicated; instead of reporting the index passed to clearCacheExceptFrame, you'll need to keep track of all the frames that were cleared. But that's probably what we care about more anyway.
https://codereview.chromium.org/2516593003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2516593003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:253: // order to run the test ImageFrameGenerator::clearMultiFrameDecode. On 2016/12/01 14:44:07, scroggo_chromium wrote: > Instead of making this virtual, how about making MockImageDecoder override > clearFrameBuffer? MockImageDecoder will have to be a little more complicated; > instead of reporting the index passed to clearCacheExceptFrame, you'll need to > keep track of all the frames that were cleared. But that's probably what we care > about more anyway. Sgtm but I propose to do that in a different CL. In that way, we can keep this CL a pure refactor and change the behavior of the clearMultiFrameDecode test in another CL.
https://codereview.chromium.org/2516593003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2516593003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:253: // order to run the test ImageFrameGenerator::clearMultiFrameDecode. On 2016/12/01 15:05:06, joostouwerling wrote: > On 2016/12/01 14:44:07, scroggo_chromium wrote: > > Instead of making this virtual, how about making MockImageDecoder override > > clearFrameBuffer? MockImageDecoder will have to be a little more complicated; > > instead of reporting the index passed to clearCacheExceptFrame, you'll need to > > keep track of all the frames that were cleared. But that's probably what we > care > > about more anyway. > > Sgtm but I propose to do that in a different CL. In that way, we can keep this > CL a pure refactor and change the behavior of the clearMultiFrameDecode test in > another CL. Okay by me, but can you add a TODO? The danger in leaving it to the next CL is that we might forget about it.
https://codereview.chromium.org/2516593003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2516593003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:253: // order to run the test ImageFrameGenerator::clearMultiFrameDecode. On 2016/12/01 15:08:00, scroggo_chromium wrote: > On 2016/12/01 15:05:06, joostouwerling wrote: > > On 2016/12/01 14:44:07, scroggo_chromium wrote: > > > Instead of making this virtual, how about making MockImageDecoder override > > > clearFrameBuffer? MockImageDecoder will have to be a little more > complicated; > > > instead of reporting the index passed to clearCacheExceptFrame, you'll need > to > > > keep track of all the frames that were cleared. But that's probably what we > > care > > > about more anyway. > > > > Sgtm but I propose to do that in a different CL. In that way, we can keep this > > CL a pure refactor and change the behavior of the clearMultiFrameDecode test > in > > another CL. > > Okay by me, but can you add a TODO? The danger in leaving it to the next CL is > that we might forget about it. Done.
lgtm
The CQ bit was checked by joostouwerling@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from cblume@chromium.org, urvang@chromium.org Link to the patchset: https://codereview.chromium.org/2516593003/#ps140001 (title: "Add todo for changing MockImageDecoder")
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": 140001, "attempt_start_ts": 1480606670828800,
"parent_rev": "41bbb17b9364aad4da961c682096b72e13e76513", "commit_rev":
"c7d74aed285a495e31718592a004acfc85fe4cb1"}
Message was sent while issue was closed.
Description was changed from ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always kept and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. ========== to ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always kept and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always kept and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. ========== to ========== Pull up clearCacheExceptFrame to ImageDecoder. Just a refactor, no changed behavior or new tests. This is a follow-up on crrev.com/2468233002 (Pull up clearCacheExceptFrame impl. to ImageDecoder), after a behavior change for WebP's cache clearing has landed in crrev.com/2499633002 (Always preserve provided frame in WebP::clearCacheExceptFrame). Pull up the implementation of clearCacheExceptFrame in WEBPImageDecoder and GIFImageDecoder to ImageDecoder. This increases code reuse, especially since PNGImageDecoder can use it for the Animated PNG implementation as well. Child classes of ImageDecoder only need to override clearFrameBuffer() and / or frameStatusIsSufficientForSuccessors(), if necessary. The baseline idea of the implementation is that the provided frame is always preserved, and that there are two cases in which a previous frame is also preserved: 1. The provided frame is not yet sufficiently decoded to decode subsequent frames, as indicated by frameStatusIsSufficientForSuccessors(). We need to keep the required previous frame to sufficiently decode the provided frame first. 2. The disposal method of the provided frame is DisposeOverwritePrevious. In this case, we need to keep the required previous frame in cache to prevent re-decoding that frame when the provided frame is disposed. WebP does not know DisposeOverwritePrevious, so the new implementation is effectively equivalent to the original method, because frameStatusIsSufficientForSuccessors returns true iff the status is FrameComplete. Gif's original implementation is somewhat different, in the sense that the roles of |clearExceptFrame| and |clearExceptFrame2| are interchanged. Now, |clearExpectFrame| is always kept and |clearExceptFrame2| contains any required previous frame. The resulting behavior is not changed, though, given the following case analysis: - The provided frame is FrameEmpty. The old implementation would only keep the nearest non-empty frame in cache, since the first block is a no-op due to the != FrameEmpty condition, and the second block would search for the nearest non-empty frame in cache. The new patch sets |clearExceptFrame2| to the required previous frame, since !frameStatusIsSufficientForSuccessors evaluates to true. The second block will find the nearest non-empty frame in cache. |clearExceptFrame| is also passed to clearCacheExceptTwoFrames, but is ignored since that method checks for FrameEmpty frames. - The provided frame is FrameComplete or FramePartial, and the disposal method is DisposeOverwritePrevious. In the old implementation this results in |clearExceptFrame| being set to the required previous frame in the first block, and |clearExceptFrame2| is set to the provided frame. The second block searches for the nearest non-empty frame, starting at |clearExceptFrame|. The new implementation stores the required previous frame in |clearExceptFrame2| because of the disposal method, and similarly searches for the nearest non-empty frame in the second block. - The provided frame is FrameComplete or FramePartial, and the disposal method is *not* DisposeOverwritePrevious. In this case, the old implementation only preserves the provided frame since the first block is a no-op due to the disposal condition, and the second block is a no-op as well since the frames' status is not FrameEmpty. The new implementation does not set |clearExceptFrame2| because of the disposal condition in the first block, and thus only preserves the provided frame. Committed: https://crrev.com/e0df7a9b0f7bab12de923b1f58d047e0ff4f4de4 Cr-Commit-Position: refs/heads/master@{#435636} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e0df7a9b0f7bab12de923b1f58d047e0ff4f4de4 Cr-Commit-Position: refs/heads/master@{#435636} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
