[PATCH weston 4/4] gl-renderer: implement view scissor

[Pekka Paalanen]

On Tue, 03 Mar 2015 17:02:29 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> Some minor pedantry below as well, but otherwise the whole series is
> 
> Reviewed-By: Derek Foreman <derekf at osg.samsung.com>
> 
> On 02/03/15 09:16 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > Implement support for weston_view_set_mask().
> > 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > Reviewed-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
> > Tested-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
> > ---
> >  src/gl-renderer.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> > index 8835765..e8b27b9 100644
> > --- a/src/gl-renderer.c
> > +++ b/src/gl-renderer.c
> > @@ -593,6 +593,8 @@ draw_view(struct weston_view *ev, struct weston_output *output,
> >  	struct gl_surface_state *gs = get_surface_state(ev->surface);
> >  	/* repaint bounding region in global coordinates: */
> >  	pixman_region32_t repaint;
> > +	/* opaque region in surface coordinates: */
> > +	pixman_region32_t surface_opaque;
> >  	/* non-opaque region in surface coordinates: */
> >  	pixman_region32_t surface_blend;
> >  	GLint filter;
> > @@ -638,10 +640,22 @@ draw_view(struct weston_view *ev, struct weston_output *output,
> >  	/* blended region is whole surface minus opaque region: */
> >  	pixman_region32_init_rect(&surface_blend, 0, 0,
> >  				  ev->surface->width, ev->surface->height);
> > -	pixman_region32_subtract(&surface_blend, &surface_blend, &ev->surface->opaque);
> > +	if (ev->geometry.scissor_enabled)
> > +		pixman_region32_intersect(&surface_blend, &surface_blend,
> > +					  &ev->geometry.scissor);
> > +	pixman_region32_subtract(&surface_blend, &surface_blend,
> > +				 &ev->surface->opaque);
> >  
> >  	/* XXX: Should we be using ev->transform.opaque here? */
> > -	if (pixman_region32_not_empty(&ev->surface->opaque)) {
> > +	pixman_region32_init(&surface_opaque);
> > +	if (ev->geometry.scissor_enabled)
> > +		pixman_region32_intersect(&surface_opaque,
> > +					  &ev->surface->opaque,
> > +					  &ev->geometry.scissor);
> > +	else
> > +		pixman_region32_copy(&surface_opaque, &ev->surface->opaque);
> 
> In the likely case (scissor not enabled), this seems to add a lot of
> superfluous region copying...
> 
> I guess we could add pixman_region32_t *surface_opaque_ptr and do
> something like:
> 
> +	if (ev->geometry.scissor_enabled) {
> +		pixman_region32_init(&surface_opaque);
> +		pixman_region32_intersect(&surface_opaque,
> +					  &ev->surface->opaque,
> +					  &ev->geometry.scissor);
> +		surface_opaque_ptr = &surface_opaque;
> +	} else
> +		surface_opaque_ptr = &ev->surface->opaque;
> 
> pass surface_opaque_ptr to repaint region, and make the
> pixman_region32_fini() conditional.
> 
> Dunno if the regions are frequently complex enough to be terribly
> concerned about this copy anyway...  I'll defer to your judgment.

Yes, we could do that indeed.

However, I tend to prefer simpler code until something implies it
really might be too heavy and needs to be smarter. Where it is easy to
avoid copying etc. I do that, but when the line count starts growing, I
lose interest.

Blame me for being lazy here. :-)

> > +
> > +	if (pixman_region32_not_empty(&surface_opaque)) {
> >  		if (gs->shader == &gr->texture_shader_rgba) {
> >  			/* Special case for RGBA textures with possibly
> >  			 * bad data in alpha channel: use the shader

Thank you for the reviews!


Thanks,
pq