[PATCH weston 01/11] compositor-headless: Support starting with zero outputs

[Pekka Paalanen]

On Sun, 19 Jun 2016 11:06:05 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

> On 18/06/2016 19:15, Armin Krezović wrote:
> > This patch adds a new command line option which can be
> > used to tell headless backend not to create any
> > virtual outputs.
> >
> > This will be used for output hotplug emulation, where
> > weston will start with no outputs available, and the
> > virtual output will be created at runtime.
> >
> > Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>  
> 
> Good, just one nitpick below to make it perfect.
> 
> Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> 
> 
> > ---
> >  src/compositor-headless.c | 7 +++++--
> >  src/compositor-headless.h | 1 +
> >  src/main.c                | 4 +++-
> >  3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/compositor-headless.c b/src/compositor-headless.c
> > index 45e47ca..05dd8af 100644
> > --- a/src/compositor-headless.c
> > +++ b/src/compositor-headless.c
> > @@ -235,8 +235,11 @@ headless_backend_create(struct weston_compositor *compositor,
> >  	if (b->use_pixman) {
> >  		pixman_renderer_init(compositor);
> >  	}
> > -	if (headless_backend_create_output(b, config) < 0)
> > -		goto err_input;
> > +
> > +	if (!config->no_outputs) {
> > +		if (headless_backend_create_output(b, config) < 0)
> > +			goto err_input;
> > +	}
> >
> >  	if (!b->use_pixman && noop_renderer_init(compositor) < 0)
> >  		goto err_input;
> > diff --git a/src/compositor-headless.h b/src/compositor-headless.h
> > index 79f39c8..b128ab0 100644
> > --- a/src/compositor-headless.h
> > +++ b/src/compositor-headless.h
> > @@ -44,6 +44,7 @@ struct weston_headless_backend_config {
> >  	int use_pixman;
> >
> >  	uint32_t transform;
> > +	int no_outputs;

Hi,

the patch is good, just one more minor comment.

This is a boolean flag, so the type should be 'bool' instead of 'int'.
As people often struggle reading double-negatives, the flag would be
more readable as 'bool create_output'. It does mean main.c needs to
invert the flag because it uses '--no-output' which I think is fine.

> >  };
> >
> >  #ifdef  __cplusplus
> > diff --git a/src/main.c b/src/main.c
> > index ec3d799..dc6529d 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -526,7 +526,8 @@ usage(int error_code)
> >  		"  --height=HEIGHT\tHeight of memory surface\n"
> >  		"  --transform=TR\tThe output transformation, TR is one of:\n"
> >  		"\tnormal 90 180 270 flipped flipped-90 flipped-180 flipped-270\n"
> > -		"  --use-pixman\t\tUse the pixman (CPU) renderer (default: no rendering)\n\n");
> > +		"  --use-pixman\t\tUse the pixman (CPU) renderer (default: no rendering)\n"
> > +		"  --no-outputs\t\tDo not create any virtual outputs\n\n");  
> 
> A tiny improvement to make here: put the last \n and the closing on its 
> own line, so we future additions/removal stop moving it around. See 
> commit e77f8ad79bdf3613dc7e587ea0cf5b9d39e4f8e0 for a similar change 
> related to the fbdev backend.

Agreed.

> 
> >  #endif
> >
> >  #if defined(BUILD_RDP_COMPOSITOR)
> > @@ -1037,6 +1038,7 @@ load_headless_backend(struct weston_compositor *c,
> >  		{ WESTON_OPTION_INTEGER, "height", 0, &config.height },
> >  		{ WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config.use_pixman },
> >  		{ WESTON_OPTION_STRING, "transform", 0, &transform },
> > +		{ WESTON_OPTION_BOOLEAN, "no-outputs", 0, &config.no_outputs },
> >  	};
> >
> >  	parse_options(options, ARRAY_LENGTH(options), argc, argv);
> >  

All these are really minor comments, so in any case:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

If you do end up re-sending the series, then it would be good to fix
them.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160620/ccdb96c9/attachment.sig>