Chris Lord [:cwiiis] Reporter
	Description • 11 years ago

We should probably enable low precision rendering on b2g after we have the tiles backend working, to better avoid checkerboarding (which currently just appears as a blank white area on b2g, I think). This will be especially effective on low-end devices.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 10 years ago

I used a stupidly simple hack to see where we stand with this. The attached patch (in conjunction with enabling progressive painting and setting a tile size of 32x32) does make low-res painting "work" in that I see stuff outside the critical displayport get painted in low res. However it also flickers and repaints a lot more than I would expect while scrolling so that might be something that needs investigation.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 10 years ago

With the above hack and other patches this is what I see - the critical displayport is painted at normal resolution and the rest is in low-res. Pretty much what I hoped for. However, as I scroll, the low-res content disappears and reappears a lot - I would expect it to stay put and only repaint the newly uncovered tiles.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 3 • 10 years ago

A video showing the flickering I was talking about is at http://www.youtube.com/watch?v=hR9tCdX6n88

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 10 years ago

Chris, any theories on what's causing the flickering in the video? I'm wondering if it's related to bug 993554 at all? I was seeing behaviour in the latest Fennec nightly that's halfway between this video and that bug, when scrolling around in textareas so I thought they might be connected somehow.

Also, I was thinking about how we currently set the critical displayport and I would like to change that. The only caller for that currently is Fennec, so it's easy to change. What I would like to do is remove the call from Fennec's browser.js and move it into gecko itself. So instead of the front-end setting both the critical displayport and the displayport, the front-end would just set a displayport. If the low-precision-buffer pref is set to true, then Gecko can use the set displayport as the critical displayport, and use an expanded rect as the displayport. The expansion could be somewhat pref-controlled but would live inside Gecko. If the pref is set to false then it just uses the displayport as the displayport. Does that seem reasonable?

Please also braindump anything else you feel is relevant to this work in this bug before you go on PTO :)

	Chris Lord [:cwiiis] Reporter
	Comment 5 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Chris, any theories on what's causing the flickering in the video? I'm
> wondering if it's related to bug 993554 at all? I was seeing behaviour in
> the latest Fennec nightly that's halfway between this video and that bug,
> when scrolling around in textareas so I thought they might be connected
> somehow.

So; that the content is disappearing from the low precision buffer indicates that either the valid region of the high precision buffer includes that content (though I expect this would cause other issues, so I don't think it's that), the visible region is somehow not including the low precision buffer (very unlikely), the low precision buffer's valid region is incorrectly being cleared (most likely), or the valid region on the low precision buffer itself is getting cleared incorrectly somehow (possible).

The code that controls the valid regions of each buffer is in ClientTiledThebesLayer, in RenderLayer, and relies on the values calculated in BeginPaint. Given that I only just audited BeginPaint, I don't expect the values to be massively off there, but I wouldn't put money on it. The code in RenderLayer, however, is pretty convoluted and I expect there are errors there as the logic is far too complicated for what it's doing.

What it's supposed to do is separate the visible region into a high precision visible region and a low precision visible region. The high precision region is updated first, then the low precision region is updated - the high precision region is removed from the low precision region to stop the same content being drawn twice for no reason. This last step could account for content disappearing while updating.

I would start by sticking in a load of printfs in this code to inspect the various regions and see if the numbers are correct. This code could also be massively simplified too, I think. It might make things easier to hook this output up to some kind of visualisation tool, but that's not something I ever got round to myself...

One problem with this code is that the valid region of the low precision buffer can change, but we don't necessarily update the buffer (if low precision updates are cancelled, the corresponding tiled buffer will not be updated) - there's some slack here to stop the low precision buffer disappearing in cases where really it's invalid, but we're imminently about to get valid content and we want to show something in the interim (think resolution changes), but the logic for this is all pretty poorly specified (sorry). Again, I think a rethink of this section of code would result in something that's much clearer and much more concise.

> Also, I was thinking about how we currently set the critical displayport and
> I would like to change that. The only caller for that currently is Fennec,
> so it's easy to change. What I would like to do is remove the call from
> Fennec's browser.js and move it into gecko itself. So instead of the
> front-end setting both the critical displayport and the displayport, the
> front-end would just set a displayport. If the low-precision-buffer pref is
> set to true, then Gecko can use the set displayport as the critical
> displayport, and use an expanded rect as the displayport. The expansion
> could be somewhat pref-controlled but would live inside Gecko. If the pref
> is set to false then it just uses the displayport as the displayport. Does
> that seem reasonable?

Yes, as it happens this is pretty much the exact same idea I'd had to implement this for b2g. Nice :)

> Please also braindump anything else you feel is relevant to this work in
> this bug before you go on PTO :)

A lot of that is above, but I'll do a summary. This is how progressive rendering works and what it's trying to do, without reference to the code:

- Split the displayport into low and high precision regions
- Render the high precision region with top priority, making sure to batch updates in a way that won't produce visible coherency errors (think animated thebes content across tile boundaries)
- Allow cancelling of rendering in the situation that the user has initiated some action (scrolling, zooming, navigation) that has invalidated the content mid-render
- Render the low precision region at a low priority, with no regards to visual coherency and fast cancelling

For the rendering side, we're trying to:

- Render the low precision content that intersects with the layer's visible region, but subtracting the high precision valid region
- Render high precision content (there should be no need for any tests wrt intersection of the visible region, as this should have been rendered with top priority and remain coherent)


This code has not survived various other code changes very well, and I think in those cases, on the whole, we'd be served better by rewriting it than by trying to fix it.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 10 years ago

With the patches on bug 1001438 this is all that should be needed.

	Chris Lord [:cwiiis] Reporter
	Comment 7 • 10 years ago

Comment on attachment 8413805 [details] [diff] [review]
Patch to enable low-res buffer

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

As a patch that turns this feature on, this looks good. Whether we should actually turn it on is another question, but I'm assuming the due diligence has been performed - This r+ assumes that all the blocking bugs for this bug are fixed first (which I think will be the case).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 9 • 10 years ago

Push backed out since one of the changesets within caused B2G OOP reftest crashes:
https://tbpl.mozilla.org/php/getParsedLog.php?id=40192551&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/edaa8aff6cc9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b07bc27f33a

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 10 years ago

Clean try: https://tbpl.mozilla.org/?tree=Try&rev=fd70b91edae7

Re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1bd5190442

	Carsten Book [:Tomcat]
	Comment 11 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/5e1bd5190442

	Phil Ringnalda (:philor)
	Comment 12 • 10 years ago

Backed out in https://hg.mozilla.org/mozilla-central/rev/962f3203939f since it didn't play well with OOP reftests.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 13 • 10 years ago

Re-landed:

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

For reference the most recent try pushes are at:
https://tbpl.mozilla.org/?tree=Try&rev=1bea98312267
https://tbpl.mozilla.org/?tree=Try&rev=47ce44eb72ec

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 10 years ago

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