Bug 238063

Summary:

[WebGPU] maxAnisotropy > 16 is clamped, rather than illegal

Product:

WebKit

Reporter:

Myles C. Maxfield <mmaxfield>

Component:

New Bugs

Assignee:

Myles C. Maxfield <mmaxfield>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, dino, djg, kkinnunen, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	kkinnunen: review+

Description Myles C. Maxfield 2022-03-17 19:52:27 PDT

[WebGPU] maxAnisotropy > 16 is clamped, rather than illegal

Comment 1 Myles C. Maxfield 2022-03-17 19:54:06 PDT

Created attachment 455062 [details]
Patch

Comment 2 Kimmo Kinnunen 2022-03-21 01:03:33 PDT

Comment on attachment 455062 [details]
Patch

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

> Source/WebGPU/WebGPU/Sampler.mm:195
> +    samplerDescriptor.maxAnisotropy = std::min(descriptor.maxAnisotropy, static_cast<uint16_t>(16));

sometimes less tokens for the human to parse, less ambiguous(not sure it compiles without errors for us, maybe?)
  std::min<uint16_t>(descriptor.maxAnisotropy, 16);

Comment 3 Myles C. Maxfield 2022-03-21 18:13:39 PDT

Comment on attachment 455062 [details]
Patch

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

> Source/WebGPU/ChangeLog:12
> +

Covered by api/operation/sampling/anisotropy.spec.ts

Comment 4 Myles C. Maxfield 2022-03-21 18:19:12 PDT

Committed r291593 (248687@trunk): <https://commits.webkit.org/248687@trunk>

Comment 5 Radar WebKit Bug Importer 2022-03-21 18:20:17 PDT

<rdar://problem/90605401>

Comment 6 Darin Adler 2022-03-22 07:55:18 PDT

Comment on attachment 455062 [details]
Patch

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

>> Source/WebGPU/WebGPU/Sampler.mm:195
>> +    samplerDescriptor.maxAnisotropy = std::min(descriptor.maxAnisotropy, static_cast<uint16_t>(16));
> 
> sometimes less tokens for the human to parse, less ambiguous(not sure it compiles without errors for us, maybe?)
>   std::min<uint16_t>(descriptor.maxAnisotropy, 16);

I don’t absolutely love this idiom, because when I read std::min<uint16_t>(descriptor.maxAnisotropy, 16) I think "is maxAnisotropy bigger than 16-bit, because if it is, this thing will chop the high bits". Because of that I would write the less terse:

    constexpr uint16_t maxMaxAnistropy = 16;
    samplerDescriptor.maxAnisotropy = std::min(descriptor.maxAnisotropy, maxMaxAnistropy);