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

Rendering glitch with animated GIFs and some hardware #301

Closed
bubbleguuum opened this issue Jan 5, 2015 · 12 comments
Closed

Rendering glitch with animated GIFs and some hardware #301

bubbleguuum opened this issue Jan 5, 2015 · 12 comments
Assignees
Labels
Milestone

Comments

@bubbleguuum
Copy link

I have a ListView for which each row contain one ImageView + TextViews.
Something very usual and unspectacular.

The first row's Imagaeview holds an animated GIF while all other rows hold regular JPG images.

On my Nexus4, somerhing funny is going on. Scrolling it the list, towards the 7th or 8th row (which are just offscreen) , a random unnanimated frame of the first GIF is displayed instead of the JPG image that should display for this row.
Sometimes these are several consecutive rows (possibly 5 or 6) that display a random frame of the animated GIF.
If I scroll enough in the list, all get back to normal as the GIF is evicted from the cache (guess).

It goes like this:

row 1: animated GIF plays fine and is correct for this row
row 2: JPG OK
...
row 7: JPG OK
row 8: displays still frame of row 1 GIF instead of correct JPG for this row
row 9: displays still frame of row 1 GIF instead of correct JPG for this row
row 10: JPG OK
...

This bug happens on my Nexus 4 running 4.4.4 but not my Nexus 5.

Looking at the logcat, there's something interesting.
Each time a row displays a frame of the animated GIF instead of the image it should, the logcat reads:

01-05 22:34:26.651: W/Adreno-ES20(26984): <core_glTexSubImage2D:575>: GL_INVALID_OPERATION
01-05 22:34:26.651: D/OpenGLRenderer(26984): GL error from OpenGLRenderer: 0x502

So the process of animating GIF somewhat trigger this (probable) driver bug when loading subsequent
images.

This looks to be a driver bug so I understand probably not much can be done at the library level although a workaround would be nice.

@sjudd
Copy link
Collaborator

sjudd commented Jan 6, 2015

What version of Glide are you using? Is the frame of the GIF that is displayed instead of the JPEGs always the same frame (maybe always the first frame)? For rows that incorrectly display frames of the GIF, do the frames change without you scrolling to new rows and back?

Glide re-uses Bitmaps, so it's possible there is a bug where in some cases we incorrectly return a Bitmap to the bitmap pool that we're still using, and then later overwrite it. Usually when this happens though, the bitmap pool is poisoned and you continue to see visual artifacts and/or incorrect images until the application is restarted, assuming not all of your images fit in the memory cache.

@bubbleguuum
Copy link
Author

I'm using recent Glide, as of master dec 22th. Thus not an official release.

The rows with an incorrect image show a random frame of the GIF, not necessarily the same one. From what I can tell, these frames do not change when scrolling. When reloading the ListView from scratch (for example restarting the app), not the same rows are affected showing the bogus images. The bogus row count is not even the same between runs. As stated in the first post, each icorrect row is exactly matched by 3 logcat lines mentionning an OpenGL blit error (glTexSubImage2D, see below). Could it be that some rendering operations that must be done in the UI thread are done by mistake in a random thread (which can lead to some hard to understand rendering glitches) ? Again, this doesn't happen on a Lollipop Nexus 5, but the rendering scheduling is different from KitKat and may not expose a possible Glide bug.

01-06 16:32:10.307: W/Adreno-ES20(17909): <core_glTexSubImage2D:575>: GL_INVALID_OPERATION
01-06 16:32:10.307: D/OpenGLRenderer(17909): GL error from OpenGLRenderer: 0x502
01-06 16:32:10.307: E/OpenGLRenderer(17909): GL_INVALID_OPERATION

@bubbleguuum
Copy link
Author

More on this, that narrow down the possible cause of the issue.

I've been able to reproduce this issue but with a 32-bit PNG with alpha instead of GIF.
But I've been able to reproduce it only once, in a scenario slightly different than the GIF one.
In that case I have 2 independent ListView living each in their own Fragment.
The first one holds only PNG with transparent areas (alpha), the second list only JPG (no alpha).
While displaying the second list, I could see exactly one of its row displaying a PNG of the first list, with the logcat showing the exact same OpenGL error log trace than the GIF case.
I now remember seeing that happening sporadically a few month ago, so this is not new.

GIF an PNG have in common that their Bitmap has hasAlpha() return true (unlike JPG for which it is false).
So I conclude that there is somewhat a weird memory cache bug when the cache contains both alpha an non-alpha bitmaps, probably related to bitmap recycling and reusing. Or maybe not a cache bug, but a reuse bitmap case that trigger a (silent) invalid rendering operation in the OpenGL driver of the Nexus 4 (and possibly other devices using the same GPU/driver) when alpha is involved.

I can test any possible tentative fix you can think of with the animated GIF case that I can reproduce 100% of the tilme (unlike the PNG one).

@bubbleguuum
Copy link
Author

More investigation:

Making Downsampler.shouldUsePool() return false trivially fixes the problem:

  private static boolean shouldUsePool(InputStream is) {
        // On KitKat+, any bitmap can be used to decode any other bitmap.
        if (Build.VERSION_CODES.KITKAT <= Build.VERSION.SDK_INT) {
            return false;
        }
        ...
  }

After more research, the problem only happens when a bitmap with hasAlpha() == true is reused for use with a bitmap with hasAlpha() == false (eg a GIF or PNG bitmap reused for a JPG).
I could somewhat fix the problem changing the reuse stragegy to discard bitmap with alpha:

  public Bitmap get(int width, int height, Bitmap.Config config) {
       ...
        Bitmap result = groupedMap.get(key);
        if (result != null) {
            if(result.hasAlpha() && Build.VERSION.SDK_INT <= Build.VERSION_CODES.KITKAT) {
                result = null; // not reusing bitmap
            } else {
                result.reconfigure(width, height, config);
                decrementBitmapOfSize(possibleSize);
            }
        }

        return result;
}

There's probably a better fix (or rather workaround as according to doc it is supposed to work).

///

Side note

Although that doesn't seem to be the problem here, the assumption "On KitKat+, any bitmap can be used to decode any other bitmap" is not true.
From https://developer.android.com/training/displaying-bitmaps/manage-memory.html:

// From Android 4.4 (KitKat) onward we can re-use if the byte size of
// the new bitmap is smaller than the reusable bitmap candidate
// allocation byte count.

Not sure if Glide always enforce this rule prior to calling shouldUsePool() but I have the feeling
this would not be the case if a JPG bitmap was reused for a PNG/GIF one (both sharing the same bitmap config but with different allocation byte count requirements not fitting the rule above)

@sjudd sjudd added the bug label Jan 7, 2015
@sjudd
Copy link
Collaborator

sjudd commented Jan 7, 2015

First of all huge thanks for looking in to this, I really appreciate your help!

One other thing worth noting is that whenever hasAlpha == true, the bitmap format is ARGB_8888, whereas typically for JPEGs or images without alpha, we decode images as RGB_565. Again as far as I can tell, neither hasAlpha nor the format should matter, but I may be misunderstanding something or there may be a framework bug.

One other thing to try: if you call setDecodeFormat with ARGB_8888 when you initialize Glide (see the wiki for more on how to do do this if you're not familiar), does that fix the problem? If so it would probably be a better workaround because you'd still get to reuse Bitmaps when decoding images.

As for the side note, you're totally correct, the comment isn't correct, although the code is (the bitmap pool only returns Bitmaps as large or larger than what we ask for). The comment in Glide was meant to refer to all of the other various restrictions (other than size) that exist pre-kitkat. I agree it's confusing, I'll update the comment.

@bubbleguuum
Copy link
Author

Your workaround works as well, at the expense of using more bitmap memory. In 99% of cases, my app just displays all JPG lists. Having a GIF or PNG is the rare (but possible) odd case.

I think this is either a bug in the Framework on KitKat (since it works on Lollipop) or more probably a bug in the OpenGL driver of the Nexus 4 with this specific combination.

@sjudd
Copy link
Collaborator

sjudd commented Jan 9, 2015

Still looking in to this, internal bug is b/18928352.

@bubbleguuum you're correct that using ARGB_8888 means 2x the memory usage per image. You get some quality and performance improvements by using ARGB_8888, and not passing in Bitmaps to BitmapFactory will cause jank.

@edcheezburger
Copy link

Thanks for the workaround @sjudd! I was banging my head against the wall after seeing this issue too. Anxiously awaiting a fix that doesn't double the mem usage though -- I'm using this in an image heavy app and doubling mem usage (especially for large animated gifs) isn't ideal.

@sjudd
Copy link
Collaborator

sjudd commented Jan 15, 2015

It doesn't look like we're likely to come up with another fix, so we will probably default and/or hard code ARGB_8888 on KitKat+ for now. If we do come up with another fix or a better work around, I'll happily do a dot release.

Defaulting to ARGB_8888 doesn't really double your memory usage with Glide. All formats that may contain transparent pixels (PNGs, GIFs) already use ARGB_8888 since RGB_565 doesn't support transparency. In addition, Glide uses fixed memory cache and bitmap pool sizes so it's more that the number of bitmaps that fit in the cache and bitmap pool decreases than the memory usage of your app increases. That said, I probably will increase the cache and pool size slightly (33% or so) to compensate.

ARGB_8888 also provides better quality images and is probably more performant to draw, so there are some benefits of using ARGB_8888. Romain Guy wrote a post about some of the differences when 2.3 was released which is still worth reading: http://www.curious-creature.com/2010/12/08/bitmap-quality-banding-and-dithering/

@edcheezburger
Copy link

Right on. Thanks for the explanation!

On Thu, Jan 15, 2015 at 10:44 AM, Sam [email protected] wrote:

It doesn't look like we're likely to come up with another fix, so we will
probably default and/or hard code ARGB_8888 on KitKat+ for now. If we do
come up with another fix or a better work around, I'll happily do a dot
release.

Defaulting to ARGB_8888 doesn't really double your memory usage with
Glide. All formats that may contain transparent pixels (PNGs, GIFs) already
use ARGB_8888 since RGB_565 doesn't support transparency. In addition,
Glide uses fixed memory cache and bitmap pool sizes so it's more that the
number of bitmaps that fit in the cache and bitmap pool decreases than the
memory usage of your app increases. That said, I probably will increase the
cache and pool size slightly (33% or so) to compensate.

ARGB_8888 also provides better quality images and is probably more
performant to draw, so there are some benefits of using ARGB_8888. Romain
Guy wrote a post about some of the differences when 2.3 was released which
is still worth reading:
http://www.curious-creature.com/2010/12/08/bitmap-quality-banding-and-dithering/


Reply to this email directly or view it on GitHub
#301 (comment).

@sjudd sjudd closed this as completed in 2664bd8 Jan 20, 2015
@sjudd sjudd added this to the 3.5 milestone Jan 21, 2015
@StephenMilone
Copy link

I'm experiencing this too but not with gifs. I'm using JPGs, but doing a transform (into black/white using Bitmap.Config.ARGB_8888) on some images in my list. Looking forward to the update. thanks!

@sjudd sjudd self-assigned this Jan 21, 2015
@sjudd
Copy link
Collaborator

sjudd commented Jan 21, 2015

Turns out there's one additional case we're missing:

We need to reject Bitmaps without ARGB_8888 configs from the BitmapPool, or otherwise ensure they're never added. Despite our requested config type, BitmapFactory may still return Bitmaps with a null config for certain GIFs. It looks like these Bitmaps may still cause this issue.

@sjudd sjudd reopened this Jan 21, 2015
sjudd added a commit to sjudd/glide that referenced this issue Jan 22, 2015
sjudd added a commit to sjudd/glide that referenced this issue Jan 22, 2015
@sjudd sjudd closed this as completed in 3422c57 Jan 22, 2015
sjudd added a commit to sjudd/glide that referenced this issue Feb 9, 2015
Fixes bumptech#335.

Allows us to re-use Bitmaps with configs other
than ARGB_8888. Differs from AttributeStrategy in
that it sorts Bitmaps by config only, and allows
Bitmaps with a given config to be reconfigured to
change their dimensions. In addition, we allow
switching configs between the hidden GIF config
and ARGB_8888 which appears not to trigger bumptech#301.
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

4 participants