square / picasso

A powerful image downloading and caching library for Android
https://square.github.io/picasso/
Apache License 2.0
18.71k stars 3.97k forks source link

Picasso and MediaStore.Images.Thumbnails bug #872

Open dextorer opened 9 years ago

dextorer commented 9 years ago

We have been experiencing a weird behavior on our music player app, which loads local cover art images using Picasso. Some users reported wrong cover art images on some albums on a total random basis: instead of the album cover art, they had a screenshot. Clearing the cache, reinstalling the app and event clearing MediaStore's data didn't seem to help.

We managed to replicate the issue on a test device and, after quite a lot of headaches, we narrowed down the problem to the MediaStoreRequestHandler class. This line

bitmap = Images.Thumbnails.getThumbnail(contentResolver, id, picassoKind.androidKind, options);

apparently returned a wrong Bitmap. Googling the issue led to this Stackoverflow question and subsequently to this official bug. Long story short, it seems that SOMETIMES the MediaStore is returning a wrong image instead of returning null. This happens especially when the asked size is MICRO_KIND (our album cover arts were resized to 328x328, which falls in this particular size).

Now, we are aware that this has nothing to do with Picasso itself, but still, this is a nasty bug which might affect other developers as well. It would be great if Picasso could be aware of that or, at least, provide a way around it.

Proposed solution

The MediaStoreRequestHandler class queries the MediaStore service in order to obtain a suitable-sized Bitmap according to the requested size. A possible solution might involve adding a dedicated flag on the Request class, so that MediaStoreRequestHandler looks like this:

@Override public Result load(Request data, int networkPolicy) throws IOException {
    ContentResolver contentResolver = context.getContentResolver();
    int exifOrientation = getExifOrientation(contentResolver, data.uri);

    String mimeType = contentResolver.getType(data.uri);
    boolean isVideo = mimeType != null && mimeType.startsWith("video/");

    if (data.hasSize() && !data.ignoreMediaStoreThumbnails) { // additional flag

      [...]

      if (bitmap != null) {
        return new Result(bitmap, DISK, exifOrientation);
      }
    }

    return new Result(decodeContentStream(data), DISK, exifOrientation);
  }

Unfortunately, since it was causing a riot on our users, we had to go for a dirty quick fix, which is:

@Override public Result load(Request data, int networkPolicy) throws IOException {
    ContentResolver contentResolver = context.getContentResolver();
    int exifOrientation = getExifOrientation(contentResolver, data.uri);
    return new Result(decodeContentStream(data), DISK, exifOrientation);
  }

Of course, that is not the way to go. Hence, we look forward to hearing from you on this.

dnkoutso commented 9 years ago

@dextorer thanks for the report. Its unfortunate but can this be handled with a custom RequestHandler in your case?

dextorer commented 9 years ago

We could probably extend MediaStoreRequestHandler and override the load() method using the three-lines long one I briefly wrote above. Not sure about how Picasso would behave having two handlers for the same authority though.

However, other developers might experience the same issue and blame Picasso for loading the wrong images from MediaStore, which is not the case. It would be great if Picasso could be aware of this particular problem, either by exposing a flag that ignore existing thumbnails or by offering another MediaStoreRequestHandler, so that painful experiences like the one we had can be avoided :+1:

Thanks for your interest, anyway.

dnkoutso commented 9 years ago

Please provide a PR and we can merge this.

dextorer commented 9 years ago

@dnkoutso Are you OK with adding a flag to the Request class (which was our proposal)? Do you suggest other approaches?

dnkoutso commented 9 years ago

Will the flag be part of the API? I find it a bit weird to expand the API only to add a flag that is relevant to thumbnails and images loaded from the mediastore.

dextorer commented 9 years ago

Agreed. Instead, this flag could live inside MediaStoreRequestHandler. However, in order to enable/disable it, the API still needs an additional method to retrieve the preloaded handlers.

It would look like this, more or less:

1) Retrieve the MediaStoreRequestHandler that is added by default 2) Enable the flag

Again, I am open to any suggestions here. :)

devankit commented 9 years ago

the same issue for me in my music player but the wrong image occurance increases 5 fold if i .fit() method when loading a image . and does picasso try 3 times for local images also??

ypresto commented 9 years ago

Maybe related: #985

JakeWharton commented 6 years ago

Good info in https://github.com/square/picasso/issues/1341 about this.