Add support for screen-shooting a single surface#4936
Open
jhenstridge wants to merge 2 commits into
Open
Conversation
ace1198 to
1917eef
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds infrastructure to support capturing a single toplevel surface (a prerequisite for implementing toplevel capture via ext_image_copy_capture_v1) by introducing a Scene implementation that only exposes one mir::scene::Surface, and extending ScreenShooterFactory to construct screen shooters against an alternate Scene.
Changes:
- Added
SingleSurfaceSceneandcreate_surface_scene()to expose renderables for a singleSurface. - Extended
mir::compositor::ScreenShooterFactorywithcreate_for_scene()and wired it throughBasicScreenShooterFactoryandNullScreenShooterFactory. - Updated scene build configuration to compile the new scene implementation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/scene/single_surface_scene.h | Declares a Scene implementation backed by a single Surface. |
| src/server/scene/single_surface_scene.cpp | Implements the single-surface scene and the create_surface_scene() factory. |
| src/server/scene/CMakeLists.txt | Adds the new scene implementation to the scene library build. |
| src/server/compositor/null_screen_shooter_factory.h | Adds the new factory override for alternate-scene shooter creation. |
| src/server/compositor/null_screen_shooter_factory.cpp | Implements create_for_scene() for the null implementation. |
| src/server/compositor/basic_screen_shooter_factory.h | Adds create_for_scene() to build BasicScreenShooter against a provided scene. |
| src/server/compositor/basic_screen_shooter_factory.cpp | Implements delegation from create() to create_for_scene(). |
| src/include/server/mir/scene/surface_scene.h | Introduces the public(ish) factory declaration for creating a surface-backed scene. |
| src/include/server/mir/compositor/screen_shooter_factory.h | Extends the screen-shooter factory interface to support alternate scenes. |
Comments suppressed due to low confidence (1)
src/server/compositor/basic_screen_shooter_factory.cpp:53
ScreenShooterFactorynow supports creating shooters for alternate scenes, andBasicScreenShooterFactory::create()delegates tocreate_for_scene(). There is existing unit coverage forcreate(), but nothing validates thatcreate_for_scene()actually uses the provided scene (vs the factory’s stored default) and behaves correctly for non-default scenes. Please extendtests/unit-tests/compositor/test_basic_screen_shooter_factory.cpp(or equivalent) to covercreate_for_scene()with a distinct scene instance.
auto mc::BasicScreenShooterFactory::create(Executor& executor) -> std::unique_ptr<ScreenShooter>
{
return create_for_scene(executor, scene);
}
auto mc::BasicScreenShooterFactory::create_for_scene(Executor& executor, std::shared_ptr<Scene> const& scene) -> std::unique_ptr<ScreenShooter>
{
return std::make_unique<BasicScreenShooter>(
scene, clock, executor, providers, renderer_factory, buffer_allocator, config, output_filter, cursor);
}
Comment on lines
+20
to
+23
| #include <mir/compositor/scene.h> | ||
| #include <mir/scene/observer.h> | ||
| #include "surface_stack.h" | ||
|
|
Comment on lines
+81
to
+92
| mc::SceneElementSequence elements; | ||
| if (auto const locked = surface.lock()) | ||
| { | ||
| for (auto& renderable : locked->generate_renderables(id)) | ||
| { | ||
| elements.emplace_back( | ||
| std::make_shared<SurfaceSceneElement>( | ||
| locked->name(), | ||
| renderable, | ||
| rendering_tracker, | ||
| id)); | ||
| } |
Comment on lines
+32
to
+67
| class SurfaceSceneElement : public mc::SceneElement | ||
| { | ||
| public: | ||
| SurfaceSceneElement( | ||
| std::string name, | ||
| std::shared_ptr<mg::Renderable> const& renderable, | ||
| std::shared_ptr<ms::RenderingTracker> const& tracker, | ||
| mc::CompositorID id) | ||
| : renderable_{renderable}, | ||
| tracker{tracker}, | ||
| cid{id}, | ||
| surface_name(name) | ||
| { | ||
| } | ||
|
|
||
| std::shared_ptr<mg::Renderable> renderable() const override | ||
| { | ||
| return renderable_; | ||
| } | ||
|
|
||
| void rendered() override | ||
| { | ||
| tracker->rendered_in(cid); | ||
| } | ||
|
|
||
| void occluded() override | ||
| { | ||
| tracker->occluded_in(cid); | ||
| } | ||
|
|
||
| private: | ||
| std::shared_ptr<mg::Renderable> const renderable_; | ||
| std::shared_ptr<ms::RenderingTracker> const tracker; | ||
| mc::CompositorID cid; | ||
| std::string const surface_name; | ||
| }; |
| } | ||
|
|
||
| ms::SingleSurfaceScene::SingleSurfaceScene( | ||
| std::shared_ptr<Surface> const &surface) |
Comment on lines
+70
to
+95
| ms::SingleSurfaceScene::SingleSurfaceScene( | ||
| std::shared_ptr<Surface> const &surface) | ||
| : surface{surface}, | ||
| rendering_tracker{std::make_shared<ms::RenderingTracker>(surface)} | ||
| { | ||
| } | ||
|
|
||
| auto ms::SingleSurfaceScene::scene_elements_for(mc::CompositorID id) -> mc::SceneElementSequence | ||
| { | ||
| RecursiveReadLock lg(guard); | ||
|
|
||
| mc::SceneElementSequence elements; | ||
| if (auto const locked = surface.lock()) | ||
| { | ||
| for (auto& renderable : locked->generate_renderables(id)) | ||
| { | ||
| elements.emplace_back( | ||
| std::make_shared<SurfaceSceneElement>( | ||
| locked->name(), | ||
| renderable, | ||
| rendering_tracker, | ||
| id)); | ||
| } | ||
| } | ||
| return elements; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: #4710
What's new?
This adds some infrastructure necessary to implement toplevel capture via
ext_image_copy_capture_v1. In particular:mir::compositor::Sceneimplementation that only includes the renderables for a singlemir::scene::Surface.mir::compositor::ScreenShooterFactoryto allow creating screen shooters for an alternative scene.Together these will let us implement an
ExtImageCopyBackendfor toplevel capture:One open question is what resolution to capture at. The simplest would be to just capture at 1x. If there are no subsurfaces, a better choice would be to capture at that surface's native resolution. If there are subsurfaces, there's no guarantee that they will have the same scale factor.
How to test
This doesn't add the Wayland protocol pieces to make use of these changes. I hacked it into the output capture protocol to use a screen shooter for the surface at (100, 100) when the session is started:
Screencast.From.2026-05-08.12-15-29.mp4
Here you can see a single window is captured by wayvnc, and continues to render when occluded. For X11 apps, the decorations were a separate surface to the application content. That might need some additional work to support if we care.
Checklist