902300 - Duplicaed glScissor calls when rendering for LayerManagerOGL/ContainerLayerOGL in some scenario

Status: RESOLVED INVALID

(Core :: Graphics: Layers, enhancement)

Description

[leroknv]

Attachment: reduce-glScissor.diff

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.43 Safari/537.31

Steps to reproduce:

Hack on B2G with Apitrace.
1.Wake up the emulator.
2.Check out unlock panel pop up and down.
Wait for a while and check the trace file produced by Apitrace.



Actual results:

There are a number of duplicated glScissor calls(10-15) per frame.


Expected results:

There is no need to call glScissor if scissor rectangle is identical to latest one.

Comment 1

[leroknv]

Attachment: remove redundant glScissor/glViewport calls

Sth wrong with indenting in previous patch. Thanks for your reminder.
Pls check the latest one.

Comment 2

[Guillaume Abadie]

Comment on attachment 789473 [details] [diff] [review]
remove redundant glScissor/glViewport calls

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

Where I can say to you what is wrong in your patch for fixes, unfortunately, I will never be able to r+ this one, because I don't develop in this part of gfx code.

::: gfx/layers/opengl/ContainerLayerOGL.cpp
@@ -272,4 @@
>        continue;
>      }
>  
> -    aContainer->gl()->fScissor(scissorRect.x, 

trailing white-spaces after the ,

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1018,5 @@
> +{
> +   if(!mOldScissor.IsEqualInterior(sRect)){
> +       mGLContext->fScissor(sRect.x, sRect.y, sRect.width, sRect.height);
> +       mOldScissor = sRect;
> +   }

The indentation in this file is 2 white-spaces.

@@ +1038,3 @@
>          mGLContext->fViewport(0, 0, aWidth, aHeight);
> +        mOldViewport = cRect;
> +    }

The indentation in this file is 2 white-spaces.

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +318,5 @@
>    /** Widget associated with this layer manager */
>    nsIWidget *mWidget;
>    nsIntSize mWidgetSize;
> +  nsIntRect mOldScissor;
> +  nsIntRect mOldViewport;

Any initial values in the constructor for those two vars?

You should get them with GL_VIEWPORT and GL_SCISSOR_BOX to be sure that your viewport and scissor tracking exactly match the context's ones, till the beginning.

(http://www.opengl.org/sdk/docs/man/xhtml/glGet.xml)

Comment 3

[Kelsey Gilbert [:jgilbert]]

While this bug is certainly true, these calls are really cheap to make, so we shouldn't care that much that we're being redundant here.

No reason not to take it though, I think. Over to mattwoodrow.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 789473 [details] [diff] [review]
remove redundant glScissor/glViewport calls

feedback- from gabadie

Comment 5

[Matt Woodrow (:mattwoodrow)]

What version of b2g are you testing that is using LayerManagerOGL?

We're using gfx/layers/opengl/CompositorOGL.cpp for everything recent, fixing any potential issues there would be much more useful.

I very much doubt we'd ever uplift changes to LayerManagerOGL into any b2g product that ships, so there isn't much point changing it sorry.

Comment 6

[Gregory Pappas [:gregp]]

Closing per comment 5