Delayed unsubscribe timing to unsubscribe after displaying the last i…

[matsuhiro]

…mage

Description

I want to stop/start animation gif.
I delay unsubscribe timing to fix an animation gif bug.

Motivation and Context

Current code of master branch, when you call stop() method of Animation interface, callback will be unsubscribed soon.
Then, in background thread, new bitmap for next frame is prepared and previous bitmap is recycled.
But, callback is already unsubscribed, so new bitmap will not be applied and previous bitmap will be used to draw other gif animation frame.
As a result, it seems that a image which is stopped animation is changed by other animation.

About reproduce:
More than two GIF animation are played, and when you stop animation of one file by using stop() method, you will find a bug.
The bug is that animating gif file will change image of not animating gif image.
You can reproduce it with this repo (https://github.com/matsuhiro/glide/tree/reproduce_bug).
(You can reproduce it by applying this patch matsuhiro@9359a7f)

1. Launch sample of giphy
2. Click some animating gif image.
3. You will find other image change the image which is click at step 2.

For reproducing, it is easy to understand that images of the same size are being played at the same time.

[googlebot]

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!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
• In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

[matsuhiro]

I signed it!

[sjudd]

What if subscribe is never called again?

[matsuhiro]

This is just fail safe.
Normally, unsubscribeRegisteredCallback() is called inside onFrameReady() and then callbacks will be unsubscribed.
When an user click image repeat quickly, subscribe() may be called before onFrameReady() is called.

[sjudd]

Thanks for putting this up, I really appreciate the detail and the repro case. This is definitely a complicated problem. I'm sorry for the delay, it took me a bit to wrap my head around this.

I think this solution isn't quite complete though. Let's say you have the same GIF animating in two views at the same time. Then, you stop both GifDrawables, one at a time, allowing exactly one additional frame to render before stopping the next GifDrawable.

When the first GifDrawable is stopped, after your change, it will end on frame N+1. Frame N will be cleared when frame N+1 loads.

When the second GifDrawable is stopped, one frame later, it will end on frame N+2. But, frame N+1, still in use by the first GifDrawable, will be cleared when frame N+2 loads.

We really should be assigning ownership of the frame to the stopped GifDrawable. The stopped GifDrawable should make a request to load the frame it currently has in view when stop() is called, which will ensure that the frame is not garbaged collected or re-used, even if the Target owned by the GifFrameLoader is cleared. When the stopped GifDrawable resumes or is destroyed, it can clear the Target it obtained in stop() and release the resource.

One way to do this might be to have stop() in the GifFrameLoader run the load and return a Target, then allow start() in GifFrameLoader to accept a @Nullable Target that could be cleared if the GifDrawable were resumed. I haven't tried that and you may also be able to come up with a better way to do even if that would work.

[sjudd]

Targeting this for 4.2 since I'm already a day late for 4.1.

[matsuhiro]

Thank you for your reply and your detailed opinion
I will try to improve my pull-request.

[googlebot]

CLAs look good, thanks!

[sjudd]

I believe this was fixed in #2526.