Bug 217198 - Support EXT_texture_compression_rgtc WebGL extension
Summary: Support EXT_texture_compression_rgtc WebGL extension
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Darpinian
URL:
Keywords: InRadar
Depends on: 217424
Blocks: 217761 217813 219219
  Show dependency treegraph
 
Reported: 2020-10-01 17:11 PDT by James Darpinian
Modified: 2020-11-30 13:47 PST (History)
17 users (show)

See Also:


Attachments
Patch (22.47 KB, patch)
2020-10-01 17:12 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (45.66 KB, patch)
2020-10-02 15:43 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (85.76 KB, patch)
2020-10-06 15:35 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (86.21 KB, patch)
2020-10-07 14:41 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (85.85 KB, patch)
2020-10-08 12:02 PDT, James Darpinian
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Darpinian 2020-10-01 17:11:03 PDT
Support EXT_texture_compression_rgtc WebGL extension
Comment 1 James Darpinian 2020-10-01 17:12:51 PDT
Created attachment 410284 [details]
Patch
Comment 2 James Darpinian 2020-10-01 17:15:54 PDT
Will add conformance test tomorrow.
Comment 3 James Darpinian 2020-10-02 15:43:09 PDT
Created attachment 410383 [details]
Patch
Comment 4 Kenneth Russell 2020-10-02 15:54:11 PDT
Comment on attachment 410383 [details]
Patch

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

Looks great! One question. r+

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6739
> +        const int kBlockHeight = 4;

Have you confirmed that these values (from the extension spec) are correct, and that the tests both successfully enable the extension, and pass?
Comment 5 James Darpinian 2020-10-02 16:14:50 PDT
Comment on attachment 410383 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6739
>> +        const int kBlockHeight = 4;
> 
> Have you confirmed that these values (from the extension spec) are correct, and that the tests both successfully enable the extension, and pass?

Yes. Although the existing conformance test doesn't appear to load any images or do any rendering. So I guess we don't truly know if any existing implementation actually works. I will try to find some sample images.
Comment 6 James Darpinian 2020-10-06 15:35:20 PDT
Created attachment 410704 [details]
Patch
Comment 7 James Darpinian 2020-10-06 15:38:39 PDT
Comment on attachment 410704 [details]
Patch

Added very thorough rendering test based on the existing S3TC test.
Comment 8 EWS 2020-10-06 16:23:15 PDT
Committed r268085: <https://trac.webkit.org/changeset/268085>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410704 [details].
Comment 9 Radar WebKit Bug Importer 2020-10-06 16:24:23 PDT
<rdar://problem/70022537>
Comment 10 Aakash Jain 2020-10-07 03:58:22 PDT
(In reply to EWS from comment #8)
> Committed r268085: <https://trac.webkit.org/changeset/268085>
The newly added test: webgl/conformance/extensions/s3tc-and-rgtc.html seems to be consistently failing on ios-wk2. EWS indicated this failure on previous version of this patch. Should have waited for ios-wk2 EWS to complete before landing.

History: https://results.webkit.org/?suite=layout-tests&test=webgl%2Fconformance%2Fextensions%2Fs3tc-and-rgtc.html
Comment 11 WebKit Commit Bot 2020-10-07 03:59:04 PDT
Re-opened since this is blocked by bug 217424
Comment 12 Kenneth Russell 2020-10-07 12:17:53 PDT
From this run:
https://build.webkit.org/results/Apple-iOS-14-Simulator-Release-WK2-Tests/r268117%20(171)/results.html
https://build.webkit.org/results/Apple-iOS-14-Simulator-Release-WK2-Tests/r268117%20(171)/webgl/conformance/extensions/s3tc-and-rgtc-pretty-diff.html

it looks like the bug in the test is that it doesn't handle the case where the extension isn't available:

 1CONSOLE MESSAGE: TypeError: undefined is not an object (evaluating 'ext_rgtc.COMPRESSED_RED_RGTC1_EXT')
Comment 13 James Darpinian 2020-10-07 14:41:04 PDT
Created attachment 410784 [details]
Patch
Comment 14 James Darpinian 2020-10-07 14:45:11 PDT
Sorry for the breakage. The code to check whether the extension was supported was buggy. Will wait for EWS before relanding.
Comment 15 Kenneth Russell 2020-10-07 15:04:32 PDT
Comment on attachment 410784 [details]
Patch

Looks good assuming the revised test has been tested locally when EXT_texture_compression_rgtc is not supported. r+
Comment 16 James Darpinian 2020-10-08 12:02:20 PDT
Created attachment 410868 [details]
Patch
Comment 17 EWS 2020-10-08 17:24:46 PDT
Committed r268234: <https://trac.webkit.org/changeset/268234>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410868 [details].