516320 - mitigate image flickering as a result of asynchronous discarded image re-decoding

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Justin Dolske [:Dolske]]

One unfortunate side effect of decode-on-draw is that images flicker into view the first time they're drawn. That's the intentional goal of the original bug (from a memory/perf POV), but results in poor user experience when the page you *just* loaded starts flickering as you scroll through it. The browser should be a bit smarter about proactively decoding images that are likely to be soon viewed by the user.

A few rough, random thoughts for things that could help:

* Only enable decode-on-draw for mobile, where the benefits outweigh the UX cost.
* Always decode all images for the active tab
* Decode images when they're close to being scrolled into view (eg, within +/- 1 window height of the current viewport)
* Keep the most recent N million pixels of decoded images in memory before discarding
* Images that are visible in background tabs should be discarded less aggressively (or never) than images than images scrolled out of view
* Track the number of times an image has been decoded (or painted?), avoid discarding images that have high counts.

Comment 1

[Justin Dolske [:Dolske]]

(Requesting blocking193, on the assumption decode on draw will be reenabled for 193.)

Comment 2

[Bobby Holley (:bholley)]

I'm pretty sure that requesting decoding and locking for everything in the active tab should do most of what we need here. I think that progressive redecoding when switching tabs is acceptable. However, we'll need to figure out a strategy for mobile, since they draw everything using that weird canvas browser setup.

Anyway, I'll get back to the "cosmetic stuff" once perf and crashers are fixed. ;-)

Comment 3

[Justin Dolske [:Dolske]]

Just handing the active tab probably won't be enough. If you're flipping back and forth between two tabs (say, Gmail and something else), you'd still get flicker each time.

Comment 4

[Bobby Holley (:bholley)]

(In reply to comment #3)
> Just handing the active tab probably won't be enough. If you're flipping back
> and forth between two tabs (say, Gmail and something else), you'd still get
> flicker each time.

Well, you'd flicker if you waited long enough for images to be discarded (this is an issue with the new way we're handling discarding, not decode on draw). IMO, this is vastly better behavior than just hanging the whole browser while the discarded image synchronously redecodes, and I think joe agrees. Try loading a big image on firefox 3.5 tabbing away from it for 15 seconds, and then tabbing back to see what I mean.

Also, FWIW, we'll probably get rid of a lot of the discard flickers with bug 511899.

Comment 5

[Justin Dolske [:Dolske]]

Sure, flickering is better than hanging, but the point is that the flicker is still undesirable. So if we can be smart about sometimes suppressing discarding or decoding when it's not noticeable, then we should do that.

Comment 6

[Bobby Holley (:bholley)]

(In reply to comment #5)
> Sure, flickering is better than hanging, but the point is that the flicker is
> still undesirable. So if we can be smart about sometimes suppressing discarding
> or decoding when it's not noticeable, then we should do that.

Agreed. However, we need to be very careful about this. Image discarding was a pretty big memory win when it was first introduced IIRC (and a memory win that's not so much measurable on Tp), and it was enough to justify the hang through at least 2 major releases of firefox. So we need to be sure that we don't regress the memory savings.

CCing stuart on this. Stuart, how did you measure the image discarding stuff when you landed it?

Comment 7

[Jim Jeffery not reading bug-mail 1/2/11]

I hope the 'flicker' is not going to be there in the final product.  A flicker for those with eye problems is very irritating and will force many away from Firefox, even me...

Comment 8

[Joe Drew (not getting mail)]

This doesn't block unless we turn on decode-on-draw for 1.9.3, which seems unlikely.

Comment 9

[Joe Drew (not getting mail)]

This blocks beta 5, the feature freeze.

Comment 10

[Bobby Holley (:bholley)]

This bug is really about discarding, which we just turned back on. Bug 512260 should fix the flicker issue.

Resolving this fixed. If anyone experiences any other flicker issues, file a bug and CC me.

Comment 11

[Bobby Holley (:bholley)]

I'm re-opining this, as I think there's some more stuff we can do here.

Comment 13

[Bobby Holley (:bholley)]

We definitely want to have discarding on (big memory wins), and I'm pretty sure we don't want to go back to the hang-y behavior of 3.6 and before. Still, I think the UX on mozilla-central isn't as good as it could be. I just talked to beltzner, limi, jonath, and joe over in #ux and we came up with some ideas. Here's a brain dump of what I'm going to be digging into (some of them may subsume others):

-JPEG scanlines shouldn't be seen going all the way down. If the image is already painted, it should be seamless. I'm guessing we're off-by-one or something here.
-It looks like we have some problems with transparency (the mozillazine banner, for example, shouldn't flash white as it redecodes)
-We should disable discarding when it isn't a big win (bug 511899).
-Right now, thanks to bug 512260, we call RequestDecode on all the images when we tab over. This causes them to redecode, sending out invalidations. However, with retained layers, we actually have the image there already. So in those cases we shouldn't be sending out invalidations that mess up a perfectly good retained layer - we should redecode silently.
-We should be operating on _much_ larger chunks (ie, much less fine-grained progressive decoding). I'm pretty sure that's to blame for slow decoding.
-We should also possible not progressively redecode at all. Images could just pop into view when they're done.
-We should consider lengthening the discard timeout.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #13)
> -Right now, thanks to bug 512260, we call RequestDecode on all the images when
> we tab over. This causes them to redecode, sending out invalidations. However,
> with retained layers, we actually have the image there already. So in those
> cases we shouldn't be sending out invalidations that mess up a perfectly good
> retained layer - we should redecode silently.

You can invalidate with the INVALIDATE_NO_THEBES_LAYERS flag to force the screen to be updated without disturbing the ThebesLayer.

Comment 15

[[e-mail]]

Bobby, should this be assigned to you? Having an unassigned b5 blocker is scary. :)

Comment 16

[Joe Drew (not getting mail)]

Yeah, Bobby's working on this.

Comment 17

[Justin Dolske [:Dolske]]

(In reply to comment #13)
> I just talked to
> beltzner, limi, jonath, and joe over in #ux and we came up with some ideas.
> Here's a brain dump of what I'm going to be digging into (some of them may
> subsume others):

I'll mention one item from comment 0 again:

* Decode images when they're close to being scrolled into view (eg, within +/-
1 window height of the current viewport)

I would not be surprised if this is a hard problem to solve precisely, but it's one of the biggest UX issues I remember from when this first landed... Scrolling through a page where images have been discarded causes them to pop into view as they're re-decoded, which destroys the feeling of scrolling a static, physical page.

Comment 18

[Bobby Holley (:bholley)]

(In reply to comment #17)

> I'll mention one item from comment 0 again:
> 
> * Decode images when they're close to being scrolled into view (eg, within +/-
> 1 window height of the current viewport)
> 
> I would not be surprised if this is a hard problem to solve precisely, but it's
> one of the biggest UX issues I remember from when this first landed...
> Scrolling through a page where images have been discarded causes them to pop
> into view as they're re-decoded, which destroys the feeling of scrolling a
> static, physical page.

This should be a non-issue. We're not discarding _anything_ on the active page anymore. Are you seeing such behavior on trunk? If so please ping me and/or file a bug.

Comment 19

[Justin Dolske [:Dolske]]

(In reply to comment #18)

> This should be a non-issue. We're not discarding _anything_ on the active page
> anymore.

I haven't checked on current trunk, but can't this still happen when a tab's in the background (thus, images discarded), and then the user switches to it and starts scrolling?

Comment 20

[Bobby Holley (:bholley)]

(In reply to comment #19)
> (In reply to comment #18)

> 
> I haven't checked on current trunk, but can't this still happen when a tab's in
> the background (thus, images discarded), and then the user switches to it and
> starts scrolling?

As soon as we switch to a tab, we instantly kick off a decode of anything in that new tab. So the only issue here is to make it less obvious that that's happening. By the time they get around to scrolling, everything in the PresShell should be decoded.

Comment 21

[Mike]

Images from visible area in background tabs should not be discarded (bug 589873). A complex heuristic based on image size, visibility of image, visibility of tab, "backwardness" of tab, should suit the need.

Also, on modern desktops with plenty of memory such micromanagement of images is an overkill. You may win few megs, but who cares? But on mobile devices it does matter, so one extra pref like "discard_images" might help.

Comment 22

[Mike]

(In reply to comment #21)
> Also, on modern desktops with plenty of memory such micromanagement of images
> is an overkill. You may win few megs, but who cares?
Correcting myself: page with lot of images may consume few hundred megs of RAM for uncompressed images.

Comment 23

[Daniel C]

(In reply to comment #13)
> -We should also possible not progressively redecode at all. Images could just
> pop into view when they're done.

I don't know if you were only referring to row by row progression or not, but I thought I'd mention that I've seen progressive JPEGs decode layer by layer when going back to a tab with big images (or when the CPU is already heavily loaded). I don't know if it's possible to request that the decoder skip displaying intermediate stages if it saves time, but I think it would look less visibly distracting when going back to a tab and expecting it to be how you left it.

There is also bug 573948 which could help.

Comment 24

[Justin Dolske [:Dolske]]

(In reply to comment #20)

> As soon as we switch to a tab, we instantly kick off a decode of anything in
> that new tab.

Ah, you're right. That avoids the problem entirely. (At the cost of some extra CPU on tab switch, but that can be optimized separately, if needed).

Comment 25

[Bobby Holley (:bholley)]

Bugs 590252 and 590260 landed, which should fix the flicker (even though there's still work on all this stuff). Resolving fixed, since this is a b5 blocker.