[Region Capture #4] Glue Viz and CC Layers Together [chromium/src : main]
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Philip Rogers, Elad Alon, mark a. foltz.
Jordan Bayles would like Elad Alon and mark a. foltz to review this change.
[Region Capture #4] Glue Viz and CC Layers Together
Overall feature:
----------------
Region Capture is a feature allowing the cropping of self-capture video
tracks to track the coordinates of an HTMLElement in the browsing
context.
Public design-doc:
https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8/edit?usp=sharing
Spec draft (very WIP): https://eladalon1983.github.io/region-capture/
(This will be finished soon and moved to the WICG; hopefully to be
eventually adopted by the W3C.)
This CL:
--------
This patch includes the final changes to get produceCropId working end
to end, by connecting the CC changes and Viz changes landed in previous
patches through the RenderSurfaceImpl class.
Next CLs:
---------
No implementation patches, only bug fixes and cleanups from here on out!
Bug: 1247761
Change-Id: I0a7527020e239dca0aef25c1f531179ca06cf757
---
M cc/layers/layer_impl.cc
M cc/layers/render_surface_impl.cc
M third_party/blink/renderer/modules/mediastream/media_devices.cc
M components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc
M cc/trees/draw_property_utils.cc
M cc/layers/region_capture_bounds.h
M cc/layers/region_capture_bounds.cc
M components/viz/service/frame_sinks/compositor_frame_sink_support.cc
M third_party/blink/renderer/modules/mediastream/media_devices.idl
M cc/layers/layer_impl.h
M cc/layers/render_surface_impl.h
11 files changed, 129 insertions(+), 9 deletions(-)
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Philip Rogers, Elad Alon, mark a. foltz.
1 comment:
Patchset:
This is still somewhat WIP, please ignore the components/ and third_party/ changes as they are for manual testing only.
I was hoping someone could point me to where layer scrolling works, I haven't had any luck getting the correct transform for when the page is scrolled. Any ideas?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
Attention is currently required from: Jordan Bayles, Philip Rogers, Elad Alon, mark a. foltz.
4 comments:
Patchset:
Are there some test pages I can try this out on?
File cc/layers/layer_impl.cc:
Patch Set #4, Line 367:
const auto viewport_bounds_delta = gfx::ToCeiledVector2d(Can you add a comment to explain this? I'm not sure I quite understand myself - we offset the capture bounds for the inner viewport scroll layer only by the current top controls visibility, and we also don't seem to adjust the width / height?
Are the bounds meant to be in layer space? If so, wouldn't the layer space bounds not need to be offset?
Patch Set #4, Line 370:
out.Offset(viewport_bounds_delta);Is this tested?
File cc/trees/layer_tree_impl.h:
Patch Set #2, Line 889:
RegionCaptureBounds capture_bounds_;Looks like this is unused?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Philip Rogers, Elad Alon, mark a. foltz.
1 comment:
File cc/layers/layer_impl.cc:
Patch Set #4, Line 367:
const auto viewport_bounds_delta = gfx::ToCeiledVector2d(
Can you add a comment to explain this? I'm not sure I quite understand myself - we offset the captur […]
Alright, this kind of gets to the meat of my issue I suppose.
My ultimate goal is to have the coordinates of an element in pixels on the viewport, so I can crop my CopyOutputRequests in the viz FrameSinkVideoCapturer to only the bounds of the element, basically this chain:
HTMLElement ->
Painter ->
PaintChunk ->
cc::Layer ->
cc::LayerImpl ->
cc::RenderSurface ->
viz::CompositorRenderPass ->
viz::FrameSinkVideoCapturerImplI also know that (1) I am definitely turned around some on what coordinate space is what and where, and (2) my CL definitely does not factor in scrolling, and when I scroll in my test page using this patch the bounds do not update properly.
I think scrolling occurs here in the compositor, right? I added this to match the behavior with bounds but obviously this isn't right as-is.
Do you have any insight here? I am focusing this week on fixing and testing this logic, any insight would be awesome since I am obviously struggling.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
mark a. foltz (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Philip Rogers, Elad Alon.
3 comments:
Patchset:
My high level understanding is that transformation from layer space to screen space happens as part of the configuration and processing of the render pass, in particular RenderPassInternal::transform_to_root_target seems important.
However exact manner in which the render pass transforms are consumed is hard to follow - clearly the surface aggregator is involved, but would need to read more code to understand fully, and documentation of the relevant data structures is sparse.
File cc/layers/region_capture_bounds.cc:
Patch Set #4, Line 25:
LOG(INFO) << "Scaling by " << factor;You'll want to remove logging statements before submitting (or convert to DVLOG if they are for temporary debugging).
Patch Set #4, Line 33:
LOG(INFO) << "Offsetting by " << offset.ToString();Ditto
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
mark a. foltz (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Philip Rogers, Elad Alon.
1 comment:
File third_party/blink/renderer/modules/mediastream/media_devices.idl:
Patch Set #4, Line 47:
// TODO: re-add runtime enabling (currently disabled for testing).FYI, currently this feature should be default disabled - it can be enabled through the --enable-blink-features=RegionCapture switch.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Philip Rogers, Elad Alon, mark a. foltz.
6 comments:
Patchset:
My high level understanding is that transformation from layer space to screen space happens as part […]
That looks super interesting, I'll take a look at it now.
Are there some test pages I can try this out on?
File cc/layers/layer_impl.cc:
Patch Set #4, Line 370:
out.Offset(viewport_bounds_delta);Is this tested?
I don't think this actually helps us here, am not seeing any examples from testing where is_inner_viewport_scroll_layer is true.
File cc/layers/region_capture_bounds.cc:
Patch Set #4, Line 25:
LOG(INFO) << "Scaling by " << factor;
You'll want to remove logging statements before submitting (or convert to DVLOG if they are for temp […]
Absolutely, these are intended to be temporary while sorting out the transformation logic. I'll leave this comments open for now.
File cc/trees/layer_tree_impl.h:
Patch Set #2, Line 889:
RegionCaptureBounds capture_bounds_;Looks like this is unused?
Yep, need to remove it.
File third_party/blink/renderer/modules/mediastream/media_devices.idl:
Patch Set #4, Line 47:
// TODO: re-add runtime enabling (currently disabled for testing).
FYI, currently this feature should be default disabled - it can be enabled through the --enable-blin […]
Nice, I'll switch to using the command line switch instead of this hack. I'll leave open until removed.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Philip Rogers, Elad Alon, mark a. foltz.
2 comments:
Patchset:
That looks super interesting, I'll take a look at it now.
transform_to_root_target is actually screen_space_transform(), which I am trying to utilize currently but definitely am missing something still.
File third_party/blink/renderer/modules/mediastream/media_devices.idl:
Patch Set #4, Line 47:
// TODO: re-add runtime enabling (currently disabled for testing).
Nice, I'll switch to using the command line switch instead of this hack. […]
Done
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Philip Rogers (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Elad Alon, mark a. foltz.
3 comments:
File cc/layers/layer_impl.h:
Patch Set #6, Line 275:
// properly includes viewport offset is returned."When retrieving, a copy that properly includes viewport offset is returned."
Is this true? I think no, and you can just remove this comment.
File cc/layers/render_surface_impl.h:
Patch Set #6, Line 278:
RegionCaptureBounds capture_bounds_;I don't see any other metadata-like code here, which makes me think this is the wrong place to store this information. If the ultimate goal is to put this on CompositorFrame, should we be storing this data on FrameData or CompositorFrameMetadata rather than render surfaces?
I think we could use some viz advice. WDYT of adding sadrul@ as a reviewer?
File cc/layers/render_surface_impl.cc:
Patch Set #6, Line 352:
gfx::Rect scaled_bounds = Filters().MapRect(IIRC, don't we ignore clipping, occlusion, and other effects? I think the filter mapping code is for something like an offset filter.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, mark a. foltz.
Jordan Bayles would like Sadrul Chowdhury and Sadrul Chowdhury to review this change.
[Region Capture #4] Glue Viz and CC Layers Together
Overall feature:
----------------
Region Capture is a feature allowing the cropping of self-capture video
tracks to track the coordinates of an HTMLElement in the browsing
context.
Public design-doc:
https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8/edit?usp=sharing
Spec draft (very WIP): https://eladalon1983.github.io/region-capture/
(This will be finished soon and moved to the WICG; hopefully to be
eventually adopted by the W3C.)
This CL:
--------
This patch includes the final changes to get produceCropId working end
to end, by connecting the CC changes and Viz changes landed in previous
patches through the RenderSurfaceImpl class.
Next CLs:
---------
No implementation patches, only bug fixes and cleanups from here on out!
Bug: 1247761
Change-Id: I0a7527020e239dca0aef25c1f531179ca06cf757
---
M third_party/blink/renderer/platform/runtime_enabled_features.json5
M cc/layers/render_surface_impl.cc
M third_party/blink/renderer/modules/mediastream/media_devices.cc
M components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc
M cc/trees/draw_property_utils.cc
M cc/layers/region_capture_bounds.h
M components/viz/service/frame_sinks/compositor_frame_sink_support.cc
M third_party/blink/renderer/modules/mediastream/media_devices.idl
M cc/layers/render_surface_impl.h
9 files changed, 97 insertions(+), 4 deletions(-)
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, mark a. foltz.
4 comments:
File cc/layers/layer_impl.h:
Patch Set #6, Line 275:
// properly includes viewport offset is returned.
"When retrieving, a copy that properly includes viewport offset is returned." […]
Done
File cc/layers/render_surface_impl.h:
Patch Set #6, Line 278:
RegionCaptureBounds capture_bounds_;
I don't see any other metadata-like code here, which makes me think this is the wrong place to store […]
Definitely agree on viz advice. Adding and leaving this open.
File cc/layers/render_surface_impl.cc:
Patch Set #6, Line 352:
gfx::Rect scaled_bounds = Filters().MapRect(
IIRC, don't we ignore clipping, occlusion, and other effects? I think the filter mapping code is for […]
Done
Patch Set #6, Line 352:
gfx::Rect scaled_bounds = Filters().MapRect(
IIRC, don't we ignore clipping, occlusion, and other effects? I think the filter mapping code is for […]
Done
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon.
3 comments:
Patchset:
We should get some tests for the various edge cases of mapping the bounds, e.g.:
- scrolls
- transforms
- non-root render surfaces with their own transforms
- non-root scrolls / clips
File cc/layers/render_surface_impl.cc:
Patch Set #7, Line 349:
screen_space_transform()I think you want to use the screen space transform of the contributing layer, not of this RenderSurfaceImpl, i.e. layer->ScreenSpaceTransform().
Patch Set #7, Line 353:
gfx::ToEnclosedRect(DrawableContentRect()));You'll probably also need to project the DrawableContentRect by the screen_space_transform() of this RenderSurfaceImpl.
I think you'll also want to clip by any other clips affecting the element visibility.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon.
Patch set 8:Commit-Queue +1
3 comments:
Patchset:
Updated, still having issues with scrolling transforms. I'll keep working on it.
File cc/layers/render_surface_impl.cc:
Patch Set #7, Line 349:
screen_space_transform()
I think you want to use the screen space transform of the contributing layer, not of this RenderSurf […]
Done
Patch Set #7, Line 353:
gfx::ToEnclosedRect(DrawableContentRect()));
You'll probably also need to project the DrawableContentRect by the screen_space_transform() of this […]
Done
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon.
1 comment:
Patchset:
Updated, still having issues with scrolling transforms. I'll keep working on it.
When I was debugging this I was seeing the scroll reflected in the screen space transform of the LayerImpl. Are you not seeing it reflected there?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon.
1 comment:
Patchset:
When I was debugging this I was seeing the scroll reflected in the screen space transform of the Lay […]
I actually am now... looks like I have a bug passing the bounds to the capturer. I should have a new revision up soon that fixes this (fingers crossed!).
Thanks for all the help so far, I'll update when I have the changes incorporated. A very exciting day!!!
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, mark a. foltz, Greg Kerr.
Jordan Bayles would like Greg Kerr to review this change.
[Region Capture #4] Glue Viz and CC Layers Together
Overall feature:
----------------
Region Capture is a feature allowing the cropping of self-capture video
tracks to track the coordinates of an HTMLElement in the browsing
context.
Public design-doc:
https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8/edit?usp=sharing
Spec draft (very WIP): https://eladalon1983.github.io/region-capture/
(This will be finished soon and moved to the WICG; hopefully to be
eventually adopted by the W3C.)
This CL:
--------
This patch includes the final changes to get produceCropId working end
to end, by connecting the CC changes and Viz changes landed in previous
patches through the RenderSurfaceImpl class.
Next CLs:
---------
No implementation patches, only bug fixes and cleanups from here on out!
Bug: 1247761
Change-Id: I0a7527020e239dca0aef25c1f531179ca06cf757
---
M cc/layers/layer_impl.cc
M components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc
M components/viz/common/surfaces/region_capture_bounds.cc
M cc/layers/region_capture_bounds.cc
M components/viz/common/quads/compositor_render_pass.h
M components/viz/service/frame_sinks/compositor_frame_sink_support.cc
M components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
M services/viz/public/cpp/compositing/mojom_traits_unittest.cc
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc
M components/viz/common/quads/compositor_frame_metadata.h
M components/viz/common/quads/compositor_render_pass_unittest.cc
M components/viz/common/surfaces/region_capture_bounds.h
M cc/layers/layer_impl.h
M cc/trees/layer_tree_host_impl.h
M services/viz/public/mojom/compositing/compositor_render_pass.mojom
M components/viz/common/quads/compositor_render_pass.cc
M third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer_test.cc
M cc/layers/layer.h
M third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer.cc
M services/viz/public/mojom/compositing/compositor_frame_metadata.mojom
M third_party/blink/renderer/core/page/scrolling/scrolling_test.cc
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.cc
M cc/trees/layer_tree_host_impl.cc
M components/viz/common/quads/compositor_frame.h
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.h
M components/viz/common/transition_utils.cc
M services/viz/public/cpp/compositing/mojom_traits_perftest.cc
M components/viz/service/display/display_unittest.cc
M cc/layers/layer.cc
30 files changed, 160 insertions(+), 94 deletions(-)
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, mark a. foltz, Greg Kerr.
Patch set 9:Commit-Queue +1
6 comments:
Patchset:
I actually am now... looks like I have a bug passing the bounds to the capturer. […]
Done
Patchset:
Greg, mind taking a look at the Viz changes? This is a refactor + cleanup as well as some new code.
File cc/layers/layer_impl.cc:
Patch Set #4, Line 367:
const auto viewport_bounds_delta = gfx::ToCeiledVector2d(Alright, this kind of gets to the meat of my issue I suppose. […]
Done
File cc/layers/region_capture_bounds.cc:
Patch Set #4, Line 25:
LOG(INFO) << "Scaling by " << factor;
Absolutely, these are intended to be temporary while sorting out the transformation logic. […]
Done
Patch Set #4, Line 33:
LOG(INFO) << "Offsetting by " << offset.ToString();Ditto
Done
File cc/layers/render_surface_impl.h:
Patch Set #6, Line 278:
RegionCaptureBounds capture_bounds_;
Definitely agree on viz advice. Adding and leaving this open.
Moved to compositor frame metadata, ended up causing a rather large refactor.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, mark a. foltz, Greg Kerr.
1 comment:
Patchset:
NOTE: still have to remove some of the hacked code before checkin, I"ll leave this open until that is resolved.
/usr/local/src/src/components/viz/service/frame_sinks/compositor_frame_sink_support.cc:1054: Missing username in TODO; it should look like "// TODO(my_username): Stuff." [readability/todo] [2]
/usr/local/src/src/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc:241: Missing username in TODO; it should look like "// TODO(my_username): Stuff." [readability/todo] [2]
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, mark a. foltz, Greg Kerr.
Patch set 10:Commit-Queue +1
1 comment:
Patchset:
Hopefully fixed the SameSizeAsLayer issue on Android, I'm guessing it's a memory alignment issue of some kind?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Philip Rogers (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Elad Alon, mark a. foltz, Greg Kerr.
1 comment:
File cc/layers/layer.cc:
Patch Set #10, Line 1114:
if (inputs_.capture_bounds == bounds) {This would previously not cause a commit (SetNeedsCommit) if there were no changes. I think we want to preserve that behavior. What was the reason for switching RegionCaptureBounds to be a unique ptr?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Greg Kerr (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Elad Alon, mark a. foltz.
1 comment:
Patchset:
Can you add me back once this has +1s from the non-owner reviewers?
Thanks,
Greg
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
mark a. foltz (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Elad Alon.
8 comments:
Patchset:
Getting closer, there were a lot of changes from PS#4 so I may have missed some design changes in the interim.
File cc/layers/layer.cc:
Patch Set #10, Line 1114:
inputs_.capture_bounds = std::move(bounds);The previous code in PS#4 checked for equality before assignment, which would shortcut the following calls. On the other hand, checking flat_maps for equality is O(N).
File cc/trees/layer_tree_host_impl.cc:
Patch Set #10, Line 2197:
auto bounds = std::make_unique<viz::RegionCaptureBounds>();Consider optimizing to only allocate when needed (i.e. the 99.99% case is there are no bounds).
Patch Set #10, Line 2219:
return bounds;This will always return a viz::RegionCaptureBounds (even if empty). Is that intentional? If not, consider returning a nullptr.
File components/viz/common/quads/compositor_render_pass.h:
File components/viz/common/surfaces/region_capture_bounds.h:
Patch Set #10, Line 25:
// https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8Nit: it's nice to use tinyurl for these long links
File components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc:
Patch Set #10, Line 241:
// TODO: remove this hack before checkin.Still relevant?
File services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc:
Patch Set #10, Line 70:
out->capture_bounds =Is out already a std::unique_ptr? If so would reset() work here?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, mark a. foltz.
Patch set 11:Commit-Queue +1
12 comments:
Patchset:
NOTE: still have to remove some of the hacked code before checkin, I"ll leave this open until that i […]
Done
Patchset:
Can you add me back once this has +1s from the non-owner reviewers? […]
Sounds good, can do.
Getting closer, there were a lot of changes from PS#4 so I may have missed some design changes in th […]
Yeah, @pdr recommended in code review feedback that this really shouldn't live on the render surface impl, and changing that turned out to be non-trivial.
Patchset:
PTAL. @kerrnel asked to be readded once some general LGTMs come in.
File cc/layers/layer.cc:
Patch Set #10, Line 1114:
if (inputs_.capture_bounds == bounds) {This would previously not cause a commit (SetNeedsCommit) if there were no changes. […]
Done
Patch Set #10, Line 1114:
inputs_.capture_bounds = std::move(bounds);
The previous code in PS#4 checked for equality before assignment, which would shortcut the following […]
Done
File cc/trees/layer_tree_host_impl.cc:
Patch Set #10, Line 2197:
auto bounds = std::make_unique<viz::RegionCaptureBounds>();Consider optimizing to only allocate when needed (i.e. the 99.99% case is there are no bounds).
Done
Patch Set #10, Line 2219:
return bounds;
This will always return a viz::RegionCaptureBounds (even if empty). […]
Done
File components/viz/common/quads/compositor_render_pass.h:
File components/viz/common/surfaces/region_capture_bounds.h:
Patch Set #10, Line 25:
// https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8Nit: it's nice to use tinyurl for these long links
Shame I can't use goo.gl.
File components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc:
Patch Set #10, Line 241:
// TODO: remove this hack before checkin.Still relevant?
Done
File services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc:
Patch Set #10, Line 70:
out->capture_bounds =Is out already a std::unique_ptr? If so would reset() work here?
out is a unique pointer, yes, but bounds is an absl::optional so I don't think reset() works here since it only takes a pointer type.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, mark a. foltz.
Jordan Bayles uploaded patch set #12 to this change.
[Region Capture #4] Glue Viz and CC Layers Together
Overall feature:
----------------
Region Capture is a feature allowing the cropping of self-capture video
tracks to track the coordinates of an HTMLElement in the browsing
context.
Public design-doc:
https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8/edit?usp=sharing
Spec draft (very WIP): https://eladalon1983.github.io/region-capture/
(This will be finished soon and moved to the WICG; hopefully to be
eventually adopted by the W3C.)
This CL:
--------
This patch includes the final changes to get produceCropId working end
to end, by connecting the CC changes and Viz changes landed in previous
patches through the compositor frame metadata class.
Some refactors are included as part of introducing these changes:
1. moving RegionCaptureBounds to a std::unique_ptr to decrease
memory usage, as an empty base::flat_map is much larger than a unique
pointer.
2. Instead of putting the capture bounds on individual render passes,
the capture bounds now reside on the CompositorFrameMetadata class and
are populated by the LayerTreeHostImpl.
Next CLs:
---------
No implementation patches, only bug fixes and cleanups from here on out!
Bug: 1247761
Change-Id: I0a7527020e239dca0aef25c1f531179ca06cf757
---
M cc/layers/layer_impl.cc
M components/viz/common/surfaces/region_capture_bounds.cc
M cc/layers/region_capture_bounds.cc
M components/viz/common/quads/compositor_render_pass.h
M components/viz/service/frame_sinks/compositor_frame_sink_support.cc
M components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
M services/viz/public/cpp/compositing/mojom_traits_unittest.cc
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc
M components/viz/common/quads/compositor_frame_metadata.h
M components/viz/common/quads/compositor_render_pass_unittest.cc
M components/viz/common/surfaces/region_capture_bounds.h
M cc/layers/layer_impl.h
M cc/trees/layer_tree_host_impl.h
M services/viz/public/mojom/compositing/compositor_render_pass.mojom
M components/viz/common/quads/compositor_render_pass.cc
M third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer_test.cc
M cc/layers/layer.h
M third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer.cc
M services/viz/public/mojom/compositing/compositor_frame_metadata.mojom
M third_party/blink/renderer/core/page/scrolling/scrolling_test.cc
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.cc
M cc/layers/region_capture_bounds.h
M cc/trees/layer_tree_host_impl.cc
M components/viz/common/quads/compositor_frame.h
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.h
M components/viz/common/transition_utils.cc
M services/viz/public/cpp/compositing/mojom_traits_perftest.cc
M components/viz/service/display/display_unittest.cc
M cc/layers/layer.cc
30 files changed, 161 insertions(+), 90 deletions(-)
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
mark a. foltz (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon.
Patch set 12:Code-Review +1
5 comments:
Patchset:
Minor nits, otherwise LGTM!
File cc/layers/layer.cc:
Patch Set #12, Line 1114:
if (!bounds && !inputs_.capture_bounds)bounds == inputs_.capture_bounds would also check for pointer equality.
Patch Set #12, Line 1116:
if (bounds && inputs_.capture_bounds && *bounds == *inputs_.capture_bounds)Nit: Extra newline after brace-less if.
File cc/trees/layer_tree_host_impl.cc:
Patch Set #12, Line 2219:
bounds = std::make_unique<viz::RegionCaptureBounds>();Slightly prefer reset(new) to assign an already-allocated unique_ptr, but don't feel strongly. They should hopefully compile to the same thing.
Also, needs a newline after a brace-less if.
File services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc:
Patch Set #10, Line 70:
out->capture_bounds =
out is a unique pointer, yes, but bounds is an absl::optional so I don't think reset() works here si […]
Ok, can you ensure that |bounds| has a move ctor so its contents can be moved into out->capture_bounds?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Philip Rogers (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Elad Alon.
1 comment:
Patchset:
Sadrul, would you be able to review this, or suggest a good reviewer of the viz approach?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Elad Alon, Greg Kerr.
Jordan Bayles would like Greg Kerr to review this change.
M cc/trees/layer_tree_host_impl_unittest.cc
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc
M components/viz/common/quads/compositor_frame_metadata.h
M components/viz/common/quads/compositor_render_pass_unittest.cc
M components/viz/common/surfaces/region_capture_bounds.h
M cc/layers/layer_impl.h
M cc/trees/layer_tree_host_impl.h
M services/viz/public/mojom/compositing/compositor_render_pass.mojom
M components/viz/common/quads/compositor_render_pass.cc
M third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer_test.cc
M cc/layers/layer.h
M third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer.cc
M services/viz/public/mojom/compositing/compositor_frame_metadata.mojom
M third_party/blink/renderer/core/page/scrolling/scrolling_test.cc
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.cc
M cc/layers/region_capture_bounds.h
M cc/trees/layer_tree_host_impl.cc
M components/viz/common/quads/compositor_frame.h
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.h
M components/viz/common/transition_utils.cc
M services/viz/public/cpp/compositing/mojom_traits_perftest.cc
M components/viz/service/display/display_unittest.cc
M cc/layers/layer.cc
31 files changed, 239 insertions(+), 98 deletions(-)
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Elad Alon, Greg Kerr.
Patch set 13:Commit-Queue +1
4 comments:
Patchset:
Replied to feedback and added some tests to the layer tree host implementation, which has the meat of the logic in this patch.
@kerrnel: I think mfoltz@ is the only non-owner reviewer (other than Elad, who I have moved to CC). I think this is ready for you to take a look.
Thanks everyone, PTAL.
File cc/layers/layer.cc:
Patch Set #12, Line 1114:
if (!bounds && !inputs_.capture_bounds)bounds == inputs_.capture_bounds would also check for pointer equality.
Done
Patch Set #12, Line 1116:
if (bounds && inputs_.capture_bounds && *bounds == *inputs_.capture_bounds)Nit: Extra newline after brace-less if.
Done
File cc/trees/layer_tree_host_impl.cc:
Patch Set #12, Line 2219:
bounds = std::make_unique<viz::RegionCaptureBounds>();
Slightly prefer reset(new) to assign an already-allocated unique_ptr, but don't feel strongly. […]
I'll just add brackets, I generally prefer them and uses the same amount of lines.
I know we prefer std::make_unique over std::unique_ptr<>(new), but interesting about the reset thing. I don't have a strong preference, I'll just change it to reset(new).
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Elad Alon, Greg Kerr.
Jordan Bayles has uploaded this change for review.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Elad Alon, Greg Kerr.
Patch set 14:Commit-Queue +1
3 comments:
File cc/layers/region_capture_bounds.h:
Patch Set #13, Line 26:
RegionCaptureBounds(single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google-explicit-constructor.html)
Please fix.
File cc/trees/layer_tree_host_impl.cc:
Patch Set #12, Line 2219:
bounds = std::make_unique<viz::RegionCaptureBounds>();
I'll just add brackets, I generally prefer them and uses the same amount of lines. […]
Got a tricium error for using std::make_unique instead. I'll switch it back.
File cc/trees/layer_tree_host_impl.cc:
Patch Set #13, Line 2231:
bounds.reset(new viz::RegionCaptureBounds());use std::make_unique instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize-make-unique.html)
Please fix.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Elad Alon (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Greg Kerr.
1 comment:
File cc/trees/layer_tree_host_impl.cc:
Patch Set #14, Line 2231:
RegionCaptureBoundsviz::RegionCaptureBounds
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Sadrul Chowdhury (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Elad Alon, Greg Kerr, kylechar.
Sadrul Chowdhury would like kylechar to review this change authored by Jordan Bayles.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Sadrul Chowdhury (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Elad Alon, Greg Kerr, kylechar.
1 comment:
Patchset:
This looks reasonable to me in a quick glance. But kylechar@ mind reviewing this CL?
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Elad Alon, Greg Kerr, kylechar.
Patch set 16:Commit-Queue +1
2 comments:
File cc/layers/layer.cc:
Patch Set #12, Line 1114:
if (!bounds && !inputs_.capture_bounds)
Done
This actually isn't what we want, and causes a failure in the CompositingSimTest.LayerUpdatesDoNotInvalidateLaterLayers method.
I didn't do this originally because I assumed it would be a pointer equality, which isn't what we want: we want a value equality, regardless if they are the same pointer or not, and since the argument is a unique_ptr it's basically impossible for them to be the same.
File cc/trees/layer_tree_host_impl.cc:
Patch Set #14, Line 2231:
RegionCaptureBoundsviz::RegionCaptureBounds
Done
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
mark a. foltz (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Elad Alon, Greg Kerr, kylechar.
Patch set 16:Code-Review +1
1 comment:
File cc/layers/layer.cc:
Patch Set #12, Line 1114:
if (!bounds && !inputs_.capture_bounds)
This actually isn't what we want, and causes a failure in the CompositingSimTest. […]
Yes, the value equality check can't be removed. Sorry for not being clearer.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Elad Alon, Greg Kerr, kylechar.
1 comment:
File cc/layers/layer.cc:
Patch Set #12, Line 1114:
if (!bounds && !inputs_.capture_bounds)
Yes, the value equality check can't be removed. Sorry for not being clearer.
Oh yeah, that was my bad. Communication is fun sometimes :).
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Elad Alon, Greg Kerr, kylechar.
Jordan Bayles has uploaded this change for review.
31 files changed, 241 insertions(+), 98 deletions(-)
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Elad Alon, Greg Kerr, kylechar.
1 comment:
Patchset:
Update reviewers.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr, kylechar.
3 comments:
File cc/layers/layer.cc:
Patch Set #10, Line 1114:
if (inputs_.capture_bounds == bounds) {Done
I'm still not sure why we need a unique_ptr here. It seems to me that we're not saving any copies since we still need to copy when pushing to the LayerImpl and in cases where we don't need to copy we can std::move the value.
File cc/trees/layer_tree_host_impl.h:
Patch Set #17, Line 1305:
std::unique_ptr<gfx::Rect> content_rect_screen_space_for_testing_;Do we need this testing structure? Can't we call active_tree()->RootRenderSurface()->SetContentRectForTesting?
File cc/trees/layer_tree_host_impl_unittest.cc:
Patch Set #17, Line 18379:
child_layer_transform.Scale(2.0, 3.0);It would also be good to have a test of a non scale/translate transform, e.g. a rotation, where we should get the bounding box right?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Pinpoint perf (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr, kylechar.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/179a1091720000
kylechar (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr.
4 comments:
Commit Message:
base::flat_map is much larger than a unique
pointer.
It's 8 vs 24 bytes (or 4 vs 12 bytes for x86), two extra pointers, so I'm not sure that qualifies as "much larger". What motivated switching to a unique_ptr exactly?
FWIW if the extra 16 bytes are important than a cleaner interface would be to make RegionCaptureBounds hold a std::unique_ptr<base::flat_map<...>> and have it encapsulate the logic.
File components/viz/common/surfaces/region_capture_bounds.h:
Patch Set #17, Line 23:
// tab capture.Can you document that all of the gfx::Rects are captured from the CompositorFrames root render pass and therefore they are in that coordinate space?
File components/viz/service/frame_sinks/compositor_frame_sink_support.cc:
Patch Set #17, Line 1045:
for (const SurfaceId& id : surface_manager_->GetCreatedSurfaceIds()) {Just to clarify here, does the renderer producing the content add the RegionCaptureBounds to it's own CompositorFrame(Metadata)?
File services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h:
Patch Set #17, Line 151:
static const absl::optional<viz::RegionCaptureBounds> capture_bounds(So assuming you want to pass around a unique_ptr<RegionCaptureBounds> you should just have the mojom type mapping go directly to unique_ptr<RegionCaptureBounds> rather than copying into/out of an optional<RegionCaptureBounds>. Look at how DelegateInkMetadata [1] does it.
Pinpoint perf (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr.
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14648a4e720000
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr, kylechar.
Patch set 19:Commit-Queue +1
8 comments:
Commit Message:
base::flat_map is much larger than a unique
pointer.
It's 8 vs 24 bytes (or 4 vs 12 bytes for x86), two extra pointers, so I'm not sure that qualifies as […]
I chatted with Philip Rogers, I was under the impression that there were more layers than there are. I have had several Pinpoint bugs around using too much memory on ElementRareData and wanted to not add additional pinpoint bugs.
Patchset:
Patch Set 17:
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/179a1091720000
File cc/layers/layer.cc:
Patch Set #10, Line 1114:
if (inputs_.capture_bounds == bounds) {
I'm still not sure why we need a unique_ptr here. […]
It's not really about saving copies, it's about saving space. 99% of layers in the wild will not have region capture bounds, and a std::unique_ptr is only one address large (4 or 8 bytes), where as an empty base::flat_map is usually something like 24 bytes.
I have had several separate pinpoint bugs for this feature and switching to unique pointer is an obvious win for memory and perf.
File cc/trees/layer_tree_host_impl.h:
Patch Set #17, Line 1305:
std::unique_ptr<gfx::Rect> content_rect_screen_space_for_testing_;
Do we need this testing structure? Can't we call active_tree()->RootRenderSurface()->SetContentRectF […]
No, because it's in screen space we would also need to set the screen_space_transform() which ends up being much more complicated. I tried that approach first but thought this was cleaner. Is there a different property equivalent to:
gfx::ToEnclosedRect(
MathUtil::MapClippedRect(root_surface->screen_space_transform(),
root_surface->DrawableContentRect()));?
File cc/trees/layer_tree_host_impl_unittest.cc:
Patch Set #17, Line 18379:
child_layer_transform.Scale(2.0, 3.0);
It would also be good to have a test of a non scale/translate transform, e.g. […]
Done
File components/viz/common/surfaces/region_capture_bounds.h:
Patch Set #17, Line 23:
// tab capture.
Can you document that all of the gfx::Rects are captured from the CompositorFrames root render pass […]
Done
File components/viz/service/frame_sinks/compositor_frame_sink_support.cc:
Patch Set #17, Line 1045:
for (const SurfaceId& id : surface_manager_->GetCreatedSurfaceIds()) {
Just to clarify here, does the renderer producing the content add the RegionCaptureBounds to it's ow […]
Did some digging and testing, looks like the current_surface (with last_activated_surface_id_) does not appear to always (or ever, so far in testing) contain the RegionCaptureBounds.
I verified this by cherry picking Elad's #5, #6, and #7 patches and refactoring this method to only use the current_surface.
If it's fine with you, it's not clear to me why it's not on the current surface and due to timeline concerns I would love to fix this in a follow up patch, especially since this iteration isn't actually being changed in this patch.
https://bugs.chromium.org/p/chromium/issues/detail?id=1263744
File services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h:
Patch Set #17, Line 151:
static const absl::optional<viz::RegionCaptureBounds> capture_bounds(
So assuming you want to pass around a unique_ptr<RegionCaptureBounds> you should just have the mojom […]
I'll just switch to RegionCaptureBounds directly starting with layers (and leave the unique pointer on PaintChunk).
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr, kylechar.
1 comment:
File components/viz/service/frame_sinks/compositor_frame_sink_support.cc:
Patch Set #17, Line 1045:
for (const SurfaceId& id : surface_manager_->GetCreatedSurfaceIds()) {
Did some digging and testing, looks like the current_surface (with last_activated_surface_id_) does […]
@kylechar, was taking a look through things specifically in regards to what surface the RegionCaptureBounds actually get set on.
Here's the current flow:
1. Element E with gets marked with a region capture crop id through produceCropId.
2. When painting, E gets turned into a paint chunk, EP.
3. EP gets turned into a layer L in paint_chunks_to_cc_layer.cc
4. The layer L gets turned into a LayerImpl.
5. The LayerTreeHostImpl, when creating CompositorFrameMetadata, calls CollectRegionCaptureBounds() which iterates through all the layers, including layer L.
6. The metadata gets sent to Viz through the CompositorFrameMetadata, which gets us to the capturer.It looks like the RegionCaptureBounds for element E are definitely on one of the surface's GetActiveFrameMetadata(), however it is not the same surface as last_activated_surface_id_.
I don't know the surface logic well enough to say, but it definitely seems to me that the contents of the tab are in a different surface than the one we think is the current surface. Any idea why that is?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
kylechar (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr.
1 comment:
File components/viz/service/frame_sinks/compositor_frame_sink_support.cc:
Patch Set #17, Line 1045:
for (const SurfaceId& id : surface_manager_->GetCreatedSurfaceIds()) {
@kylechar, was taking a look through things specifically in regards to what surface the RegionCaptur […]
What is |last_activated_surface_id_| vs what is the SurfaceId of the surface that has the RegionCaptureBounds?
If I understand the feature correctly renderer R1 has some area of it's content identified as a region for capture and RegionCaptureBounds are added to it's CompositorFrameMetadata. That captured content is going to be used by renderer R2 to embed/show/whatever.
It sounds like you're creating a FrameSinkVideoCaptuerer for the FrameSinkId/CompositorFrameSink of R2 instead of R1? Something like that.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
kylechar (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr.
4 comments:
Commit Message:
base::flat_map is much larger than a unique
pointer.
I chatted with Philip Rogers, I was under the impression that there were more layers than there are. […]
Gotcha, those 16 bytes will add up a lot faster on each layer. There are a lot of Layers/LayerImpls for every CompositorFrame.
Can you update the commit message though. This isn't being changed in this CL anymore right?
File cc/layers/region_capture_bounds.h:
Patch Set #20, Line 22:
class CC_EXPORT RegionCaptureBounds final {So this can be a follow up.. but why is this class duplicated in cc and viz? You can use viz::RegionCpatureBounds from inside //cc.
File components/viz/service/frame_sinks/compositor_frame_sink_support.cc:
if (bounds.bounds().empty()) {
continue;
}nit: This isn't needed anymore.
File services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h:
Patch Set #17, Line 151:
static const absl::optional<viz::RegionCaptureBounds> capture_bounds(
I'll just switch to RegionCaptureBounds directly starting with layers (and leave the unique pointer […]
Gotcha. Well whatever pinpoint identifies as a memory win sounds reasonable but by the time you get to viz the unique_ptr doens't seem necessary. FWIW there can be lots of layers plus there are multiple copies (layer tree on main plus active/pending layer tree impls on impl thread) but it sounds like there are even more PaintChunks?
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr.
Jordan Bayles uploaded patch set #21 to this change.
[Region Capture #4] Glue Viz and CC Layers Together
Overall feature:
----------------
Region Capture is a feature allowing the cropping of self-capture video
tracks to track the coordinates of an HTMLElement in the browsing
context.
Public design-doc:
https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8/edit?usp=sharing
Spec draft (very WIP): https://eladalon1983.github.io/region-capture/
(This will be finished soon and moved to the WICG; hopefully to be
eventually adopted by the W3C.)
This CL:
--------
This patch includes the final changes to get produceCropId working end
to end, by connecting the CC changes and Viz changes landed in previous
patches through the compositor frame metadata class.
Instead of putting the capture bounds on individual render passes,
the capture bounds now reside on the CompositorFrameMetadata class and
are populated by the LayerTreeHostImpl.
Next CLs:
---------
No implementation patches, only bug fixes and cleanups from here on out!
Bug: 1247761
Change-Id: I0a7527020e239dca0aef25c1f531179ca06cf757
---
M services/viz/public/mojom/compositing/compositor_frame_metadata.mojom
M components/viz/common/surfaces/region_capture_bounds.cc
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.cc
M cc/layers/region_capture_bounds.h
M cc/layers/region_capture_bounds.cc
M cc/trees/layer_tree_host_impl.cc
M components/viz/common/quads/compositor_render_pass.h
M components/viz/service/frame_sinks/compositor_frame_sink_support.cc
M components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
M components/viz/common/quads/compositor_frame.h
M services/viz/public/cpp/compositing/mojom_traits_unittest.cc
M cc/trees/layer_tree_host_impl_unittest.cc
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.h
M components/viz/common/transition_utils.cc
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc
M components/viz/common/quads/compositor_frame_metadata.h
M services/viz/public/cpp/compositing/mojom_traits_perftest.cc
M components/viz/service/display/display_unittest.cc
M components/viz/common/quads/compositor_render_pass_unittest.cc
M components/viz/common/surfaces/region_capture_bounds.h
M cc/trees/layer_tree_host_impl.h
M services/viz/public/mojom/compositing/compositor_render_pass.mojom
M components/viz/common/quads/compositor_render_pass.cc
24 files changed, 212 insertions(+), 85 deletions(-)
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr, kylechar.
Patch set 22:Commit-Queue +1
6 comments:
Commit Message:
base::flat_map is much larger than a unique
pointer.
Gotcha, those 16 bytes will add up a lot faster on each layer. […]
Yeah we can do this as a follow up if necessary.
Patchset:
Fixed the nits, doing some surface ID testing still.
File cc/layers/region_capture_bounds.h:
Patch Set #20, Line 22:
class CC_EXPORT RegionCaptureBounds final {
So this can be a follow up.. […]
I wanted to separate layers more, but realizing how coupled CC and Viz are I think deduplicating it is likely worth it and relatively easy as a follow up.
https://bugs.chromium.org/p/chromium/issues/detail?id=1264029
File components/viz/service/frame_sinks/compositor_frame_sink_support.cc:
if (bounds.bounds().empty()) {
continue;
}nit: This isn't needed anymore.
Done
if (bounds.bounds().empty()) {
continue;
}nit: This isn't needed anymore.
Done
if (bounds.bounds().empty()) {
continue;
}nit: This isn't needed anymore.
Done
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr, kylechar.
2 comments:
File cc/layers/layer.cc:
Patch Set #10, Line 1114:
if (inputs_.capture_bounds == bounds) {
It's not really about saving copies, it's about saving space. […]
Got it, with such an impact it sounds like we should consider creating a rare properties structure to house this and a bunch of other rarely set fields (e.g. wheel_event_region, touch_action_region, non_fast_scrollable_region). Fine to just move this field for now, but can you leave a TODO in layer.h and possibly link a bug with some of those pinpoint results?
File cc/trees/layer_tree_host_impl.h:
Patch Set #17, Line 1305:
std::unique_ptr<gfx::Rect> content_rect_screen_space_for_testing_;
No, because it's in screen space we would also need to set the screen_space_transform() which ends u […]
This adds complexity to the code - keeping a debugging member around and switching out part of the code based on it. In general we should avoid adding specific one off hooks like this and rely on higher level test setup - i.e. setting up a render surface with the correct bounds and tranform.
I would imagine that the default screen_space_transform() in unit tests would be the identity transform. There is also an API to set the screen space transform on the render surface.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Robert Flack, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr, kylechar.
Patch set 25:Commit-Queue +1
4 comments:
Patchset:
Fixed up the feedback and tests and did some manual testing around the surface identifier concerns.
PTAL.
File cc/layers/layer.cc:
Patch Set #10, Line 1114:
if (inputs_.capture_bounds == bounds) {
Got it, with such an impact it sounds like we should consider creating a rare properties structure t […]
https://bugs.chromium.org/p/chromium/issues/detail?id=1264177
File cc/trees/layer_tree_host_impl.h:
Patch Set #17, Line 1305:
std::unique_ptr<gfx::Rect> content_rect_screen_space_for_testing_;
This adds complexity to the code - keeping a debugging member around and switching out part of the c […]
I was having some issues setting up the render surface correctly and figured this would be better but I do definitely agree it is additional complication in runtime that is not necessary. Mocking and class dependency management in C++ is not great, I am not really a fan of how tightly coupled this test suite is to the render surface.
I was able to resolve this much more elegantly, I didn't realize I need to call CopyProperties() for every child layer, as well as UpdateDrawProperties and DrawFrame to set up the RenderSurfaceImpls. Definitely more setup with side effects than I would like for a unit test, so added some explanatory comments and hopefully it's maintainable for future devs.
File components/viz/service/frame_sinks/compositor_frame_sink_support.cc:
Patch Set #17, Line 1045:
for (const SurfaceId& id : surface_manager_->GetCreatedSurfaceIds()) {
What is |last_activated_surface_id_| vs what is the SurfaceId of the surface that has the RegionCapt […]
Added some logging, TL;DR is the surface is actually on a different frame sink and different local surface ID looks like?
I have a separate bug that I think we could handle as a follow up to this patch: https://bugs.chromium.org/p/chromium/issues/detail?id=1263744, but let me know if you think this is actually not a problem or want it resolved in this patch and have suggestions.
Full logs:
[2104102:2104125:1027/162749.904436:INFO:compositor_frame_sink_support.cc(1045)] Last activated surface ID: SurfaceId(FrameSinkId(47, 2), LocalSurfaceId(4, 1, 3F94...))
[2104102:2104125:1027/162749.904558:INFO:compositor_frame_sink_support.cc(1046)] Crop ID: 0847F958014B455AB8D9BD7A02A668C4
[2104102:2104125:1027/162749.904674:INFO:compositor_frame_sink_support.cc(1058)] found non empty bounds on surface SurfaceId(FrameSinkId(50, 6), LocalSurfaceId(5, 1, 9154...)) with bounds {{0847F958014B455AB8D9BD7A02A668C4, 353,23 391x267}}
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
kylechar (Gerrit)
Attention is currently required from: Robert Flack, Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr.
Patch set 25:Code-Review +1
2 comments:
Patchset:
viz/* lgtm.
File components/viz/service/frame_sinks/compositor_frame_sink_support.cc:
Patch Set #17, Line 1045:
for (const SurfaceId& id : surface_manager_->GetCreatedSurfaceIds()) {
Added some logging, TL;DR is the surface is actually on a different frame sink and different local s […]
That.. doesn't seem right. Let's move discussion to the bug though since it's directly not related to this CL.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr.
Patchset:
LGTM with nit
File cc/layers/region_capture_bounds.cc:
Patch Set #25, Line 18:
: bounds_(bounds) {}nit: This should accept the flat_map by value and std::move it into bounds_.
Currently there will always be 1 copy of the map when it's copied into bounds_, but if you accept the parameter by value and std::move it into bounds_ then if the caller passes an r-value (as the unit tests do) there will be no copy, and at worse there will be 1 copy into the value bounds.
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Robert Flack (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Philip Rogers, Elad Alon, Greg Kerr.
1 comment:
File cc/trees/layer_tree_host_impl.h:
Patch Set #17, Line 1305:
std::unique_ptr<gfx::Rect> content_rect_screen_space_for_testing_;
I was having some issues setting up the render surface correctly and figured this would be better bu […]
Looks much better! Thanks!
Philip Rogers (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Elad Alon, Greg Kerr.
Patch set 25:Code-Review +1
1 comment:
Patchset:
LGTM
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Greg Kerr (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Elad Alon.
1 comment:
Patchset:
I'm overbooked this week, can you ask another mojom reviewer?
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Elad Alon (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Emily Stark.
Elad Alon would like Emily Stark to review this change authored by Jordan Bayles.
M components/viz/common/surfaces/region_capture_bounds.cc
M cc/layers/region_capture_bounds.cc
M components/viz/common/quads/compositor_render_pass.h
M components/viz/service/frame_sinks/compositor_frame_sink_support.cc
M components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc
M services/viz/public/cpp/compositing/mojom_traits_unittest.cc
M cc/trees/layer_tree_host_impl_unittest.cc
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc
M components/viz/common/quads/compositor_frame_metadata.h
M components/viz/common/quads/compositor_render_pass_unittest.cc
M components/viz/common/surfaces/region_capture_bounds.h
M cc/trees/layer_tree_host_impl.h
M services/viz/public/mojom/compositing/compositor_render_pass.mojom
M components/viz/common/quads/compositor_render_pass.cc
M cc/layers/layer.h
M services/viz/public/mojom/compositing/compositor_frame_metadata.mojom
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.cc
M cc/layers/region_capture_bounds.h
M cc/trees/layer_tree_host_impl.cc
M components/viz/common/quads/compositor_frame.h
M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h
M services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.h
M components/viz/common/transition_utils.cc
M services/viz/public/cpp/compositing/mojom_traits_perftest.cc
M components/viz/service/display/display_unittest.cc
25 files changed, 204 insertions(+), 88 deletions(-)
Elad Alon (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury, Emily Stark.
1 comment:
Patchset:
estark:
services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc
services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h
services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.cc
services/viz/public/cpp/compositing/compositor_render_pass_mojom_traits.h
services/viz/public/mojom/compositing/compositor_frame_metadata.mojom
services/viz/public/mojom/compositing/compositor_render_pass.mojom
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Jordan Bayles (Gerrit)
Attention is currently required from: Sadrul Chowdhury, Emily Stark.
Patch set 26:Commit-Queue +1
2 comments:
Patchset:
Thanks Emily for jumping in! This is my last patch for this feature for M97 so hoping we can get this checked in ASAP. I'll try to apply any feedback ASAP.
File cc/layers/region_capture_bounds.cc:
Patch Set #25, Line 18:
: bounds_(bounds) {}
nit: This should accept the flat_map by value and std::move it into bounds_. […]
Done
To view, visit change 3229811. To unsubscribe, or for help writing mail filters, visit settings.
Emily Stark (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury.
Patch set 26:Code-Review +1
Jordan Bayles (Gerrit)
Attention is currently required from: Jordan Bayles, Sadrul Chowdhury.
Patch set 26:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Region Capture #4] Glue Viz and CC Layers Together
Overall feature:
----------------
Region Capture is a feature allowing the cropping of self-capture video
tracks to track the coordinates of an HTMLElement in the browsing
context.
Public design-doc:
https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8/edit?usp=sharing
Spec draft (very WIP): https://eladalon1983.github.io/region-capture/
(This will be finished soon and moved to the WICG; hopefully to be
eventually adopted by the W3C.)
This CL:
--------
This patch includes the final changes to get produceCropId working end
to end, by connecting the CC changes and Viz changes landed in previous
patches through the compositor frame metadata class.
Instead of putting the capture bounds on individual render passes,
the capture bounds now reside on the CompositorFrameMetadata class and
are populated by the LayerTreeHostImpl.
Next CLs:
---------
No implementation patches, only bug fixes and cleanups from here on out!
Bug: 1247761
Change-Id: I0a7527020e239dca0aef25c1f531179ca06cf757
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3229811
Reviewed-by: Emily Stark <est...@chromium.org>
Reviewed-by: kylechar <kyle...@chromium.org>
Reviewed-by: Robert Flack <fla...@chromium.org>
Reviewed-by: Philip Rogers <p...@chromium.org>
Reviewed-by: mark a. foltz <mfo...@chromium.org>
Commit-Queue: Jordan Bayles <jop...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#937202}
25 files changed, 212 insertions(+), 88 deletions(-)