674549 - Flush invalidations on decoded images according to bug 666446's refresh notifications

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Justin Lebar (not reading bugmail)]

One problem we discovered with bug 666352's yielding during image decode after 5ms is that we repaint images this quickly.  This end up being much more expensive than the decode itself.

We should try to use bug 666446's refresh notifications to repaint at the right rate.

Comment 1

[Justin Lebar (not reading bugmail)]

jesup pointed out on IRC that it would probably be helpful to understand exactly why flushing invalidations more often is more expensive than flushing less often.  This might yield a smarter solution than simply decreasing the frequency with which we draw.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Large images are the likely place where this is a win:

The test is "<jlebar> jesup, The testcase is: Load a page with many large JPEGs in a new tab. Discard the images, then switch to the tab."

For large images, time to decode is >> 50ms.  So jlebar's test of drawing every 5ms versus drawing every 50ms means 10x less trips through the Draw code path.

Note that the anim refresh timer is normally ~16ms, so the difference would be much smaller (but may still be significant).  We could also have it draw every N anim refresh times instead of every one - note that the longer between draws once you're over the screen refresh rate (normally 60Hz, sometimes 75-85Hz or more), the more "jerky" it will appear.  Lines will appear in groups.

The final draw on decode done should be done immediately, and not wait for the timer.

Note also that decoding one image at a time instead of all images simultaneously will greatly reduce the number of partial-image draws that occur.  This was the one-worker versus n-workers discussion from bug 666352 (I believe).

It would be interesting to do a differential jprof between two draw rates, but I think the above analysis is a very likely explanation, so it's probably not needed.

Comment 3

[Markus Stange [:mstange]]

(In reply to comment #1)
> jesup pointed out on IRC that it would probably be helpful to understand
> exactly why flushing invalidations more often is more expensive than
> flushing less often.

The problem is that currently an invalidation will always result in a paint by the next trip through the event loop, no matter how long ago the last paint was.
Instead, what should happen is that we limit the repaint rate to 60 FPS and let invalidations accumulate during the time in between. That's the goal of bug 598482.
In fact, bug 598482 should make any work in this bug unnecessary. Unfortunately there's quite a bit of work left until bug 598482 is in a landable state.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

I should note (again) that 60Hz isn't always the best choice, and that on a non-60Hz monitor will cause some slightly odd/annoying behavior, especially when scrolling or animating.

Best would be to match it to the refresh rate, if we can - that will minimize the oddities, though not necessarily produce the lowest-latency display (since it wouldn't be synced to the monitor refresh).

Comment 5

[Joe Drew (not getting mail)]

This will very likely help with bug 698298.

Comment 6

[Justin Lebar (not reading bugmail)]

Note that when decoding previously-discarded images, we only flush invalidations when we're completely done decoding the image.  So this only affects the initial, progressive display of an image while we download it.

Comment 7

[John Givens]

Attachment: same problem

Thanks from conttedeme1977.