[PATCH] desktop-shell: support an option to use 16 bit color or not, instead of using it by default

[nerdopolis]

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...
> > +	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...

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?
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...
I
> Bryce
>