975772 - cleanup RectTriangles and allow DrawQuads to take triangles or triangle strips

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andreas Gal :gal]

I am about to use large meshes via the same path we pipe RectTriangles through and we should really be using triangle strips here. Degenerate triangles are used to emit multiple quads as a single triangle strip.

Comment 1

[Andreas Gal :gal]

Attachment: patch

This implements the above and also optimizes the way we allocate vertices and generally removes a bunch of redundant and repetitive code.

Comment 2

[Andreas Gal :gal]

Actually never mind. GL_TRIANGLES is just as compact for this specific use case. I will instead generalize the DrawQuads path so it can do triangles and triangle strips and merely clean up DecomposeIntoNoRepeatTriangles and land that and that unblocks me to use strips where I want to.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Andreas Gal :gal]

Attachment: patch

Comment 5

[Andreas Gal :gal]

Attachment: patch

Comment 6

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

Comment on attachment 8384157 [details] [diff] [review]
patch

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

Lots of great code simplification here, just one blocking nit about the new void* pointers which look like they could be replaced by something more typed, but tell me if I'm missing something.

::: gfx/gl/DecomposeIntoNoRepeatTriangles.cpp
@@ +11,5 @@
>  namespace gl {
>  
> +template <typename T>
> +static void
> +AddTriangle(InfallibleTArray<T>& array, GLfloat x0, GLfloat y0, GLfloat x1, GLfloat y1)

See comment in DecomposeIntoNoRepeatTriangles.h: at this point, we're better off unifying the two 'coord' structs and de-templatifying this function.

Also, the name of this function can't be quite right: this function adds 6 vertices, visibly representing 1 quad subdivided into 2 triangles, so it shouldn't be named "AddTriangle" singular. "AppendRectToCoordArray" would be more to the point?

@@ +42,5 @@
> +    if (mVertexCoords.Length() == 6 &&
> +        mVertexCoords[0].x == 0.0f &&
> +        mVertexCoords[0].y == 0.0f &&
> +        mVertexCoords[5].x == 1.0f &&
> +        mVertexCoords[5].y == 1.0f) {

Brace { on new line after multi-line if(...) condition.

I do like having this computed rather than returning additional state, mIsSimpleQuad and mTextureTransform, that was redundant. Redundant state is evil.

::: gfx/gl/DecomposeIntoNoRepeatTriangles.h
@@ +51,4 @@
>      }
>  
>      typedef struct { GLfloat x,y; } vert_coord;
> +    typedef struct { GLfloat x,y; } tex_coord;

I think at this point it makes more sense to just unify these two structs. I was all for strong typing, but at this point, we're making them usable interchangeably by making AddTriangles templated in the coord type, which negates that.... so better have only one type, and de-templatify AddTriangles in DecomposeIntoNoRepeatTriangles.cpp.

::: gfx/gl/GLBlitTextureImageHelper.cpp
@@ +135,5 @@
>                  DecomposeIntoNoRepeatTriangles(srcSubRect, realTexSize, rects);
>  
>                  // now put the coords into the d[xy]0 .. d[xy]1 coordinate space
>                  // from the 0..1 that it comes out of decompose
> +                RectTriangles::vert_coord* v = (RectTriangles::vert_coord*)rects.vertCoords();

While you're here please remove this C-style cast here.

Since we do have a use for that vertCoords() method returning float* below, we might want to have another, differently named, getter for getting at the array of coord structs; and it would be a lot safer to return a Array& reference than a plain coord* pointer.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +81,5 @@
>            ShaderProgramOGL *aProg,
> +          GLenum aMode,
> +          GLsizei aElements,
> +          void *aVertCoords,
> +          void *aTexCoords)

Oh no, please no new void* pointers here!

Comment 7

[Andreas Gal :gal]

Attachment: patch

Comment 8

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

The void* pointers seem to still be there! If there is a strong reason for them, please add a comment.

Comment 9

[Andreas Gal :gal]

Attachment: patch

Comment 10

[Andreas Gal :gal]

Comment on attachment 8385770 [details] [diff] [review]
patch

Will be more apparent with the next patch. I will just fold it in later.

Comment 11

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

Comment on attachment 8385770 [details] [diff] [review]
patch

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

::: gfx/gl/DecomposeIntoNoRepeatTriangles.cpp
@@ +14,5 @@
> +RectTriangles::AppendRectToCoordArray(InfallibleTArray<coord>& array,
> +                                      GLfloat x0, GLfloat y0,
> +                                      GLfloat x1, GLfloat y1)
> +{
> +    coord* v = array.AppendElements(6);

Rather than operating on a raw pointer like this, it would be safer to use operator[] on 'array' directly, so as to be covered by assertions, but I don't mind strongly.

Comment 12

[Andreas Gal :gal]

(In reply to Benoit Jacob [:bjacob] from comment #11)
> Comment on attachment 8385770 [details] [diff] [review]
> patch
> 
> Review of attachment 8385770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/DecomposeIntoNoRepeatTriangles.cpp
> @@ +14,5 @@
> > +RectTriangles::AppendRectToCoordArray(InfallibleTArray<coord>& array,
> > +                                      GLfloat x0, GLfloat y0,
> > +                                      GLfloat x1, GLfloat y1)
> > +{
> > +    coord* v = array.AppendElements(6);
> 
> Rather than operating on a raw pointer like this, it would be safer to use
> operator[] on 'array' directly, so as to be covered by assertions, but I
> don't mind strongly.

In this particular case its possible to visually confirm correctness. Calling repeatedly overloaded operators that are defined elsewhere and might independently change seems more risky (from a performance perspective in particular).

Comment 13

[Andreas Gal :gal]

This doesn't have the right checkin comment set. I will try to get to that later today but if someone beats me to it and lands it, I owe someone a drink :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Andreas, can you please make sure you have hg configured to generate patches with commit information? Makes life much easier with the checkin-neededs :)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 15

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

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