Allow internal flags to be set on a WebGPUMailboxTexture. [chromium/src : main]

0 views
Skip to first unread message

John Stiles (Gerrit)

unread,
Apr 24, 2024, 11:35:18 AMApr 24
to Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
Attention needed from Corentin Wallez

John Stiles voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Corentin Wallez
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
Gerrit-Change-Number: 5482830
Gerrit-PatchSet: 2
Gerrit-Owner: John Stiles <johns...@google.com>
Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
Gerrit-Reviewer: John Stiles <johns...@google.com>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Corentin Wallez <cwa...@chromium.org>
Gerrit-Comment-Date: Wed, 24 Apr 2024 15:35:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

John Stiles (Gerrit)

unread,
Apr 24, 2024, 11:36:02 AMApr 24
to Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
Attention needed from Corentin Wallez and Jean-Philippe Gravel

John Stiles added 1 comment

Patchset-level comments
File-level comment, Patchset 2 (Latest):
John Stiles . unresolved

+jpgravel@ for OWNERS in canvas2D

Open in Gerrit

Related details

Attention is currently required from:
  • Corentin Wallez
  • Jean-Philippe Gravel
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 2
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Wed, 24 Apr 2024 15:35:48 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Corentin Wallez (Gerrit)

    unread,
    Apr 25, 2024, 3:54:04 AMApr 25
    to John Stiles, Jean-Philippe Gravel, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Jean-Philippe Gravel and John Stiles

    Corentin Wallez voted and added 4 comments

    Votes added by Corentin Wallez

    Code-Review+1

    4 comments

    Patchset-level comments
    Corentin Wallez . resolved

    LGTM

    File third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
    Line 3476, Patchset 2 (Latest): kUsage, kUsage, image, image_info,
    Corentin Wallez . unresolved

    Why isn't this one None for now as well?

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 127, Patchset 2 (Latest): // Add on any internal-usage flags that aren't already present in `usage`.
    internal_usage_flags = static_cast<WGPUTextureUsage>(
    static_cast<int>(internal_usage_flags) & ~static_cast<int>(usage));
    WGPUDawnTextureInternalUsageDescriptor internal_usage_desc = {};
    Corentin Wallez . unresolved

    IMHO it's not really necessary to actually remove the extra usages that are already specified.

    Line 132, Patchset 2 (Latest): internal_usage_desc.internalUsage = internal_usage_flags,
    Corentin Wallez . unresolved

    I think we need to set the sType here otherwise Dawn won't know what the structure is. (in the C++ binding this is done for you, but we are still in the process of making Blink use them)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Jean-Philippe Gravel
    • John Stiles
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 2
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: John Stiles <johns...@google.com>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 07:53:51 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 9:56:32 AMApr 25
    to Corentin Wallez, Jean-Philippe Gravel, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Jean-Philippe Gravel

    John Stiles added 3 comments

    File third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
    Line 3476, Patchset 2: kUsage, kUsage, image, image_info,
    Corentin Wallez . resolved

    Why isn't this one None for now as well?

    John Stiles

    Done.

    (Both have the same effect, and it will change in the next CL anyway)

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 127, Patchset 2: // Add on any internal-usage flags that aren't already present in `usage`.

    internal_usage_flags = static_cast<WGPUTextureUsage>(
    static_cast<int>(internal_usage_flags) & ~static_cast<int>(usage));
    WGPUDawnTextureInternalUsageDescriptor internal_usage_desc = {};
    Corentin Wallez . resolved

    IMHO it's not really necessary to actually remove the extra usages that are already specified.

    John Stiles

    Done.

    Masking-off step removed

    Line 132, Patchset 2: internal_usage_desc.internalUsage = internal_usage_flags,
    Corentin Wallez . resolved

    I think we need to set the sType here otherwise Dawn won't know what the structure is. (in the C++ binding this is done for you, but we are still in the process of making Blink use them)

    John Stiles

    Thanks, good catch :) Done.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Jean-Philippe Gravel
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 3
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 13:56:19 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Corentin Wallez <cwa...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Corentin Wallez (Gerrit)

    unread,
    Apr 25, 2024, 10:16:44 AMApr 25
    to John Stiles, Jean-Philippe Gravel, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Jean-Philippe Gravel and John Stiles

    Corentin Wallez voted and added 1 comment

    Votes added by Corentin Wallez

    Code-Review+1

    1 comment

    Patchset-level comments
    File-level comment, Patchset 3 (Latest):
    Corentin Wallez . resolved

    Still LGTM!

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Jean-Philippe Gravel
    • John Stiles
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 3
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: John Stiles <johns...@google.com>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 14:16:20 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Jean-Philippe Gravel (Gerrit)

    unread,
    Apr 25, 2024, 10:39:52 AMApr 25
    to John Stiles, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from John Stiles

    Jean-Philippe Gravel voted and added 5 comments

    Votes added by Jean-Philippe Gravel

    Code-Review+1

    5 comments

    Patchset-level comments
    Jean-Philippe Gravel . resolved

    Looks good, about from a nit and a few questions.

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.h
    Line 38, Patchset 3 (Latest): WGPUTextureUsage internal_usage_flags,
    Jean-Philippe Gravel . unresolved

    Just wondering: why is this API using the C version of the enum instead of the C++ version? The C++ version is vastly more popular, with [2746](https://source.chromium.org/search?q=%5CbTextureUsage::%5Cw%2B%5Cb&sq=&ss=chromium) Code Search results vs. [121](https://source.chromium.org/search?q=%5CbWGPUTextureUsage_%5Cw%2B%5Cb&ss=chromium).

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 119, Patchset 3 (Latest):
    WGPUTextureDescriptor tex_desc = {};
    tex_desc.usage = usage;
    tex_desc.dimension = WGPUTextureDimension_2D;
    tex_desc.size.width = size.width();
    tex_desc.size.height = size.height();
    tex_desc.format = VizToWGPUFormat(canvas_resource->GetSharedImageFormat());
    Jean-Philippe Gravel . unresolved

    Drive-by and unrelated to this CL: couldn't this be initialized with designated initializers, instead of default-initializing and then assigning?
    https://abseil.io/tips/172

    Line 130, Patchset 3 (Latest): internal_usage_desc.chain.sType = WGPUSType_DawnTextureInternalUsageDescriptor;
    Jean-Philippe Gravel . unresolved

    Nit: keep within 80 characters to prevent wrapping?

    Line 132, Patchset 3 (Latest): tex_desc.nextInChain = &internal_usage_desc.chain;
    Jean-Philippe Gravel . unresolved

    I was worried about this pointer being used after the function terminates, but looking in Code Search, it looks like it's not used at all. Is this coming in a future change? Or am I missing something?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • John Stiles
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 3
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: John Stiles <johns...@google.com>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 14:39:36 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 10:46:27 AMApr 25
    to Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Jean-Philippe Gravel

    John Stiles added 1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 132, Patchset 3 (Latest): tex_desc.nextInChain = &internal_usage_desc.chain;
    Jean-Philippe Gravel . unresolved

    I was worried about this pointer being used after the function terminates, but looking in Code Search, it looks like it's not used at all. Is this coming in a future change? Or am I missing something?

    John Stiles

    Which pointer are you saying isn't used exactly? I'm not sure what you mean.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Jean-Philippe Gravel
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 3
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 14:46:07 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Jean-Philippe Gravel <jpgr...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 10:48:16 AMApr 25
    to Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Jean-Philippe Gravel

    John Stiles added 1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.h
    Line 38, Patchset 3 (Latest): WGPUTextureUsage internal_usage_flags,
    Jean-Philippe Gravel . unresolved

    Just wondering: why is this API using the C version of the enum instead of the C++ version? The C++ version is vastly more popular, with [2746](https://source.chromium.org/search?q=%5CbTextureUsage::%5Cw%2B%5Cb&sq=&ss=chromium) Code Search results vs. [121](https://source.chromium.org/search?q=%5CbWGPUTextureUsage_%5Cw%2B%5Cb&ss=chromium).

    John Stiles

    `WGPU` is part of Blink, `wgpu::` is part of Dawn. Blink wraps Dawn in a large set of parallel classes/types/etc.

    Gerrit-Comment-Date: Thu, 25 Apr 2024 14:47:59 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Corentin Wallez (Gerrit)

    unread,
    Apr 25, 2024, 10:50:40 AMApr 25
    to John Stiles, Austin Eng, Jean-Philippe Gravel, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng and Jean-Philippe Gravel

    Corentin Wallez voted and added 2 comments

    Votes added by Corentin Wallez

    Code-Review+0

    2 comments

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.h
    Line 38, Patchset 3 (Latest): WGPUTextureUsage internal_usage_flags,
    Jean-Philippe Gravel . unresolved

    Just wondering: why is this API using the C version of the enum instead of the C++ version? The C++ version is vastly more popular, with [2746](https://source.chromium.org/search?q=%5CbTextureUsage::%5Cw%2B%5Cb&sq=&ss=chromium) Code Search results vs. [121](https://source.chromium.org/search?q=%5CbWGPUTextureUsage_%5Cw%2B%5Cb&ss=chromium).

    Corentin Wallez

    Somewhat historical reasons, it will be fixed very soon as https://chromium-review.googlesource.com/c/chromium/src/+/5460722 lands

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 132, Patchset 3 (Latest): tex_desc.nextInChain = &internal_usage_desc.chain;
    Jean-Philippe Gravel . unresolved

    I was worried about this pointer being used after the function terminates, but looking in Code Search, it looks like it's not used at all. Is this coming in a future change? Or am I missing something?

    Corentin Wallez

    Uh right, I was surprised that the sType missing didn't cause crashes. John do you have tests that unsupported usages are actually not supported but CopySrc is?

    It seems like we might need to add an internalUsage argument to AssociateMailbox and handle it in webgpu_decoder_impl.cc and all the SI stuff. +CC Austin, any concern with adding this?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Austin Eng
    • Jean-Philippe Gravel
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 3
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Austin Eng <en...@chromium.org>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: Austin Eng <en...@chromium.org>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 14:50:27 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Jean-Philippe Gravel <jpgr...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 10:52:29 AMApr 25
    to Austin Eng, Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng, Corentin Wallez and Jean-Philippe Gravel

    John Stiles added 1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.h
    Line 38, Patchset 3 (Latest): WGPUTextureUsage internal_usage_flags,
    Jean-Philippe Gravel . unresolved

    Just wondering: why is this API using the C version of the enum instead of the C++ version? The C++ version is vastly more popular, with [2746](https://source.chromium.org/search?q=%5CbTextureUsage::%5Cw%2B%5Cb&sq=&ss=chromium) Code Search results vs. [121](https://source.chromium.org/search?q=%5CbWGPUTextureUsage_%5Cw%2B%5Cb&ss=chromium).

    Corentin Wallez

    Somewhat historical reasons, it will be fixed very soon as https://chromium-review.googlesource.com/c/chromium/src/+/5460722 lands

    John Stiles

    Oh, that's exciting! I didn't realize we were going to migrate all these. That is a solid improvement.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Austin Eng
    • Corentin Wallez
    • Jean-Philippe Gravel
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 3
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Austin Eng <en...@chromium.org>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: Austin Eng <en...@chromium.org>
    Gerrit-Attention: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 14:52:12 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Corentin Wallez <cwa...@chromium.org>
    Comment-In-Reply-To: Jean-Philippe Gravel <jpgr...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 10:55:37 AMApr 25
    to Austin Eng, Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng, Corentin Wallez and Jean-Philippe Gravel

    John Stiles added 1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 132, Patchset 3 (Latest): tex_desc.nextInChain = &internal_usage_desc.chain;
    Jean-Philippe Gravel . unresolved

    I was worried about this pointer being used after the function terminates, but looking in Code Search, it looks like it's not used at all. Is this coming in a future change? Or am I missing something?

    Corentin Wallez

    Uh right, I was surprised that the sType missing didn't cause crashes. John do you have tests that unsupported usages are actually not supported but CopySrc is?

    It seems like we might need to add an internalUsage argument to AssociateMailbox and handle it in webgpu_decoder_impl.cc and all the SI stuff. +CC Austin, any concern with adding this?

    John Stiles

    You're right, at one point I had thought that `desc` was passed in and the `nextInChain` would automatically kick in, but I see now that `AssociateMailbox` takes `desc.usage` directly and not the entire `desc`, so it isn't going to see the rest of the chain.

    Gerrit-Comment-Date: Thu, 25 Apr 2024 14:55:23 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 11:10:40 AMApr 25
    to Austin Eng, Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng, Corentin Wallez and Jean-Philippe Gravel

    John Stiles added 1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.h
    Line 38, Patchset 3 (Latest): WGPUTextureUsage internal_usage_flags,
    Jean-Philippe Gravel . resolved

    Just wondering: why is this API using the C version of the enum instead of the C++ version? The C++ version is vastly more popular, with [2746](https://source.chromium.org/search?q=%5CbTextureUsage::%5Cw%2B%5Cb&sq=&ss=chromium) Code Search results vs. [121](https://source.chromium.org/search?q=%5CbWGPUTextureUsage_%5Cw%2B%5Cb&ss=chromium).

    Corentin Wallez

    Somewhat historical reasons, it will be fixed very soon as https://chromium-review.googlesource.com/c/chromium/src/+/5460722 lands

    John Stiles

    Oh, that's exciting! I didn't realize we were going to migrate all these. That is a solid improvement.

    John Stiles

    Acknowledged

    Gerrit-Comment-Date: Thu, 25 Apr 2024 15:10:25 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: John Stiles <johns...@google.com>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 11:20:57 AMApr 25
    to Austin Eng, Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng, Corentin Wallez and Jean-Philippe Gravel

    John Stiles added 1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 119, Patchset 3 (Latest):
    WGPUTextureDescriptor tex_desc = {};
    tex_desc.usage = usage;
    tex_desc.dimension = WGPUTextureDimension_2D;
    tex_desc.size.width = size.width();
    tex_desc.size.height = size.height();
    tex_desc.format = VizToWGPUFormat(canvas_resource->GetSharedImageFormat());
    Jean-Philippe Gravel . unresolved

    Drive-by and unrelated to this CL: couldn't this be initialized with designated initializers, instead of default-initializing and then assigning?
    https://abseil.io/tips/172

    John Stiles

    So, this actually has nested structures and type narrowing, so it's not quite a 1:1 replacement. It ends up looking like this:

    ```
    WGPUTextureDescriptor tex_desc = {
    .usage = usage,
    .dimension = WGPUTextureDimension_2D,
    .size = {.width = static_cast<uint32_t>(size.width()),
    .height = static_cast<uint32_t>(size.height())},
    .format = VizToWGPUFormat(canvas_resource->GetSharedImageFormat()),
    };
    ```

    WDYT, is this an improvement? To my eyes it's less readable overall (the casts add more noise/clutter than the removal of `tex_desc.` saves IMO) but I am not particular and will defer to your opinion.

    Gerrit-Comment-Date: Thu, 25 Apr 2024 15:20:44 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Jean-Philippe Gravel <jpgr...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 11:35:21 AMApr 25
    to Austin Eng, Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng, Corentin Wallez and Jean-Philippe Gravel

    John Stiles added 4 comments

    Patchset-level comments
    File-level comment, Patchset 2:
    John Stiles . resolved

    +jpgravel@ for OWNERS in canvas2D

    John Stiles

    Acknowledged

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc

    WGPUTextureDescriptor tex_desc = {};
    tex_desc.usage = usage;
    tex_desc.dimension = WGPUTextureDimension_2D;
    tex_desc.size.width = size.width();
    tex_desc.size.height = size.height();
    tex_desc.format = VizToWGPUFormat(canvas_resource->GetSharedImageFormat());
    Jean-Philippe Gravel . unresolved

    Drive-by and unrelated to this CL: couldn't this be initialized with designated initializers, instead of default-initializing and then assigning?
    https://abseil.io/tips/172

    John Stiles

    So, this actually has nested structures and type narrowing, so it's not quite a 1:1 replacement. It ends up looking like this:

    ```
    WGPUTextureDescriptor tex_desc = {
    .usage = usage,
    .dimension = WGPUTextureDimension_2D,
    .size = {.width = static_cast<uint32_t>(size.width()),
    .height = static_cast<uint32_t>(size.height())},
    .format = VizToWGPUFormat(canvas_resource->GetSharedImageFormat()),
    };
    ```

    WDYT, is this an improvement? To my eyes it's less readable overall (the casts add more noise/clutter than the removal of `tex_desc.` saves IMO) but I am not particular and will defer to your opinion.

    John Stiles

    PTAL

    Line 130, Patchset 3: internal_usage_desc.chain.sType = WGPUSType_DawnTextureInternalUsageDescriptor;
    Jean-Philippe Gravel . resolved

    Nit: keep within 80 characters to prevent wrapping?

    John Stiles

    Done

    Line 132, Patchset 3: tex_desc.nextInChain = &internal_usage_desc.chain;
    Jean-Philippe Gravel . unresolved

    I was worried about this pointer being used after the function terminates, but looking in Code Search, it looks like it's not used at all. Is this coming in a future change? Or am I missing something?

    Corentin Wallez

    Uh right, I was surprised that the sType missing didn't cause crashes. John do you have tests that unsupported usages are actually not supported but CopySrc is?

    It seems like we might need to add an internalUsage argument to AssociateMailbox and handle it in webgpu_decoder_impl.cc and all the SI stuff. +CC Austin, any concern with adding this?

    John Stiles

    You're right, at one point I had thought that `desc` was passed in and the `nextInChain` would automatically kick in, but I see now that `AssociateMailbox` takes `desc.usage` directly and not the entire `desc`, so it isn't going to see the rest of the chain.

    John Stiles

    Looking at it, the rabbit hole for `AssociateMailbox` goes very deep. I'd like enga@'s input before making any changes to it.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Austin Eng
    • Corentin Wallez
    • Jean-Philippe Gravel
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 4
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Austin Eng <en...@chromium.org>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: Austin Eng <en...@chromium.org>
    Gerrit-Attention: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 15:35:05 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Austin Eng (Gerrit)

    unread,
    Apr 25, 2024, 12:34:44 PMApr 25
    to John Stiles, Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Corentin Wallez, Jean-Philippe Gravel and John Stiles

    Austin Eng added 1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 132, Patchset 3: tex_desc.nextInChain = &internal_usage_desc.chain;
    Jean-Philippe Gravel . unresolved

    I was worried about this pointer being used after the function terminates, but looking in Code Search, it looks like it's not used at all. Is this coming in a future change? Or am I missing something?

    Corentin Wallez

    Uh right, I was surprised that the sType missing didn't cause crashes. John do you have tests that unsupported usages are actually not supported but CopySrc is?

    It seems like we might need to add an internalUsage argument to AssociateMailbox and handle it in webgpu_decoder_impl.cc and all the SI stuff. +CC Austin, any concern with adding this?

    John Stiles

    You're right, at one point I had thought that `desc` was passed in and the `nextInChain` would automatically kick in, but I see now that `AssociateMailbox` takes `desc.usage` directly and not the entire `desc`, so it isn't going to see the rest of the chain.

    John Stiles

    Looking at it, the rabbit hole for `AssociateMailbox` goes very deep. I'd like enga@'s input before making any changes to it.

    Austin Eng

    a lot of the shared image backings already add the internal usage of CopySrc
    https://source.chromium.org/search?q=internalUsage%20%22CopySrc%22%20-f:third_party%2Fdawn&sq=&ss=chromium%2Fchromium%2Fsrc

    were you seeing it was not added for some reason?


    (that said, that automatic internal usages may cause us problems in the future since some of them automatically add RenderAttachment - it messes up concurrent read/write determination. And so we indeed may end up needing to add internal usage to AssociateMailbox)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Corentin Wallez
    • Jean-Philippe Gravel
    • John Stiles
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 4
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Austin Eng <en...@chromium.org>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: John Stiles <johns...@google.com>
    Gerrit-Attention: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 16:34:31 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 12:57:53 PMApr 25
    to Austin Eng, Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng, Corentin Wallez and Jean-Philippe Gravel

    John Stiles added 1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 132, Patchset 3: tex_desc.nextInChain = &internal_usage_desc.chain;
    Jean-Philippe Gravel . unresolved

    I was worried about this pointer being used after the function terminates, but looking in Code Search, it looks like it's not used at all. Is this coming in a future change? Or am I missing something?

    Corentin Wallez

    Uh right, I was surprised that the sType missing didn't cause crashes. John do you have tests that unsupported usages are actually not supported but CopySrc is?

    It seems like we might need to add an internalUsage argument to AssociateMailbox and handle it in webgpu_decoder_impl.cc and all the SI stuff. +CC Austin, any concern with adding this?

    John Stiles

    You're right, at one point I had thought that `desc` was passed in and the `nextInChain` would automatically kick in, but I see now that `AssociateMailbox` takes `desc.usage` directly and not the entire `desc`, so it isn't going to see the rest of the chain.

    John Stiles

    Looking at it, the rabbit hole for `AssociateMailbox` goes very deep. I'd like enga@'s input before making any changes to it.

    Austin Eng

    a lot of the shared image backings already add the internal usage of CopySrc
    https://source.chromium.org/search?q=internalUsage%20%22CopySrc%22%20-f:third_party%2Fdawn&sq=&ss=chromium%2Fchromium%2Fsrc

    were you seeing it was not added for some reason?


    (that said, that automatic internal usages may cause us problems in the future since some of them automatically add RenderAttachment - it messes up concurrent read/write determination. And so we indeed may end up needing to add internal usage to AssociateMailbox)

    John Stiles

    In practice things do seem to be working, so it's possible we are also hitting a path where CopySrc is being tacked on for us and that is the explanation for it. But this sounds more like "we are getting away with something" instead of a robust solution to me? I need to _make sure_ that CopySrc is possible when `endWebGPUAccess` does its copyback, so what's the right way of doing that?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Austin Eng
    • Corentin Wallez
    • Jean-Philippe Gravel
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 4
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Austin Eng <en...@chromium.org>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-Attention: Austin Eng <en...@chromium.org>
    Gerrit-Attention: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Comment-Date: Thu, 25 Apr 2024 16:57:36 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Austin Eng <en...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    John Stiles (Gerrit)

    unread,
    Apr 25, 2024, 1:25:02 PMApr 25
    to Austin Eng, Jean-Philippe Gravel, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng, Corentin Wallez and Jean-Philippe Gravel

    John Stiles added 1 comment

    Patchset-level comments
    File-level comment, Patchset 4 (Latest):
    John Stiles . unresolved

    Chatted with enga@ a bit. We have a hypothesis that we don't actually need internal flags here, because our copying to/from IOSurfaces happens via RasterInterface and not via WebGPU, and it should always just work. I'm going to check this theory.

    Gerrit-Comment-Date: Thu, 25 Apr 2024 17:24:43 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Jean-Philippe Gravel (Gerrit)

    unread,
    Apr 25, 2024, 2:19:36 PMApr 25
    to John Stiles, Austin Eng, Corentin Wallez, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-reviews-p...@chromium.org, blink-...@chromium.org, cwalle...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kainin...@chromium.org, kinuko...@chromium.org
    Attention needed from Austin Eng and Corentin Wallez

    Jean-Philippe Gravel voted and added 1 comment

    Votes added by Jean-Philippe Gravel

    Code-Review+0

    1 comment

    File third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
    Line 119, Patchset 3:
    WGPUTextureDescriptor tex_desc = {};
    tex_desc.usage = usage;
    tex_desc.dimension = WGPUTextureDimension_2D;
    tex_desc.size.width = size.width();
    tex_desc.size.height = size.height();
    tex_desc.format = VizToWGPUFormat(canvas_resource->GetSharedImageFormat());
    Jean-Philippe Gravel . unresolved

    Drive-by and unrelated to this CL: couldn't this be initialized with designated initializers, instead of default-initializing and then assigning?
    https://abseil.io/tips/172

    John Stiles

    So, this actually has nested structures and type narrowing, so it's not quite a 1:1 replacement. It ends up looking like this:

    ```
    WGPUTextureDescriptor tex_desc = {
    .usage = usage,
    .dimension = WGPUTextureDimension_2D,
    .size = {.width = static_cast<uint32_t>(size.width()),
    .height = static_cast<uint32_t>(size.height())},
    .format = VizToWGPUFormat(canvas_resource->GetSharedImageFormat()),
    };
    ```

    WDYT, is this an improvement? To my eyes it's less readable overall (the casts add more noise/clutter than the removal of `tex_desc.` saves IMO) but I am not particular and will defer to your opinion.

    John Stiles

    PTAL

    Jean-Philippe Gravel

    I think this is better for a couple of reasons, though you should probably use a safer type conversion than `static_cast` (see last point about that).

     - [The style guide recommends initializing data on declaration, instead of declaring and initializing later.](https://google.github.io/styleguide/cppguide.html#Local_Variables) This is more efficient and can help avoid bugs from uninitialized data (e.g. if someone adds or moves code in there).
     - The indentation makes it very clear that this whole code block is about initializing the struct. For readers, it becomes trivial to just skip over if not deemed interesting.

    - Designated initializers are in general safer because any unspecified POD fields will be initialized to zero. In the original code, if you had forgotten the easily omittable `= {}`, you'd get undefined behavior. Adopting the habit of using designated initializers makes this error less likely.

    - I think that the cast here is a feature, not a bug. It brings a real pitfall to the attention of the reader, one that I hadn't noticed before: this statement can overflow. Since the size here is ultimately user-control, you have to handle this edge case. As such, you should use a cast from [base/numerics](https://chromium.googlesource.com/chromium/src/+/main/base/numerics/README.md) instead of a `static_cast`.
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Austin Eng
    • Corentin Wallez
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ibf8267708533f3e0089eeabd7510b095e9c18339
    Gerrit-Change-Number: 5482830
    Gerrit-PatchSet: 4
    Gerrit-Owner: John Stiles <johns...@google.com>
    Gerrit-Reviewer: Austin Eng <en...@chromium.org>
    Gerrit-Reviewer: Corentin Wallez <cwa...@chromium.org>
    Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>
    Gerrit-Reviewer: John Stiles <johns...@google.com>
    Gerri