clear all pixels when dispose is 3 but previousImage is null #2521
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
There was a problem hiding this comment.
thanks for the fix!
Please run gradlew check locally, there seems to be some violation; probably a long line.
I also suggest you run this example with the new code to see if there's a regression:
https://github.com/TWiStErRob/glide-support/blob/master/src/glide3/java/com/bumptech/glide/supportapp/github/_1094_gif_disposal/TestFragment.java
| @@ -418,8 +418,9 @@ private Bitmap setPixels(GifFrame currentFrame, GifFrame previousFrame) { | |||
| // Final location of blended pixels. | |||
| final int[] dest = mainScratch; | |||
|
|
|||
| // clear all pixels when meet first frame | |||
| if (previousFrame == null) { | |||
| // clear all pixels when meet first frame or dispose is 3 but previousImage is null | |||
There was a problem hiding this comment.
"dispose is 3 but previousImage is null" part doesn't explain much, it's exactly the same as the code below it, you should probably explain what it means, or why we need to drop the prev image/fill to black.
There was a problem hiding this comment.
OK. I think conditions should be separate. We need to drop the prev image when meet first frame like we do clear all pixels. If prev image exist in second frame, prev image which saved in prev loop will be fill back in new loop and cause overdraw.
|
:third_party:gif_decoder:checkstyle[ant:checkstyle] /home/travis/build/bumptech/glide/third_party/gif_decoder/src/main/java/com/bumptech/glide/gifdecoder/StandardGifDecoder.java:422: error: Line is longer than 100 characters (found 105). |
|
Thank you for your reply. |
|
The two travis failures are setup related and can be ignored. |
| if (previousFrame == null) { | ||
| previousImage = null; |
There was a problem hiding this comment.
Here you probably should be releasing previousImage so the Bitmap can be re-used, see the four lines in clear():
|
LGTM. Thank you for putting this up and your patience. I really appreciate the help! |
…h#2521) * clear all pixels when dispose is 3 but previousImage is null * Separate conditions and explain why we need to drop the prev image * release previousImage for re-use
------------- Avoid blocking Futures forever on unexpected load failures. ------------- Add a system for emulator based regression testing. The tests can both generate canonical output specific to one or more Android SDKs and compare the current output to previously generated images to detect regressions. Ideally we can add top level tests for most Glide functionality using these tests to detect regression across the library without a ton of overhead either when new tests are added or when functionality is intentionally. ------------- Add an explicit 2m timeout to Firebase sample tests. ------------- Run emulator tests on all available Android versions on Firebase. ------------- Run instrumentation tests on Travis always. This should help us keep track of flakes so they don’t only appear when someone makes a pull request. ------------- Remove Android API 20 emulator from travis. It’s unreliable and basically just KitKat anyway. ------------- Update usage script for split_by_sdk. ------------- Add more transformation regression tests. ------------- Avoid clearing Bitmaps in use by paused GifDrawables. Fixes bumptech#2526. ------------- clear all pixels when dispose is 3 but previousImage is null (bumptech#2521) * clear all pixels when dispose is 3 but previousImage is null * Separate conditions and explain why we need to drop the prev image * release previousImage for re-use ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=e86fd41e16aac1b95884494c1097417b4ab15a5a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=173974844
Description
Fix Issues #2271 and part of #1094
Motivation and Context
Dispose in frame should not be DISPOSAL_PREVIOUS when there did not save previous image. GIF like #2271 is wrong because the dispose is DISPOSAL_PREVIOUS in first frame.