943190 - Implement WEBGL_compressed_texture_etc1

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

There is a webgl spec, but it says not to implement. I've implemented it anyway.

Comment 1

[Kelsey Gilbert [:jgilbert]]

I'll get this moved to Draft.

Comment 2

[u480271]

Attachment: Add ETC1 compressed texture format to WebGL.

Comment 3

[u480271]

Attachment: Add ETC1 compressed texture format to WebGL.

Comment 4

[u480271]

Attachment: Add ETC1 compressed texture format to WebGL.

Jeff, Looking for a sanity check on this one. Reading the spec at
https://www.khronos.org/registry/gles/extensions/OES/OES_compressed_ETC1_RGB8_texture.txt
says that:

INVALID_OPERATION is generated by CompressedTexSubImage2D,
TexSubImage2D, or CopyTexSubImage2D if the texture image <level>
bound to <target> has internal format ETC1_RGB8_OES.

which doesn't feel right.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #4)
> Created attachment 8341366 [details] [diff] [review]
> Add ETC1 compressed texture format to WebGL.
> 
> Jeff, Looking for a sanity check on this one. Reading the spec at
> https://www.khronos.org/registry/gles/extensions/OES/
> OES_compressed_ETC1_RGB8_texture.txt
> says that:
> 
> INVALID_OPERATION is generated by CompressedTexSubImage2D,
> TexSubImage2D, or CopyTexSubImage2D if the texture image <level>
> bound to <target> has internal format ETC1_RGB8_OES.
> 
> which doesn't feel right.

That's what it looks like though. I can think of a couple reasons this might be the case, I guess, though I'm still disappointed. Let's generate a JS warning here, since this is a little surprising.

Comment 6

[Kelsey Gilbert [:jgilbert]]

This extension has been moved to draft, so we're good to implement now.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8341366 [details] [diff] [review]
Add ETC1 compressed texture format to WebGL.

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

Let's do another round.

::: content/canvas/src/WebGLContextGL.cpp
@@ +3400,5 @@
>          return;
>      }
>  
> +    if (format == LOCAL_GL_ETC1_RGB8_OES) {
> +        ErrorInvalidOperation("compressedTexImage2D: texture is LOCAL_GL_ETC1_RGB8_OES");

s/TexImage/TexSubImage/

::: content/canvas/src/WebGLExtensionCompressedTextureETC.cpp
@@ +7,5 @@
> +#include "mozilla/dom/WebGLRenderingContextBinding.h"
> +
> +using namespace mozilla;
> +
> +WebGLExtensionCompressedTextureETC1::WebGLExtensionCompressedTextureETC1(WebGLContext* context)

We should probably also call this file ...TextureETC1, since we *might* be getting a ...TextureETC2, depending on how we name the next one.

::: content/canvas/test/webgl/conformance/extensions/webgl-compressed-texture-etc1.html
@@ +153,5 @@
> +
> +    // check that all format enums exist.
> +    for (name in validFormats) {
> +        var expected = "0x" + validFormats[name].toString(16);
> +        var actual = "ext['" + name + "']";

`actual` looks like it will equal "ext['0']" here. This doesn't seem right, does it?

@@ +233,5 @@
> +        ];
> +
> +    function extend_4to8bits(r, g, b) {
> +        return [
> +            (r & 0xf0) | ((r >> 4) & 0x0f),

This is unusual. Do the four bits of `r` actually live in 0xf0? Usually these are at 0x0f, as if we had a uint4_t type that has been up-cast to a larger int type.

@@ +265,5 @@
> +                                    src[2] + 8 * _deltatable[(src[2] & 0x7)]);
> +            return [ col_1, col_2 ];
> +        }
> +
> +        return [];

Shouldn't this be invalid? (non-I-or-D mode?) If so, we should probably just `throw "Invalid mode: " + mode`.

@@ +414,5 @@
> +      glErrorShouldBe(gl, gl.NO_ERROR, "valid dimensions for level > 0");
> +    }
> +
> +/*
> +    // pick a wrong format that uses the same amount of data.

Just remove this, since there's only one format in this ext.

@@ +437,5 @@
> +*/
> +
> +    gl.compressedTexSubImage2D(gl.TEXTURE_2D, 0, 0, 0, width + 4, height, format, data);
> +//  glErrorShouldBe(gl, gl.INVALID_VALUE, "data size does not match dimensions");
> +    glErrorShouldBe(gl, gl.INVALID_OPERATION, "data size does not match dimensions");

Yep, the spec for ETC1 seems to forbid CompressedTexSubImage calls.

::: content/canvas/test/webgl/conformance/resources/webgl-test-utils.js
@@ +1308,5 @@
> + * @param {string} name Name of extension to look for
> + * @return {string} name of extension found or undefined if not
> + *     found.
> + */
> +var getSupportedExtensionWithKnownPrefixes = function(gl, name) {

Did you write this, or was this pulled down from a newer version of the test suite?

::: gfx/gl/GLContextFeatures.cpp
@@ +48,5 @@
>      },
>      {
> +        "compressed_texture_etc1",
> +        999, // OpenGL version
> +        999, // OpenGL ES version

0 means 'none', actually.

Comment 8

[Kelsey Gilbert [:jgilbert]]

I'm taking this to finish it up.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Implement

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Test

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=37e961f6550a

Comment 12

[u480271]

Comment on attachment 8387976 [details] [diff] [review]
patch 1: Implement

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

The inability to call glCompressedTexSubImage2D seems wrong, but that's what the spec says. :-(

Good to kill all the trailing whitespace in WebGLRenderingContext.webidl.

Comment 13

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/636a4f14af75
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/91c5ede1cb08

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Implement

r=kamidphish
de-bitrotted

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/636a4f14af75
https://hg.mozilla.org/mozilla-central/rev/91c5ede1cb08