Closed Bug 956266 Opened 6 years ago Closed 6 years ago

Don't rebuffer quad vertex + texture coordinates when drawing simple quads in CompositorOGL

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

CompositorOGL::BindAndDrawQuadWithTextureRect currently always calls DrawQuads, which re-submits the coordinates of the drawn quads every time. This is unnecessarily costly if the call to DecomposeIntoNoRepeatTriangles resulted in only one rect.
With tiling enabled, we hit this on every tile, see the trace in bug 949692.

We can use mQuadVBO and a texture transform on the shader in the single-quad case instead.
Attached patch v1 (obsolete) — Splinter Review
Attachment #8356752 - Flags: review?(jgilbert)
Comment on attachment 8356752 [details] [diff] [review]
v1

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

Not sure who the best person to look at this is.
Attachment #8356752 - Flags: review?(jgilbert) → review?(ncameron)
Comment on attachment 8356752 [details] [diff] [review]
v1

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

::: gfx/gl/DecomposeIntoNoRepeatTriangles.h
@@ +32,5 @@
> +    // with a pre-buffered unity quad which has 0,0,1,1 as both vertex
> +    // positions and texture coordinates.
> +    // aOutTextureTransform returns the transform that maps 0,0,1,1 texture
> +    // coordinates to the correct ones.
> +    bool isSimpleQuad(gfx3DMatrix& aOutTextureTransform) const {

nit: is -> Is

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1237,5 @@
>        if (maskType != MaskNone) {
>          BindMaskForProgram(program, sourceMask, LOCAL_GL_TEXTURE1, maskQuadTransform);
>        }
>  
> +      BindAndDrawQuadWithTextureRect(program, source->AsSourceOGL()->GetTextureTransform(),

nit: for each BindAnd... call, if you are putting args on separate lines, put each arg on its on line rather than just some of them.
Attachment #8356752 - Flags: review?(ncameron) → review+
Attached patch v2Splinter Review
Tryserver caught a bug: I wasn't flipping the texture transform in the flip_y case.
Attachment #8356752 - Attachment is obsolete: true
Attachment #8357035 - Flags: review?(ncameron)
Attachment #8357035 - Flags: review?(ncameron) → review+
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff04cca465b
but backed out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c991409dc20

There's an #ifdef MOZ_WIDGET_ANDROID block that uses the textureTransform variable which I removed.
Try push with fixed patch: https://tbpl.mozilla.org/?tree=Try&rev=872c42fc4e56
That came back green, so I pushed to inbound again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/268795df42a1
https://hg.mozilla.org/mozilla-central/rev/268795df42a1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.