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

[Pekka Paalanen]

On Thu, 21 May 2015 23:15:54 -0400
nerdopolis <bluescreen_avenger at verizon.net> 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

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

In addition to log... eh, no good way to make Weston quit, so yeah,
logging is all you can do, and pick a default.

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

I'm fine changing the default back to 32. Just mention it in the commit
message. That said, it probably excludes this patch from RC2 and
therefore from 1.8.0.

So, your pick: change the default to 32 or keep it as 16. If this
choice affects any tests, it should also fix the tests.

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

Yeah, it doesn't matter.

Using 16 instead of 32 bitspp will cut the background size in bytes to
half, obviously. This saves space in XDG_RUNTIME_DIR primarily on all
platforms, but it also saves scanout memory and bandwidth on the rpi.
So for rpi it was double or triple savings.

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

Yes.

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

You can ignore all cairo-gl paths as far as I'm concerned. I'm also
fine leaving the testing for (much much) later, this is not an
important feature.


Thanks,
pq