Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1574283002:
GIF Animations "clear from cache related glitch" fix (Closed)
|
Created:
4 years, 11 months ago by aleksandar.stojiljkovic Modified:
4 years, 11 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, danakj, Rik, f(malita), blink-reviews, vmpstr+blinkwatch_chromium.org, kinuko+watch, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGIF Animations "clear from cache related glitch" fix
Fix for two separate situations where dependent frames are removed from cache
resulting with decoding frames from 0..N-1 when decoding frame N.
User experience in this case often suffered from ~500ms glitches in animation
rendering or skipped frames.
Measurements are available in Issue 507273 comments.
BUG=481250, 507273
Committed: https://crrev.com/0a79aa1c58370ca041c85aa5781a74dd5279d634
Cr-Commit-Position: refs/heads/master@{#369950}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Review comment #5 fix #Patch Set 3 : @pkasting, great that you figured out this - yes, DisposeOverwritePrevious fixes this. #
Total comments: 10
Patch Set 4 : Handling #17 #Patch Set 5 : Nit: blank line before the comment. #Patch Set 6 : more correct way to write it. #
Total comments: 1
Patch Set 7 : Split graphics patch. using @pkasting's algorithm #
Total comments: 4
Patch Set 8 : nits. @pkasting thanks for comments. #
Messages
Total messages: 29 (8 generated)
Description was changed from ========== GIF Animations "clear from cache related glitch" fix Fix for two separate situations where dependent frames are removed from cache resulting with decoding frames from 0..N-1 when decoding frame N. User experience in this case often suffered from ~500ms glitches in animation rendering or skipped frames. BUG=481250, 507273 ========== to ========== GIF Animations "clear from cache related glitch" fix Fix for two separate situations where dependent frames are removed from cache resulting with decoding frames from 0..N-1 when decoding frame N. User experience in this case often suffered from ~500ms glitches in animation rendering or skipped frames. Measurements are available in Issue 507273 comments. BUG=481250, 507273 ==========
tomhudson@google.com changed reviewers: + tomhudson@google.com
https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:199: // Don't clear if there are no frames or only one frame. This comment is backwards. It's nearly obvious from the following two lines of code that you aren't clearing if there are 0 or 1 frames, but it's completely inobvious *WHY*. https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:225: // 2. We don't clear any frame from which a two* future initFrameBuffer() calls Nit: 'which a two' -> 'which two', although this could use a broader rewriting https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:242: return ImageDecoder::clearCacheExceptFrame(clearExceptFrames.size() ? *(clearExceptFrames.begin()) : kNotFound); These three lines are significantly more complicated to understand than just calling return clearCacheExceptFrame(clearExceptFrames). Is there really that huge a performance difference? Or does this point to the "if (m_frameBufferCache.size() <= 1) return 0;" clause possibly being a poor design?
https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:183: // Don't clear if there are no frames or only one frame. On 2016/01/12 16:15:59, tomhudson wrote: > This comment is backwards. It's nearly obvious from the following two lines of > code that you aren't clearing if there are 0 or 1 frames, but it's completely > inobvious *WHY*. Copied this code including the comment from here. Agree about comment. Code looks like a patch disabling cache clearing for single frame images from DeferredImageDecoder: void BitmapImage::destroyDecodedDataIfNecessary() { // Animated images >5MB are considered large enough that we'll only hang on // to one frame at a time. static const size_t cLargeAnimationCutoff = 5242880; .... destroyDecodedData(false); destroyDecodedData(true) calls clearCacheExceptFrame(kNotFound) that would remove all frames but is not used any more (there is only call to destroyDecodedData(false)). In other words, there are calls to clearCacheExceptFrame(kNotFound) only in webkit tests. I'll remove the comment from my change in this patch. https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:199: // Don't clear if there are no frames or only one frame. On 2016/01/12 16:15:59, tomhudson wrote: > This comment is backwards. It's nearly obvious from the following two lines of > code that you aren't clearing if there are 0 or 1 frames, but it's completely > inobvious *WHY*. Done. https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:225: // 2. We don't clear any frame from which a two* future initFrameBuffer() calls On 2016/01/12 16:15:59, tomhudson wrote: > Nit: 'which a two' -> 'which two', although this could use a broader rewriting Done. https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:242: return ImageDecoder::clearCacheExceptFrame(clearExceptFrames.size() ? *(clearExceptFrames.begin()) : kNotFound); On 2016/01/12 16:15:59, tomhudson wrote: > These three lines are significantly more complicated to understand than just > calling > return clearCacheExceptFrame(clearExceptFrames). > > Is there really that huge a performance difference? Or does this point to the > "if (m_frameBufferCache.size() <= 1) return 0;" clause possibly being a poor > design? Acknowledged.
https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:229: // on the previous or the frame before previous. As an extreme example, what if all frames in a GIF use RESTORE_TO_PREVIOUS disposal method? In that case, any frame in GIF would depend on the 1st frame due to the dependency chain (and not on the previous two frames). What I'm trying to say is that, the number 'two' seems kind of arbitrary. In future, if you see another GIF example that depends on 3rd frame before that, would you increase this number to 3 then?
https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:229: // on the previous or the frame before previous. On 2016/01/12 18:02:01, urvang wrote: > As an extreme example, what if all frames in a GIF use RESTORE_TO_PREVIOUS > disposal method? In that case, any frame in GIF would depend on the 1st frame > due to the dependency chain (and not on the previous two frames). > Theoretically, you're right. While exploring GIF related issues reported and checking various GIFs details, I didn't see this case (except in our unit tests). > What I'm trying to say is that, the number 'two' seems kind of arbitrary. In > future, if you see another GIF example that depends on 3rd frame before that, > would you increase this number to 3 then? Depends on frequency and severity. In performance issues related to GIF animations and those I checked while doing GIF Index8 decoding, dependency to frame-1 or frame-2 is much more common. Case of dependency to frame-3 this patch doesn't change - performance after this patch is the same as it was.
Patch Set 2 : Review comment #5 fix https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:242: return ImageDecoder::clearCacheExceptFrame(clearExceptFrames.size() ? *(clearExceptFrames.begin()) : kNotFound); On 2016/01/12 16:15:59, tomhudson wrote: > These three lines are significantly more complicated to understand than just > calling > return clearCacheExceptFrame(clearExceptFrames). > > Is there really that huge a performance difference? Or does this point to the > "if (m_frameBufferCache.size() <= 1) return 0;" clause possibly being a poor > design? Fixed this in new patch. Thanks for pointing - measurements on K3 Note show that clearCacheExceptFrame (existing code and clearCacheExceptFrames for one item in hashset) takes ~0.3ms in average for 100 frames animation (screen size 512*384). Need to check it further and submit a bug.
On 2016/01/12 18:02:01, urvang wrote: > https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp > (right): > > https://codereview.chromium.org/1574283002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:229: > // on the previous or the frame before previous. > As an extreme example, what if all frames in a GIF use RESTORE_TO_PREVIOUS > disposal method? In that case, any frame in GIF would depend on the 1st frame > due to the dependency chain (and not on the previous two frames). > > What I'm trying to say is that, the number 'two' seems kind of arbitrary. In > future, if you see another GIF example that depends on 3rd frame before that, > would you increase this number to 3 then? With current aggressive cleanup, I could do an opposite approach - to keep up to N frames in cache that are most likely to be used in future. Then again, N is also about heuristics and I would start with 2 for gif animations frame dependency. If you have other idea, please advise. Most importantly, user experience improvement is great so I feel motivated to spend some more time in finding better way to do this.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
I don't think this is the right way to do this. This basically makes the clearing code defy its caller by preserving more frames than the caller asked for. The correct fix IMO is for deferred decoding to not unconditionally clear all previous frames every time it decodes. In other words, we should fix this by asking to clear less, not by clearing less than we're asked.
On 2016/01/12 20:31:44, Peter Kasting wrote:
> I don't think this is the right way to do this. This basically makes the
> clearing code defy its caller by preserving more frames than the caller asked
> for. The correct fix IMO is for deferred decoding to not unconditionally
clear
> all previous frames every time it decodes. In other words, we should fix this
> by asking to clear less, not by clearing less than we're asked.
Thanks.
ImageFrameGenerator is doing it [1]
as well as DeferredImageDecoder based on 5242880 bytes heuristics [2]
GIF and WebP decoders are already not doing what asked [3] (in a way since they
keep other frames not the one asked) as they override
clearCacheExceptFrame(size_t clearExceptFrame).
So I was thinking if it is better to do non intrusive fix (this patch) or
replace it with something like clearCacheExceptRequiredFrames(size_t
clearExceptFrame, size_t maximumRequiredFramesToKeep). Let me try the other way.
[1]
bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap*
bitmap) {
...
(*decoder)->clearCacheExceptFrame(index);
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[2]
void BitmapImage::destroyDecodedDataIfNecessary()
{
// Animated images >5MB are considered large enough that we'll only hang on
// to one frame at a time.
static const size_t cLargeAnimationCutoff = 5242880;
size_t allFrameBytes = 0;
for (size_t i = 0; i < m_frames.size(); ++i)
allFrameBytes += m_frames[i].m_frameBytes;
if (allFrameBytes > cLargeAnimationCutoff) {
destroyDecodedData(false);
}
}
[3]
size_t WEBPImageDecoder::clearCacheExceptFrame(size_t clearExceptFrame)
{
// If |clearExceptFrame| has status FrameComplete, we preserve that frame.
// Otherwise, we preserve a previous frame with status FrameComplete whose
data is required
// to decode |clearExceptFrame|, either in initFrameBuffer() or
ApplyPostProcessing().
// All other frames can be cleared.
while ((clearExceptFrame < m_frameBufferCache.size()) &&
(m_frameBufferCache[clearExceptFrame].status() != ImageFrame::FrameComplete))
clearExceptFrame =
m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex();
return ImageDecoder::clearCacheExceptFrame(clearExceptFrame);
}
On 2016/01/12 20:56:38, aleksandar_stojiljkovic wrote:
> On 2016/01/12 20:31:44, Peter Kasting wrote:
> > I don't think this is the right way to do this. This basically makes the
> > clearing code defy its caller by preserving more frames than the caller
asked
> > for. The correct fix IMO is for deferred decoding to not unconditionally
> clear
> > all previous frames every time it decodes. In other words, we should fix
this
> > by asking to clear less, not by clearing less than we're asked.
>
> Thanks.
> ImageFrameGenerator is doing it [1]
> as well as DeferredImageDecoder based on 5242880 bytes heuristics [2]
The issue I'm referring to in bug 481250 is [1]. See my description in bug
481250 comment 0 for why this is a problem. Basically, the ImageFrameGenerator
is trying to be stateless, but stateless GIF decoding is A Bad Thing. It's not
clear to me who should really own the underlying GIF decoder and how and when
its state should actually be cleared. Fixing this is likely going to require
re-architecting the way drawing happens somehow and will not be easy.
However, on reading more, I'm not convinced you're trying to fix this bug at
all. I think you're working on a different bug where, if we have frames set to
DisposeOverwritePrevious, we sometimes won't preserve the right frames. For
example, you cite an image where frame 92 depends on frame 90, presumably
because frame 91 is set to DisposeOverwritePrevious. In this case, I think the
existing code in GIFImageDecoder::clearCahceExceptFrame() is indeed wrong, but I
don't think your fix is correct.
It seems like what we really want to do is:
* If clearExceptFrame's status is FrameEmpty, clear everything but the frame's
requiredPreviousFrameIndex() (or, if that's also FrameEmpty, search for _its_
required previous frame, and so on until we find a nonempty frame or run out of
frames).
* Otherwise, if clearExceptFrame is marked as DisposeOverwritePrevious, then
clear everything but this frame AND its requiredPreviousFrameIndex() (using the
same algorithm as above).
* Otherwise, clear everything but this frame.
The existing code does the first and third bullets; what's missing is the middle
one. We could implement this with something like:
size_t frame1 = kNotFound, frame2 = kNotFound;
if ((clearExceptFrame < m_frameBufferCache.size()) {
ImageFrame* const frame = &m_frameBufferCache[clearExceptFrame];
if ((frame->status() != ImageFrame::FrameEmpty) &&
(frame->disposalMethod() == ImageFrame::DisposeOverwritePrevious)) {
frame2 = clearExceptFrame;
clearExceptFrame = frame->requiredPreviousFrameIndex();
}
}
while ((clearExceptFrame < m_frameBufferCache.size()) &&
(m_frameBufferCache[clearExceptFrame].status() == ImageFrame::FrameEmpty))
clearExceptFrame =
m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex();
if (clearExceptFrame < m_frameBufferCache.size())
frame1 = clearExceptFrame;
return ImageDecoder::clearCacheExceptFrames(frame1, frame2);
This would require adding a 2-arg clearCacheExceptFrames() method. We don't
need to be more general and handle n frames, nor do we need to do something like
looping through 2 frames looking, because given a frame in a GIF, all future
frames will either depend on this frame, or on this frame's required previous
frame.
Also, if you fix this, I suspect that my complaint in bug 481250 may not be as critical as I thought there -- I thought we'd always clear everything, but if we're at least preserving the frames we need to continue decoding forward, then the only real issue compared to the pre-deferred code is that small GIFs (under 5 MB decoded) will never get fully cached, so as long as we're looping them, we'll have to continually re-decode them, as we do for large GIFs. This could be improved, but it's much less bad than "we always force GIFs to redecode all frames whenever any frame relies on a previous one".
On 2016/01/12 21:41:56, Peter Kasting wrote: > Also, if you fix this, I suspect that my complaint in bug 481250 may not be as > critical as I thought there -- I thought we'd always clear everything, but if > we're at least preserving the frames we need to continue decoding forward, then > the only real issue compared to the pre-deferred code is that small GIFs (under > 5 MB decoded) will never get fully cached, so as long as we're looping them, > we'll have to continually re-decode them, as we do for large GIFs. This could > be improved, but it's much less bad than "we always force GIFs to redecode all > frames whenever any frame relies on a previous one". @pkasting, you were right - it is about dispose overwrite previous and that makes the patch much simpler. Submitted new patch for review. >I suspect that my complaint in bug 481250 The path covered here; skia->decoder is explained on this diagram - raster path on [1]. Any comments on the design document are more than welcome. Thanks. Let me please analyze deeper if there is possibility to do some cleanup in DeferredImageDecoder (related to m_actualDecoder). >so as long as we're looping them, > we'll have to continually re-decode them Discardable cache in skia is supposed to handle this but happens that it is not large enough to keep frames not evicting each other from cache while looping. However, with Index8 decoding I saw the cache working good in some cases and keeping all of the frames (decoding only during first loop). [1] https://docs.google.com/document/d/1oWeD8IAqbFhBhu9DjYa3UUXRRbXNypf1S_vnDG7VG...
https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:297: removeDecoder = m_frameCount && (decodedFrameCount == m_frameCount) && (index == m_frameCount - 1); Why is this additional check needed? If all frames are complete, it shouldn't matter what the current frame index is. https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:232: return clearCacheExceptTwoFrames(clearExceptFrame, m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex()); I don't think this is as correct as the algorithm I proposed. Let's say we have a GIF with four frames: 0: Nonempty, DisposeNotSpecified 1: Empty, DisposeNotSpecified 2: Nonempty, DisposeOverwritePrevious 3: Empty, DisposeNotSpecified Now we call this method with clearExceptFrame == 2. With your code, the while loop terminates immediately because frame 2 is nonempty. The conditional then fires, and we calculate frame 2's required previous frame as frame 1. So we end up asking to save frames 1 and 2. Frame 0 is cleared (as is frame 3, which doesn't have any effect). With my code, the initial conditional fires and we set frame2 = 2. Then we set clearExceptFrame = 1 and run the while loop. This traverses back to frame 0, and we end up asking to save frames 0 and 2. Frames 1 and 3 are cleared, which have no effect. Now the decoder attempts to decode frame 3. Frame 3's required previous frame is frame 1, which is empty, so we have to look at its required previous frame, which is frame 0. In my code, frame 0 has been preserved, so all we need do is decode frame 1, and we're ready to decode frame 3. In your code, we need to decode frame 0, then frame 1, then frame 3. The key here is that we want to do the "traverse past empty frames" after the DisposeOverwritePrevious check, not before. https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h (right): https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:83: // If disposal method is DisposeOverwritePrevious, keep two frames: current Nit: Blank line above this https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:84: // and it's required previous frame. This comment is technically a bit of a layering violation, because this describes how callers are using this rather than how the function actually works. I would do this: // Like clearCacheExceptFrame(), but preserves two frames instead of one. size_t clearCacheExceptFrames(size_t frame1, size_t frame2);
https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:297: removeDecoder = m_frameCount && (decodedFrameCount == m_frameCount) && (index == m_frameCount - 1); On 2016/01/14 00:17:29, Peter Kasting wrote: > Why is this additional check needed? If all frames are complete, it shouldn't > matter what the current frame index is. This is about different issue (unrelated to DisposeOverwritePrevious), described here [1]. It is incorrect to clear all frames (which happens if decoder is removed) in the middle of animation loop as the next frame would need to decode all the way from 0 frame. i.e. decoded frames for 7 frame animation: sequence: 0,1,2,3,4,<navigate away from gif so that there is no ImageFrameGenerator to decode frame5>,6,0,1,2,3,4,5 <now decoder gets removed and all the frames>,6<decoding 6 would require decoding all the frames 0-5>. [1] https://code.google.com/p/chromium/issues/detail?id=507273#c7 https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:232: return clearCacheExceptTwoFrames(clearExceptFrame, m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex()); On 2016/01/14 00:17:29, Peter Kasting wrote: > I don't think this is as correct as the algorithm I proposed. > > Let's say we have a GIF with four frames: > > 0: Nonempty, DisposeNotSpecified > 1: Empty, DisposeNotSpecified > 2: Nonempty, DisposeOverwritePrevious > 3: Empty, DisposeNotSpecified > > Now we call this method with clearExceptFrame == 2. > > With your code, the while loop terminates immediately because frame 2 is > nonempty. The conditional then fires, and we calculate frame 2's required > previous frame as frame 1. So we end up asking to save frames 1 and 2. Frame 0 > is cleared (as is frame 3, which doesn't have any effect). > > With my code, the initial conditional fires and we set frame2 = 2. Then we set > clearExceptFrame = 1 and run the while loop. This traverses back to frame 0, > and we end up asking to save frames 0 and 2. Frames 1 and 3 are cleared, which > have no effect. > > Now the decoder attempts to decode frame 3. Frame 3's required previous frame > is frame 1, which is empty, so we have to look at its required previous frame, > which is frame 0. > > In my code, frame 0 has been preserved, so all we need do is decode frame 1, and > we're ready to decode frame 3. In your code, we need to decode frame 0, then > frame 1, then frame 3. > > The key here is that we want to do the "traverse past empty frames" after the > DisposeOverwritePrevious check, not before. I should have explained for not doing as you suggested. Let's follow your example. Dependencies: 0<-1<-2 1<-3 The case above cannot happen with current ImageFrameGenerator::decode - it is always calling clear all except currently decoded frames. Fair enough, need to cover potential future changes for the sake of algorithm correctness. However, the case that needs to be handled is: 0: Empty, DisposeNotSpecified 1: Nonempty, DisposeNotSpecified 2: Nonempty, DisposeOverwritePrevious 3: Empty, DisposeNotSpecified and then call to clearexceptframe == 3. This could happen with partial decoding (when all data is not yet received) - that is why existing code was traversing from current frame back to first available nonempty (in this case 2) and keeping it. The change I proposed was handling this case. To handle the case you specified, I have now introduced another loop after reaching 2 - for DisposeOverwritePrevious, find the closest previous required frame that is not empty (0 in your example). https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h (right): https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:83: // If disposal method is DisposeOverwritePrevious, keep two frames: current On 2016/01/14 00:17:29, Peter Kasting wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:84: // and it's required previous frame. On 2016/01/14 00:17:29, Peter Kasting wrote: > This comment is technically a bit of a layering violation, because this > describes how callers are using this rather than how the function actually > works. I would do this: > > // Like clearCacheExceptFrame(), but preserves two frames instead of one. > size_t clearCacheExceptFrames(size_t frame1, size_t frame2); Done.
https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:297: removeDecoder = m_frameCount && (decodedFrameCount == m_frameCount) && (index == m_frameCount - 1); I didn't write any of this code, so I'm having a hard time properly reviewing it. Note that I'm not in the OWNERS list for this directory. I suggest you split this change out and get it reviewed separately. When you do, you need to update the comments here to reflect what you're actually doing and why. And the explanation you gave me isn't really clear enough; in particular, I'm not clear on precisely what objects are being cleared where and who holds copies of SkBitmaps or other decoded image data. While deleting a decoder and then asking to decode frame <n> would indeed be problematic, it looks to me as if the code here assumes that once all frames are complete, someone will have cached all the decoded frames and we won't ever need to decode again. If that's not actually happening, I wonder if deleting the decoder here is the right thing to do at all. https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:232: return clearCacheExceptTwoFrames(clearExceptFrame, m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex()); On 2016/01/14 11:32:53, aleksandar.stojiljkovic wrote: > However, the case that needs to be handled is: > > 0: Empty, DisposeNotSpecified > 1: Nonempty, DisposeNotSpecified > 2: Nonempty, DisposeOverwritePrevious > 3: Empty, DisposeNotSpecified > > and then call to clearexceptframe == 3. > This could happen with partial decoding (when all data is not yet received) - > that is why existing code was traversing from current frame back to first > available nonempty (in this case 2) and keeping it. > > The change I proposed was handling this case. > To handle the case you specified, I have now introduced another loop after > reaching 2 - for DisposeOverwritePrevious, find the closest previous required > frame that is not empty (0 in your example). I think you're mistaken about how both the existing code and my proposal handle this. In the case you describe, the requiredPreviousFrameIndex() for frame 3 will be frame 1 -- not frame 2. The existing code (without either of our changes) already handles this case correctly: the initial loop sees that frame 3 is empty, traverses directly to frame 1, sees that that is nonempty, and clears everything but that. The key here is that the loop goes back using requiredPreviousFrameIndex() rather than just by subtracting one from |clearExceptFrame|. The case that needs to be handled better is the case where we're asked to preserve frame _2_ in the example you give above, where the existing code will preserve only frame 2, but we need to preserve both 1 and 2. Both my proposal and yours achieve this goal, but mine is simpler. (To think about this another way, note that in the newest code you propose, either the while loop must terminate immediately, or the following conditional can never succeed, because no frame marked DisposeOverwritePrevious can ever be the requiredPreviousFrameIndex() of a subsequent frame; see ImageDecoder::findRequiredPreviousFrame().) https://codereview.chromium.org/1574283002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h (right): https://codereview.chromium.org/1574283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h:85: size_t clearCacheExceptTwoFrames(size_t dontClearFrame1, size_t dontClearFrame2); Nit: For maximum parallelism with clearCacheExceptFrame, name this size_t clearCacheExceptFrames(size_t clearExceptFrame1, size_t clearExceptFrame2);
On 2016/01/14 22:58:39, Peter Kasting wrote: > https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp > (right): > > https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:297: > removeDecoder = m_frameCount && (decodedFrameCount == m_frameCount) && (index == > m_frameCount - 1); > I didn't write any of this code, so I'm having a hard time properly reviewing > it. Note that I'm not in the OWNERS list for this directory. I suggest you > split this change out and get it reviewed separately. Done. > > When you do, you need to update the comments here to reflect what you're > actually doing and why. And the explanation you gave me isn't really clear > enough; in particular, I'm not clear on precisely what objects are being cleared > where and who holds copies of SkBitmaps or other decoded image data. While > deleting a decoder and then asking to decode frame <n> would indeed be > problematic, it looks to me as if the code here assumes that once all frames are > complete, someone will have cached all the decoded frames and we won't ever need > to decode again. If that's not actually happening, I wonder if deleting the > decoder here is the right thing to do at all. > > https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp > (right): > > https://codereview.chromium.org/1574283002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:232: > return clearCacheExceptTwoFrames(clearExceptFrame, > m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex()); > On 2016/01/14 11:32:53, aleksandar.stojiljkovic wrote: > > However, the case that needs to be handled is: > > > > 0: Empty, DisposeNotSpecified > > 1: Nonempty, DisposeNotSpecified > > 2: Nonempty, DisposeOverwritePrevious > > 3: Empty, DisposeNotSpecified > > > > and then call to clearexceptframe == 3. > > This could happen with partial decoding (when all data is not yet received) - > > that is why existing code was traversing from current frame back to first > > available nonempty (in this case 2) and keeping it. > > > > The change I proposed was handling this case. > > To handle the case you specified, I have now introduced another loop after > > reaching 2 - for DisposeOverwritePrevious, find the closest previous required > > frame that is not empty (0 in your example). > > I think you're mistaken about how both the existing code and my proposal handle > this. Using your algorithm - yes, it is simpler and does the work. I was trying to handle the case where there would be frame 4 -> frame 2 (example above) and then clear frame 4 - yes, it is clear to me that frame 3 -> frame 1. In all the effort spent to find such example (3->1 && 4->2 dependency in the same time) I didn't find it so no point in doing second loop like I wrote. > Nit: For maximum parallelism with clearCacheExceptFrame, name this > > size_t clearCacheExceptFrames(size_t clearExceptFrame1, size_t > clearExceptFrame2); Done.
LGTM https://codereview.chromium.org/1574283002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:227: // All other frames can be cleared. Nit: Change this comment as follows, then follow my suggestion below. // We expect that after this call, we'll be asked to decode frames after // this one. So we want to avoid clearing frames such that those requests // would force re-decoding from the beginning of the image. // // When |clearExceptFrame| is e.g. DisposeKeep, simply not clearing that // frame is sufficient, as the next frame will be based on it, and in // general future frames can't be based on anything previous. // // However, if this frame is DisposeOverwritePrevious, then subsequent // frames will depend on this frame's required previous frame. In this // case, we need to preserve both this frame and that one. https://codereview.chromium.org/1574283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:236: while ((clearExceptFrame < m_frameBufferCache.size()) && (m_frameBufferCache[clearExceptFrame].status() == ImageFrame::FrameEmpty)) Nit: Now add a blank line above this and then this comment: // Now |clearExceptFrame| indicates the frame that future frames will // depend on. But if decoding is skipping forward past intermediate frames, // this frame may be FrameEmpty. So we need to keep traversing back through // the required previous frames until we find the nearest non-empty // ancestor. Preserving that will minimize the amount of future decoding // needed.
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1574283002/#ps140001 (title: "nits. @pkasting thanks for comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574283002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574283002/140001
https://codereview.chromium.org/1574283002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp (right): https://codereview.chromium.org/1574283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:227: // All other frames can be cleared. On 2016/01/15 21:57:57, Peter Kasting wrote: > Nit: Change this comment as follows, then follow my suggestion below. > > // We expect that after this call, we'll be asked to decode frames after > // this one. So we want to avoid clearing frames such that those requests > // would force re-decoding from the beginning of the image. > // > // When |clearExceptFrame| is e.g. DisposeKeep, simply not clearing that > // frame is sufficient, as the next frame will be based on it, and in > // general future frames can't be based on anything previous. > // > // However, if this frame is DisposeOverwritePrevious, then subsequent > // frames will depend on this frame's required previous frame. In this > // case, we need to preserve both this frame and that one. Done. https://codereview.chromium.org/1574283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp:236: while ((clearExceptFrame < m_frameBufferCache.size()) && (m_frameBufferCache[clearExceptFrame].status() == ImageFrame::FrameEmpty)) On 2016/01/15 21:57:57, Peter Kasting wrote: > Nit: Now add a blank line above this and then this comment: > > // Now |clearExceptFrame| indicates the frame that future frames will > // depend on. But if decoding is skipping forward past intermediate frames, > // this frame may be FrameEmpty. So we need to keep traversing back through > // the required previous frames until we find the nearest non-empty > // ancestor. Preserving that will minimize the amount of future decoding > // needed. Done.
Message was sent while issue was closed.
Description was changed from ========== GIF Animations "clear from cache related glitch" fix Fix for two separate situations where dependent frames are removed from cache resulting with decoding frames from 0..N-1 when decoding frame N. User experience in this case often suffered from ~500ms glitches in animation rendering or skipped frames. Measurements are available in Issue 507273 comments. BUG=481250, 507273 ========== to ========== GIF Animations "clear from cache related glitch" fix Fix for two separate situations where dependent frames are removed from cache resulting with decoding frames from 0..N-1 when decoding frame N. User experience in this case often suffered from ~500ms glitches in animation rendering or skipped frames. Measurements are available in Issue 507273 comments. BUG=481250, 507273 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== GIF Animations "clear from cache related glitch" fix Fix for two separate situations where dependent frames are removed from cache resulting with decoding frames from 0..N-1 when decoding frame N. User experience in this case often suffered from ~500ms glitches in animation rendering or skipped frames. Measurements are available in Issue 507273 comments. BUG=481250, 507273 ========== to ========== GIF Animations "clear from cache related glitch" fix Fix for two separate situations where dependent frames are removed from cache resulting with decoding frames from 0..N-1 when decoding frame N. User experience in this case often suffered from ~500ms glitches in animation rendering or skipped frames. Measurements are available in Issue 507273 comments. BUG=481250, 507273 Committed: https://crrev.com/0a79aa1c58370ca041c85aa5781a74dd5279d634 Cr-Commit-Position: refs/heads/master@{#369950} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0a79aa1c58370ca041c85aa5781a74dd5279d634 Cr-Commit-Position: refs/heads/master@{#369950} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
