Implement XRMediaBinding and accelerated video-frame copy for Media Layers [chromium/src : main]
Yong Li (xWF) (Gerrit)
Yong Li (xWF) added 12 comments![]( "Open in Gerrit")
"+ui/gfx/gpu_fence.h",Yong Li (xWF)I don't think this is used anymore?
Done
nullptr, drawing_context);Yong Li (xWF)I'm not seeing an update to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_frame_provider.cc;drc=94b8dff8418534977a39c201e209e2efc30b48d7;l=770, which looks to (now) mistakenly expect that binding is always set.
The input is a XRProjectionLayer where the binding should always exist.
uint16_t w = video_->videoWidth();
uint16_t h = video_->videoHeight();Yong Li (xWF)Chromium style generally frowns on this kind of name shortening, please use `width` and `height` instead of `w` and `h`.
Done
double scale = std::min(static_cast<double>(max_texture_size_) / w,
static_cast<double>(max_texture_size_) / h);
width_ = std::max(base::checked_cast<uint16_t>(1),
base::checked_cast<uint16_t>(w * scale));
height_ = std::max(base::checked_cast<uint16_t>(1),Yong Li (xWF)I think max_texture_size_ *must* be greater than 0 here; so therefore w, h, and scale all have to be as well. `checked_cast` will CHECK/crash in the event of an overflow, not return 0, so the std::max's are not necessary..
In extreme case, e.g. height is very big, but width is small, width could be 0 after being converted to integer. Let's say width is 1, and height is 3x max size. In reality it probably will never happen, but still possible
if (auto* wmp = video_->GetWebMediaPlayer()) {Yong Li (xWF)See earlier comment re: name shortenings, `player` would be acceptable here though.
Done
if (!media_video_frame) {
return;
}Yong Li (xWF)The presence of this check means that I think you can just turn 70 into an early return as well, and then you don't have to default initialize the two values in it's if block. If you can't get a media player, you'll always early return here. (Though obviously still need to check here).
Done
gpu::raster::RasterInterface* ri =Yong Li (xWF)Can't shorten names.
Done
// This 2-copy solution is the best we can do for now without a bigBrandon Jones😮
Yong Li (xWF)This made me 😮 too, but looking through the rest of the code I believe this path should only kick in when the video is larger than the max texture size? (Which... I'm somewhat skeptical that could even happen normally?) In fact, I'm tempted to say that we should consider adding validation to the spec that rejects in that case, but that's for a later discussion.
In any case, the large texture size will make this brutally slow BUT you're also already pushing the limits of what most GPUs will handle at that point, so you're in for a bad time whether or not this scaling needs to take place.
I would recommend adding something to the comment to explain that this path should only kick in in extreme circumstances.
Yong Li (xWF)Yeah, there is no good solution for now in such a case. Will add more comments
Yong Li (xWF)BTW, in reality, the limit is usually 8192
Done
XRGraphicsBinding* GetGraphicsBinding() const {
if (graphics_bindings_.empty()) {
return nullptr;
}
return graphics_bindings_.begin()->Get();
}Brandon JonesDoesn't seem to be used anymore. Further, something like this isn't really right either.
Yong Li (xWF)Agreed. It's not clear under which circumstances it would be useful to know whatever the first registered graphics binding is.
Done
if (!init->hasWidth()) {
exception_state.ThrowTypeError("width is required.");
return nullptr;
}
if (!init->hasHeight()) {
exception_state.ThrowTypeError("height is required.");
return nullptr;
}Yong Li (xWF)Can be removed like the other items since `Validate` should check this now?
Edit: Actually, it doesn't look like validate checks that width/height exist, which feels like it should do so.
The Validate method doesn't check these because video layer has video width/height. The spec says width/height are optional
if (!ValidateCylinderLayerInit(init, exception_state)) {Yong Li (xWF)This doesn't validate that the required items are set.
those are not required. The spec provides default values for those. We didn't follow the spec last time
status: "stable",Yong Li (xWF)Shouldn't be stable yet; should leave it as experimental until we can do a chromestatus for it.
Done
Related details
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yong Li (xWF) (Gerrit)
Yong Li (xWF) added 1 comment![]( "Open in Gerrit")
Implement XRMediaBinding and media layersYong Li (xWF)More of a description about how we're implementing it would be good.
Done
Related details
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yong Li (xWF) (Gerrit)
Yong Li (xWF) added 1 comment![]( "Open in Gerrit")
// Flush the commands to ensure the GPU executes the copy before OpenXR
// consumes it.
ri->Flush();Yong Li (xWF)I *think* plumbing the sync tokens down would do the same thing and wouldn't introduce a potential sync action here that can hang things.
It seems this is required because it uses a different context (SharedGpuContext). So flushing with the GL context is not enough.
Alexander Cooper (Gerrit)
Alexander Cooper added 6 comments![]( "Open in Gerrit")
nullptr, drawing_context);Yong Li (xWF)I'm not seeing an update to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/xr/xr_frame_provider.cc;drc=94b8dff8418534977a39c201e209e2efc30b48d7;l=770, which looks to (now) mistakenly expect that binding is always set.
The input is a XRProjectionLayer where the binding should always exist.
Acknowledged. Missed that part of it.
double scale = std::min(static_cast<double>(max_texture_size_) / w,
static_cast<double>(max_texture_size_) / h);
width_ = std::max(base::checked_cast<uint16_t>(1),
base::checked_cast<uint16_t>(w * scale));
height_ = std::max(base::checked_cast<uint16_t>(1),I think max_texture_size_ *must* be greater than 0 here; so therefore w, h, and scale all have to be as well. `checked_cast` will CHECK/crash in the event of an overflow, not return 0, so the std::max's are not necessary..
In extreme case, e.g. height is very big, but width is small, width could be 0 after being converted to integer. Let's say width is 1, and height is 3x max size. In reality it probably will never happen, but still possible
Add a comment please, I thought at first this was some kind of underflow protection. Further, I don't think you need a `checked_cast` to uint16_t with 1. There should be a suffix or you can probably specialize std::max to uint16_t to clean it up and avoid the one cast at least.
media::VideoFrameSharedImageCache* yuv_shared_image_cache = nullptr;
scoped_refptr<media::VideoFrame> media_video_frame;
if (auto* media_layer = video_->GetWebMediaPlayer()) {
media_video_frame = media_layer->GetCurrentFrameThenUpdate();
yuv_shared_image_cache = media_layer->GetYUVSharedImageCache();
}
if (!media_video_frame) {
return;
}```suggestion
auto* media_layer = video_->GetMediaWebPlayer();
if (!media_layer) {
return;
}
auto* media_video_frame = media_layer->GetCurrentFrameThenUpdate();
if (!media_video_frame) {
return;
}
auto yuv_shared_image_cache = media_layer->GetYUVSharedImageCache();
```
// Flush the commands to ensure the GPU executes the copy before OpenXR
// consumes it.
ri->Flush();Yong Li (xWF)I *think* plumbing the sync tokens down would do the same thing and wouldn't introduce a potential sync action here that can hang things.
It seems this is required because it uses a different context (SharedGpuContext). So flushing with the GL context is not enough.
@baj...@chromium.org for his thoughts here, because I thought `flush()` was typically a slow, synchronous operation that we wouldn't want as part of this hot path.
if (!init->hasWidth()) {
exception_state.ThrowTypeError("width is required.");
return nullptr;
}
if (!init->hasHeight()) {
exception_state.ThrowTypeError("height is required.");
return nullptr;
}Yong Li (xWF)Can be removed like the other items since `Validate` should check this now?
Edit: Actually, it doesn't look like validate checks that width/height exist, which feels like it should do so.
The Validate method doesn't check these because video layer has video width/height. The spec says width/height are optional
Actually looks like for base quad layers there's a default of 1 as well, similar to the comment you have about cylinder layers.
if (!ValidateCylinderLayerInit(init, exception_state)) {Yong Li (xWF)This doesn't validate that the required items are set.
those are not required. The spec provides default values for those. We didn't follow the spec last time
Acknowledged, looks like maybe this should be updated for Quad as well as it seems to also have a default.
Related details
- Brandon Jones
- Yong Li (xWF)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |