Closed
Bug 731570
Opened 12 years ago
Closed 12 years ago
ScrollbarLayer uploads a new texture whenever opacity changes
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: cwiiis, Assigned: cwiiis)
Details
Attachments
(2 files, 2 obsolete files)
8.02 KB,
patch
|
pcwalton
:
review+
|
Details | Diff | Splinter Review |
11.50 KB,
patch
|
cwiiis
:
review+
pcwalton
:
review-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
This patch makes ScrollbarLayer use a shader to later opacity instead of regenerating the texture.
Attachment #601590 -
Flags: review?(bugmail.mozilla)
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Attachment #601589 -
Flags: review?(blyakher.a) → review?(pwalton)
Assignee | ||
Comment 6•12 years ago
|
||
Make change suggested by kats.
Attachment #601589 -
Attachment is obsolete: true
Attachment #601919 -
Flags: review?(pwalton)
Attachment #601589 -
Flags: review?(pwalton)
Assignee | ||
Comment 7•12 years ago
|
||
With kats' suggested change, carrying over r+.
Attachment #601590 -
Attachment is obsolete: true
Attachment #601920 -
Flags: review+
Comment 8•12 years ago
|
||
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 9•12 years ago
|
||
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-
Assignee | ||
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Comment 11•12 years ago
|
||
http://hg.mozilla.org/projects/maple/rev/181ef4839383 http://hg.mozilla.org/projects/maple/rev/e595d6c1d837
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•