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

OOM callback #859

Closed
tibbi opened this issue Jan 4, 2016 · 15 comments
Closed

OOM callback #859

tibbi opened this issue Jan 4, 2016 · 15 comments
Labels

Comments

@tibbi
Copy link

tibbi commented Jan 4, 2016

Hello, Im sometimes loading very huge images, so out of memory on some devices is common. Its not a huge issue, but Id like to react on it. Is there any callback for Out of memory, or any way to find it out? Shouldnt onLoadFailed be called if some exception is thrown? I can create a workaround with onResourceReady and some timer to check if the image was loaded properly, but Id like some nicer solution. So how should I react to the internal Glide Out of memory exception?

@tibbi
Copy link
Author

tibbi commented Jan 4, 2016

seems to be related to #491

@TWiStErRob
Copy link
Collaborator

Yes... the problem with delegating OOM to onLoadFailed is that it's not an Exception, it's an Error which are two distinct subclasses of Throwable. Though you may have a solution by replacing the default model loader and adding a try-catch to it, I'm looking.

@TWiStErRob
Copy link
Collaborator

TWiStErRob commented Jan 4, 2016

Edit: since #1057 was merged to 3.8.0 this is no longer required. OOMs are delegated as load failures, you can check for them with e != null && e.getCause() instanceof OutOfMemoryError.

Sorry, not custom loader, custom decoder:

class OOMReadyStreamBitmapDecoder extends StreamBitmapDecoder {
    public OOMReadyStreamBitmapDecoder(Context context) {
        super(context);
    } // or any other ctor you want from super
    @Override public Resource<Bitmap> decode(InputStream source, int width, int height) {
        try {
            return super.decode(source, width, height);
        } catch (OutOfMemoryError err) {
            throw new RuntimeException(err);
        }
    }
}

and then just use it:

String url = "https://upload.wikimedia.org/wikipedia/commons/c/c6/BlankMap-World-large3.png";
BitmapRequestBuilder<String, Bitmap> oomRequest = Glide
        .with(context)
        .fromString()
        .asBitmap()
        .imageDecoder(new OOMReadyStreamBitmapDecoder(context))
        //.cacheDecoder(new FileToStreamDecoder<>(new OOMReadyStreamBitmapDecoder(context))) // needed for .diskCacheStrategy(SOURCE|ALL)
        //.error(R.drawable.ic_launcher) // displayed when OOM (because throw RuntimeException)
        //.override(Target.SIZE_ORIGINAL, Target.SIZE_ORIGINAL) // to make sure it OOMs (url is huge)
        ;

oomRequest is created to prevent instantiations for each bind, it's a "template" for the loads.

oomRequest
        .load(url)
        .listener(new RequestListener<String, Bitmap>() {
            @Override public boolean onException(Exception e, String model, Target<Bitmap> target, boolean isFirstResource) {
                if (e.getCause() instanceof OutOfMemoryError) {
                    // do something (you also have model available)
                    // if you return true, .error() won't be displayed
                }
                return false;
            }
            @Override public boolean onResourceReady(Bitmap resource, String model, Target<Bitmap> target, boolean isFromMemoryCache, boolean isFirstResource) {
                return false;
            }
        })
;

or

oomRequest
        .load(url)
        .into(new BitmapImageViewTarget(imageView) {
            @Override public void onLoadFailed(Exception e, Drawable errorDrawable) {
                if (e.getCause() instanceof OutOfMemoryError) {
                    // do something
                }
                super.onLoadFailed(e, errorDrawable);
            }
        })
;

It's not a global solution, you can do this for any load separately.
The above is also applicable for normal loads (GlideDrawable), but it's more complicated to pass a custom .decoder().

@tibbi
Copy link
Author

tibbi commented Jan 4, 2016

cool it works, thanks a lot for the fast reply. Imho it should be the default behaviour, an exception should call onLoadFailed too. If Im loading the bitmap into GlideDrawableImageViewTarget, the onLoadFaileds' first parameter is an Exception so it should be there.

@TWiStErRob
Copy link
Collaborator

an exception should call onLoadFailed too

doesn't this happen if you have an Exception? It won't happen on OutOfMemoryError, becuase it's not instanceof Exception so there would be a casting compile error there. If you have an exception thrown and it is not called, maybe you returned true from the listener.

@tibbi
Copy link
Author

tibbi commented Jan 4, 2016

oh ye really, out of memory is an error, not an exception. It makes sense now. Im using the fourth code block you wrote with BitmapImageViewTarget , not the listener one. Works fine now, I can catch the oom in onLoadFailed. Thanks a lot for the trick, guess the issue can be closed.

@nogueirasjorge
Copy link

Hello guys, I've found this solution very helpful. However, there are some cases where ( specially in devices with very low memory capabilities ) I can't catch the OOM Exception. The app does not crash but I can't do what's necessary when an exception like this comes out.
Does anyone know what could be happening here? Did I miss something?

Another comment about the solution that you proposed. In those lines of code

if (e.getCause() instanceof OutOfMemoryError) {
                   // do something
}

In some cases 'e' is null and the method getCause() could return null too. It should be checked.
I'll appreciate any idea you may have. Thanks a lot.

@TWiStErRob
Copy link
Collaborator

I guess you're not asking for e != null && and instanceof Anything is false for null.

A quick "Analyze data flow to here" with "Group by leaf expression" in IDEA on RequestListener.onException#e showed:

image

  • 3 hand-created exceptions (non-null)
  • one null when model is null
  • EngineRunnable.run#exception which is when the decoder throws
    Notice that any Exception thrown from decode is caught, this is the fact that my solution exploits. Notice that if an Error is thrown from decode that is not caught!
  • EngineRunnable.run#exception is null when the decoder returns null!
    So you always want to throw when a decoder fails, otherwise you hide what's wrong.

Going deeper into decode you can see that there are several ways it can go. Setting the imageDecoder only covers the decodeFromSource path, but the decodeFrom*Cache can still fail with OOM Error (not Exception). To cover all bases you might want to add cacheDecoder as I pointed out in the original code's comments.

Edit: I just noticed that transformation or transcoder can also throw an OOM which is uncaught so you'll need to wrap .transform(new OOMReadyBitmapTransformation(new CenterCrop())) for transcoder see Glide class constructor where transcoderRegistry is used. The transcoder part goes into really advanced usage, I think it's safe to assume it won't throw because usually it creates only a Drawable which is just a few bytes.

@osama
Copy link

osama commented Jun 16, 2016

Hey @TWiStErRob , I've been trying to follow this advice to come up with a decoder for a DrawableRequestBuilder, and am wondering if I'm on the right track (it's looking a little odd to me).

ResourceDecoder decoder = new GifBitmapWrapperResourceDecoder(
  new ImageVideoBitmapDecoder(new StreamBitmapDecoder(context), new FileDescriptorBitmapDecoder(context)) {
    @Override
    public Resource<Bitmap> decode(ImageVideoWrapper source, int width, int height) throws IOException {
      try {
        return super.decode(source, width, height);
      } catch (OutOfMemoryError e) {
        throw new RuntimeException("OOM when loading GIF");
      }
    }
  },
  new GifResourceDecoder(context) {
    @Override
    public GifDrawableResource decode(InputStream source, int width, int height) {
      try {
        return super.decode(source, width, height);
      } catch (OutOfMemoryError e) {
        throw new RuntimeException("OOM when loading GIF");
      }
    }
  }, Glide.get(context).getBitmapPool());

I'm then setting these as request.decoder(decoder).cacheDecoder(new FileToStreamDecoder<GifBitmapWrapper>(decoder)); I think the cache decoder is definitely incorrect but I'm not sure what would be used instead.

@TWiStErRob
Copy link
Collaborator

@mcoded There was a reason I used .asBitmap() ;)

You seem to be really close, but you forgot to subclass the most important one: StreamBitmapDecoder (first argument of ImageVideoBitmapDecoder). That's what is being used most of the time, for normal images. FileDescriptor one is for local video files, and Gif is for GIFs only.

Try:

GifBitmapWrapperResourceDecoder decoder = ...;
...
    .decoder(decoder)
    .cacheDecoder(new FileToStreamDecoder<>( // from ImageVideoGifDrawableLoadProvider
        new GifBitmapWrapperStreamResourceDecoder(decoder))) 

Declare decoder with concrete type to help with unchecked warnings, you should be able to do everything without warnings, that's when you know you made it :)

Tip: Subscribe to #1057 so you know when you can get rid of these constructs.

@osama
Copy link

osama commented Jun 17, 2016

Thanks for the pointers, works great now :)
(The PR you linked is actually how I came across this solution, hopefully it gets merged soon!)

@osama
Copy link

osama commented Jun 21, 2016

@TWiStErRob I've been seeing a few complaints (via Leak Canary) of memory leaks since putting in the decoders mentioned earlier, particularly due to GifResourceDecoder. It looks like this decoder isn't being destroyed after the load is complete, thus causing the Context it holds to leak. This doesn't look like a race between Leak Canary and Glide because the Activity is staying the heap long after it has been destroyed. Is there something I should be doing to manage/destroy this decoder manually?

D/LeakCanary: * com.app.social.SocialHubActivity has leaked:
D/LeakCanary: * GC ROOT static com.bumptech.glide.Glide.glide
D/LeakCanary: * references com.bumptech.glide.Glide.memoryCache
D/LeakCanary: * references com.bumptech.glide.load.engine.cache.LruResourceCache.cache
D/LeakCanary: * references java.util.LinkedHashMap.table
D/LeakCanary: * references array java.util.HashMap$HashMapEntry[].[8]
D/LeakCanary: * references java.util.LinkedHashMap$LinkedEntry.key
D/LeakCanary: * references com.bumptech.glide.load.engine.EngineKey.decoder
D/LeakCanary: * references com.bumptech.glide.load.resource.gifbitmap.GifBitmapWrapperResourceDecoder.gifDecoder
D/LeakCanary: * references com.app.util.image.ImageLoader$3.context (anonymous subclass of com.bumptech.glide.load.resource.gif.GifResourceDecoder)
D/LeakCanary: * leaks com.app.social.SocialHubActivity instance
D/LeakCanary: * Retaining: 108 KB.

@TWiStErRob
Copy link
Collaborator

@mcoded it looks like your resources are kept in memory cache, which is a good thing, but to hit the cache all the decoders are also kept within EngineKey as you can see in the reference chain. I think the simplest thing is to put a context = context.getApplicationContext() in front of your decoder initialization. (Be careful to still pass the tightest context to Glide.with.)

This is also probably a bug in GifResourceDecoder, the context field should be initialized as in GifDrawable.GifState, opened #1277, feel free to check what would be affected by the change and create PR if you want.

@Kostyshina
Copy link

@TWiStErRob you wrote about catching OOM for Glide v3 and I just wonder how to do that in v4. StreamBitmapDecoder quite changed

@TWiStErRob
Copy link
Collaborator

Hmm, @sjudd, I wonder if #1057 ever made it to master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants