[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

Sign up for GitHub

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

Jump to bottom

SimpleDraweeView did not recycled in RecyclerView #414

Closed
06peng opened this issue Jun 30, 2015 · 15 comments
Closed

SimpleDraweeView did not recycled in RecyclerView #414

06peng opened this issue Jun 30, 2015 · 15 comments

Comments

@06peng
Copy link
06peng commented Jun 30, 2015

233

I am using SimpleDraweeView to load image in RecyclerView,At first the memory is 40M,When I scrolling the RecyclerView slowly,the memory increase to 200M and didn't recycled.
Is there a method can manual recycled the memory? Thanks!
@kancic
Copy link
kancic commented Jul 6, 2015

I also experience this. Can be also reproduced on rotation.

This is my code:

public class PhotoAdapter extends BaseRecyclerAdapter<PhotoAdapter.ViewHolder>
{
public PhotoAdapter(ArrayList itemList)
{
super(itemList);
}

public class ViewHolder extends RecyclerView.ViewHolder
{
    private final SimpleDraweeView photo;

    public ViewHolder(View itemView)
    {
        super(itemView);
        photo = (SimpleDraweeView) itemView.findViewById(R.id.image);
    }
}

@Override
public ViewHolder onCreateViewHolder(ViewGroup parent, int viewType)
{
    super.onCreateViewHolder(parent, viewType);
    View itemView = LayoutInflater.from(parent.getContext()).inflate(R.layout.row_photo, parent, false);
    ViewHolder viewHolder = new ViewHolder(itemView);
    return viewHolder;
}

@Override
public void onBindViewHolder(ViewHolder holder, int position)
{
    super.onBindViewHolder(holder, position);
    Image photo = (Image) itemList.get(position);
    setupImage(holder.photo, photo.getUri(context));
}

private void setupImage(SimpleDraweeView simpleDraweeView, Uri uri)
{
    simpleDraweeView.getViewTreeObserver().addOnPreDrawListener(new ViewTreeObserver.OnPreDrawListener()
    {
        @Override
        public boolean onPreDraw()
        {
            simpleDraweeView.getViewTreeObserver().removeOnPreDrawListener(this);
            ImageRequest request = ImageRequestBuilder.newBuilderWithSource(uri)
                                                      .setResizeOptions(new ResizeOptions(simpleDraweeView.getWidth(), simpleDraweeView.getHeight()))
                                                      .setAutoRotateEnabled(true)
                                                      .build();
            PipelineDraweeController controller = (PipelineDraweeController) Fresco.newDraweeControllerBuilder()
                                                                                   .setOldController(simpleDraweeView.getController())
                                                                                   .setImageRequest(request)
                                                                                   .build();
            simpleDraweeView.setController(controller);
            simpleDraweeView.setImageURI(uri);
            return true;
        }
    });
}

}

@plamenko plamenko added the needs-details This issue or PR is currently not actionable as it misses details (e.g. for reproducing the problem) label Jul 6, 2015
@plamenko
Copy link
Contributor
plamenko commented Jul 6, 2015

One thing that is wrong with your code is that you are setting both the controller and the uri. You should only do one of those.
Also, I am not sure that the problem is in the SimpleDraweeView as those listeners look sketchy too.
One way to narrow down the issue is to completely remove the following two lines and see whether you still have memory leaks:

        simpleDraweeView.setController(controller);
        simpleDraweeView.setImageURI(uri);

Note, for this experiment you won't see the images, but that's expected. Please let me know if you are still observing the leaks after removing those two lines and we can continue from there.

Also, how many instances of SimpleDraweeView are there? Easy way to check would be to just log the reference of simpleDraweeView in the logcat from the setupImage method.

@kancic
Copy link
kancic commented Jul 7, 2015

Thank you for your answer. I somehow missed the last line during refactoring. By removing these lines the leeks stopped (or at least where very small). Are these the logs you are looking for? (photo.toString())

07-07 09:50:29.600  29131-29131/hr.alfabit.rankstar I/DRAWEE﹕ SimpleDraweeView{holder=DraweeHolder{controllerAttached=false, holderAttached=false, drawableVisible=true, activityStarted=true, events=[ON_SET_HIERARCHY]}}
07-07 09:50:29.602  29131-29131/hr.alfabit.rankstar I/DRAWEE﹕ SimpleDraweeView{holder=DraweeHolder{controllerAttached=false, holderAttached=false, drawableVisible=true, activityStarted=true, events=[ON_SET_HIERARCHY]}}
07-07 09:50:29.604  29131-29131/hr.alfabit.rankstar I/DRAWEE﹕ SimpleDraweeView{holder=DraweeHolder{controllerAttached=false, holderAttached=false, drawableVisible=true, activityStarted=true, events=[ON_SET_HIERARCHY]}}
07-07 09:50:29.605  29131-29131/hr.alfabit.rankstar I/DRAWEE﹕ SimpleDraweeView{holder=DraweeHolder{controllerAttached=false, holderAttached=false, drawableVisible=true, activityStarted=true, events=[ON_SET_HIERARCHY]}}

Next, I simplified my code to

private void setupImage(SimpleDraweeView photo, Uri uri)
{
     photo.setImageURI(uri);
}

And the leaks and oom crashes still occur on rotate.

And bdw pozdrav iz Zagreba. 😃

@kancic
Copy link
kancic commented Jul 7, 2015

Some more information. The memory leak occurred on my 5.1.1 Android device. Memory usage is quite excessive. Starts with 46MB for 5 pictures and gets to 59MB on rotate, then 76 on next until OOM. The same code and the same pictures on my 4.3 tablet don't have the leak as I can see. It starts with 23MB then 25, 27 then at around 35 it goes down. So it seems the problem is related to Android version maybe? This is my drawee view (row_photo.xml) just for reference:

<?xml version="1.0" encoding="utf-8"?>
<com.facebook.drawee.view.SimpleDraweeView
    android:id="@+id/image"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:fresco="http://schemas.android.com/apk/res-auto"
    android:scaleType="centerCrop"
    fresco:viewAspectRatio="1"/>

@kancic
Copy link
kancic commented Jul 8, 2015

and here is the log if it helps

java.lang.OutOfMemoryError: Failed to allocate a 4665612 byte allocation with 3768820 free bytes and 3MB until OOM
            at dalvik.system.VMRuntime.newNonMovableArray(Native Method)
            at android.graphics.Bitmap.nativeCreate(Native Method)
            at android.graphics.Bitmap.createBitmap(Bitmap.java:812)
            at android.graphics.Bitmap.createBitmap(Bitmap.java:789)
            at android.graphics.Bitmap.createBitmap(Bitmap.java:756)
            at com.facebook.imagepipeline.memory.BitmapPool.alloc(BitmapPool.java:55)
            at com.facebook.imagepipeline.memory.BitmapPool.alloc(BitmapPool.java:30)
            at com.facebook.imagepipeline.memory.BasePool.get(BasePool.java:259)
            at com.facebook.imagepipeline.bitmaps.ArtBitmapFactory.doDecodeStaticImage(ArtBitmapFactory.java:141)
            at com.facebook.imagepipeline.bitmaps.ArtBitmapFactory.decodeJPEGFromEncodedImage(ArtBitmapFactory.java:128)
            at com.facebook.imagepipeline.bitmaps.PlatformBitmapFactory.decodeJPEGFromEncodedImage(PlatformBitmapFactory.java:106)
            at com.facebook.imagepipeline.decoder.ImageDecoder.decodeJpeg(ImageDecoder.java:139)
            at com.facebook.imagepipeline.decoder.ImageDecoder.decodeImage(ImageDecoder.java:80)
            at com.facebook.imagepipeline.producers.DecodeProducer$ProgressiveDecoder.doDecode(DecodeProducer.java:178)
            at com.facebook.imagepipeline.producers.DecodeProducer$ProgressiveDecoder.access$100(DecodeProducer.java:94)
            at com.facebook.imagepipeline.producers.DecodeProducer$ProgressiveDecoder$1.run(DecodeProducer.java:122)
            at com.facebook.imagepipeline.producers.JobScheduler.doJob(JobScheduler.java:192)
            at com.facebook.imagepipeline.producers.JobScheduler.access$000(JobScheduler.java:26)
            at com.facebook.imagepipeline.producers.JobScheduler$1.run(JobScheduler.java:62)
            at com.facebook.common.executors.SerialDelegatingExecutor.executeSingleCommand(SerialDelegatingExecutor.java:76)
            at com.facebook.common.executors.SerialDelegatingExecutor.access$000(SerialDelegatingExecutor.java:24)
            at com.facebook.common.executors.SerialDelegatingExecutor$1.run(SerialDelegatingExecutor.java:47)
            at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
            at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
            at java.lang.Thread.run(Thread.java:818)

@plamenko
Copy link
Contributor
plamenko commented Jul 8, 2015

Hmm, there may be some issues with Drawee + RecyclerView combination. If you are familiar with MAT, it would be very helpful to see what is referencing the leaked Bitmaps. We will also investigate on our own, but since you are able to reproduce the issue, any help would be greatly appreciated!

As for the Android version, Fresco on Lollipop keeps the bitmaps on the Java heap and that's why you are getting OOMs. That doesn't mean that the leak is not happening on older Android versions where we keep bitmaps in the native memory. The leak may still be there, but it won't be detected by the Java VM.

@plamenko plamenko removed the needs-details This issue or PR is currently not actionable as it misses details (e.g. for reproducing the problem) label Jul 8, 2015
@kancic
Copy link
kancic commented Jul 8, 2015

I'm willing to help but I'm not familiar with MAT. Can you link it? Also is this of any help to you? https://github.com/square/leakcanary

I have made some new discoveries, and maybe this will seem a huge newbie mistake to you, but after moving

Fresco.initialize(this);

from the beginning of every activity to an application class, the leak seams to be gone or at very least harder to reproduce. If indeed this is the intended use, maybe it would be best to highlight it a bit more. If it is a mistake from my part sorry for wasting your time.

@plamenko
Copy link
Contributor
plamenko commented Jul 8, 2015

Oh yes, you should only call Fresco.initialize once, near the application startup. Doing it in each Activity is wrong. We shall update the documentation to reflect this more clearly.

Also, this might be of interest:
http://android-developers.blogspot.co.uk/2011/03/memory-analysis-for-android.html
http://www.eclipse.org/mat/

@kancic
Copy link
kancic commented Jul 8, 2015

Thank you.

@mheras
Copy link
mheras commented Jul 10, 2015

@r1m Thank you VERY much... That made my OOMs go away.
I still have a question... What if we want to use Fresco in an Android Library (aar)? Should I ask the user to initialize Fresco in his Application because I'm using it? Isn't there any other option in which it is initialized transparently for the user of the aar? Thanks.

@michalgr
Copy link
Contributor

@mheras There is no other option - you have to pass Context to Fresco.initialize once and before it is used for the first time. Application is convenient place to do this initialization. You should either ask your users to initialize Fresco in Application or initialize your library there (and your library would in turn initialize Fresco).

@tyronen
Copy link
Contributor
tyronen commented Jul 15, 2015

The app author shouldn't have to initialize Fresco just because you are using it. However, if they happen to be using Fresco on their own, your library has to reflect their config.

You should offer two initialization methods: one that takes an ImagePipelineConfig object, and one that doesn't. Only the second should call Fresco.initialize; the first should assume the app will do it.

@tyronen tyronen closed this as completed Jul 15, 2015
@sungerk
Copy link
sungerk commented Apr 9, 2016

@tyronen i had do two initialization methods,but it's also oom

@leofirespotter
Copy link

Seems like the answer here is a major discrepancy with the documentation (http://frescolib.org/docs/using-controllerbuilder.html) where it clearly says

You should call setOldController when building a new controller. This will allow for the old controller to be reused and a couple of unnecessary memory allocations to be avoided.

@ZhangFengG
Copy link
ZhangFengG commented Dec 16, 2016 •
edited
Loading

I have the same experience, and find that is the image url witch you have request from server is too big to your device, you should call ImageRequest and setResizeOptions to resize the image. @06peng

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@mheras @michalgr @tyronen @plamenko @leofirespotter @06peng @sungerk @ZhangFengG @kancic