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

GlideBitmapDrawable sometimes shares state (including ColorFilter) #276

Closed
TWiStErRob opened this issue Nov 21, 2014 · 2 comments
Closed
Labels
Milestone

Comments

@TWiStErRob
Copy link
Collaborator

I have two ImageViews: original and preview. I load the same Uri into both using the same method:

void load(Uri uri, ImageView imageView) {
    Glide.with(this).load(uri).dontTransform().listener(maybeNull).into(imageview);
}

It always loads the images correctly, but sometimes (I hate this word in IT) the color filter I set shows up on both images:

void setColorFilter(ColorFilter colorFilter) {
    preview.setColorFilter(colorFilter);
    original.setColorFilter(null);

I tried to add the second (null) line above to make sure I clear any accidental color filters, but it still shows up, I took a dump (Java heap ;) and checked:

ImageView(original).mColorFilter == null
ImageView(preview).mColorFilter == colorFilter
ImageView(preview).mDrawable.state.paint.mColorFilter == colorFilter
ImageView(preview).mDrawable != ImageView(original).mDrawable
ImageView(preview).mDrawable.state == ImageView(original).mDrawable.state
ImageView(preview).mDrawable.mutated == true
ImageView(original).mDrawable.mutated == false

The problem is the state line.
Everything else looks normal and not shared (bounds, callback, ...)

@TWiStErRob
Copy link
Collaborator Author

preview.setColorFilter(colorFilter);
original.setColorFilter(colorFilter);
original.setColorFilter(null);

correctly forces a mutate on the original's drawable and detaches the state from preview's.

original.setColorFilter(null) was not enough because ImageView does a noop if the color filter is the same: remember ImageView(original).mColorFilter == null.

@sjudd
Copy link
Collaborator

sjudd commented Nov 22, 2014

Here's one way this could happen:

  1. I call get() on my DrawableResource, which, the first time it's called, returns the original Drawable
  2. I call mutate on the original Drawable which creates a new state, but returns the original Drawable.
  3. I call setColorFilter on the original Drawable which sets the color filter of the paint belonging to the state of the original drawable.
  4. I call get() on my DrawableResource a second time, which creates a new Drawable using newDrawable(). The new drawable however, still points to the original Drawable's state.
  5. I draw my new drawable and the image is drawn with the color filter I set on the original drawable.

This could only happen if you set the color filter in between the first load completing and the second load completing and the first load completed without any other calls to get() which may be subject to a race.

I believe the solution is going to be always calling newDrawable() on get() so we always return a pristine unmodified Drawable. We could also mutate the state on newDrawable, but that isn't in line with what BitmapDrawable does.

@sjudd sjudd added the bug label Nov 22, 2014
@sjudd sjudd added this to the 3.5 milestone Nov 22, 2014
sjudd added a commit to sjudd/glide that referenced this issue Nov 24, 2014
@sjudd sjudd closed this as completed in 1bd66cb Nov 24, 2014
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

2 participants