Bug 185272 – [WebGL] WEBGL_compressed_texture_astc support

• Component: WebGL
• Status: RESOLVED FIXED

[Justin Fan]

ASTC compressed textures are not supported in software, although iOS hardware supports it as of the A8 (iPhone 6).

[Radar WebKit Bug Importer]

<rdar://problem/39958547>

[Justin Fan]

<rdar://problem/15745737>

[Radar WebKit Bug Importer]

<rdar://problem/39958669>

[Justin Fan]

<rdar://problem/15745737>

[Justin Fan]

Created attachment 339500 [details]
Patch

[Justin Fan]

Created attachment 339503 [details]
Patch

[EWS Watchlist]

Comment on attachment 339503 [details]
Patch

Attachment 339503 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7564245

New failing tests:
imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_menuitem-element.html

[EWS Watchlist]

Created attachment 339545 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Justin Fan]

Failing test was not caused by my patch (also fails on master), but may have found a regression from changes in HistoryItem. Filing a new bug.

[Myles C. Maxfield]

Comment on attachment 339503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339503&action=review

I think you forgot to include the test.

> Source/WebCore/ChangeLog:10
> +        Test: fast/canvas/webgl/webgl-compressed-texture-astc.html

This patch doesn't seem to include the test.

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:81
> +            || context.graphicsContext3D()->getExtensions().supports(ASCIILiteral { "GL_KHR_texture_compression_astc_ldr" }));

Shouldn't we do this in the constructor and convert it to a bitfield?

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5429
> +    case Extensions3D::COMPRESSED_RGBA_ASTC_4x4_KHR:
> +    case Extensions3D::COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR:
> +        {
> +            const int kBlockSize = 16;
> +            const int kBlockWidth = 4;
> +            const int kBlockHeight = 4;
> +            bytesRequired = ((width + kBlockWidth - 1) / kBlockWidth) * ((height + kBlockHeight - 1) / kBlockHeight) * kBlockSize;
> +        }
> +        break;

Can't we do this using a lookup table?

[Justin Fan]

Created attachment 339607 [details]
Patch

[Justin Fan]

Created attachment 339698 [details]
Patch

[Dean Jackson]

Comment on attachment 339698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339698&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:4109
> +		D0A20D572092A0A700E0C259 /* WebGLCompressedTextureASTC.h in Headers */ = {isa = PBXBuildFile; fileRef = D0A20D542092A0A600E0C259 /* WebGLCompressedTextureASTC.h */; };
> +		D0A20D582092A0A700E0C259 /* WebGLCompressedTextureASTC.cpp in Sources */ = {isa = PBXBuildFile; fileRef = D0A20D562092A0A600E0C259 /* WebGLCompressedTextureASTC.cpp */; };

Technically, you could just add this to Sources.txt rather than to the Xcode project. I plan to move all the WebGL stuff into Sources.txt at some point.

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:28
> +#include "config.h"
> +#if ENABLE(WEBGL)
> +#include "WebGLCompressedTextureASTC.h"

Nit: Should be:

#include "config.h"

#include "WebGLCompressedTextureASTC.h"

#if ENABLE(WEBGL)

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:89
> +    {
> +        Vector<String> result;
> +        
> +        if (m_isHDRSupported)
> +            result.append(ASCIILiteral { "hdr" });
> +        if (m_isLDRSupported)
> +            result.append(ASCIILiteral { "ldr" });
> +        
> +        return result;
> +    }

Nit: de-indent

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:95
> +    return RuntimeEnabledFeatures::sharedFeatures().webGLCompressedTextureASTCSupportEnabled()
> +        && (context.graphicsContext3D()->getExtensions().supports(ASCIILiteral { "GL_KHR_texture_compression_astc_hdr" })
> +            || context.graphicsContext3D()->getExtensions().supports(ASCIILiteral { "GL_KHR_texture_compression_astc_ldr" }));

Nit: We don't indent that third line.

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.h:41
> +    static bool supported(const WebGLRenderingContextBase&);
> +private:

Nit: Empty line here.

> LayoutTests/ChangeLog:9
> +        * fast/canvas/webgl/resources/js-test-post.js: Added.

It's weird that we didn't already have these files. Are you sure they are not there?

> LayoutTests/ChangeLog:28
> +        * fast/canvas/webgl/resources/js-test-pre.js: Added.
> +        (window.console.log):
> +        (window.console.error):
> +        (initNonKhronosFramework):
> +        (this.initTestingHarness):
> +        (getUrlOptions):
> +        (typeof.quietMode.string_appeared_here.quietMode):
> +        (nonKhronosFrameworkNotifyDone):
> +        (reportTestResultsToHarness):
> +        (reportSkippedTestResultsToHarness):
> +        (notifyFinishedToHarness):
> +        (bufferedLogToConsole):
> +        (_flushBufferedLogsToConsole):
> +        (enableJSTestPreVerboseLogging):
> +        (description):
> +        (_addSpan):
> +        (debug):
> +        (escapeHTML):
> +        (TestFailedException):

I usually remove all the function names in the ChangeLog, and just leave the line saying you added a file (for JS files).

[Justin Fan]

Created attachment 339777 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 339777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339777&action=review

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.h:43
> +    bool m_isHDRSupported : 1;

I thought the style checker required that these be unsigned ints

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5398
> +        BlockParameters(4, 4, kASTCBlockSize),

You should use { } notation to construct the structs instead of constructors

[Justin Fan]

Created attachment 339901 [details]
Patch

[Tim Horton]

Comment on attachment 339901 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339901&action=review

> Source/WebCore/ChangeLog:5
> +        Hooked up ASTC support in WebGL. Requires OpenGL ES 3 context to work. 
> +        Also added in Khronos' ASTC test from version 1.0.4 beta of their conformance test suite,
> +        although again, this requires OpenGL ES 3 context for WebKit to detect proper support.

Pretty weird to have multiple lines here. This is usually a title, description goes down below the test.

> Source/WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=185272

Radar, also?

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:95
> +        && (context.graphicsContext3D()->getExtensions().supports(ASCIILiteral { "GL_KHR_texture_compression_astc_hdr" })

Maybe slurp context.graphicsContext3D()->getExtensions() into a local? Or maybe not.

[Myles C. Maxfield]

Comment on attachment 339901 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339901&action=review

>> Source/WebCore/ChangeLog:5
>> +        although again, this requires OpenGL ES 3 context for WebKit to detect proper support.
> 
> Pretty weird to have multiple lines here. This is usually a title, description goes down below the test.

Yeah, Tim is right. The prose should go below the radar line

[Justin Fan]

Created attachment 339905 [details]
Patch

[Justin Fan]

Created attachment 339910 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 339910 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339910&action=review

> Source/WebCore/ChangeLog:5
> +        <rdar://15745737>

<rdar://problem/15745737>

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5385
> +    

:(

> LayoutTests/ChangeLog:5
> +        <rdar://15745737>

Ditto.

[Justin Fan]

Created attachment 340005 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 340005 [details]
Patch

Clearing flags on attachment: 340005

Committed r231590: <https://trac.webkit.org/changeset/231590>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.