Closed Bug 917505 Opened 11 years ago Closed 8 years ago

Add ETC2 compressed texture format for WebGL

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: bjacob, Assigned: mtseng)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: webgl-extension)

Attachments

(1 file, 5 obsolete files)

No WebGL ext spec yet, but core in WebGL 2

http://www.khronos.org/registry/webgl/specs/latest/2.0/

See this wiki page about our WebGL 2 impl (and in particular, how to turn on webgl2)

https://wiki.mozilla.org/Platform/GFX/WebGL2

General wiki page about impl webgl extensions:

https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions
We should spec this for WebGL 1.
Absolutely. That is sort of what I had unconsciously in mind when I linked to the wiki page on Extensions above ;-)
How do we go about spec'ing this for WebGL 1? Do I take https://www.khronos.org/registry/webgl/extensions/proposals/WEBGL_compressed_texture_etc1/ as an example?
Depends on: 843668
Depends on: 943190
Jeff, I'm looking for feedback on this patch to check whether it's heading in
the right direction. I've spent a lot time working on ETC2 block decompressor
in JS since that is what the S3TC conformance test does.

There is still a lot of work to be done. Off the top of my head:
 * Check sRGB texture interaction
 * "punch through" and ETC2_EAC alpha
 * single and dual channel textures
Attachment #8341350 - Flags: review?(jgilbert)
Attachment #8340978 - Attachment is obsolete: true
(In reply to Dan Glastonbury :djg :kamidphish from comment #5)
> Created attachment 8341350 [details] [diff] [review]
> Add ETC2 compressed texture format to WebGL.
> 
> Jeff, I'm looking for feedback on this patch to check whether it's heading in
> the right direction. I've spent a lot time working on ETC2 block decompressor
> in JS since that is what the S3TC conformance test does.
> 
> There is still a lot of work to be done. Off the top of my head:
>  * Check sRGB texture interaction
>  * "punch through" and ETC2_EAC alpha
>  * single and dual channel textures

Honestly, we should probably just include an array containing the decompressed values in the test. If it passes on different drivers, we can be sure that the decompressed values we include match what we would get from completing the decompression algorithm.
Attachment #8341350 - Flags: review?(jgilbert)
Jeff, Dan, do you think one of you could pick this up again? Modern Android devices support ETC2 via GLES3 (and GLES2 with extensions), so we could really improve the games situation there without requiring them to use webgl2.
Flags: needinfo?(jgilbert)
Flags: needinfo?(dglastonbury)
We can find someone to do this.
This is why I proposed https://www.khronos.org/registry/webgl/extensions/WEBGL_compressed_texture_es3/
Flags: needinfo?(dglastonbury)
Flags: needinfo?(jgilbert)
Is there any work need to be done?
Flags: needinfo?(jgilbert)
Flags: needinfo?(jgilbert)
(In reply to Ethan Lin[:ethlin] from comment #9)
> Is there any work need to be done?

Yes, implement at your leisure.
Assignee: dglastonbury → nobody
Flags: needinfo?(jgilbert)
Attached patch add etc2 support. (obsolete) — Splinter Review
Attachment #8702479 - Flags: review?(jgilbert)
Attachment #8341350 - Attachment is obsolete: true
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Comment on attachment 8702479 [details] [diff] [review]
add etc2 support.

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

::: dom/canvas/WebGLContextExtensions.cpp
@@ +127,5 @@
>          return WebGLExtensionColorBufferFloat::IsSupported(this);
>      case WebGLExtensionID::WEBGL_compressed_texture_atc:
>          return gl->IsExtensionSupported(gl::GLContext::AMD_compressed_ATC_texture);
> +    case WebGLExtensionID::WEBGL_compressed_texture_es3:
> +        return gl->IsExtensionSupported(gl::GLContext::ARB_ES3_compatibility);

You have to add this to the 'drafts' switch, not this main one. (r-)

::: dom/canvas/test/webgl-conformance/conformance/extensions/00_test_list.txt
@@ +3,5 @@
>  oes-texture-float.html
>  oes-vertex-array-object.html
>  webgl-debug-renderer-info.html
>  webgl-debug-shaders.html
> +webgl-compressed-texture-es3.html

Add this to webgl-mochitest until we can upstream it and pull it down.

::: dom/canvas/test/webgl-conformance/conformance/extensions/webgl-compressed-texture-es3.html
@@ +92,5 @@
> +
> +debug("");
> +
> +var wtu = WebGLTestUtils;
> +var canvas = document.getElementById("canvas");

Don't bother attaching to an existing canvas unless you want it to be seen for some reason. Just use:
var canvas = document.createElement('canvas')

@@ +98,5 @@
> +var program = wtu.setupTexturedQuad(gl);
> +var ext = null;
> +var vao = null;
> +var validFormats = {
> +    COMPRESSED_R11_EAC_WEBGL                        : 0x9270,

No _WEBGL suffix.

@@ +443,5 @@
> +function copyRect(data, srcX, srcY, dstX, dstY, width, height, stride) {
> +  var bytesPerLine = width * 4;
> +  var srcOffset = srcX * 4 + srcY * stride;
> +  var dstOffset = dstX * 4 + dstY * stride;
> +  for (; height > 0; --height) {

Just use var jj like normal.

@@ +591,5 @@
> +        glErrorShouldBe(gl, gl.INVALID_OPERATION, "invalid offset");
> +    }
> +
> +    if (width < 32 && height < 32)
> +    {

) {

@@ +636,5 @@
> +    img.src = c.toDataURL();
> +    return img;
> +}
> +function compareRect(
> +        actualWidth, actualHeight, actualChannels,

function compareRect(actualWidth, actualHeight, actualChannels,

Also newline between } and function

@@ +638,5 @@
> +}
> +function compareRect(
> +        actualWidth, actualHeight, actualChannels,
> +        dataWidth, dataHeight, expectedData,
> +        testData, testFormat) {

|                  testData, testFormat)
|{

@@ +646,5 @@
> +
> +    var div = document.createElement("div");
> +    div.className = "testimages";
> +    insertImg(div, "expected", makeImage(
> +            actualWidth, actualHeight, dataWidth, expectedData,

insertImg(div, "expected", makeImage(actualWidth, actualHeight,
                                     dataWidth, expectedData,...

Really just pull the makeImage retval into a temporary variable. This will keep things clear. There no reason to put this all in the same statement.

@@ +666,5 @@
> +            switch (testFormat) {
> +              case ext.COMPRESSED_SRGB8_ETC2_WEBGL:
> +              case ext.COMPRESSED_SRGB8_PUNCHTHROUGH_ALPHA1_ETC2_WEBGL:
> +              case ext.COMPRESSED_SRGB8_ALPHA8_ETC2_EAC_WEBGL:
> +                  var convertSRGB = val => {

convertFrom/ToSRGB is common enough that it should be pulled out into a proper function, not embedded as a lambda.

@@ +700,5 @@
> +            for (var channel = 0; channel < actualChannels; ++channel) {
> +                diff[channel] = Math.abs(expected[channel] - actual[actualOffset + channel]);
> +            }
> +
> +            if(diff.some(element => element > maxDiffPixel)) {

if<space>(

But there's no need for this block to exist. Just integrate this code into the `for (var channel` block. You won't need the diff array either.

::: dom/canvas/test/webgl-conformance/conformance/resources/etc2-data.js
@@ +1,1 @@
> +var img_4x4_r11_eac = {

Include a comment header that says where you sourced this data. Also call this 'es3' not 'etc2', since the EAC ones sorta aren't ETC2.

::: dom/webidl/WebGLRenderingContext.webidl
@@ +829,5 @@
>  
>  [NoInterfaceObject]
> +interface WEBGL_compressed_texture_es3
> +{
> +    const GLenum COMPRESSED_R11_EAC_WEBGL                           = 0x9270;

These don't have _WEBGL suffixes according to the spec:
https://www.khronos.org/registry/webgl/extensions/WEBGL_compressed_texture_es3/
Attachment #8702479 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> ::: dom/canvas/test/webgl-conformance/conformance/resources/etc2-data.js
> @@ +1,1 @@
> > +var img_4x4_r11_eac = {
> 
> Include a comment header that says where you sourced this data. Also call
> this 'es3' not 'etc2', since the EAC ones sorta aren't ETC2.
All data includes both compressed and decompressed are almost created by myself (I only took the original data from Dan's patch). So I won't leave comment here.
Attached patch add etc2 support v2. (obsolete) — Splinter Review
Attachment #8703511 - Flags: review?(jgilbert)
Attachment #8702479 - Attachment is obsolete: true
(In reply to Morris Tseng [:mtseng] from comment #13)
> (In reply to Jeff Gilbert [:jgilbert] from comment #12)
> > ::: dom/canvas/test/webgl-conformance/conformance/resources/etc2-data.js
> > @@ +1,1 @@
> > > +var img_4x4_r11_eac = {
> > 
> > Include a comment header that says where you sourced this data. Also call
> > this 'es3' not 'etc2', since the EAC ones sorta aren't ETC2.
> All data includes both compressed and decompressed are almost created by
> myself (I only took the original data from Dan's patch). So I won't leave
> comment here.

What, no! Definitely leave a comment saying this. We need to know where this comes from, even if it's from one of us.

Ideally you would even include a high-level guide for how you pulled the data out. (Name of program or which website you got it from)
(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> What, no! Definitely leave a comment saying this. We need to know where this
> comes from, even if it's from one of us.
> 
> Ideally you would even include a high-level guide for how you pulled the
> data out. (Name of program or which website you got it from)
Ok, I got it. I'll add comment in my next patch.
Add comment in es3-data.js.
Attachment #8703926 - Flags: review?(jgilbert)
Attachment #8703511 - Attachment is obsolete: true
Attachment #8703511 - Flags: review?(jgilbert)
Attachment #8703926 - Flags: review?(jgilbert) → review+
:smaug, would you review the webidl changes here? Thanks.
Attachment #8704952 - Flags: review?(bugs)
Attachment #8703926 - Attachment is obsolete: true
Comment on attachment 8704952 [details] [diff] [review]
Add WEBGL_compressed_texture_es3 support v2. (carry r+: jgilbert)

r+ webidl part.
Attachment #8704952 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/c9694372b25e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.