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

[Bryce Harrington]

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.

>  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:

  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.

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

>  .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().

Bryce