Add a --wallpaper argument to optionally disable wallpaper draws #157

mattkae · 2023-10-19T20:06:44Z

Depends on

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

To test

Checkout Support for not compositing until at least one renderable has been sent to the compositor mir#3086 and run sudo make install
Checkout Add a --wallpaper argument to optionally disable wallpaper draws #157 and build it
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

Wait however long you like and note that you still see the splash
Run some other app (e.g. gedit) and note that the splash disappears

What's new?

Added a wallpaper_enabled flag that is set to true by default
Only doing things in BackgroundClient::Self::draw_screen if we have errors or the wallpaper is enabled
If we have an error and we don't want the wallpaper but we don't have a diagnostic file, then we draw the wallpaper as we did before (this may be undesirable, but its a very rare case, and it maintains previous behavior)

Saviq

@mattkae were you able to confirm this DTRT as far as smooth boot is concerned?

If I leave plymouth running and deactivate it, then start Frame without the wallpaper on tty1, I get black immediately, until the timeout passes and we're back to the wallpaper. We want the splash screen to stay until we draw the client / wallpaper / diagnostic. Do we need to bring in some of #3033 for this?

That said, mir_demo_server behaves similarly - it goes to black straight up.

I'm on qemu, mind you, and see this warning, so maybe that's why?

 < -warning- > gbm-kms: Unable to determine the current display mode.

I'd probably go for just --wallpaper=<true|false>, BTW.

AlanGriffiths · 2023-10-20T08:35:20Z

@mattkae were you able to confirm this DTRT as far as smooth boot is concerned?

I don't think this can be to address seamless boot directly: that needs the server to defer compositing until content from the client app is available. This just means that compositing will start without a background (as you say, just like mir_demo_server, or miriway without swaybg).

The idea might be to simplify detecting whether there's an app providing content by preventing the "wallpaper client" from providing content. But I'm not convinced that's necessary, or a good idea.

mattkae · 2023-10-20T13:22:20Z

@Saviq @AlanGriffiths Alan is right on this one. The compositor itself is going to clear the screen once it is ready to start drawing surfaces. As Alan said, we could make it not do anything until it first has a "renderable" to render. Otherwise, we could do the tougher task of reading in the plymouth buffer and rendering that ourselves until the user has supplied another renderable.

Saviq · 2023-10-20T13:34:18Z

I don't think this can be to address seamless boot directly: that needs the server to defer compositing until content from the client app is available. This just means that compositing will start without a background (as you say, just like mir_demo_server, or miriway without swaybg).

As Alan said, we could make it not do anything until it first has a "renderable" to render.

Yeah that's what I think we should be doing. Off the top I can't think of why it couldn't be the default behaviour?

The idea might be to simplify detecting whether there's an app providing content by preventing the "wallpaper client" from providing content. But I'm not convinced that's necessary, or a good idea.

Do you have another approach in mind? Why / how would we want to treat the background client in a special way? In truth, we could probably (when this, or similar, option is engaged) just delay the background client altogether by $timeout, no smaller than the diagnostic timeout?

Otherwise, we could do the tougher task of reading in the plymouth buffer and rendering that ourselves until the user has supplied another renderable.

That wouldn't change much compared to the above, would it?

I mean, not until we start doing something more with the buffer.

Saviq · 2023-10-24T12:25:45Z

OK so this, together with canonical/mir#3086 looks exactly like what we need :)

I'm just trying to think about what's the best user experience. A --wallpaper-timeout came to mind, but then isn't the diagnostic what we'd like to see first, if configured? So the wallpaper timeout would have to be counted towards the diagnostic one.

But do we really need another timeout? The behaviour here, I think, is mostly correct - the first thing we see (with --wallpaper=false) is the diagnostic. We still see the wallpaper if there is no diagnostic text, so that's the icky bit - alternative would be to stay on the splash screen indefinitely, but maybe that's what we should do?

If someone disabled the wallpaper, and does not have the diagnostic set up - maybe the splash screen should stay on screen?

mattkae · 2023-10-24T14:02:04Z

OK so this, together with MirServer/mir#3086 looks exactly like what we need :)

I'm just trying to think about what's the best user experience. A --wallpaper-timeout came to mind, but then isn't the diagnostic what we'd like to see first, if configured? So the wallpaper timeout would have to be counted towards the diagnostic one.

But do we really need another timeout? The behaviour here, I think, is mostly correct - the first thing we see (with --wallpaper=false) is the diagnostic. We still see the wallpaper if there is no diagnostic text, so that's the icky bit - alternative would be to stay on the splash screen indefinitely, but maybe that's what we should do?

If someone disabled the wallpaper, and does not have the diagnostic set up - maybe the splash screen should stay on screen?

Maybe if there isn't any diagnostic text but there is some error, then we show some default diagnostic text? I agree that it feels weird to show the background out of nowhere when there's an error. I'm up for either some default text or just keeping the splash on screen. Although, leaving the splash on screen makes it a bit trickier to debug from a user's perspective

Saviq · 2023-10-24T14:11:05Z

Maybe if there isn't any diagnostic text but there is some error, then we show some default diagnostic text? I agree that it feels weird to show the background out of nowhere when there's an error.

It's not that there is an error, necessarily. And even if there is - Frame can't know what it is.

I'm up for either some default text or just keeping the splash on screen. Although, leaving the splash on screen makes it a bit trickier to debug from a user's perspective

Well, that's exactly what the diagnostic is for. The "user" in the case of Frame shouldn't be doing any debugging, just call the number that's on screen :)

And just to confirm we're DTRT:

The before:

Nagranie.ekranu.z.2023-10-24.16-10-07.webm

And the after:

Nagranie.ekranu.z.2023-10-24.16-02-07.webm

The white flash is rendered by Flutter, so it's in the application author's hands now.

Saviq · 2023-10-24T14:19:07Z

@mattkae so personally I'm erring on --wallpaper=false doing what it says on the tin… just not draw the wallpaper, ever. If there is a diagnostic on, that will pop up as usual.

@AlanGriffiths thoughts?

mattkae · 2023-10-24T14:28:31Z

Well, that's exactly what the diagnostic is for. The "user" in the case of Frame shouldn't be doing any debugging, just call the number that's on screen :)

That makes sense to me! So if they fail to set up the diagnostic and the wallpaper is turned off, then we keep showing the splash. Does that sound right?

And just to confirm we're DTRT:

And yup, that's what I would expect. The splash is shown all the way up to the first render of the app.

Saviq · 2023-10-24T15:01:21Z

That makes sense to me! So if they fail to set up the diagnostic and the wallpaper is turned off, then we keep showing the splash. Does that sound right?

Yeah exactly.

And just to confirm we're DTRT:

And yup, that's what I would expect. The splash is shown all the way up to the first render of the app.

Oh I wasn't asking ;D

mattkae · 2023-10-24T15:41:31Z

@Saviq I have implemented + tested only showing the diagnostic screen when we have diagnostic info 👍

Saviq · 2023-10-24T15:52:41Z

@mattkae that if/else dance looks like it could use some rework?

mattkae · 2023-10-24T17:13:15Z

@mattkae that if/else dance looks like it could use some rework?

As in its too verbose, or is it incorrect?

Saviq · 2023-10-24T18:06:57Z

As in its too verbose, or is it incorrect?

I mean this:

https://github.com/MirServer/ubuntu-frame/blob/9f247ce1b325f8b74fe3ddb7d400346a9e36232a/src/background_client.cpp#L401-L416

and then this:

https://github.com/MirServer/ubuntu-frame/blob/9f247ce1b325f8b74fe3ddb7d400346a9e36232a/src/background_client.cpp#L457-L467

The control flow got quite twisted?

Saviq

Looks good to me!

@AlanGriffiths would like your ACK in case I'm not thinking of something here…

AlanGriffiths

Some small const cleanup and some (non blocking) bikeshedding about the naming

AlanGriffiths · 2023-10-25T09:52:16Z

src/background_client.cpp

@@ -398,21 +398,10 @@ void BackgroundClient::Self::draw_screen(SurfaceInfo& info, bool draws_crash) co
        file_exists = false;
    }

-    if (!wallpaper_enabled)
+    bool should_draw_crash = draws_crash && file_exists;


Suggested change

bool should_draw_crash = draws_crash && file_exists;

bool const should_draw_crash = draws_crash && file_exists;

Maybe the variable should be should_show_diagnostic?

AlanGriffiths · 2023-10-25T10:01:37Z

src/background_client.cpp

@@ -105,6 +106,7 @@ struct BackgroundClient::Self : egmde::FullscreenClient

    void render_text(uint32_t width, uint32_t height, unsigned char* buffer) const;

+    bool wallpaper_enabled;


This is never updated after initialization, so should be const.

AlanGriffiths · 2023-10-25T10:05:22Z

src/background_client.cpp

+    bool file_exists;
+    if (fs::exists(diagnostic_path.value_or("")))
+    {
+        file_exists = fs::file_size(diagnostic_path.value());
+    }
+    else
+    {
+        file_exists = false;
+    }


Suggested change

bool file_exists;

if (fs::exists(diagnostic_path.value_or("")))

{

file_exists = fs::file_size(diagnostic_path.value());

}

else

{

file_exists = false;

}

bool const file_exists = diagnostic_path && fs::exists(diagnostic_path.value()) && fs::file_size(diagnostic_path.value());

Although, maybe the variable should be have_diagnostic - the "file" is an implementation detail.

AlanGriffiths

LGTM

mattkae requested review from AlanGriffiths, Saviq and RAOF October 19, 2023 20:07

Saviq reviewed Oct 20, 2023

View reviewed changes

mattkae changed the title ~~Add a --wallpaper-enabled argument to optionally disable wallpaper draws~~ Add a --wallpaper argument to optionally disable wallpaper draws Oct 20, 2023

mattkae mentioned this pull request Oct 20, 2023

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

Merged

mattkae requested a review from Saviq October 20, 2023 19:35

Saviq assigned mattkae Oct 23, 2023

mattkae force-pushed the feature/disable_background_option branch from 6c67cc4 to 2289ec0 Compare October 24, 2023 22:15

Saviq approved these changes Oct 25, 2023

View reviewed changes

AlanGriffiths requested changes Oct 25, 2023

View reviewed changes

Add a --wallpaper argument to optionally disable wallpaper draws

a8f0b2c

mattkae force-pushed the feature/disable_background_option branch from cf3cbed to a8f0b2c Compare October 25, 2023 13:07

mattkae requested a review from AlanGriffiths October 25, 2023 13:08

AlanGriffiths approved these changes Oct 25, 2023

View reviewed changes

AlanGriffiths added this pull request to the merge queue Oct 25, 2023

Merged via the queue into main with commit b1504c6 Oct 25, 2023
3 checks passed

AlanGriffiths deleted the feature/disable_background_option branch October 25, 2023 13:26

Saviq added the needs test label Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a --wallpaper argument to optionally disable wallpaper draws #157

Add a --wallpaper argument to optionally disable wallpaper draws #157

mattkae commented Oct 19, 2023 •

edited

Loading

Saviq left a comment •

edited

Loading

AlanGriffiths commented Oct 20, 2023

mattkae commented Oct 20, 2023 •

edited

Loading

Saviq commented Oct 20, 2023

Saviq commented Oct 24, 2023

mattkae commented Oct 24, 2023

Saviq commented Oct 24, 2023 •

edited

Loading

Saviq commented Oct 24, 2023

mattkae commented Oct 24, 2023

Saviq commented Oct 24, 2023

mattkae commented Oct 24, 2023

Saviq commented Oct 24, 2023

mattkae commented Oct 24, 2023

Saviq commented Oct 24, 2023

Saviq left a comment

AlanGriffiths left a comment

AlanGriffiths Oct 25, 2023

AlanGriffiths Oct 25, 2023

AlanGriffiths Oct 25, 2023

AlanGriffiths Oct 25, 2023

AlanGriffiths Oct 25, 2023

AlanGriffiths left a comment

	bool should_draw_crash = draws_crash && file_exists;
	bool const should_draw_crash = draws_crash && file_exists;

		@@ -105,6 +106,7 @@ struct BackgroundClient::Self : egmde::FullscreenClient

		void render_text(uint32_t width, uint32_t height, unsigned char* buffer) const;

		bool wallpaper_enabled;

Add a --wallpaper argument to optionally disable wallpaper draws #157

Add a --wallpaper argument to optionally disable wallpaper draws #157

Conversation

mattkae commented Oct 19, 2023 • edited Loading

Depends on

To test

What's new?

Saviq left a comment • edited Loading

Choose a reason for hiding this comment

AlanGriffiths commented Oct 20, 2023

mattkae commented Oct 20, 2023 • edited Loading

Saviq commented Oct 20, 2023

Saviq commented Oct 24, 2023

mattkae commented Oct 24, 2023

Saviq commented Oct 24, 2023 • edited Loading

Saviq commented Oct 24, 2023

mattkae commented Oct 24, 2023

Saviq commented Oct 24, 2023

mattkae commented Oct 24, 2023

Saviq commented Oct 24, 2023

mattkae commented Oct 24, 2023

Saviq commented Oct 24, 2023

Saviq left a comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Oct 25, 2023

Choose a reason for hiding this comment

AlanGriffiths Oct 25, 2023

Choose a reason for hiding this comment

AlanGriffiths Oct 25, 2023

Choose a reason for hiding this comment

AlanGriffiths Oct 25, 2023

Choose a reason for hiding this comment

AlanGriffiths Oct 25, 2023

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

mattkae commented Oct 19, 2023 •

edited

Loading

Saviq left a comment •

edited

Loading

mattkae commented Oct 20, 2023 •

edited

Loading

Saviq commented Oct 24, 2023 •

edited

Loading