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
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel