[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 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?)

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