963268 - Optimize snapshot usage

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=962517#c0 for the background information.

For example, snapshots during app switching are creating SkiaGL canvas - this takes more memory and doesn't actually give us performance benefit.  Bug 884226 introduced a flag willReadFrequently that we already used to fix bug 939962.  This lets us specify that we don't want SkiaGL canvas and helps us with memory usage, without the performance penalty (in fact, with performance improvements.)

Probably the wrong component, can somebody pick the right one?

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

We should really just demote to a software canvas immediately whenever DrawWindow is used. Shouldn't need the flag for this. If DrawWindow is the first operation, we don't even need to demote -- just the thing that prevents a hardware canvas from being created.

Comment 2

[Andreas Gal :gal]

We have to make sure we don't create a GLContext if you want to automate this. If you touch gl you force GL drivers to be loaded into the content process. I would like to review the patch for this bug.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

My plan isn't going to work, because the snapshotter calls scale() before drawWindow(), which causes a GL context to be created first. Lose. We need a more sophisticated optimization for this, which I outlined in bug 963559. I'm putting this back in Gaia so we can do the willReadFrequently hack (as much as I hate it).

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

To clarify why :snorp doesn't like it - while the end result is what we want (we don't create GL canvas), in this case the reason for it isn't really because we "willReadFrequently", so it feels a bit hacky.  It would still give us the behavior we want.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> To clarify why :snorp doesn't like it - while the end result is what we want
> (we don't create GL canvas), in this case the reason for it isn't really
> because we "willReadFrequently", so it feels a bit hacky.  It would still
> give us the behavior we want.

Right. I would almost rather have another 'willUseDrawWindow' flag.

Additionally, I would really like to have an entirely separate drawWindow API that isn't reliant on canvas at all. I could just return a blob that you can then draw to a canvas if you wish, but you could also just inspect the buffer like we do for reftest, or stick it in an <img>, etc.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Probably worth filing a separate bug so that this thought doesn't get lost.

Comment 7

[Andreas Gal :gal]

Yeah, I agree with comment 5. We should split snapshotting. Window could have a chrome only API on it. In the meantime please drive the gaia work. We need this fixed now. We should have immediately done that instead of exploring the hard route first. Lets be incremental :)

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

I haven't been working on this, a Gaia person should pick this up and add the 'willReadFrequently' flag as in bug 939962.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: canvas-no-libegl.patch

Hi snorp, we actually do the screenshot on the chrome side (and send back a blob).

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8369559 [details] [diff] [review]
canvas-no-libegl.patch

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

Looks good

Comment 11

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/ed9eececa94c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/27ba9a44c81e

Comment 14

[Joe Cheng [:jcheng] (please needinfo)]

marking 1.3T+ as it's in tarako branch