Closed Bug 723036 Opened 10 years ago Closed 9 years ago

Port to GLES 2.0

Categories

(Firefox for Android Graveyard :: General, defect, P1)

ARM
Android
defect

Tracking

(firefox14 verified, firefox15 verified, blocking-fennec1.0 beta+)

VERIFIED FIXED
Firefox 14
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- beta+

People

(Reporter: rkd, Assigned: pcwalton)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch file to port to GLES20 (obsolete) — Splinter Review
This is a patch that replaces all of GLES 1.0 with GLES 2.0. Did some preliminary testing on the Transformer eee, Galaxy Nexus, and Galaxy S II. The functionality looks the same as the nightly build.
Attachment #593364 - Flags: review?(pwalton)
Comment on attachment 593364 [details] [diff] [review]
patch file to port to GLES20

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

r=me with comments addressed. Nice work!

::: mobile/android/base/gfx/Layer.java
@@ +91,5 @@
>          }
>      }
>  
>      /** Subclasses override this function to draw the layer. */
> +    public abstract void draw(RenderContext context, int mPositionHandle, int mTextureHandle, FloatBuffer buff);

Could you add the position handle, the texture handle, and the buffer to the RenderContext?

::: mobile/android/base/gfx/LayerRenderer.java
@@ +67,5 @@
> +
> +import android.opengl.GLES20;
> +import java.nio.ByteOrder;
> +import java.nio.FloatBuffer;
> +import org.mozilla.gecko.GeckoAppShell;

nit: Mix these imports into the imports above.

@@ +108,5 @@
> +    private int mTextureHandle;
> +    private int mSampleHandle;
> +    private int mTMatrixHandle;
> +    
> +    // Allocate a ByteBuffer to be used for storing vertice and texture coordinates

nit: vertice -> vertex

@@ +112,5 @@
> +    // Allocate a ByteBuffer to be used for storing vertice and texture coordinates
> +    public static ByteBuffer bb = GeckoAppShell.allocateDirectBuffer(80);
> +    public static FloatBuffer buff;
> +    
> +    // column-major matrix applied to each vertice to shift the viewport from

nite: vertice -> vertex

@@ +134,5 @@
> +    " gl_Position = uTMatrix * vPosition; \n" +
> +    " vTexCoord   = aTexCoord;            \n" +
> +    "}                                    \n";
> +    
> +    // Note we flip the y-coordinate in the fragment shaderto transfer between

nit: "shaderto" -> "shader from"

@@ +149,5 @@
> +    /*
> +     * create a vertex shader type (GLES20.GL_VERTEX_SHADER)
> +     * or a fragment shader type (GLES20.GL_FRAGMENT_SHADER)
> +     */
> +    private int loadShader(int type, String shaderCode){

nit: space before {

::: mobile/android/base/gfx/NinePatchTileLayer.java
@@ +114,4 @@
>      }
>  
> +    private void drawPatch(RenderContext context, int textureX, int textureY, float tileX, float tileY, float tileWidth, float tileHeight, int mPositionHandle, int mTextureHandle, FloatBuffer buff) {
> +        int[] cropRect = { textureX, textureY, PATCH_SIZE, PATCH_SIZE };

This variable seems to be unused and can be removed.

@@ +130,5 @@
> +            
> +        };
> +        //ByteBuffer bb = GeckoAppShell.allocateDirectBuffer(coords.length * 4);
> +        //bb.order(ByteOrder.nativeOrder());
> +        //FloatBuffer buff = bb.asFloatBuffer();

Remove this commented-out code.

::: mobile/android/base/gfx/ScrollbarLayer.java
@@ +76,5 @@
> +    private static final float tex_width = 8.0f;
> +    
> +    // Texture coordinates for the scrollbar's body
> +    // We take a 1x1 pixel from the center of the image and scale it to become the bar
> +    private static final float[] texCoords = {

nit: name this bodyTexCoords

@@ +85,5 @@
> +    (CAP_RADIUS+1)/tex_width, (CAP_RADIUS+1)/tex_height
> +    };
> +    
> +    // Texture coordinates for the top cap of the scrollbar
> +    private static final float[] top_cap_tex = {

nit: topCapTexCoords

@@ +94,5 @@
> +    BAR_SIZE/tex_width, 1.0f
> +    };
> +    
> +    // Texture coordinates for the bottom cap of the scrollbar
> +    private static final float[] bot_cap_tex = {

nit: botCapTexCoords

@@ +103,5 @@
> +    BAR_SIZE/tex_width, 1.0f - CAP_RADIUS/tex_height
> +    };
> +    
> +    // Texture coordinates for the left cap of the scrollbar
> +    private static final float[] left_cap_tex = {

nit: leftCapTexCoords

@@ +112,5 @@
> +    CAP_RADIUS/tex_width, 1.0f
> +    };
> +    
> +    // Texture coordinates for the right cap of the scrollbar
> +    private static final float[] right_cap_tex = {

nit: rightCapTexCoords

@@ +255,2 @@
>                  // top endcap
> +                float[] top_cap = {

nit: topCap

@@ +276,2 @@
>                  // bottom endcap
> +                float[] bot_cap = {

nit: botCap

@@ +298,2 @@
>                  // left endcap
> +                float[] left_cap = {

nit: leftCap

@@ +319,2 @@
>                  // right endcap
> +                float[] right_cap = {

nit: rightCap

@@ +329,5 @@
> +                buff.position(0);
> +                
> +                GLES20.glVertexAttribPointer(mPositionHandle, 3, GLES20.GL_FLOAT, false, 20, buff);       
> +                
> +                buff.position(3);

Put a comment here explaining why we position at 3.

::: mobile/android/base/gfx/SingleTileLayer.java
@@ +106,2 @@
>              cropRect = new int[] { validTexture.left,
> +                validTexture.top,

nit: Maintain the same alignment as before; line up validTexture.top under validTexture.left.
Attachment #593364 - Flags: review?(pwalton) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → blyakher.a
OS: Mac OS X → Android
Hardware: x86_64 → ARM
Also, please try to remove whitespace at the end of lines.
Arkady, this will require post eclair support?
We would only be able to run on phones with API level 8 or above, so Froyo and beyond. The GLES 2.0 libraries were not supported before that.
Attached patch GLES20 patch (obsolete) — Splinter Review
added the handles and buffer used for drawing to the renderContext object.
Attachment #593364 - Attachment is obsolete: true
Attachment #593985 - Flags: review?(pwalton)
https://hg.mozilla.org/projects/maple/rev/6cf9f5c757ae

Will this also land on m-c? This would simplify the Maple merge.
This is current on Maple, which means for us to merge we need to patch reviewed either stand-alone or part of OMTC. Reviewing this standalone is strictly better.

Will this be on reviewed by the end of the week?
Blocks: land-maple
I'll prepare an updated patch shortly.
patrick, this landed on maple.  can you bless the patch and lets close out this bug.
Priority: -- → P1
Comment on attachment 593985 [details] [diff] [review]
GLES20 patch

I made some changes that will need re-review.
Attachment #593985 - Flags: review?(pwalton)
If it's not to much trouble can this patch land on m-c? Since we will likely be reviewing the diff on Maple when merging having this on m-c would reduce the diff.
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED
Can we get this reviewed & landed on m-c sometime soon?
Preparing the patch right now. Cwiiis should review.
Attached patch Proposed patch. (obsolete) — Splinter Review
Here's a version of the patch that reflects what's currently on maple. A simple smoketest seems to confirm that it's working. I believe we want to land this in advance of maple itself landing.
Assignee: blyakher.a → pwalton
Attachment #593985 - Attachment is obsolete: true
Attachment #603091 - Flags: review?(chrislord.net)
Comment on attachment 603091 [details] [diff] [review]
Proposed patch.

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

I'm not happy about the amount of code duplication going on here, but r+ with a follow-up bug to address that.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +186,5 @@
>          mShowFrameRate = false;
> +
> +        // Initialize the FloatBuffer that will be used to store all vertices and texture
> +        // coordinates in draw() commands.
> +        ByteBuffer byteBuffer = GeckoAppShell.allocateDirectBuffer(COORD_BUFFER_SIZE * 4);

Is it possible for the LayerView to be reinstantiated without the app closing? If so, there might be a leak here?

::: mobile/android/base/gfx/NinePatchTileLayer.java
@@ +138,5 @@
> +        int positionHandle = context.positionHandle;
> +        int textureHandle = context.textureHandle;
> +
> +        // Make sure we are at position zero in the buffer in case other draw methods did not clean
> +        // up after themselves

Given that this draw method doesn't clean up after itself, the latter half of this comment is redundant. Layers should just assume that they can't make assumptions about the buffer position.

::: mobile/android/base/gfx/ScrollbarLayer.java
@@ +396,5 @@
> +                coordBuffer.position(3);
> +                GLES20.glVertexAttribPointer(textureHandle, 2, GLES20.GL_FLOAT, false, 20,
> +                                             coordBuffer);
> +
> +                GLES20.glDrawArrays(GLES20.GL_TRIANGLE_STRIP, 0, 4);

This whole section is gigantic compared to what it replaces... Couldn't this have been factored out into a function?

::: mobile/android/base/gfx/SingleTileLayer.java
@@ +120,5 @@
> +
> +        // Texture coordinates are texture_x, texture_y starting at position 3 into the buffer.
> +        coordBuffer.position(3);
> +        GLES20.glVertexAttribPointer(textureHandle, 2, GLES20.GL_FLOAT, false, 20, coordBuffer);
> +        GLES20.glDrawArrays(GLES20.GL_TRIANGLE_STRIP, 0, 4);

And here's the same code again - maybe this should be a static utility function in TileLayer?

::: mobile/android/base/gfx/WidgetTileLayer.java
@@ +147,5 @@
> +
> +        // Texture coordinates are texture_x, texture_y starting at position 3 into the buffer.
> +        coordBuffer.position(3);
> +        GLES20.glVertexAttribPointer(textureHandle, 2, GLES20.GL_FLOAT, false, 20, coordBuffer);
> +        GLES20.glDrawArrays(GLES20.GL_TRIANGLE_STRIP, 0, 4);

And in here too.
Attachment #603091 - Flags: review?(chrislord.net) → review+
Comment on attachment 603091 [details] [diff] [review]
Proposed patch.

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

Sorry for the flip-flop, but I've changed my mind on this - it wouldn't be much work to add a function to remove all that duplication, so let's get it done before landing it on central.
Attachment #603091 - Flags: review+ → review-
Comment on attachment 603091 [details] [diff] [review]
Proposed patch.

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

Sorry for the flip-flop, but I've changed my mind on this - it wouldn't be much work to add a function to remove all that duplication, so let's get it done before landing it on central.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +140,5 @@
> +    // The shaders run on the GPU directly, the vertex shader is only applying the
> +    // matrix transform detailed above
> +    private static final String VERTEX_SHADER =
> +        "uniform mat4 uTMatrix;\n" +
> +        "attribute vec4 vPosition;\n" +

Why is this vPosition and not aPosition?
Patch version 2 addresses review comments.
Attachment #603091 - Attachment is obsolete: true
Attachment #603394 - Flags: review?(chrislord.net)
Comment on attachment 603394 [details] [diff] [review]
Proposed patch, version 2.

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

Looks good. Maybe drawRect ought to be in LayerRenderer rather than Layer, but it doesn't bother me to have it in either. One note, to use drawRect with non-default shader programs will require creating a new RenderContext now, but maybe that's a nice encapsulation.
Attachment #603394 - Flags: review?(chrislord.net) → review+
Since this is already on maple, this bug is fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Verified fixed on:

Firefox 15.0a1 (2012-05-28)
Firefox 14.0a2 (2012-05-28)

Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.