Skip to content
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 from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -7,11 +7,10 @@

package com.facebook.drawee.controller;

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

import android.graphics.drawable.Animatable;
import android.graphics.drawable.Drawable;
import android.view.MotionEvent;

import com.facebook.common.internal.Objects;
import com.facebook.common.internal.Preconditions;
import com.facebook.common.logging.FLog;
Expand All @@ -27,10 +26,14 @@
import com.facebook.drawee.interfaces.SettableDraweeHierarchy;
import com.facebook.imagepipeline.systrace.FrescoSystrace;
import com.facebook.infer.annotation.ReturnsOwnership;

import java.util.concurrent.Executor;

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

import static com.facebook.drawee.components.DraweeEventTracker.Event;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Abstract Drawee controller that implements common functionality
* regardless of the backend used to fetch the image.
Expand Down Expand Up @@ -443,7 +446,7 @@ protected void submitRequest() {
mEventTracker.recordEvent(Event.ON_SUBMIT_CACHE_HIT);
getControllerListener().onSubmit(mId, mCallerContext);
onImageLoadedFromCacheImmediately(mId, closeableImage);
onNewResultInternal(mId, mDataSource, closeableImage, 1.0f, true, true);
onNewResultInternal(mId, mDataSource, closeableImage, 1.0f, true, true, true);
FrescoSystrace.endSection();
return;
}
Expand All @@ -470,10 +473,11 @@ public void onNewResultImpl(DataSource<T> dataSource) {
// isFinished must be obtained before image, otherwise we might set intermediate result
// as final image.
boolean isFinished = dataSource.isFinished();
boolean isReadyToPlay = dataSource.isReadyToPlay();
float progress = dataSource.getProgress();
T image = dataSource.getResult();
if (image != null) {
onNewResultInternal(id, dataSource, image, progress, isFinished, wasImmediate);
onNewResultInternal(id, dataSource, image, progress, isFinished, wasImmediate, isReadyToPlay);
} else if (isFinished) {
onFailureInternal(id, dataSource, new NullPointerException(), /* isFinished */ true);
}
Expand All @@ -499,7 +503,8 @@ private void onNewResultInternal(
@Nullable T image,
float progress,
boolean isFinished,
boolean wasImmediate) {
boolean wasImmediate,
boolean isReadyToPlay) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try {
FrescoSystrace.beginSection("AbstractDraweeController#onNewResultInternal");
// ignore late callbacks (data source that returned the new result is not the one we expected)
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll add it

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
logMessageAndImage("set_intermediate_result @ onNewResult", image);
mSettableDraweeHierarchy.setImage(drawable, progress, wasImmediate);
Expand Down
Expand Up @@ -319,4 +319,9 @@ public void run() {
});
}
}

@Override
public boolean isReadyToPlay() {
return false;
}
}
7 changes: 7 additions & 0 deletions fbcore/src/main/java/com/facebook/datasource/DataSource.java
Expand Up @@ -49,6 +49,13 @@ public interface DataSource<T> {
*/
boolean hasResult();

/**
*
* @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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


/**
* @return true if request is finished, false otherwise
*/
Expand Down
Expand Up @@ -139,5 +139,10 @@ public void onProgressUpdate(DataSource<T> dataSource) {
RetainingDataSource.this.onDatasourceProgress(dataSource);
}
}

@Override
public boolean isReadyToPlay() {
return true;
}
}
}
Expand Up @@ -11,34 +11,52 @@
*/
package com.facebook.fresco.samples.showcase.drawee;

import android.graphics.drawable.Animatable;
import android.net.Uri;
import android.os.Bundle;
import android.support.annotation.Nullable;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;

import com.facebook.common.references.CloseableReference;
import com.facebook.datasource.RetainingDataSourceSupplier;
import com.facebook.drawee.backends.pipeline.Fresco;
import com.facebook.drawee.controller.BaseControllerListener;
import com.facebook.drawee.controller.ControllerListener;
import com.facebook.drawee.view.SimpleDraweeView;
import com.facebook.fresco.samples.showcase.BaseShowcaseFragment;
import com.facebook.fresco.samples.showcase.R;
import com.facebook.fresco.samples.showcase.misc.ImageUriProvider;
import com.facebook.imagepipeline.image.CloseableImage;
import com.facebook.imagepipeline.image.ImageInfo;
import com.facebook.imagepipeline.request.ImageRequest;

import java.util.List;

public class RetainingDataSourceSupplierFragment extends BaseShowcaseFragment {

private List<Uri> mSampleUris;
private int mUriIndex = 0;

private ControllerListener controllerListener = new BaseControllerListener<ImageInfo>() {
@Override
public void onFinalImageSet(
String id,
@Nullable ImageInfo imageInfo,
@Nullable Animatable anim) {
if (anim != null) {
// app-specific logic to enable animation starting
anim.start();
}
}
};

@Override
public void onCreate(@Nullable Bundle savedInstanceState) {
super.onCreate(savedInstanceState);

mSampleUris =
ImageUriProvider.getInstance(getContext()).getSampleUris(ImageUriProvider.ImageSize.M);
mSampleUris = ImageUriProvider.getInstance(getContext()).getSampleGifUris();
}

@Nullable
Expand All @@ -54,7 +72,10 @@ public void onViewCreated(View view, @Nullable Bundle savedInstanceState) {
final RetainingDataSourceSupplier<CloseableReference<CloseableImage>> retainingSupplier =
new RetainingDataSourceSupplier<>();
simpleDraweeView.setController(
Fresco.newDraweeControllerBuilder().setDataSourceSupplier(retainingSupplier).build());
Fresco.newDraweeControllerBuilder()
.setDataSourceSupplier(retainingSupplier)
.setControllerListener(controllerListener)
.build());
replaceImage(retainingSupplier);
simpleDraweeView.setOnClickListener(
new View.OnClickListener() {
Expand Down
Expand Up @@ -146,6 +146,13 @@ public enum ImageSize {
private static final String SAMPLE_URI_WEBP_ANIMATED =
"https://www.gstatic.com/webp/animated/1.webp";

private static final String[] SAMPLE_URIS_GIFS =
new String [] {
"https://media2.giphy.com/media/3oge84qhopFbFFkwec/giphy.gif",
"https://media3.giphy.com/media/uegrGBitPHtKM/giphy.gif",
"https://media0.giphy.com/media/SWd9mTHEMIxQ4/giphy.gif"
};

private static ImageUriProvider sInstance;

private final SharedPreferences mSharedPreferences;
Expand Down Expand Up @@ -279,6 +286,14 @@ public List<Uri> getRandomSampleUris(final ImageSize imageSize, final int numIma
return data;
}

public List<Uri> getSampleGifUris(){
ArrayList<Uri> uris = new ArrayList<>();
for (String uri : SAMPLE_URIS_GIFS) {
uris.add(Uri.parse(uri));
}
return uris;
}

private Uri applyOverrideSettings(
String uriString,
UriModification urlModification) {
Expand Down