Bug 238710

Summary: [WebGPU] Implement Texture view format compatibility
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: WebGPUAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, djg, kkinnunen, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 238711    
Attachments:
Description Flags
Patch
none
Patch
mmaxfield: review-, ews-feeder: commit-queue-
Patch kkinnunen: review+, ews-feeder: commit-queue-

Description Myles C. Maxfield 2022-04-02 23:56:24 PDT
.
Comment 1 Myles C. Maxfield 2022-04-03 00:06:16 PDT
Created attachment 456500 [details]
Patch
Comment 2 Myles C. Maxfield 2022-04-03 00:52:00 PDT
Created attachment 456504 [details]
Patch
Comment 3 Dan Glastonbury 2022-04-03 17:15:13 PDT
Comment on attachment 456504 [details]
Patch

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

> Source/WebGPU/WebGPU/Texture.mm:1119
> +        return std::nullopt;

There appears to be an inconsistency with the logic in this function. If `WGPUTextureFormat_ASTC4x4Unorm` returns `WGPUTextureFormat_ASTC4x4Unorm`, surely these should return themselves? What am I missing?

> Source/WebGPU/WebGPU/Texture.mm:1243
> +    if (canonicalizedFormat1 != canonicalizedFormat1)

Comparison to self?!
Comment 4 Myles C. Maxfield 2022-04-03 23:08:26 PDT
Comment on attachment 456504 [details]
Patch

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

>> Source/WebGPU/WebGPU/Texture.mm:1119
>> +        return std::nullopt;
> 
> There appears to be an inconsistency with the logic in this function. If `WGPUTextureFormat_ASTC4x4Unorm` returns `WGPUTextureFormat_ASTC4x4Unorm`, surely these should return themselves? What am I missing?

Yeah, I see what you mean.

Originally, this function was intended to create equivalence classes via normalization; e.g. RGBA8Unorm and RGBA8UnormSrgb should both be members of the same equivalence class, and this function returns a representative member of that class (sort of like how union set works). ASTC4x4Unorm and ASTC4x4UnormSrgb are in the same equivalence class. I was originally thinking that formats like R8Unorm wouldn't be part of any equivalence class, so they would return nullopt here, but I can also understand where you're coming from about saying R8Unorm should be in an equivalence class of 1. I'll change the implementation to do that.

>> Source/WebGPU/WebGPU/Texture.mm:1243
>> +    if (canonicalizedFormat1 != canonicalizedFormat1)
> 
> Comparison to self?!

🤦🏻‍♂️
Comment 5 Dan Glastonbury 2022-04-03 23:12:38 PDT
Comment on attachment 456504 [details]
Patch

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

>>> Source/WebGPU/WebGPU/Texture.mm:1119
>>> +        return std::nullopt;
>> 
>> There appears to be an inconsistency with the logic in this function. If `WGPUTextureFormat_ASTC4x4Unorm` returns `WGPUTextureFormat_ASTC4x4Unorm`, surely these should return themselves? What am I missing?
> 
> Yeah, I see what you mean.
> 
> Originally, this function was intended to create equivalence classes via normalization; e.g. RGBA8Unorm and RGBA8UnormSrgb should both be members of the same equivalence class, and this function returns a representative member of that class (sort of like how union set works). ASTC4x4Unorm and ASTC4x4UnormSrgb are in the same equivalence class. I was originally thinking that formats like R8Unorm wouldn't be part of any equivalence class, so they would return nullopt here, but I can also understand where you're coming from about saying R8Unorm should be in an equivalence class of 1. I'll change the implementation to do that.

And with that change this function can be `WGPUTextureFormat Text::removeSRGBSuffix(...)`
Comment 6 Myles C. Maxfield 2022-04-03 23:21:07 PDT
Created attachment 456533 [details]
Patch
Comment 7 Kimmo Kinnunen 2022-04-07 02:35:12 PDT
Comment on attachment 456533 [details]
Patch

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

> Source/WebGPU/WebGPU/Texture.mm:2255
> +    bool foundFormat = descriptor.format == m_descriptor.format;

if (descriptor.format != m_descriptor.format && !m_viewFormats.contains(descriptor.format)
     return false;
Comment 8 Kimmo Kinnunen 2022-04-07 02:40:55 PDT
Comment on attachment 456533 [details]
Patch

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

> Source/WebGPU/WebGPU/Texture.mm:2017
> +        viewFormats.reserveInitialCapacity(descriptorViewFormats.viewFormatsCount);

viewFormats = Vector { descriptorViewFormats.viewFormats, descriptorViewFormats.viewFormatsCount };
Comment 9 Myles C. Maxfield 2022-04-07 12:20:20 PDT
Committed r292552 (249390@trunk): <https://commits.webkit.org/249390@trunk>
Comment 10 Radar WebKit Bug Importer 2022-04-07 12:21:22 PDT
<rdar://problem/91437290>