Implement XRMediaBinding and media layers [chromium/src : main]
Yong Li (xWF) (Gerrit)
Yong Li (xWF) added 1 comment![]( "Open in Gerrit")
const void* pixels = video_frame->data(VideoFrame::kARGB);Please fix this WARNING reported by autoreview issue finding: If `use_visible_rect` is true and the frame's visible rect is cropped, `video_frame->data(VideoFrame::kARGB)` will read from the un-offset origin of the coded data instead of the visible rect's origin.
You should conditionally use `video_frame->visible_data(VideoFrame::kARGB)` when `use_visible_rect` is true. Similarly, compute the dimensions for `SkImageInfo` using `use_visible_rect ? video_frame->visible_rect() : gfx::Rect(video_frame->coded_size())` like it's done elsewhere in this file.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper added 17 comments![]( "Open in Gerrit")
Implement XRMediaBinding and media layersMore of a description about how we're implementing it would be good.
gpu::SHARED_IMAGE_USAGE_RASTER_WRITE;Raster read/write probably worth commenting? Or at least explaining here in CR
"+ui/gfx/gpu_fence.h",I don't think this is used anymore?
XRCompositionLayer(XRSession* session,
XRGraphicsBinding* binding,I won't make you split this out now, but a lot of the size/files of this CL are due to this change. In the future, if you identify a small but viral change like this as being necessary, a prep-CL to do that conversion with the existing types would be ideal to separate out.
nullptr, drawing_context);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.
uint16_t w = video_->videoWidth();
uint16_t h = video_->videoHeight();Chromium style generally frowns on this kind of name shortening, please use `width` and `height` instead of `w` and `h`.
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..
if (auto* wmp = video_->GetWebMediaPlayer()) {See earlier comment re: name shortenings, `player` would be acceptable here though.
if (!media_video_frame) {
return;
}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).
gpu::raster::RasterInterface* ri =Can't shorten names.
// This 2-copy solution is the best we can do for now without a big😮
// Flush the commands to ensure the GPU executes the copy before OpenXR
// consumes it.
ri->Flush();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.
XRGraphicsBinding* GetGraphicsBinding() const {
if (graphics_bindings_.empty()) {
return nullptr;
}
return graphics_bindings_.begin()->Get();
}Doesn't seem to be used anymore. Further, something like this isn't really right either.
if (!init->hasWidth()) {
exception_state.ThrowTypeError("width is required.");
return nullptr;
}
if (!init->hasHeight()) {
exception_state.ThrowTypeError("height is required.");
return nullptr;
}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.
if (!ValidateCylinderLayerInit(init, exception_state)) {This doesn't validate that the required items are set.
auto* drawing_context = layer->drawing_context();
if (!drawing_context || !drawing_context->IsWebGL()) {
exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
"Layer drawing context is not WebGL.");
return nullptr;
}
auto* webgl_context = static_cast<XRWebGLDrawingContext*>(drawing_context);So, the only way this would trigger is if we try to call this method with a Media Layer. I *think* an internal bool on the layer indicating that it's a media layer would *maybe* be cleaner, and we can assert that if it is *not* a media layer, it should have a WebGL drawing context. I *think* that may feel like a cleaner interface to me than an `IsWebGL` boolean, but open to discussion.
Separately, @baj...@chromium.org, should the spec be modified to throw if you try to call this for a media layer always anyway?
status: "stable",Shouldn't be stable yet; should leave it as experimental until we can do a chromestatus for it.
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. |
Brandon Jones (Gerrit)
Brandon Jones added 5 comments![]( "Open in Gerrit")
gpu::SHARED_IMAGE_USAGE_RASTER_WRITE;Raster read/write probably worth commenting? Or at least explaining here in CR
Does this have any performance implications for non-media layers? If so, or if we're not certain, is there a way we can apply these usages only to the images created for media layers?
// This 2-copy solution is the best we can do for now without a bigBrandon Jones😮
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.
XRGraphicsBinding* GetGraphicsBinding() const {
if (graphics_bindings_.empty()) {
return nullptr;
}
return graphics_bindings_.begin()->Get();
}Doesn't seem to be used anymore. Further, something like this isn't really right either.
Agreed. It's not clear under which circumstances it would be useful to know whatever the first registered graphics binding is.
auto* drawing_context = layer->drawing_context();
if (!drawing_context || !drawing_context->IsWebGL()) {
exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
"Layer drawing context is not WebGL.");
return nullptr;
}
auto* webgl_context = static_cast<XRWebGLDrawingContext*>(drawing_context);So, the only way this would trigger is if we try to call this method with a Media Layer. I *think* an internal bool on the layer indicating that it's a media layer would *maybe* be cleaner, and we can assert that if it is *not* a media layer, it should have a WebGL drawing context. I *think* that may feel like a cleaner interface to me than an `IsWebGL` boolean, but open to discussion.
Separately, @baj...@chromium.org, should the spec be modified to throw if you try to call this for a media layer always anyway?
We absolutely should have something in the spec that throws here. That kind of fits the discussion in https://github.com/immersive-web/layers/issues/326 but we didn't consider media layers as part of it.
In any case, sounds like the intent is that we will start throwing if `get{View}SubImage()` is called on anything that wasn't created by the same binding, regardless of if it's a media or WebGL layer. Ideally that can be implemented here to simplify the logic?
return nullptr;Ditto the conversation above here.
Related details
- Yong Li (xWF)
Alexander Cooper (Gerrit)
Alexander Cooper added 1 comment![]( "Open in Gerrit")
auto* drawing_context = layer->drawing_context();
if (!drawing_context || !drawing_context->IsWebGL()) {
exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
"Layer drawing context is not WebGL.");
return nullptr;
}
auto* webgl_context = static_cast<XRWebGLDrawingContext*>(drawing_context);Brandon JonesSo, the only way this would trigger is if we try to call this method with a Media Layer. I *think* an internal bool on the layer indicating that it's a media layer would *maybe* be cleaner, and we can assert that if it is *not* a media layer, it should have a WebGL drawing context. I *think* that may feel like a cleaner interface to me than an `IsWebGL` boolean, but open to discussion.
Separately, @baj...@chromium.org, should the spec be modified to throw if you try to call this for a media layer always anyway?
We absolutely should have something in the spec that throws here. That kind of fits the discussion in https://github.com/immersive-web/layers/issues/326 but we didn't consider media layers as part of it.
In any case, sounds like the intent is that we will start throwing if `get{View}SubImage()` is called on anything that wasn't created by the same binding, regardless of if it's a media or WebGL layer. Ideally that can be implemented here to simplify the logic?
but we didn't consider media layers as part of it.
I think they're implicitly covered, because they're created by an XrMediaBinding and *not* the XrWebGlBinding or XrGpuBinding, so they'd throw if passed into here, because this binding didn't create them. Even with Yong's current logic of the `binding()` member of the Media layers being nullptr, I think a trivial `if (layer->binding() != this)` would satisfy the proposed spec change and make our guarantees here.
Yong Li (xWF) (Gerrit)
Yong Li (xWF) added 3 comments![]( "Open in Gerrit")
gpu::SHARED_IMAGE_USAGE_RASTER_WRITE;Brandon JonesRaster read/write probably worth commenting? Or at least explaining here in CR
Does this have any performance implications for non-media layers? If so, or if we're not certain, is there a way we can apply these usages only to the images created for media layers?
@Alex, yes we need them for using CopyVideoFrameToSharedImage. @Brandon, yeah, I am going to add a boolean in mojom struct to indicate if we need raster access
// This 2-copy solution is the best we can do for now without a bigBrandon Jones😮
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.
Yeah, there is no good solution for now in such a case. Will add more comments
auto* drawing_context = layer->drawing_context();
if (!drawing_context || !drawing_context->IsWebGL()) {
exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
"Layer drawing context is not WebGL.");
return nullptr;
}
auto* webgl_context = static_cast<XRWebGLDrawingContext*>(drawing_context);Brandon JonesSo, the only way this would trigger is if we try to call this method with a Media Layer. I *think* an internal bool on the layer indicating that it's a media layer would *maybe* be cleaner, and we can assert that if it is *not* a media layer, it should have a WebGL drawing context. I *think* that may feel like a cleaner interface to me than an `IsWebGL` boolean, but open to discussion.
Separately, @baj...@chromium.org, should the spec be modified to throw if you try to call this for a media layer always anyway?
Alexander CooperWe absolutely should have something in the spec that throws here. That kind of fits the discussion in https://github.com/immersive-web/layers/issues/326 but we didn't consider media layers as part of it.
In any case, sounds like the intent is that we will start throwing if `get{View}SubImage()` is called on anything that wasn't created by the same binding, regardless of if it's a media or WebGL layer. Ideally that can be implemented here to simplify the logic?
but we didn't consider media layers as part of it.
I think they're implicitly covered, because they're created by an XrMediaBinding and *not* the XrWebGlBinding or XrGpuBinding, so they'd throw if passed into here, because this binding didn't create them. Even with Yong's current logic of the `binding()` member of the Media layers being nullptr, I think a trivial `if (layer->binding() != this)` would satisfy the proposed spec change and make our guarantees here.
I remember the spec does allow using layers with different (GL) bindings. We fixed a problem like that, because the samples do use one binding to create layers and use another binding to render them.
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 2 comments![]( "Open in Gerrit")
gpu::SHARED_IMAGE_USAGE_RASTER_WRITE;Brandon JonesRaster read/write probably worth commenting? Or at least explaining here in CR
Yong Li (xWF)Does this have any performance implications for non-media layers? If so, or if we're not certain, is there a way we can apply these usages only to the images created for media layers?
@Alex, yes we need them for using CopyVideoFrameToSharedImage. @Brandon, yeah, I am going to add a boolean in mojom struct to indicate if we need raster access
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.
Yeah, there is no good solution for now in such a case. Will add more comments
BTW, in reality, the limit is usually 8192
Alexander Cooper (Gerrit)
Alexander Cooper added 1 comment![]( "Open in Gerrit")
auto* drawing_context = layer->drawing_context();
if (!drawing_context || !drawing_context->IsWebGL()) {
exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
"Layer drawing context is not WebGL.");
return nullptr;
}
auto* webgl_context = static_cast<XRWebGLDrawingContext*>(drawing_context);Brandon JonesSo, the only way this would trigger is if we try to call this method with a Media Layer. I *think* an internal bool on the layer indicating that it's a media layer would *maybe* be cleaner, and we can assert that if it is *not* a media layer, it should have a WebGL drawing context. I *think* that may feel like a cleaner interface to me than an `IsWebGL` boolean, but open to discussion.
Separately, @baj...@chromium.org, should the spec be modified to throw if you try to call this for a media layer always anyway?
Alexander CooperWe absolutely should have something in the spec that throws here. That kind of fits the discussion in https://github.com/immersive-web/layers/issues/326 but we didn't consider media layers as part of it.
In any case, sounds like the intent is that we will start throwing if `get{View}SubImage()` is called on anything that wasn't created by the same binding, regardless of if it's a media or WebGL layer. Ideally that can be implemented here to simplify the logic?
Yong Li (xWF)but we didn't consider media layers as part of it.
I think they're implicitly covered, because they're created by an XrMediaBinding and *not* the XrWebGlBinding or XrGpuBinding, so they'd throw if passed into here, because this binding didn't create them. Even with Yong's current logic of the `binding()` member of the Media layers being nullptr, I think a trivial `if (layer->binding() != this)` would satisfy the proposed spec change and make our guarantees here.
I remember the spec does allow using layers with different (GL) bindings. We fixed a problem like that, because the samples do use one binding to create layers and use another binding to render them.
Hmm, that's a potential wrinkle for us making that spec change; but I think overall we should be in the clear to not allow getting the texture for a media layer, given that the long term intention is DRM and that if you wanted to do anything with the texture you should just not use MediaLayers to begin with.
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. |