Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(216)

Issue 2887123003: Fix required frame bug in APNGs (Closed)

Created:
3 years, 7 months ago by scroggo_chromium
Modified:
3 years, 7 months ago
Reviewers:
Peter Kasting
CC:
chromium-reviews, blink-reviews, kinuko+watch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix required frame bug in APNGs If frame |i|'s disposal method is kDisposeOverwritePrevious, frame |i+1| does not necessarily share |i|'s required frame. If |i| is independent (e.g. by filling the screen and being opaque or using kBlendAtopBgcolor), |i+1| may still depend on |i-1|. When looking for the required frame, skip over frames marked kDisposeOverwritePrevious. This fixes a bug where frames are drawn on top of transparent instead of the appropriate prior frame. Add a LayoutTest that exercises the problem. BUG=722072 Review-Url: https://codereview.chromium.org/2887123003 Cr-Commit-Position: refs/heads/master@{#472897} Committed: https://chromium.googlesource.com/chromium/src/+/55b2599c12a1f482bf13f07f855f6caf643af390

Patch Set 1 #

Patch Set 2 : Add a LayoutTest #

Total comments: 2

Patch Set 3 : Move comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -6 lines) Patch
A third_party/WebKit/LayoutTests/images/dispose-previous.html View 1 1 chunk +17 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/images/dispose-previous-expected.html View 1 1 chunk +1 line, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/crbug722072.png View 1 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/green.png View 1 Binary file 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp View 1 2 3 chunks +13 lines, -6 lines 0 comments Download

Messages

Total messages: 16 (9 generated)
scroggo_chromium
3 years, 7 months ago (2017-05-17 20:16:22 UTC) #3
Peter Kasting
Nice subtlety; it took me a couple read-throughs to truly understand why the new code ...
3 years, 7 months ago (2017-05-17 23:30:54 UTC) #4
scroggo_chromium
https://codereview.chromium.org/2887123003/diff/20001/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2887123003/diff/20001/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp#newcode447 third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:447: // Frames that use the DisposeOverwritePrevious method are effectively ...
3 years, 7 months ago (2017-05-18 12:24:06 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2887123003/40001
3 years, 7 months ago (2017-05-18 12:24:27 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/430456)
3 years, 7 months ago (2017-05-18 14:39:19 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2887123003/40001
3 years, 7 months ago (2017-05-18 15:59:28 UTC) #13
commit-bot: I haz the power
3 years, 7 months ago (2017-05-18 19:19:43 UTC) #16
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/55b2599c12a1f482bf13f07f855f...

Powered by Google App Engine
This is Rietveld 408576698