723036 - Port to GLES 2.0

Status: VERIFIED FIXED

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

Description

[Arkady Blyakher [:rkd]]

Attachment: patch file to port to GLES20

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.

Comment 1

[Patrick Walton (:pcwalton)]

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.

Comment 2

[Patrick Walton (:pcwalton)]

Also, please try to remove whitespace at the end of lines.

Comment 3

[Doug Turner (:dougt)]

Arkady, this will require post eclair support?

Comment 4

[Arkady Blyakher [:rkd]]

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.

Comment 5

[Arkady Blyakher [:rkd]]

Attachment: GLES20 patch

added the handles and buffer used for drawing to the renderContext object.

Comment 6

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/projects/maple/rev/6cf9f5c757ae

Will this also land on m-c? This would simplify the Maple merge.

Comment 7

[Benoit Girard (:BenWa)]

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?

Comment 8

[Patrick Walton (:pcwalton)]

I'll prepare an updated patch shortly.

Comment 9

[Doug Turner (:dougt)]

patrick, this landed on maple.  can you bless the patch and lets close out this bug.

Comment 10

[Patrick Walton (:pcwalton)]

Comment on attachment 593985 [details] [diff] [review]
GLES20 patch

I made some changes that will need re-review.

Comment 11

[Benoit Girard (:BenWa)]

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.

Comment 12

[Joe Drew (not getting mail)]

Can we get this reviewed & landed on m-c sometime soon?

Comment 13

[Patrick Walton (:pcwalton)]

Preparing the patch right now. Cwiiis should review.

Comment 14

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

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.

Comment 15

[Chris Lord [:cwiiis]]

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.

Comment 16

[Chris Lord [:cwiiis]]

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.

Comment 17

[Chris Lord [:cwiiis]]

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?

Comment 18

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

Patch version 2 addresses review comments.

Comment 19

[Chris Lord [:cwiiis]]

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.

Comment 20

[Joe Drew (not getting mail)]

Since this is already on maple, this bug is fixed.

Comment 21

[Cristian Nicolae (:xti)]

Verified fixed on:

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

Device: Galaxy Nexus
OS: Android 4.0.2