807143 - images appear and disappear on b2g (fixed by enabling image locking in b2g)

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[David Flanagan [:djf]]

Attachment: reduced test case; apply the patch to your gaia tree

I'm opening this new bug as a continuation of #803049.  That bug was a Gaia smoketest blocker, and I resolved it with a gaia workaround.  But there is something going on with gecko here, and this bug includes a reduced test case to prove it.

I've filed this under core/imagelib, but it might actually have more to do with blobs and bloburls than with the images, so it might be in the wrong category.

Cc'ing Chris because he knows about graphics issues.
Cc'ing Jonas because he knows about blobs and about images.
Cc'ing Andreas because he said to me "I really hope the regression has a bug filed"

I figure that one of the three of you will know who to assign this to and what category to file this under.

Steps to reproduce:

1) Apply the attached patch to your gaia tree. It creates a new app in test_apps/imagebug/

2) make install-gaia

3) Open the imagebug app and see 25 copies of the same image (just a red rectangle in a black square). These images are 512x384 jpegs stored in blobs, accessed via blob urls, and set on the background-image property of a 106x106 li element.  This is all just like in the gallery app.

4) scroll up and down

4a) note that the images take a little while to appear after you scroll.

4b) scroll longer and/or faster and note that the screen just goes white and the images do not appear

4c) note that the app has not crashed. You can bring the images back by:

- tapping the home button and then clicking on the imagebug app again

- press and hold the home button to bring up the task switcher, and then just switch back to the imagebug app

- press the sleep button, then rewake the phone and unlock it.

This is the same problem we were having in the gallery. In that app, I solved it by not using such big thumbnail images.  But the thumbnail image size is not new in the gallery, so there is some sort of regression in gecko, and I think this bears looking into.

Comment 1

[Andreas Gal :gal]

You are _much_ more likely to get a quick resolution if you manage to make a stand-alone testcase for the desktop browser. I know thats almost always hard, and often impossible.

Comment 2

[David Flanagan [:djf]]

I've tried another version of my test app. This time instead of creating the image with a canvas and blobs, I just used 25 image files as part of the app.

The bug symptoms are still present, though not quite as bad. When you pan up and down, they take a while to appear, but the app never gets into the state where the screen just goes white and stays white.  In this case, the bug is just that it takes a while for the images to appear after panning.  And, after they have appeared, there is a repaint and they all flash again.

I'm guessing that something changed to make gecko more aggressively discard images that have scrolled off screen, and now we're seeing the side effect which is that they have to be decoded again when they scroll back on screen?

But there's more to it than that as the blobs test case demonstrates.

I'm not going to try to attach this new version of my test since it includes 25 jpeg files.  But if you want to try it out, you can get it from my 'imagetest' branch: https://github.com/davidflanagan/gaia/tree/imagetest  Look in test_apps/imagebug2

Note that I've done all my testing on an unagi with whatever build auto update put on it today.  The settings app says that the build id is 20121030070823

Comment 3

[David Flanagan [:djf]]

I can't reproduce this in today's Nightly on MacOS, even when I scale the images up to 3000x3000 pixels to try to exert similar memory pressure.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Moar imglib people!

Comment 5

[Justin Lebar (not reading bugmail)]

In b2g.js:

> pref("content.image.allow_locking", false);

Try setting that to true.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm not an imglib people.

And +1 to comment 5.

Comment 7

[Joe Drew (not getting mail)]

More background information:

> pref("content.image.allow_locking", false);

This means that you're *explicitly* trading off memory usage with image flashing. We make this call on mobile because of low memory availability, but it has costs, as we find here.

However:

>  4b) scroll longer and/or faster and note that the screen just goes white and the images do
> not appear

This feels to me like we're not *asking* these images to redraw. This could be due to pan-zoom controller stuff that I have no knowledge of, though. If you put a breakpoint in RasterImage::Draw() and fling, we should hit that breakpoint, request a decode, and then once the decode's done redraw the image.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

apzc isn't used here.

Comment 9

[Justin Lebar (not reading bugmail)]

> This feels to me like we're not *asking* these images to redraw. 

Could it be that we're decoding them and then discarding the result too quickly?

Comment 10

[Joe Drew (not getting mail)]

It's possible, though that should lead to us spinning the CPU in a redraw, redecode, discard, redraw loop.

Comment 11

[Doug Turner (:dougt)]

Let's block on this.

Does this only happen in the browser, or does it also happen in apps?

Comment 12

[Joe Drew (not getting mail)]

Mobile Firefox has shipped with this for years, fwiw. I am not in favour of blocking on this for web content.

Comment 13

[Justin Lebar (not reading bugmail)]

> Let's block on this.

We have a metabug; bug 809077.  Can we please clear the flag on this bug, since it's just one particular manifestation of a general problem?

(In reply to Joe Drew (:JOEDREW! \o/) from comment #12)
> Mobile Firefox has shipped with this for years, fwiw. I am not in favour of
> blocking on this for web content.

And if we were to enable image locking for apps, I bet we'd have even more OOMs with image-heavy apps like the gallery.

I also don't think we should assume that the fix for this bug is better than the requisite medicine.

That said, 5mb may be too low for the decoded images cap.  Increasing that limit will not fix this problem, but may make it happen less often.  Someone who understands how to reproduce this bug: Can you please experiment with different values for image.mem.max_decoded_image_kb and see what value mitigates this problem?

Comment 14

[Justin Lebar (not reading bugmail)]

> We have a metabug; bug 809077.

Actually, let's just dupe bug 809077 here.

Comment 17

[John Shih]

I can help on testing this
Bug 804055 is a good example for this issue
it is about the image display problem in a 3rd party web app
And after I remove the memory limit of image decoding, the problem will gone

Comment 18

[Justin Lebar (not reading bugmail)]

(In reply to John Shih from comment #17)
> I can help on testing this
> Bug 804055 is a good example for this issue
> it is about the image display problem in a 3rd party web app
> And after I remove the memory limit of image decoding, the problem will gone

I'd like to know what is the smallest value we can set max_decoded_image_kb to while avoiding flickering problems.

Comment 19

[John Shih]

work on it

Comment 20

[John Shih]

Hi Justin, 
I found a big performance gape between 7M (7168) and 7.5M (7680) in this web app
when set to 7M, the display problem will still happen and make me not able to play the game
and with 7.5M, everything become normal

but I'm afraid that this may not be a general case.

Comment 21

[Justin Lebar (not reading bugmail)]

>> pref("content.image.allow_locking", false);
>
> This means that you're *explicitly* trading off memory usage with image flashing. We make 
> this call on mobile because of low memory availability, but it has costs, as we find here

Looking through current m-c, I don't believe that anything other than B2G is respecting allow_locking == false.  The only time we read the pref, in nsDocument, we do

  if (XRE_GetProcessType() == GeckoProcessType_Content &&
      !Preferences::GetBool("content.image.allow_locking", true)) {
    return NS_OK;
  }

Fennec uses just one process, so regardless of what the pref is set to on Fennec, we're locking images there.

So I think a reasonable first step here would be to enable locking on B2G and see what happens.  There are lots of knobs we can tweak (and that we might want to add), but let's start with the simplest thing which could possibly work?

Comment 22

[Justin Lebar (not reading bugmail)]

Attachment: Flip the allow_locking pref in B2G

This patch should solve the flickering issues, but it makes us more likely to crash due to OOM, particularly when a single page has a ton of images.

Unfortunately there's no good solution here short of bug 689623, which has been a MemShrink:P1 for upwards of 10 months now, and which will not be fixed for B2G v1.  All we can do without that bug fixed is tweak the trade-off between flickering more often and OOMing more often.  I don't know that this patch is the right trade-off, but it's the simplest thing which could possibly work, so I think it's worth a try before we try something more complex.

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 681182 [details] [diff] [review]
Flip the allow_locking pref in B2G

Review of attachment 681182 [details] [diff] [review]:
-----------------------------------------------------------------

As reasonable a solution as any.

Comment 24

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a74ea50bc4

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0a74ea50bc4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee86a43aee0d

Comment 27

[John Shih]

verified
though the blocks bugs are all resolved (bug 804055, bug 806007, bug 811131)

build info
gaia master : 9e5fb101b199a3c93870cda24e497b83599b5efc
mozilla-aurora : ea8661c0067f