Open Bug 1016086 Opened 6 years ago Updated 6 years ago

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


(Core :: Graphics: Layers, defect)

Not set





(Reporter: gal, Assigned: gal)




(4 files, 1 obsolete file)

No description provided.
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.
Assignee: nobody → gal
Attachment #8428901 - Flags: review?(bas)
Attachment #8428904 - Flags: review?(bas)
Attachment #8428905 - Flags: review?(bas)
This patch significantly improves our GL trace. For most layers we just set texture bindings, set one uniform (layer rect), and then draw.
Depends on: 1012407
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?
Attachment #8428901 - Flags: review?(bas) → review+
Comment 5: look at part 3, that's exactly why I am doing this :)
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.
Attachment #8428904 - Flags: review?(bas) → review+
The whole texture 2 texture blit stuff is only used with ContentClientIncremental and only on mac, and there its cheap enough.
Ping for the last part.
Attachment #8428905 - Flags: review?(bas) → review+
Thanks for the backout.
Flags: needinfo?(gal)
I think ReadTexImage stomps over our attribute state. Fix coming.
Attachment #8428905 - Attachment description: Part 3: Setup attribute configuration when we start compositing instead of for every draw command → Part 4: Setup attribute configuration when we start compositing instead of for every draw command
Attachment #8430774 - Flags: review?(bas)
Whiteboard: keep-open
Attachment #8430774 - Flags: review?(bas) → review+
Try server run pending, ready to check in once that passes.
Keywords: checkin-needed
Whiteboard: keep-open
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.
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
We forgot a part here.
Resolution: FIXED → ---
Part 3 was supposed to have fixed that. Thanks for the backout!
You need to log in before you can comment on or make changes to this bug.