1016086 - Use a single attribute in all shaders and avoid switching attribute configuration while compositing

Status: REOPENED

(Core :: Graphics: Layers, defect)

Comment 1

[Andreas Gal :gal]

Attachment: Part 1: Use a single attribute in all shaders

Comment 2

[Andreas Gal :gal]

Attachment: Part 2: Save/restore attribute state in blit helper

This is needed so we can just setup the attribute configuration for all composition shaders once as we start compositing the frame. The blit helper runs as we upload textures with ContentHostIncremental and can stomp over the attribute state.

Comment 3

[Andreas Gal :gal]

Attachment: Part 4: Setup attribute configuration when we start compositing instead of for every draw command

Comment 4

[Andreas Gal :gal]

This patch significantly improves our GL trace. For most layers we just set texture bindings, set one uniform (layer rect), and then draw.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8428901 [details] [diff] [review]
Part 1: Use a single attribute in all shaders

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

I'm a little hesitant we'll never need any of this sort of thing again, but for now it seems okay.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1502,5 @@
>    NS_ASSERTION(aProg->HasInitialized(), "Shader program not correctly initialized");
>  
> +  const GLuint coordAttribIndex = 0;
> +
> +  mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, mQuadVBO);

If we're almost always going to have this bound, isn't there value in only calling this in BeginFrame?

Comment 6

[Andreas Gal :gal]

Comment 5: look at part 3, that's exactly why I am doing this :)

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 8428904 [details] [diff] [review]
Part 2: Save/restore attribute state in blit helper

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

::: gfx/gl/ScopedGLHelpers.cpp
@@ +375,5 @@
> +     * or alternatively in the internal vertex array state
> +     * for a buffer object.
> +    */
> +
> +    mGL->fGetVertexAttribiv(mAttribIndex, LOCAL_GL_VERTEX_ATTRIB_ARRAY_ENABLED, &mAttribEnabled);

I expect these getters to all be practically free... I do really hope that's true on all drivers though.

Comment 8

[Andreas Gal :gal]

The whole texture 2 texture blit stuff is only used with ContentClientIncremental and only on mac, and there its cheap enough.

Comment 9

[Andreas Gal :gal]

Ping for the last part.

Comment 10

[Andreas Gal :gal]

Landed part 1 to 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e572e6623e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b0a5086a81
https://hg.mozilla.org/integration/mozilla-inbound/rev/450878674b08

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out parts 1-3 in https://hg.mozilla.org/integration/mozilla-inbound/rev/8d095e8cac56 for breaking robocop tests: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=40575241&tree=Mozilla-Inbound 
https://tbpl.mozilla.org/php/getParsedLog.php?id=40575038&tree=Mozilla-Inbound 
https://tbpl.mozilla.org/php/getParsedLog.php?id=40576485&tree=Mozilla-Inbound

Comment 12

[Andreas Gal :gal]

Thanks for the backout.

Comment 13

[Andreas Gal :gal]

I think ReadTexImage stomps over our attribute state. Fix coming.

Comment 14

[Andreas Gal :gal]

Attachment: Part 3: Make sure ReadTexImageHelper doesn't change the attribute state during composition.

Comment 15

[Andreas Gal :gal]

Pushed part 1 and 2.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b455e3d3261
https://hg.mozilla.org/integration/mozilla-inbound/rev/97a6db3c8b0e

Comment 16

[Andreas Gal :gal]

Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/a9263fda991e

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b455e3d3261
https://hg.mozilla.org/mozilla-central/rev/97a6db3c8b0e

Comment 18

[Andreas Gal :gal]

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

Comment 19

[Andreas Gal :gal]

Attachment: Part 4: Setup attribute configuration when we start compositing instead of for every draw command (r=Bas)

Comment 20

[Andreas Gal :gal]

Try server run pending, ready to check in once that passes.

Comment 21

[Andreas Gal :gal]

Assuming this sticks, most draws now require only binding the textures, 1 uniform set (the layer rect, the texture rect is set but filtered since it tends to be (0,0)(1,1)) and then the draw command itself. When switching layer types the program has to be switched in addition.

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Andreas Gal :gal]

We forgot a part here.

Comment 24

[Andreas Gal :gal]

https://hg.mozilla.org/integration/mozilla-inbound/rev/710b11cd8e47

Comment 25

[Phil Ringnalda (:philor)]

That forgotten part would be the part that breaks a bunch of Android robocop tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=40833719&tree=Mozilla-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=40834089&tree=Mozilla-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=40833394&tree=Mozilla-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=40833434&tree=Mozilla-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=40833558&tree=Mozilla-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=40833354&tree=Mozilla-Inbound, https://tbpl.mozilla.org/php/getParsedLog.php?id=40833656&tree=Mozilla-Inbound.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1c15c3937c.

Comment 26

[Andreas Gal :gal]

Part 3 was supposed to have fixed that. Thanks for the backout!

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.