Last Comment Bug 733894 - Refactor LayerManagerOGLProgram.h
: Refactor LayerManagerOGLProgram.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla15
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on:
Blocks: GPU-clipping-rounded
  Show dependency treegraph
 
Reported: 2012-03-07 13:38 PST by Nick Cameron [:nrc]
Modified: 2012-05-04 02:41 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (64.17 KB, patch)
2012-03-07 19:34 PST, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
updated patch (64.28 KB, patch)
2012-03-15 21:31 PDT, Nick Cameron [:nrc]
jgilbert: feedback+
Details | Diff | Splinter Review
updated patch (65.35 KB, patch)
2012-03-19 12:29 PDT, Nick Cameron [:nrc]
bgirard: review+
ncameron: feedback+
joe: feedback+
Details | Diff | Splinter Review
updated patch (68.15 KB, patch)
2012-04-30 16:46 PDT, Nick Cameron [:nrc]
bgirard: review+
Details | Diff | Splinter Review
updated patch (68.15 KB, patch)
2012-04-30 20:26 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-03-07 13:38:32 PST
The classes in LayerManagerOGLProgram.h reflect the OGL shader programs used for implementing the OGL layers backend. Any shader program with different uniforms or attributes requires its own class. This is efficient and statically prevents passing the wrong uniform/attribute to a shader, However, it means we can't add orthogonally add functionality by adding shaders without duplicating a lot of code both in LayerManagerOGLProgram.h and in other places (e.g., http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#220).

I propose replacing this system with a single class for all shader programs, with an instantiation for each progam. We lose a little static safety (which can all be recovered, albeit dynamically, with assertions) and a few cycles during OGL initialisation; but we gain flexibility and more reuse.
Comment 1 Nick Cameron [:nrc] 2012-03-07 14:45:32 PST
One thing I'm not sure about is the binding of common attributes (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGLProgram.h#207). I don't know why it is necessary and I'm not sure it is consistent, in particular there is VertexAttrib and VertexCoordAttrib defined separately in the current version, and I see no reason why one is used rather than the other or any assurance that they must refer to the same index.
Comment 2 Nick Cameron [:nrc] 2012-03-07 19:34:57 PST
Created attachment 603963 [details] [diff] [review]
patch

No rush for a proper review, but would be good to know if this is a good idea, or a disaster which will break lots of things.
Comment 3 Nick Cameron [:nrc] 2012-03-15 21:31:21 PDT
Created attachment 606448 [details] [diff] [review]
updated patch
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-18 18:55:54 PDT
Maybe bjacob or BenWa would be better reviewers here?
Comment 5 Jeff Gilbert [:jgilbert] 2012-03-18 21:38:51 PDT
Comment on attachment 606448 [details] [diff] [review]
updated patch

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

While this wasn't requested, I thought I'd go through and look at the changes.
I'm not familiar enough with OGL Layers code to actually review this, but these changes look pretty straight-forward and in a good direction. I think BenWa might be the best person to review this, though.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +194,5 @@
>  LayerManagerOGL::Initialize(nsRefPtr<GLContext> aContext, bool force)
>  {
>    ScopedGfxFeatureReporter reporter("GL Layers", force);
>  
>    // Do not allow double intiailization

Can we fix this 'intiailization', too?

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +185,4 @@
>    {
>      if (aIsRGB) {
>        return aOpaque
> +        ? GetProgram(gl::RGBXLayerProgramType)

Since always call the same GetProgram func now, can we branch to choose the format, then call GetProgram(format) after the branches?
Comment 6 Nick Cameron [:nrc] 2012-03-19 12:29:26 PDT
Created attachment 607260 [details] [diff] [review]
updated patch
Comment 7 Nick Cameron [:nrc] 2012-03-19 12:31:08 PDT
> ::: gfx/layers/opengl/LayerManagerOGL.h
> @@ +185,4 @@
> >    {
> >      if (aIsRGB) {
> >        return aOpaque
> > +        ? GetProgram(gl::RGBXLayerProgramType)
> 
> Since always call the same GetProgram func now, can we branch to choose the
> format, then call GetProgram(format) after the branches?

I made both changes, but wasn't sure why you wanted this one, could you explain for me please?
Comment 8 Jeff Gilbert [:jgilbert] 2012-03-19 17:14:24 PDT
We have four 'GetProgram' calls here, each only differing by the arg they're called with. Since we know we're always calling GetProgram, we should just figure out which type we want to use, then call based on the format. This change separates the logic of format selection from the usage.
Comment 9 Benoit Girard (:BenWa) 2012-03-21 07:29:07 PDT
The patch looks good to do the changes you described but I'm not sold on why it's preferable. The argument you gave is code duplication. I feel a bit reluctant to sacrifice static safety to make it easier to add shaders in the future.

I'll discuss it with other GFX peers and see what they think.

As a heads up I have a patch in bug 712716 that will add optional shaders (shaders that we can try to compile and will ignore if they fail because the required extension is not present)
Comment 10 Nick Cameron [:nrc] 2012-03-21 13:44:05 PDT
(In reply to Benoit Girard (:BenWa) from comment #9)
> The patch looks good to do the changes you described but I'm not sold on why
> it's preferable. The argument you gave is code duplication. I feel a bit
> reluctant to sacrifice static safety to make it easier to add shaders in the
> future.
> 
> I'll discuss it with other GFX peers and see what they think.

Thanks for the comments. Assuming we want to do things like Bug 716439, or other things that require more shaders, in particular where we might duplicate each shader, we would get a lot of code duplication with the current setup. This patch brings OGL into line with the DirectX backends in terms of static/dynamic setup of shader programs. I'm pretty sure that all errors that could be prevented before, can be prevented now by assertions and that there is enough coverage in our tests to exercise them.
> 
> As a heads up I have a patch in bug 712716 that will add optional shaders
> (shaders that we can try to compile and will ignore if they fail because the
> required extension is not present)

Will this be easier or harder with this patch?
Comment 11 Nick Cameron [:nrc] 2012-03-21 13:48:30 PDT
(In reply to Nick Cameron from comment #10)
> (In reply to Benoit Girard (:BenWa) from comment #9)
> > The patch looks good to do the changes you described but I'm not sold on why

> > As a heads up I have a patch in bug 712716 that will add optional shaders
> > (shaders that we can try to compile and will ignore if they fail because the
> > required extension is not present)
> 
> Will this be easier or harder with this patch?

Just had a look at your patch, I don't think it will be a problem to integrate the two things, thanks for the heads up.
Comment 12 Benoit Girard (:BenWa) 2012-03-22 06:47:11 PDT
Comment on attachment 607260 [details] [diff] [review]
updated patch

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

The change looks good to me.

I just want to make sure that Joe also agrees. Joe, this patch here will remove types for each shader and instead uses a generic ShaderProgramOGL and DEBUG assert. The aim is to make it easier to add clipping shaders. What do you think?

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1056,5 @@
>    for (size_t lpindex = 0;                                  \
>         lpindex < ArrayLength(sLayerProgramTypes);           \
>         ++lpindex)                                           \
>    {                                                         \
> +    ShaderProgramOGL *vname = static_cast<ShaderProgramOGL*>        \

Fix \ spacing

::: gfx/layers/opengl/LayerManagerOGLProgram.h
@@ +50,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +/**
> + * This struct represents the shaders that make up a program and the unform

unform->uniform

@@ +178,5 @@
>    }
>  
> +  /**
> +   * The following set of methods set a uniform argument to the shader program.
> +   * Not all uniforms may be set for all programs, and such uses will through

through->throw
Comment 13 Joe Drew (not getting mail) 2012-03-22 09:00:09 PDT
Comment on attachment 607260 [details] [diff] [review]
updated patch

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

LGTM!
Comment 14 Nick Cameron [:nrc] 2012-04-30 16:46:16 PDT
Created attachment 619760 [details] [diff] [review]
updated patch

Rebased and handles tiled buffers, very minor changes.
Comment 15 Benoit Girard (:BenWa) 2012-04-30 19:44:36 PDT
Comment on attachment 619760 [details] [diff] [review]
updated patch

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

I had more time to think about this change and I like it now.
Looks good, r+

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +190,5 @@
> +        format = gl::RGBXLayerProgramType;
> +      } else {
> +        format = gl::RGBALayerProgramType;
> +      }
> +     } else {

Nit: extra space
Comment 16 Nick Cameron [:nrc] 2012-04-30 20:26:49 PDT
Created attachment 619822 [details] [diff] [review]
updated patch

Corrected the spacing nit, carrying r=BenWa
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-02 22:35:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/423015379a53
Comment 18 Ed Morley [:emorley] 2012-05-04 02:41:45 PDT
https://hg.mozilla.org/mozilla-central/rev/423015379a53

Note You need to log in before you can comment on or make changes to this bug.