Media Foundation Frame Server Mode: Part 1: Frame Tagging [chromium/src : main]
William Carr (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Xiaohan Wang.
William Carr would like Dale Curtis, Zhenyao Mo and Xiaohan Wang to review this change.
Media Foundation Frame Server Mode: Part 1: Frame Tagging
This change adds infrastructure to allow the existing
wants_promotion_hint tagging, currently used by Android, to be applied
to video frames backed by Texture Quads & Streaming Video Quads.
Media Foundation Renderer Client will apply this tag to frames for
for clear content scenarios. Tagging is currently included for the
existing DComp mode which is backed by Streaming Video frames. In a
future changelist Frame Server mode will be added which will be backed
by Texture frames and these frames will similarly be tagged by the
Media Foundation Renderer Client.
Additional future work will read this tagging in the DC Overlay
Processor to allow communication of promotional status back to the
Media Foundation Renderer Client to aid in deciding whether to use
Frame Server or DComp mode.
Bug: 1258887
Change-Id: I0ebf334b9ea7da8eb5aa52413dc3f375eaa9dd76
---
M cc/test/render_pass_test_utils.cc
M components/viz/common/quads/stream_video_draw_quad.h
M components/viz/service/display/surface_aggregator_unittest.cc
M components/viz/test/compositor_frame_helpers.cc
M components/viz/common/quads/draw_quad_unittest.cc
M components/viz/common/quads/texture_draw_quad.cc
M services/viz/public/cpp/compositing/mojom_traits_unittest.cc
M components/viz/common/quads/render_pass_io.cc
M components/viz/service/display/surface_aggregator_perftest.cc
M services/viz/public/cpp/compositing/quads_mojom_traits.h
M services/viz/public/cpp/compositing/quads_mojom_traits.cc
M services/viz/public/mojom/compositing/quads.mojom
M components/viz/service/display/dc_layer_overlay.cc
M media/mojo/clients/win/media_foundation_renderer_client.cc
M services/viz/public/cpp/compositing/mojom_traits_perftest.cc
M components/viz/common/quads/stream_video_draw_quad.cc
M components/viz/common/quads/texture_draw_quad.h
M media/renderers/video_resource_updater.cc
M media/base/video_frame_metadata.h
M components/viz/common/quads/render_pass_io_unittest.cc
M third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc
21 files changed, 242 insertions(+), 77 deletions(-)
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Xiaohan Wang.
Rafael Cintron (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Xiaohan Wang, William Carr, Frank Liberato.
Rafael Cintron would like Frank Liberato to review this change authored by William Carr.
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Xiaohan Wang, William Carr, Frank Liberato.
9 comments:
Patchset:
Did a first pass but Frank/Dale should cover the details as I am not intimately familiar with these code areas.
File components/viz/common/quads/draw_quad_unittest.cc:
Patch Set #2, Line 349:
EXPECT_EQ(false, copy_quad->wants_promotion_hint);Nit: For regular boolean checks, I think EXPECT_FALSE should be sufficient here.
Patch Set #2, Line 611:
EXPECT_EQ(false, quad_new->wants_promotion_hint);Nit: For regular boolean checks, I think EXPECT_FALSE should be sufficient here.
File components/viz/common/quads/stream_video_draw_quad.h:
bool wants_promotion_hint : 1;
gfx::ProtectedVideoType protected_video_type : 2;Is there a particular reason we need these to be bit fields? If so, please add a comment.
bool wants_promotion_hint : 1;
gfx::ProtectedVideoType protected_video_type : 2;To avoid uninitialized data, please initialized these to reasonable default. We can have the constructor continue to be = default.
File components/viz/service/display/dc_layer_overlay.cc:
/* TODO (wicarr): If a Stream Video Draw Quad is tagged with the
* wants_promotion_hint tag then the creator wants to
* be notified of cases where promotion may fail so the Renderer can
* switch to a non-DC dependent texture type.
const StreamVideoDrawQuad* sv_quad =
StreamVideoDrawQuad::MaterialCast(*it);
if (sv_quad->wants_promotion_hint) {
// Process for hinting
}
*/Is this commented out section a hint about a future change or did you mean for this code to be active in this CL?
If the latter, please uncomment. If the former, please augment the comment with details about when (and what needs to happen) for the code to be uncommented in the future.
/* TODO (wicarr): If a Texture Draw Quad is tagged with the
* wants_promotion_hint tag then the creator wants to
* be notified of cases where the surface may be promoted to a DC overlay
// so the Renderer may switch to a more performant DC dependent texture
type. const TextureDrawQuad* tex_quad =
TextureDrawQuad::MaterialCast(*it); if (tex_quad->wants_promotion_hint)
{
// Process for hinting
}
*/Same here, please either uncomment or add a comment about when the code will be uncomment.
File media/mojo/clients/win/media_foundation_renderer_client.cc:
Patch Set #2, Line 334:
// TODO(WiCarr): Should we set hw_protected when appropriate?Do you have an answer for this question you're asking yourself in your own CL?
File media/renderers/video_resource_updater.cc:
gfx::ProtectedVideoType protected_video_type =
gfx::ProtectedVideoType::kClear;
if (frame->metadata().protected_video) {
if (frame->metadata().hw_protected)
protected_video_type = gfx::ProtectedVideoType::kHardwareProtected;
else
protected_video_type = gfx::ProtectedVideoType::kSoftwareProtected;
}I see this code copied/pasted in a couple other places in this function. Consider factoring it into a helper called something like ProtectedVideoTypeFromMetadata.
Frank Liberato (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Xiaohan Wang, William Carr.
3 comments:
Patchset:
overall seems fine.
one thing i'd point out is that it's possible that the renderer might want to to switch into frame server mode to access the video texture without overlay promotion failing. overlay promotion won't find out if, for example, one copies the video frame to a canvas. that doesn't go through viz.
i don't remember if you mention cases like that in your doc, but i didn't want to lose it in case not.
thanks
-fl
File components/viz/common/quads/stream_video_draw_quad.h:
Patch Set #2, Line 35:
gfx::ProtectedVideoType video_type);promotion hint?
Patch Set #2, Line 46:
bool promotion_hinting);it's a style nit, so i defer to the owners but i'll often:
StreamVideoDrawQuad {
enum class PromotionHint { kNo = 0, kYes };
};or similar, so that one doesn't have lots of undocumented, potentially out-of-order booleans in the SetAll args list. /*promotion_hinting=*/ is helpful, but isn't enforced anywhere that i'm aware of. though i think if one does include it, then clang will get mad if one puts the wrong name in the comment. but one may also (i think) omit it.
the `wants_promotion_hint` member can still be a bool.
like i said, i'm not an owner of this file so your mileage may vary. :)
Zhenyao Mo (Gerrit)
Attention is currently required from: Dale Curtis, Xiaohan Wang, William Carr, Sunny Sachanandani.
Zhenyao Mo would like Sunny Sachanandani to review this change authored by William Carr.
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Xiaohan Wang, William Carr, Sunny Sachanandani.
1 comment:
Patchset:
(Defer to Frank since he's most familiar with this, overall seems to me too)
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Xiaohan Wang, Frank Liberato, Sunny Sachanandani, Rafael Cintron.
9 comments:
File components/viz/common/quads/draw_quad_unittest.cc:
Patch Set #2, Line 349:
EXPECT_EQ(false, copy_quad->wants_promotion_hint);Nit: For regular boolean checks, I think EXPECT_FALSE should be sufficient here.
Ack
Patch Set #2, Line 611:
EXPECT_EQ(false, quad_new->wants_promotion_hint);Nit: For regular boolean checks, I think EXPECT_FALSE should be sufficient here.
Ack
File components/viz/common/quads/stream_video_draw_quad.h:
Patch Set #2, Line 35:
gfx::ProtectedVideoType video_type);promotion hint?
Ack
bool wants_promotion_hint : 1;
gfx::ProtectedVideoType protected_video_type : 2;Is there a particular reason we need these to be bit fields? If so, please add a comment.
So I went this direction because wants_promotion_hint & protected_video_type are shared concepts with other existing QUADS and I wanted to draw on the existing implementations.
The use of bit fields was already established for Texture QUADs here:
https://source.chromium.org/chromium/chromium/src/+/main:components/viz/common/quads/texture_draw_quad.hHowever - some other QUAD types didn't use them like YUV and StreamVideo.
I'll go ahead & align this one to not use bit fields as that aligns fine with the rest of StreamVideo (particularly given the other comment about initialization), but I'll keep TextureQuad with bit fields to match the rest of that file.
General alignment across QUADs can be a future endeavor if anyone ever wants to undertake it.
bool wants_promotion_hint : 1;
gfx::ProtectedVideoType protected_video_type : 2;
To avoid uninitialized data, please initialized these to reasonable default. […]
I believe inline bit field initialization isn't supported until C++20.
I can switch these to be non-bitfields though (given the YUV QUAD example (no bit fields) vs Texture QUAD example (bit fields)).
File components/viz/service/display/dc_layer_overlay.cc:
/* TODO (wicarr): If a Stream Video Draw Quad is tagged with the
* wants_promotion_hint tag then the creator wants to
* be notified of cases where promotion may fail so the Renderer can
* switch to a non-DC dependent texture type.
const StreamVideoDrawQuad* sv_quad =
StreamVideoDrawQuad::MaterialCast(*it);
if (sv_quad->wants_promotion_hint) {
// Process for hinting
}
*/
Is this commented out section a hint about a future change or did you mean for this code to be activ […]
Yeah - this is basically the hint at where the new wants_promotion_hinting is going to be used.
It requires introduction of the Promotion Hint Aggregation service on Windows.
In order to avoid cluttering up the code base I'll go ahead and remove these comments for now (folks can see the intention based on Patchset 1 or 2). The intention is that next piece should be fast following but it is a bit of large change.
/* TODO (wicarr): If a Texture Draw Quad is tagged with the
* wants_promotion_hint tag then the creator wants to
* be notified of cases where the surface may be promoted to a DC overlay
// so the Renderer may switch to a more performant DC dependent texture
type. const TextureDrawQuad* tex_quad =
TextureDrawQuad::MaterialCast(*it); if (tex_quad->wants_promotion_hint)
{
// Process for hinting
}
*/Same here, please either uncomment or add a comment about when the code will be uncomment.
Ack
File media/mojo/clients/win/media_foundation_renderer_client.cc:
Patch Set #2, Line 334:
// TODO(WiCarr): Should we set hw_protected when appropriate?Do you have an answer for this question you're asking yourself in your own CL?
I need to do a bit more research here.
We should be able to determine that we're in a HW DRM state based on the key request. (I need to look at the timing there though).
The more interesting piece is whether setting this would be interesting/of use in the GPU process. The StreamQuad that backs our DComp implementation used to always be assumed to be protected content but never differentiated between Software/Hardware (e.g. in DC Overlay Processor). With MF for Clear there will be scenarios where the StreamQuad isn't protected content but it still does need similar treatment (in the sense that it only currently works with promotion to an overlay). As such - I'm not sure yet whether there's any interesting consumption if we do set this appropriately. (It of course may be interesting for some degree of telemetry or things like that).
File media/renderers/video_resource_updater.cc:
gfx::ProtectedVideoType protected_video_type =
gfx::ProtectedVideoType::kClear;
if (frame->metadata().protected_video) {
if (frame->metadata().hw_protected)
protected_video_type = gfx::ProtectedVideoType::kHardwareProtected;
else
protected_video_type = gfx::ProtectedVideoType::kSoftwareProtected;
}
I see this code copied/pasted in a couple other places in this function. […]
Ack
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Xiaohan Wang, Frank Liberato, Sunny Sachanandani, Rafael Cintron.
1 comment:
Patchset:
overall seems fine. […]
So - I have a partial solution for this piece but I'm not sure I have all of the various pieces accounted for.
The partial solution involves detecting the Frame Readback request coming into Web Media Player Impl & the plan is to propagate that as an additional signal.
The signals from DC Overlay Processor + Web Media Player Impl will both be sent to Media Foundation Renderer Client which will make the final decision to switch between Frame Server & DComp mode based on the two sets of signals.
I'm not sure if there are additional GPU side frame readback signals that I'll also need to add logic to detect & send to the Media Foundation Renderer Client. The prototype I have for the piece that lives in the GPU process & is informed of signals from the DC Overlay Processor I'm planning to use the mailbox as the common identifier for since that is also known on the Renderer side, so I think the big trick will be at the time of processing if we have access to the QUAD to get the mailbox ID from for any such cases. Outside of that I plan to basically create an aggregator service that will live in the GPU service as a singleton-esque point of aggregation for all the GPU side signals.
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Xiaohan Wang, Frank Liberato, Sunny Sachanandani, Rafael Cintron.
1 comment:
File chrome/test/data/safe_browsing/documents/doc_containing_macros.doc:
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
Xiaohan Wang (Gerrit)
Attention is currently required from: William Carr, Frank Liberato, Sunny Sachanandani, Rafael Cintron.
1 comment:
Patchset:
Moving myself to CC since you have liberato@ covering the media/ side. Please let me know if there's anything/file I should take a look.
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: William Carr, Sunny Sachanandani, Rafael Cintron.
Patch set 6:Code-Review +1
3 comments:
Patchset:
media/ lgtm % nit. the metadata issue needs some discussion, maybe, but since it's just a comment in this CL, seems okay :)
thanks
-fl
File media/mojo/clients/win/media_foundation_renderer_client.cc:
Patch Set #2, Line 334:
// TODO(WiCarr): Should we set hw_protected when appropriate?
I need to do a bit more research here. […]
i don't think that hw_protected should be overloaded to force promotion of a clear texture. if that's all it's used for anywhere, then renaming it seems appropriate to me. else, a second flag for 'requires overlay' is fine.
seems like some quad type used to have a flag for 'requires overlay', but i can't find it now.
though nowadays, i'd expect that the backing could handle most of that anyway. the renderer might still need to know that certain operations won't work (e.g., copy to canvas), so that it can request frameserver mode.
File media/renderers/video_resource_updater.cc:
Patch Set #6, Line 64:
VideoFrameMetadataconst&
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
3 comments:
File chrome/test/data/safe_browsing/documents/doc_containing_macros.doc:
File media/mojo/clients/win/media_foundation_renderer_client.cc:
Patch Set #2, Line 334:
// TODO(WiCarr): Should we set hw_protected when appropriate?
i don't think that hw_protected should be overloaded to force promotion of a clear texture. […]
So - MF Renderer won't require it for DComp Mode w/ Clear Content. DComp Mode always uses a StreamVideo Quad as its backer & the DC Overlay Processor _always_ promotes StreamVideo Quads.
There is some code in swap_chain_presenter though that looks at the HW Protected value & uses it to modify some things on the swap chain:
if (protected_video_type == gfx::ProtectedVideoType::kHardwareProtected)
desc.Flags |= DXGI_SWAP_CHAIN_FLAG_HW_PROTECTED;// Always prefer YUV swap chain for hardware protected video for now.
if (protected_video_type == gfx::ProtectedVideoType::kHardwareProtected)
return yuv_overlay_format;<I'd need to look under debugger to be sure - but I believe all of these don't apply to MF Renderer as it will go through PresentDCompSurface & early exit SwapChainPresenter::PresentToSwapChain>
All that being said - we'd only tag this for actual HW Protected DRM scenarios, but I more wanted to call out that we DON'T do this today (and things seem to work as-is, so probably not required, but maybe we're missing some telemetry or something as a result).
File media/renderers/video_resource_updater.cc:
Patch Set #6, Line 64:
VideoFrameMetadataconst&
Done
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
William Carr abandoned this change.
To view, visit change 3230502. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Frank Li.
William Carr would like Frank Liberato, Sunny Sachanandani and Frank Li to review this change.
Media Foundation Frame Server Mode: Part 1: Frame Tagging
This change adds infrastructure to allow the existing
wants_promotion_hint tagging, currently used by Android, to be applied
to video frames backed by Texture Quads & Streaming Video Quads.
Media Foundation Renderer Client will apply this tag to frames for
for clear content scenarios. Tagging is currently included for the
existing DComp mode which is backed by Streaming Video frames. In a
future changelist Frame Server mode will be added which will be backed
by Texture frames and these frames will similarly be tagged by the
Media Foundation Renderer Client.
Additional future work will read this tagging in the DC Overlay
Processor to allow communication of promotional status back to the
Media Foundation Renderer Client to aid in deciding whether to use
Frame Server or DComp mode.
Bug: 1258887
Change-Id: I2783f4f4fb895571ba0ed84e420f9df86baf5679
---
M cc/test/render_pass_test_utils.cc
M components/viz/service/display/surface_aggregator_unittest.cc
M components/viz/service/display/gl_renderer_unittest.cc
M components/viz/test/compositor_frame_helpers.cc
M components/exo/surface.cc
M services/viz/public/cpp/compositing/mojom_traits_unittest.cc
M components/viz/common/quads/render_pass_io.cc
M components/viz/service/display/resolved_frame_data_unittest.cc
M services/viz/public/cpp/compositing/quads_mojom_traits.cc
M components/viz/common/quads/draw_quad_perftest.cc
M components/viz/common/quads/stream_video_draw_quad.cc
M components/viz/service/display/overlay_unittest.cc
M components/viz/service/display/overlay_ca_unittest.cc
M media/base/video_frame_metadata.h
M cc/layers/painted_overlay_scrollbar_layer_impl.cc
M cc/layers/painted_scrollbar_layer_impl.cc
M components/viz/service/display/renderer_perftest.cc
M third_party/blink/renderer/platform/graphics/canvas_resource_dispatcher.cc
M components/viz/service/transitions/surface_animation_manager.cc
M components/viz/service/display/display_perftest.cc
M cc/layers/texture_layer_impl.cc
M cc/layers/ui_resource_layer_impl.cc
M components/viz/common/quads/stream_video_draw_quad.h
M cc/layers/heads_up_display_layer_impl.cc
M components/viz/common/quads/draw_quad_unittest.cc
M components/viz/common/quads/texture_draw_quad.cc
M ash/fast_ink/view_tree_host_root_view.cc
M components/viz/service/display/renderer_pixeltest.cc
M components/viz/service/display/surface_aggregator_perftest.cc
M services/viz/public/cpp/compositing/quads_mojom_traits.h
M services/viz/public/mojom/compositing/quads.mojom
M media/mojo/clients/win/media_foundation_renderer_client.cc
M ash/fast_ink/fast_ink_host.cc
M device/vr/android/arcore/ar_compositor_frame_sink.cc
M services/viz/public/cpp/compositing/mojom_traits_perftest.cc
M cc/layers/nine_patch_generator.cc
M components/viz/common/quads/texture_draw_quad.h
M media/renderers/video_resource_updater.cc
M components/viz/common/quads/render_pass_io_unittest.cc
39 files changed, 345 insertions(+), 157 deletions(-)
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Frank Li.
1 comment:
Patchset:
I accidently created the original version of this change (https://chromium-review.googlesource.com/c/chromium/src/+/3230502) against chromium master, not realizing that switch to main had been done this summer. This resulted in some unexpected files being touched.
This is a remake of the same set of changes against main.
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: William Carr, Sunny Sachanandani, Frank Li.
Patch set 1:Code-Review +1
William Carr (Gerrit)
Attention is currently required from: Alex Gough, Will Harris, Sunny Sachanandani, Frank Li.
William Carr would like Alex Gough and Will Harris to review this change.
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Alex Gough, Will Harris, Sunny Sachanandani, Frank Li.
1 comment:
Patchset:
@Will & Alex - would one of you have time to review the quads_mojom_traits.cc change?
Thanks,
Bill
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Alex Gough, William Carr, Will Harris, Frank Li.
4 comments:
Patchset:
It seems there's a wants_promotion_hint already in TransferableResource - though it's OS_ANDROID only. What's the motivation for adding it to the quads too? Can't we just make the TransferableResource::wants_promotion_hint (and DisplayResourceProvider::DoesResourceWantPromotionHint) work for OS_WIN too?
That will get rid of most of the churn in this patch.
File components/viz/common/quads/stream_video_draw_quad.h:
Patch Set #1, Line 36:
promotion_hintingnit: use the same name as the member var i.e. wants_promotion_hint - also same order i.e. wants_promotion_hint before protected_video_type (or vice-versa in StreamVideoDrawQuad)
File components/viz/common/quads/texture_draw_quad.h:
Patch Set #1, Line 45:
promotion_hintingnit: same name as the member var i.e. wants_promotion_hint
Patch Set #1, Line 90:
bool wants_promotion_hint : 1;nit: can you describe what this is in a comment? also, you might want to put this above the damage_rect optional, otherwise struct layout and alignment will cause this to take up a full byte making the bit field declaration redundant.
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
Will Harris (Gerrit)
Attention is currently required from: William Carr, Frank Li.
2 comments:
Patchset:
@Will & Alex - would one of you have time to review the quads_mojom_traits.cc change? […]
please only add one security reviewer at a time.
this looks generally good but I will wait for other reviewers to make sure the API won't change substantially, as there's quite a few comments still. please mark me back for attn when it's ready for final security review.
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
Alex Gough (Gerrit)
Commit Message:
Patch Set #1, Line 13:
Media Foundation Renderer Client will apply this tag to frames fornit: for \n for
Patchset:
leaving for wfh but saw a nit.
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: William Carr, Frank Li.
William Carr uploaded patch set #2 to this change.
Media Foundation Frame Server Mode: Part 1: Frame Tagging
This change adds infrastructure to allow the existing
wants_promotion_hint tagging, currently used by Android, to be applied
to video frames backed by Texture Quads & Streaming Video Quads.
Media Foundation Renderer Client will apply this tag to frames for
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Sunny Sachanandani, Frank Li.
4 comments:
Commit Message:
Patch Set #1, Line 13:
Media Foundation Renderer Client will apply this tag to frames fornit: for \n for
Thanks! Fixed.
File components/viz/common/quads/stream_video_draw_quad.h:
Patch Set #1, Line 36:
promotion_hinting
nit: use the same name as the member var i.e. wants_promotion_hint - also same order i.e. […]
Done!
File components/viz/common/quads/texture_draw_quad.h:
Patch Set #1, Line 45:
promotion_hinting
nit: same name as the member var i.e. […]
So I've renamed the ones I introduced.
I was following the QUAD practice on naming here as these are exposed as public member variables the class members don't have the "_" to differentiate the class member from the function argument, so I've used "this" to disambiguate.
I didn't update the other members in the QUAD files I touched or other QUAD files, but I can if that's desired.
Patch Set #1, Line 90:
bool wants_promotion_hint : 1;
nit: can you describe what this is in a comment? also, you might want to put this above the damage_r […]
Added what I hope is a concise comment here.
Long term what's going to happen is if wants_promotion_hint is true then DC Overlay Processor will send the promotion status (and potentially some additional information like whether there's a capture scenario) to a new (not yet introduced) promotion hint aggregation service in the GPU. THe Promotion Hint Aggregation Service in GPU will allow clients to subscribe based on mailbox (e.g. Media Foundation Renderer Client) to know when a particular video frame (by mailbox) is promoted / demoted.
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: William Carr, Frank Li.
1 comment:
Patchset:
Hi wicarr@ - not sure if you saw this comment I made earlier:
"It seems there's a wants_promotion_hint already in TransferableResource - though it's OS_ANDROID only. What's the motivation for adding it to the quads too? Can't we just make the TransferableResource::wants_promotion_hint (and DisplayResourceProvider::DoesResourceWantPromotionHint) work for OS_WIN too?
That will get rid of most of the churn in this patch."
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Sunny Sachanandani, Frank Li.
1 comment:
Patchset:
Hi wicarr@ - not sure if you saw this comment I made earlier: […]
Hey Sunny - apologies for the delay was dealing with a fire drill.
Just saw your comment, I'm going to unresolve this comment to track.
I need to do a bit of reading up on the TransferableResource as I'm not familiar with how that ties into the VideoFrame/Quad relationship, but I assume it should be manageable so long as both the client & DC Overlay Processor have the appropriate access. This feature is intended to be similar to how Android's want_promotion_hint works, it just has some added cross process pieces, so if I can align that better I think that's a win.
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Sunny Sachanandani, Frank Li.
2 comments:
Patchset:
Hey Sunny - apologies for the delay was dealing with a fire drill. […]
Done
Patchset:
Latest version has update to use TranferableResource in stead of adding new tags on Quads.
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: William Carr, Frank Li.
2 comments:
File components/viz/common/resources/transferable_resource.h:
nit: empty line between the two #if blocks - here and in the other files too
File media/renderers/video_resource_updater.cc:
if (metadata.hw_protected)
video_type = gfx::ProtectedVideoType::kHardwareProtected;
else
video_type = gfx::ProtectedVideoType::kSoftwareProtected;
nit: braces around the if else blocks
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: William Carr, Frank Li.
Patch set 4:Code-Review +1
1 comment:
Patchset:
lgtm % nits
Thanks for addressing the comments!
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Frank Li.
3 comments:
Patchset:
Wfh@ - Could I get your review on the transferable_resource_mojom_traits.* changes? (Pretty reduced scope from previous iteration & very minor change to expose some functionality in Windows previously just on Android).
Thanks,
Bill
File components/viz/common/resources/transferable_resource.h:
nit: empty line between the two #if blocks - here and in the other files too
Done
File media/renderers/video_resource_updater.cc:
if (metadata.hw_protected)
video_type = gfx::ProtectedVideoType::kHardwareProtected;
else
video_type = gfx::ProtectedVideoType::kSoftwareProtected;nit: braces around the if else blocks
Done
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
Will Harris (Gerrit)
Attention is currently required from: William Carr, Frank Li.
Patch set 5:Code-Review +1
1 comment:
Patchset:
lgtm mojo traits
To view, visit change 3268536. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: William Carr, Frank Li.
Patch set 5:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Media Foundation Frame Server Mode: Part 1: Frame Tagging
This change adds infrastructure to allow the existing
wants_promotion_hint tagging, currently used by Android, to be applied
to video frames backed by Texture Quads & Streaming Video Quads.
Media Foundation Renderer Client will apply this tag to frames for
clear content scenarios. Tagging is currently included for the
existing DComp mode which is backed by Streaming Video frames. In a
future changelist Frame Server mode will be added which will be backed
by Texture frames and these frames will similarly be tagged by the
Media Foundation Renderer Client.
Additional future work will read this tagging in the DC Overlay
Processor to allow communication of promotional status back to the
Media Foundation Renderer Client to aid in deciding whether to use
Frame Server or DComp mode.
Bug: 1258887
Change-Id: I2783f4f4fb895571ba0ed84e420f9df86baf5679
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3268536
Reviewed-by: Sunny Sachanandani <sun...@chromium.org>
Reviewed-by: Frank Liberato <libe...@chromium.org>
Reviewed-by: Will Harris <w...@chromium.org>
Commit-Queue: William Carr <wic...@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#949143}
---
M components/viz/service/display/display_resource_provider.cc
M components/viz/common/resources/transferable_resource.h
M services/viz/public/cpp/compositing/transferable_resource_mojom_traits.h
M media/mojo/clients/win/media_foundation_renderer_client.cc
M components/viz/service/display/display_resource_provider.h
M media/renderers/video_resource_updater.cc
M media/base/video_frame_metadata.h
M services/viz/public/cpp/compositing/transferable_resource_mojom_traits.cc
8 files changed, 75 insertions(+), 21 deletions(-)