cleanup RectTriangles and allow DrawQuads to take triangles or triangle strips

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

(Blocks 2 bugs)

Trunk
mozilla30
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
Posted patch patch (obsolete) — Splinter Review
This implements the above and also optimizes the way we allocate vertices and generally removes a bunch of redundant and repetitive code.
Assignee: nobody → 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.
Summary: optimize RectTriangles to use triangle strips → cleanup RectTriangles and allow DrawQuads to take triangles or triangle strips
Posted patch patch (obsolete) — Splinter Review
Attachment #8380252 - Attachment is obsolete: true
Attachment #8380331 - Flags: review?(vladimir)
Blocks: 975764
Posted patch patch (obsolete) — Splinter Review
Attachment #8380331 - Attachment is obsolete: true
Attachment #8380331 - Flags: review?(vladimir)
Attachment #8384152 - Flags: review?(vladimir)
Posted patch patch (obsolete) — Splinter Review
Attachment #8384152 - Attachment is obsolete: true
Attachment #8384152 - Flags: review?(vladimir)
Attachment #8384157 - Flags: review?(bjacob)
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!
Attachment #8384157 - Flags: review?(bjacob) → review-
Posted patch patch (obsolete) — Splinter Review
Attachment #8384157 - Attachment is obsolete: true
Attachment #8385755 - Flags: review?(bjacob)
The void* pointers seem to still be there! If there is a strong reason for them, please add a comment.
Posted patch patchSplinter Review
Attachment #8385755 - Attachment is obsolete: true
Attachment #8385755 - Flags: review?(bjacob)
Comment on attachment 8385770 [details] [diff] [review]
patch

Will be more apparent with the next patch. I will just fold it in later.
Attachment #8385770 - Flags: review?(bjacob)
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.
Attachment #8385770 - Flags: review?(bjacob) → review+
(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).
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 :)
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac4f9ca4fb27
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Blocks: 978515
You need to log in before you can comment on or make changes to this bug.