New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper support for GIFs when using a RetainingDataSource #2185

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Crysis21
Contributor

Crysis21 commented Aug 16, 2018

Motivation (required)

During the development of the Giphy app we choose to use fresco as our main image library. The application loads different types of image renditions, starting from low-quality images to high-quality ones. We wanted to be able to change the quality of a GIF displayed in a DraweeView.

When making a simple request to change the resource of a DraweeView, fresco will clear the current content, display the placeholder, then load the new resource. Even with the resources preloaded, there is a noticeable flickering.

Available solutions:

  1. Use a low-res/hi-res schema for loading images. Unfortunately, in this schema, the low-res image is not animated.
  2. Use a RetainingDataSource. While it's purpose was exactly what we were looking for, it turns out that this data source, does it's magic by lying to the AbstractDataSource code and keeping it in a forever PROGRESS state. That means, the datasource will never get to notify the listening controllers. This is the reason GIFs are not playing and controller callbacks are not fired using this DataSource.

Test Plan (required)

Given a list of GIFs, by using a RetainingDataSource we should be able to replace the current playing view with a freshly loaded one, without the user notice any flickering or blank spaces between the 2 resouces.

I've created a set of test GIFS in the ImageUriProvider and I updated the RetainingDataSourceSupplierFragment to use those resouces, in order to demonstrate the new achieved behaviour.

Demo video:
https://youtu.be/A67X6Vn9VjY

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Aug 16, 2018

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Aug 16, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@bogdantmm92

This comment has been minimized.

bogdantmm92 commented Aug 21, 2018

Really good change! Thanks @Crysis21

@amarkovits

This comment has been minimized.

amarkovits commented Aug 21, 2018

Great solution.

@oprisnik

Thanks for the pull request @Crysis21 . I've added some comments but I think the main approach here should be fine.

* @return true if any resources poured in the datasource should be sent to controllers. Usefull
* with a RetainingDataSource to loaded resources are displayed correctly.
*/
boolean isReadyToPlay();

This comment has been minimized.

@oprisnik

oprisnik Aug 21, 2018

Contributor

Data source is, as the javadoc mentions, "An alternative to Java Futures for the image pipeline". It shouldn't have any animation-related logic in the interface.

This comment has been minimized.

@oprisnik

oprisnik Aug 21, 2018

Contributor

I suggest renaming this to something like boolean hasMultipleResults() and using that one instead.

This comment has been minimized.

@Crysis21

Crysis21 Aug 22, 2018

Contributor

👍

@@ -532,7 +537,10 @@ private void onNewResultInternal(
mDataSource = null;
mSettableDraweeHierarchy.setImage(drawable, 1f, wasImmediate);
getControllerListener().onFinalImageSet(id, getImageInfo(image), getAnimatable());
// IMPORTANT: do not execute any instance-specific code after this point

This comment has been minimized.

@oprisnik

oprisnik Aug 21, 2018

Contributor

This warning shouldn't be removed since it is important :)

This comment has been minimized.

@Crysis21

Crysis21 Aug 22, 2018

Contributor

ok, I'll add it

import javax.annotation.Nullable;
import javax.annotation.concurrent.NotThreadSafe;

import static com.facebook.drawee.components.DraweeEventTracker.Event;

This comment has been minimized.

@oprisnik

oprisnik Aug 21, 2018

Contributor

The imports shouldn't be re-ordered. We deleted our code style settings .intellij/codeStyleSettings.idea by accident but they will be re-added shortly (cdbd6ea). Sorry for that.

This comment has been minimized.

@Crysis21

Crysis21 Aug 22, 2018

Contributor

AndroidStudio did that, imports optimisation was active. Will check for changes and revert them.

} else if (isReadyToPlay) {
logMessageAndImage("set_temporary_result @ onNewResult", image);
mSettableDraweeHierarchy.setImage(drawable, 1f, wasImmediate);
getControllerListener().onFinalImageSet(id, getImageInfo(image), getAnimatable());

This comment has been minimized.

@oprisnik

oprisnik Aug 21, 2018

Contributor

The problem here is that this is not true. It's not the final image but one in a series. I guess in this case it makes sense though since each image could be considered as an image on its own.

@@ -499,7 +503,8 @@ private void onNewResultInternal(
@Nullable T image,
float progress,
boolean isFinished,
boolean wasImmediate) {
boolean wasImmediate,
boolean isReadyToPlay) {

This comment has been minimized.

@oprisnik

oprisnik Aug 21, 2018

Contributor

The naming here is confusing if you don't have context.

This comment has been minimized.

@Crysis21

Crysis21 Aug 22, 2018

Contributor

I renamed it to deliverTempResult. It's more accurate to what the parameter does.

@Crysis21

This comment has been minimized.

Contributor

Crysis21 commented Aug 22, 2018

I applied the requested changes.

@oprisnik

Thanks, I'll import it for internal testing.

@facebook-github-bot

oprisnik has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment