Allow internal flags to be set on a WebGPUMailboxTexture. [chromium/src : main]
John Stiles (Gerrit)
John Stiles voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Corentin Wallez
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
- Corentin Wallez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Corentin Wallez (Gerrit)
Corentin Wallez voted and added 4 comments![]( "Open in Gerrit")
Votes added by Corentin Wallez
| Code-Review | +1 |
4 comments
LGTM
kUsage, kUsage, image, image_info,Why isn't this one None for now as well?
// 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 = {};IMHO it's not really necessary to actually remove the extra usages that are already specified.
internal_usage_desc.internalUsage = internal_usage_flags,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)
Related details
- Jean-Philippe Gravel
- John Stiles
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 3 comments![]( "Open in Gerrit")
Why isn't this one None for now as well?
Done.
(Both have the same effect, and it will change in the next CL anyway)
// 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 = {};IMHO it's not really necessary to actually remove the extra usages that are already specified.
Done.
Masking-off step removed
internal_usage_desc.internalUsage = internal_usage_flags,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)
Thanks, good catch :) Done.
Related details
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Corentin Wallez (Gerrit)
Corentin Wallez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Corentin Wallez
| Code-Review | +1 |
- Jean-Philippe Gravel
- John Stiles
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel voted and added 5 comments![]( "Open in Gerrit")
Votes added by Jean-Philippe Gravel
| Code-Review | +1 |
5 comments
Looks good, about from a nit and a few questions.
WGPUTextureUsage internal_usage_flags,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).
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());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
internal_usage_desc.chain.sType = WGPUSType_DawnTextureInternalUsageDescriptor;Nit: keep within 80 characters to prevent wrapping?
tex_desc.nextInChain = &internal_usage_desc.chain;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?
Related details
- John Stiles
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
tex_desc.nextInChain = &internal_usage_desc.chain;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?
Which pointer are you saying isn't used exactly? I'm not sure what you mean.
Related details
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
WGPUTextureUsage internal_usage_flags,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).
`WGPU` is part of Blink, `wgpu::` is part of Dawn. Blink wraps Dawn in a large set of parallel classes/types/etc.
Corentin Wallez (Gerrit)
Corentin Wallez voted and added 2 comments![]( "Open in Gerrit")
Votes added by Corentin Wallez
| Code-Review | +0 |
2 comments
WGPUTextureUsage internal_usage_flags,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).
Somewhat historical reasons, it will be fixed very soon as https://chromium-review.googlesource.com/c/chromium/src/+/5460722 lands
tex_desc.nextInChain = &internal_usage_desc.chain;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?
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?
Related details
- Austin Eng
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
WGPUTextureUsage internal_usage_flags,Corentin WallezJust 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).
Somewhat historical reasons, it will be fixed very soon as https://chromium-review.googlesource.com/c/chromium/src/+/5460722 lands
Oh, that's exciting! I didn't realize we were going to migrate all these. That is a solid improvement.
Related details
- Austin Eng
- Corentin Wallez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
tex_desc.nextInChain = &internal_usage_desc.chain;Corentin WallezI 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?
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?
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 (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
WGPUTextureUsage internal_usage_flags,Corentin WallezJust 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 StilesSomewhat historical reasons, it will be fixed very soon as https://chromium-review.googlesource.com/c/chromium/src/+/5460722 lands
Oh, that's exciting! I didn't realize we were going to migrate all these. That is a solid improvement.
Acknowledged
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
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());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
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 (Gerrit)
John Stiles added 4 comments![]( "Open in Gerrit")
+jpgravel@ for OWNERS in canvas2D
Acknowledged
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());John StilesDrive-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
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.
PTAL
internal_usage_desc.chain.sType = WGPUSType_DawnTextureInternalUsageDescriptor;Nit: keep within 80 characters to prevent wrapping?
Done
tex_desc.nextInChain = &internal_usage_desc.chain;Corentin WallezI 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 StilesUh 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?
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.
Looking at it, the rabbit hole for `AssociateMailbox` goes very deep. I'd like enga@'s input before making any changes to it.
Related details
- Austin Eng
- Corentin Wallez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Austin Eng (Gerrit)
Austin Eng added 1 comment![]( "Open in Gerrit")
tex_desc.nextInChain = &internal_usage_desc.chain;Corentin WallezI 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 StilesUh 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 StilesYou'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.
Looking at it, the rabbit hole for `AssociateMailbox` goes very deep. I'd like enga@'s input before making any changes to it.
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)
Related details
- Corentin Wallez
- Jean-Philippe Gravel
- John Stiles
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
tex_desc.nextInChain = &internal_usage_desc.chain;Corentin WallezI 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 StilesUh 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 StilesYou'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.
Austin EngLooking at it, the rabbit hole for `AssociateMailbox` goes very deep. I'd like enga@'s input before making any changes to it.
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%2Fsrcwere 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)
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?
Related details
- Austin Eng
- Corentin Wallez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
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.
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jean-Philippe Gravel
| Code-Review | +0 |
1 comment
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());John StilesDrive-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 StilesSo, 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.
PTAL
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`.
Related details
- Austin Eng
- Corentin Wallez
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
SGTM. I'll move out this change to a separate CL, since it seems possible/likely that this CL will be abandoned shortly.
(BTW I don't recommend telling folks that designated initialization is more efficient unless you can prove it, because I strongly suspect this isn't true. Even if there are dead stores, these are trivial to eliminate even in -O1. All the other points are reasonable and make the case strongly enough!)
Related details
- Austin Eng
- Corentin Wallez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 1 comment![]( "Open in Gerrit")
BTW I don't recommend telling folks that designated initialization is more efficient unless you can prove it, because I strongly suspect this isn't true.
- Austin Eng
- Corentin Wallez
- John Stiles
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
You wrote code that requires different side effects from the C++ virtual machine, so the output is different because the C++ standard requires it to be different. I changed the code to actually be a normal constructor (initializing members) and it emits identical code for both paths.
https://godbolt.org/z/9s1nqWa1G
https://screenshot.googleplex.com/94q8EAyVVKkjina
(r14 is 21474836484, that's `0x500000004` since Clang packed the store of 4 and 5 into a single move.)
Related details
- Austin Eng
- Corentin Wallez
- Jean-Philippe Gravel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
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());Done at https://crrev.com/c/5491301
John Stiles (Gerrit)
John Stiles abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 1 comment![]( "Open in Gerrit")
In Chromium, we have a clang plugin that disallows inlining complex constructors. In these cases, the compiler cannot see what the constructor does, and so, it cannot exclude the possibility that it might have side effects:
https://godbolt.org/z/G8zK1G3G9
Besides, even with inlined constructors, there are limits to how deep the compiler can inspect and optimize code away. Just using an std::string causes the compiler to generate sub-optimal code here:
https://godbolt.org/z/7zrnxKjE8
So, I stand my ground. Using designated initializers can be more efficient.
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Stiles (Gerrit)
John Stiles added 1 comment![]( "Open in Gerrit")
Expertly demonstrated. I agree that the `std::string` example is compelling.
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |