[PATCH] desktop-shell: support an option to use 16 bit color or not, instead of using it by default
nerdopolis
bluescreen_avenger at verizon.net
Sat May 23 10:10:20 PDT 2015
On Friday, May 22, 2015 01:19:58 AM you wrote:
> On Thu, May 21, 2015 at 11:15:54PM -0400, nerdopolis wrote:
> > On Thursday, May 21, 2015 05:23:30 PM you wrote:
> > > On Thu, May 21, 2015 at 04:35:16PM -0400, nerdopolis wrote:
> > > > ---
> > > > clients/desktop-shell.c | 10 ++++++++--
> > > > man/weston.ini.man | 4 ++++
> > > > 2 files changed, 12 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
> > > > index e2f9f80..cc4a502 100644
> > > > --- a/clients/desktop-shell.c
> > > > +++ b/clients/desktop-shell.c
> > > > @@ -94,6 +94,7 @@ struct background {
> > > > char *image;
> > > > int type;
> > > > uint32_t color;
> > > > + int32_t low_bpp;
> > > > };
> > >
> > > I agree with Bill and pq that this name needs to be thought out better.
> > > 'low' is a relative term. 32 bpp is low compared with 64, and 16 bpp is
> > > high compared with 8.
> > >
> > > Internally I suggest this just be 'bpp', with values 16 or 32.
> > > Externally, you could still use boolean options for selecting it, if you
> > > really must.
> > >
> > OK I'll do background_bpp
> > > > struct output {
> > > > @@ -1015,10 +1016,15 @@ background_create(struct desktop *desktop)
> > > > window_set_user_data(background->window, background);
> > > > widget_set_redraw_handler(background->widget, background_draw);
> > > > widget_set_transparent(background->widget, 0);
> > > > - window_set_preferred_format(background->window,
> > > > - WINDOW_PREFERRED_FORMAT_RGB565);
> > > >
> > > > s = weston_config_get_section(desktop->config, "shell", NULL, NULL);
> > > > + weston_config_section_get_bool(s, "background-low-bpp",
> > > > + &background->low_bpp, NULL);
> > >
> > > The last parameter must be an int. NULL is a void*. This causes a
> > > build warning:
> > OK. I'll fix this...
> > >
> > > CC clients/weston_desktop_shell-desktop-shell.o
> > > clients/desktop-shell.c: In function ‘background_create’:
> > > clients/desktop-shell.c:1022:7: warning: passing argument 4 of ‘weston_config_section_get_bool’ makes integer from pointer without a cast [enabled by default]
> > > &background->low_bpp, NULL);
> > > ^
> > > In file included from clients/window.h:31:0,
> > > from clients/desktop-shell.c:44:
> > > clients/../shared/config-parser.h:94:1: note: expected ‘int’ but argument is of type ‘void *’
> > > weston_config_section_get_bool(struct weston_config_section *section,
> > > ^
> > > CCLD weston-desktop-shell
> > >
> > > So I think what you want is:
> > >
> > > weston_config_section_get_bool(s, "background-low-bpp",
> > > &background->low_bpp, 0);
> > >
> > > But, I think the option needs a bit more thinking...
> > >
> > > Options as --background-16-bpp and --background-32-bpp might be clearer.
> > > Makes also feasible adding other bpp's later.
> > >
> > > But what happens if you set both to true? Or both false?
> > >
> > > So, I agree with Bill that a --background-bpp=[32|16] is the most
> > > sensible, with documentation specifying the legal values, and input
> > > checks that error if something other than 16 or 32 is specified.
> > >
> > So if the background-bpp=16, I set the wallpaper to RGB565, if it's 32 I do nothing, and anything else,
> > I weston_log that it's an invalid value?
> >
> > but should 16bpp for the wallpaper be the default, if it's not specified or set to 29 or something? or 32 bit?
> > I feel like 16bpp really only benefits users on extremely limited memory platforms, and most users would want
> > it in 32bpp, but I can change it if it breaks tests...
>
> In that case, it's fine but you'll need to update the test reference
> image. This is easy, just copy the ./internal-screenshot-00.png to
> tests/reference/internal-screenshot-00.png as one of the changes in your
> patchset.
>
> This particular test isn't there to validate how the background is
> drawn, but rather to flag changes in rendering behavior. This ensures
> we spot changes early and can ensure they're what we intend. So, looks
> like the test did exactly what it was supposed to do! :-)
>
> > > > + if (background->low_bpp) {
> > > > + window_set_preferred_format(background->window,
> > > > + WINDOW_PREFERRED_FORMAT_RGB565);
> > > > + }
> > >
> > > This changes the default behavior of weston. Before, it was setting
> > > this format as the default, now it sets it only when the low bpp option
> > > is given.
> > >
> > > In fact, this breaks the internal-screenshot test case.
> > >
> > > > weston_config_section_get_string(s, "background-image",
> > > > &background->image, NULL);
> > > > weston_config_section_get_uint(s, "background-color",
> > > > diff --git a/man/weston.ini.man b/man/weston.ini.man
> > > > index fe86bb6..3a8b2a4 100644
> > > > --- a/man/weston.ini.man
> > > > +++ b/man/weston.ini.man
> > > > @@ -216,6 +216,10 @@ output. Tile repeats the background image to fill the output.
> > > > sets the color of the background (unsigned integer). The hexadecimal
> > > > digit pairs are in order alpha, red, green, and blue.
> > > > .TP 7
> > > > +.BI "background-low-bpp=" true
> > > > +specify to reduce the background to 16 bit color (boolean). This option is
> > > > +useful for low memory platforms. This only affects the background.
> > > > +.TP 7
> > >
> > > You might mention what amount of memory savings this produces, and under
> > > what circumstances.
> > >
> > I'm not sure of the memory savings myself... I had to test, and it uses 1.5MB less shared memory on 1024x768,
> > but (I'm not sure what ksysguard uses as the value in the "Memory" column, which might be private memory)
> > stays about the same... Someone said the wallpaper was defaulted to 16 bit for just the RPi...
>
> Right, different conditions could result in more or less. You could
> just say for some embedded devices it was measured to save in the
> ballpark of a couple MB's shared memory.
>
> > I could say, it's for low memory platforms, such as embedded devices...
> > > > .BI "panel-color=" 0xAARRGGBB
> > > > sets the color of the panel (unsigned integer). The hexadecimal
> > > > digit pairs are in order transparency, red, green, and blue. Examples:
> > >
> > > You should also add a test case for this. You can reuse the
> > > internal-screenshot-test.c test case, adding '--background-16-bpp' (or
> > > whatever) to the server_parameters, and replacing the reference PNG file
> > > with a 16 bpp one. You may need to modify or provide 16bpp alternatives
> > > to the check routines, check_surfaces_equal() and/or
> > > check_surfaces_match_in_clip().
> > >
> > This is going to be a configuration variable in weston.ini, not an argument though?
>
> Ah, even so, that's still doable, since you can now do test-specific ini
> files. The internal-screenshot test includes an ini file, so you can
> just make a copy and add in your config variable.
>
> > It would be harder to write a whole new test for this...
> > Weston setting the wallpaper to 16 bit only when compiled with with-cairo=image.
> > when compiled with-cairo=gl, the wallpaper forcing to RGB565 didn't even work, and it shows up 32 bits...
>
> Huh, interesting. I thought pq said the only thing cairo-gl affected
> were a few of the demos.
>
> So, the test will need to include two reference images, one for
> cairo=image, the other for cairo=gl, and the test will somehow need to
> discern which one is in use (maybe it's indicated in config.h?)
>
Cairo-gl apparently wasn't *supposed* to affect the 16 bit wallpaper not working. I think it might be a bug
in Weston that cairo-gl is ignoring window_set_preferred_format
> I know it probably seems hard to write a test for this, but it's a habit
> we know we need to get into more. In the past the argument was we
> didn't have the infrastructure, but we do now. And actually I think
> that in this particular case it should be pretty straightforward.
> So if you run into trouble or have questions, give me a ping and I can
> give you a hand.
>
> Bryce
More information about the wayland-devel
mailing list