[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