Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for not compositing until at least one renderable has been sent to the compositor #3086

Merged
merged 8 commits into from
Oct 26, 2023

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Oct 20, 2023

To test

  1. Checkout Support for not compositing until at least one renderable has been sent to the compositor #3086 and run sudo make install
  2. Checkout Add a --wallpaper argument to optionally disable wallpaper draws ubuntu-frame#157 and build it
  3. From a VT, run plymouth
sudo plymouthd
sudo plymouth show-splash
sudo plymouth deactivate

The splash should still be on the screen
4. Run frame:

./frame --wallpaper=false
  1. Wait however long you like and note that you still see the splash
  2. Run some other app (e.g. gedit) and note that the splash disappears

What's new?

  • DefaultDisplayBufferCompositor::composite now returns a boolean telling the caller if any compositing happened or not
  • If the MultithreadedCompositor needs compositing, a post will happen, otherwise nothing will happen

@mattkae mattkae marked this pull request as ready for review October 20, 2023 19:39
@Saviq
Copy link
Collaborator

Saviq commented Oct 24, 2023

Empirically, this behaves fine, the only difference seems to be that e.g. mir_demo_server will not draw until there's a renderable, and may look stuck?

We could want to allow compositors to force composition? Or maybe log (periodically?) that we're waiting for a renderable? @AlanGriffiths thoughts?

Couple tests to fix:

The following tests FAILED:
	 55 - mir_integration_tests.SurfaceStackCompositor.* (Failed)
	110 - mir_unit_tests.DefaultDisplayBufferCompositor.* (Failed)

@mattkae
Copy link
Contributor Author

mattkae commented Oct 24, 2023

Couple tests to fix:

The following tests FAILED:
	 55 - mir_integration_tests.SurfaceStackCompositor.* (Failed)
	110 - mir_unit_tests.DefaultDisplayBufferCompositor.* (Failed)

There are quite a few tests here so I'll fix them once we know that this is the right solution. One question I have is that do we need to Renderer::suspend in this case as well?

I appear to have broken a bunch of the tests where we're trying to hit the render optimization with zero renderables. That optimization path probably shouldn't be hit if we have this other optimization, but perhaps someone more familiar with that optimization can tell me if I'm wrong :)

@AlanGriffiths
Copy link
Collaborator

This approach looks sensible.

The failing tests are partly because we've changed the expected behaviour, but more because they are bad tests (testing the implementation works the way it is implemented, not the requirements, and testing more than one behaviour in a test).

It may be worth reworking the tests as a pre-requisite

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #3086 (dc8730a) into main (60a5409) will decrease coverage by 0.01%.
The diff coverage is 97.40%.

@@            Coverage Diff             @@
##             main    #3086      +/-   ##
==========================================
- Coverage   78.42%   78.41%   -0.01%     
==========================================
  Files        1073     1073              
  Lines       74714    74689      -25     
==========================================
- Hits        58593    58567      -26     
- Misses      16121    16122       +1     
Files Coverage Δ
.../server/mir/compositor/display_buffer_compositor.h 100.00% <ø> (ø)
...r/compositor/default_display_buffer_compositor.cpp 77.35% <100.00%> (+1.84%) ⬆️
...rc/server/compositor/multi_threaded_compositor.cpp 96.35% <100.00%> (+0.05%) ⬆️
...t/doubles/null_display_buffer_compositor_factory.h 100.00% <100.00%> (ø)
...ork/headless_display_buffer_compositor_factory.cpp 84.00% <100.00%> (+0.32%) ⬆️
...positor/test_default_display_buffer_compositor.cpp 99.30% <100.00%> (-0.21%) ⬇️
...ests/compositor/test_multi_threaded_compositor.cpp 98.22% <100.00%> (+0.22%) ⬆️
...ation-tests/test_surface_stack_with_compositor.cpp 94.14% <92.85%> (-0.27%) ⬇️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@AlanGriffiths AlanGriffiths added this pull request to the merge queue Oct 26, 2023
Merged via the queue into main with commit 0c846c6 Oct 26, 2023
26 of 33 checks passed
@AlanGriffiths AlanGriffiths deleted the feature/not_compositing_right_away branch October 26, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants