956141 - Move y-flipping feature out of GLContext, into CompositorOGL

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Move flipping out of GLContext into CompositorOGL

OpenGL knows natively only one coordinate system, where the y axis is upwards.

But in a browser's layout engine, one naturally wants the y axis to point downwards, as web pages are typically scrolled downwards from "the top of the page".

So, some y-flipping needs to take place, somewhere.

So far, the y-flipping was a feature of GLContext. GLContext had a mFlipped flag defaulting to false, and the compositor set it to true on its GLContext.

GLContext was a bad choice of a place to implement that flipping. Even leaving aside aesthetic or practicality considerations, there was a basic problem: in order to implement such a flipping, one needs to know a "height" so one can compute the (y -> height - y) transformation, and OpenGL contexts do not have a "height" notion. That problem was hacked around by using the viewport height (as in glViewport). That in turn created a deeper problem: glViewport is only meant to specify a coordinate transformation, and using its height to implement the flipping made our GLContext class depart significantly from standard OpenGL semantics. For example, in standard OpenGL, glViewport does not affect glScissor at all, but with GLContext's flipping, it does.

Since the only user of the flipping feature was the GL compositor, it seems natural to move it there. Since some more flipping had to live there anyway, this also has the benefit of putting all the related flipping in one place.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Move flipping out of GLContext into CompositorOGL

Removed a bad change in CopyToTarget which was unneeded and causing reftest-sanity to fail. Now passing reftest-sanity locally.

Comment 2

[Nick Cameron [:nrc]]

Comment on attachment 8355769 [details] [diff] [review]
Move flipping out of GLContext into CompositorOGL

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

Nice.

::: gfx/layers/opengl/CompositorOGL.h
@@ +327,5 @@
>     * Records the passed frame timestamp and returns the current estimated FPS.
>     */
>    double AddFrameAndGetFps(const TimeStamp& timestamp);
>  
> +  GLint FlippedY(GLint y) const { return mHeight - y; }

Could this have a better name and a comment please? It is pretty non-obvious what is going on.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8355769 [details] [diff] [review]
Move flipping out of GLContext into CompositorOGL

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

Thank you so much!

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b75d69931d4a

Comment 5

[Carsten Book [:Tomcat]]

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