Closed Bug 731570 Opened 9 years ago Closed 9 years ago

ScrollbarLayer uploads a new texture whenever opacity changes

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cwiiis, Assigned: cwiiis)

Details

Attachments

(2 files, 2 obsolete files)

ScrollbarLayer, instead of using alpha-blending, uploads a new texture with the blending already done. This isn't *too* big a deal, as the texture is only 16x16, but every texture upload during drawing hurts and memory bandwidth is at a premium.

Instead, we should just use a shader and alter the fragment colour with an opacity uniform. Patch incoming.
This patch:
- renames the shader programs in LayerRenderer to make it clear that they're the default, always-applied shaders
- adds LayerRenderer to RenderContext
- makes loadShader public (and static)

This makes it easier to use shaders in Layer sub-classes.
Attachment #601589 - Flags: review?(blyakher.a)
This patch makes ScrollbarLayer use a shader to later opacity instead of regenerating the texture.
Attachment #601590 - Flags: review?(bugmail.mozilla)
Comment on attachment 601589 [details] [diff] [review]
Part 1 - Make it easier to implement shaders in Layer sub-classes

>     public static class RenderContext {
>+        public final LayerRenderer renderer;
>         public final RectF viewport;
[snip]
> 
>-        public RenderContext(RectF aViewport, FloatSize aPageSize, float aZoomFactor,
>+        public RenderContext(LayerRenderer aRenderer, RectF aViewport, FloatSize aPageSize, float aZoomFactor,
>                              int aPositionHandle, int aTextureHandle, FloatBuffer aCoordBuffer) {
>+            renderer = aRenderer;
>             viewport = aViewport;
[snip]
>     private RenderContext createContext(RectF viewport, FloatSize pageSize, float zoomFactor) {
>-        return new RenderContext(viewport, pageSize, zoomFactor, mPositionHandle, mTextureHandle,
>-                                 mCoordBuffer);
>+        return new RenderContext(this, viewport, pageSize, zoomFactor, mPositionHandle,
>+                                 mTextureHandle, mCoordBuffer);
>     }

I think these hunks don't belong in this patch; if anything they should be in the other patch. But really I never liked the idea of the RenderContext; I'd prefer if you just passed the LayerRenderer into the ScrollbarLayer.create function and keep it as an instance variable that way.
Comment on attachment 601590 [details] [diff] [review]
Part 2 - Use a shader in ScrollbarLayer to alter opacity

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

Looks ok to me, but I don't know enough OpenGL to really verify that the shader program is correct. Also see the comment I made on the other patch. r+ if you fold those hunks into this patch, or switch to passing in the renderer to ScrollbarLayer.create instead.
Attachment #601590 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (:kats) from comment #3)
> Comment on attachment 601589 [details] [diff] [review]
> Part 1 - Make it easier to implement shaders in Layer sub-classes
> 
> >     public static class RenderContext {
> >+        public final LayerRenderer renderer;
> >         public final RectF viewport;
> [snip]
> > 
> >-        public RenderContext(RectF aViewport, FloatSize aPageSize, float aZoomFactor,
> >+        public RenderContext(LayerRenderer aRenderer, RectF aViewport, FloatSize aPageSize, float aZoomFactor,
> >                              int aPositionHandle, int aTextureHandle, FloatBuffer aCoordBuffer) {
> >+            renderer = aRenderer;
> >             viewport = aViewport;
> [snip]
> >     private RenderContext createContext(RectF viewport, FloatSize pageSize, float zoomFactor) {
> >-        return new RenderContext(viewport, pageSize, zoomFactor, mPositionHandle, mTextureHandle,
> >-                                 mCoordBuffer);
> >+        return new RenderContext(this, viewport, pageSize, zoomFactor, mPositionHandle,
> >+                                 mTextureHandle, mCoordBuffer);
> >     }
> 
> I think these hunks don't belong in this patch; if anything they should be
> in the other patch. But really I never liked the idea of the RenderContext;
> I'd prefer if you just passed the LayerRenderer into the
> ScrollbarLayer.create function and keep it as an instance variable that way.

I meant for these hunks to be in this patch, but I could just pass LayerRenderer in as part of the construction of ScrollbarLayer I suppose. I felt that having it as part of the RenderContext felt nicer, but I don't feel strongly about it, so I'll make this change.
Attachment #601589 - Flags: review?(blyakher.a) → review?(pwalton)
Make change suggested by kats.
Attachment #601589 - Attachment is obsolete: true
Attachment #601919 - Flags: review?(pwalton)
Attachment #601589 - Flags: review?(pwalton)
With kats' suggested change, carrying over r+.
Attachment #601590 - Attachment is obsolete: true
Attachment #601920 - Flags: review+
Comment on attachment 601919 [details] [diff] [review]
Part 1 - Make it easier to implement shaders in Layer sub-classes

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

r=me
Attachment #601919 - Flags: review?(pwalton) → review+
Comment on attachment 601920 [details] [diff] [review]
Part 2 - Use a shader in ScrollbarLayer to alter opacity

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

Here's a drive-by review of the OpenGL stuff.

::: mobile/android/base/gfx/ScrollbarLayer.java
@@ +83,5 @@
> +    private static final String FRAGMENT_SHADER =
> +        "precision mediump float;\n" +
> +        "varying vec2 vTexCoord;\n" +
> +        "uniform sampler2D sTexture;\n" +
> +        "uniform float opacity;\n" +

Call this "uOpacity" to match our other uniforms.

@@ +197,5 @@
> +        mPositionHandle = GLES20.glGetAttribLocation(mProgram, "vPosition");
> +        mTextureHandle = GLES20.glGetAttribLocation(mProgram, "aTexCoord");
> +        mSampleHandle = GLES20.glGetUniformLocation(mProgram, "sTexture");
> +        mTMatrixHandle = GLES20.glGetUniformLocation(mProgram, "uTMatrix");
> +        mOpacityHandle = GLES20.glGetUniformLocation(mProgram, "opacity");

I kinda wish the code for this for vPosition, aTexCoord, sTexture, and uTMatrix could be reused from LayerRenderer somehow.

@@ +210,5 @@
> +                                  LayerRenderer.DEFAULT_TEXTURE_MATRIX, 0);
> +
> +        // Enable the arrays from which we get the vertex and texture coordinates
> +        GLES20.glEnableVertexAttribArray(mPositionHandle);
> +        GLES20.glEnableVertexAttribArray(mTextureHandle);

Ditto here, could you use the code from LayerRenderer a bit more maybe?
Attachment #601920 - Flags: review-
(In reply to Patrick Walton (:pcwalton) from comment #9)
> Comment on attachment 601920 [details] [diff] [review]
> Part 2 - Use a shader in ScrollbarLayer to alter opacity
> 
> Review of attachment 601920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here's a drive-by review of the OpenGL stuff.
> 
> ::: mobile/android/base/gfx/ScrollbarLayer.java
> @@ +83,5 @@
> > +    private static final String FRAGMENT_SHADER =
> > +        "precision mediump float;\n" +
> > +        "varying vec2 vTexCoord;\n" +
> > +        "uniform sampler2D sTexture;\n" +
> > +        "uniform float opacity;\n" +
> 
> Call this "uOpacity" to match our other uniforms.

Will do.

> @@ +197,5 @@
> > +        mPositionHandle = GLES20.glGetAttribLocation(mProgram, "vPosition");
> > +        mTextureHandle = GLES20.glGetAttribLocation(mProgram, "aTexCoord");
> > +        mSampleHandle = GLES20.glGetUniformLocation(mProgram, "sTexture");
> > +        mTMatrixHandle = GLES20.glGetUniformLocation(mProgram, "uTMatrix");
> > +        mOpacityHandle = GLES20.glGetUniformLocation(mProgram, "opacity");
> 
> I kinda wish the code for this for vPosition, aTexCoord, sTexture, and
> uTMatrix could be reused from LayerRenderer somehow.
> 
> @@ +210,5 @@
> > +                                  LayerRenderer.DEFAULT_TEXTURE_MATRIX, 0);
> > +
> > +        // Enable the arrays from which we get the vertex and texture coordinates
> > +        GLES20.glEnableVertexAttribArray(mPositionHandle);
> > +        GLES20.glEnableVertexAttribArray(mTextureHandle);
> 
> Ditto here, could you use the code from LayerRenderer a bit more maybe?

I don't see how we can easily reuse this without rethinking quite a bit, and that's outside the scope of this bug. I'm pushing ahead, please file a bug to be able to share more shader-related code between LayerRenderer and Layer implementations.
http://hg.mozilla.org/projects/maple/rev/181ef4839383
http://hg.mozilla.org/projects/maple/rev/e595d6c1d837
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.