Bug 185272 - [WebGL] WEBGL_compressed_texture_astc support
Summary: [WebGL] WEBGL_compressed_texture_astc support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks: 202836
  Show dependency treegraph
 
Reported: 2018-05-03 16:02 PDT by Justin Fan
Modified: 2019-10-10 17:51 PDT (History)
12 users (show)

See Also:


Attachments
Patch (334.93 KB, patch)
2018-05-03 18:35 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (40.60 KB, patch)
2018-05-03 18:59 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-sierra (3.27 MB, application/zip)
2018-05-04 08:26 PDT, Build Bot
no flags Details
Patch (336.06 KB, patch)
2018-05-04 16:25 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (334.43 KB, patch)
2018-05-06 17:36 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (328.38 KB, patch)
2018-05-07 17:21 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (328.98 KB, patch)
2018-05-08 16:51 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (329.00 KB, patch)
2018-05-08 17:13 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (328.33 KB, patch)
2018-05-08 18:32 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (328.32 KB, patch)
2018-05-09 12:44 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2018-05-03 16:02:53 PDT
ASTC compressed textures are not supported in software, although iOS hardware supports it as of the A8 (iPhone 6).
Comment 1 Radar WebKit Bug Importer 2018-05-03 16:03:26 PDT
<rdar://problem/39958547>
Comment 2 Justin Fan 2018-05-03 16:06:29 PDT
<rdar://problem/15745737>
Comment 3 Radar WebKit Bug Importer 2018-05-03 16:07:04 PDT
<rdar://problem/39958669>
Comment 4 Justin Fan 2018-05-03 16:13:25 PDT
<rdar://problem/15745737>
Comment 5 Justin Fan 2018-05-03 18:35:21 PDT
Created attachment 339500 [details]
Patch
Comment 6 Justin Fan 2018-05-03 18:59:50 PDT
Created attachment 339503 [details]
Patch
Comment 7 Build Bot 2018-05-04 08:26:52 PDT
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
Comment 8 Build Bot 2018-05-04 08:26:53 PDT
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
Comment 9 Justin Fan 2018-05-04 11:15:11 PDT
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.
Comment 10 Myles C. Maxfield 2018-05-04 16:16:28 PDT
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?
Comment 11 Justin Fan 2018-05-04 16:25:57 PDT
Created attachment 339607 [details]
Patch
Comment 12 Justin Fan 2018-05-06 17:36:34 PDT
Created attachment 339698 [details]
Patch
Comment 13 Dean Jackson 2018-05-07 13:30:23 PDT
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).
Comment 14 Justin Fan 2018-05-07 17:21:29 PDT
Created attachment 339777 [details]
Patch
Comment 15 Myles C. Maxfield 2018-05-08 15:58:59 PDT
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
Comment 16 Justin Fan 2018-05-08 16:51:29 PDT
Created attachment 339901 [details]
Patch
Comment 17 Tim Horton 2018-05-08 17:00:50 PDT
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.
Comment 18 Myles C. Maxfield 2018-05-08 17:09:57 PDT
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
Comment 19 Justin Fan 2018-05-08 17:13:11 PDT
Created attachment 339905 [details]
Patch
Comment 20 Justin Fan 2018-05-08 18:32:09 PDT
Created attachment 339910 [details]
Patch
Comment 21 Myles C. Maxfield 2018-05-09 12:35:13 PDT
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.
Comment 22 Justin Fan 2018-05-09 12:44:59 PDT
Created attachment 340005 [details]
Patch
Comment 23 WebKit Commit Bot 2018-05-09 13:24:29 PDT
Comment on attachment 340005 [details]
Patch

Clearing flags on attachment: 340005

Committed r231590: <https://trac.webkit.org/changeset/231590>
Comment 24 WebKit Commit Bot 2018-05-09 13:24:31 PDT
All reviewed patches have been landed.  Closing bug.