[PATCH weston 2/4] compositor: add weston_view_set_mask() API and state

[Pekka Paalanen]

On Tue, 03 Mar 2015 16:50:47 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> Minor pedantry below.
> 
> On 02/03/15 09:15 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > Add API for setting a clip ('scissor' in the code) rectangle per view,
> > in surface coordinates. Ivi-shell requires this feature to be able to
> > implement the IVI Layer Manager API, which includes clipping of
> > surfaces.
> > 
> > The names weston_view_set_mask() and weston_view_set_mask_infinite()
> > mirror the existing weston_layer_set_mask*() functions.
> > 
> > This view clipping complements the weston_layer clipping, because view
> > clipping is defined in surface local coordinates, while layer
> > mask/clipping is defined in global coordinates.
> > 
> > View clipping requires explicit support from the renderers. Therefore a
> > new Weston capability bit is added: WESTON_CAP_VIEW_CLIP_MASK. Shells
> > (and all users) of this new API are required to check the capability bit
> > is set before using the API. Otherwise the rendering will not be what
> > they expect.
> > 
> > View clips are inherited through the transformation inheritance
> > mechanism. However, there are restrictions. The clip rectangle can be
> > set only on the root view of a transformation inheritance tree. The
> > additional transformations in child views must not rotate the coordinate
> > axes. These restrictions avoid corner cases in clip inheritance, and
> > keep the renderer implementations as simple as they are right now.
> > Renderers only need to do an additional intersection with the clip
> > rectangle which is always aligned to the surface coordinate system.
> > 
> > For more details, see the API documentation in the patch.
> > 
> > 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/compositor.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  src/compositor.h |  14 +++++
> >  2 files changed, 169 insertions(+), 9 deletions(-)

...

> > +/** Set a clip mask rectangle on a view
> > + *
> > + * \param view The view to set the clip mask on.
> > + * \param x Top-left corner X coordinate of the clip rectangle.
> > + * \param y Top-left corner X coordinate of the clip rectangle.
> 
> Should say Y coordinate...

Oops :-)

> > + * \param width Width of the clip rectangle, non-negative.
> > + * \param height Height of the clip rectangle, non-negative.
> > + *
> > + * A shell may set a clip mask rectangle on a view. Everything outside
> > + * the rectangle is cut away for input and output purposes: it is
> > + * not drawn and cannot be hit by hit-test based input like pointer
> > + * motion or touch-downs. Everything inside the rectangle will behave
> > + * normal. Clients are unaware of clipping.
> 
> s/normally/normally/
> 
> > + *
> > + * The rectangle is set in the surface local coordinates. Setting a clip
> > + * mask rectangle does not affect the view position, the view is positioned
> > + * as it would be without a clip. The clip also does not change
> > + * weston_surface::width,height.
> > + *
> > + * The clip mask rectangle is part of transformation inheritance
> > + * (weston_view_set_transform_parent()). A clip set in the root of the
> > + * transformation inheritance tree will affect all views in the tree.
> > + * A clip can be set only on the root view. Attempting to set a clip
> > + * on view that has a transformation parent will fail. Assigning a parent
> > + * to a view that has a clip set will cause the clip to be forgotten.
> > + *
> > + * Because the clip mask is an axis-aligned rectangle, it poses restrictions
> > + * on the additional transformations in the child views. These transformations
> > + * may not rotate the coordinate axes, i.e., only translation and scaling
> > + * are allowed. Violating this restriction causes the clipping to malfcuntion.
> 
> malfunction
> 
> > + * Furthermore, using scaling may cause rounding errors in child clipping.
> > + *
> > + * The clip mask rectangle is not automatically adjusted based on
> > + * wl_surface.attach dx and dy arguments.
> > + *
> > + * A clip mask rectangle can be set only if the compositor capability
> > + * WESTON_CAP_VIEW_CLIP_MASK is present.
> > + *
> > + * This function sets the clip mask rectangle and schedules a repaint for
> > + * the view.
> > + */
> > +WL_EXPORT void
> > +weston_view_set_mask(struct weston_view *view,
> > +		     int x, int y, int width, int height)
> > +{
> > +	struct weston_compositor *compositor = view->surface->compositor;
> > +
> > +	if (!(compositor->capabilities & WESTON_CAP_VIEW_CLIP_MASK)) {
> > +		weston_log("%s not allowed without capability!\n", __func__);
> > +		return;
> > +	}
> > +
> > +	if (view->geometry.parent) {
> > +		weston_log("view %p has a parent, clip forbidden!\n", view);
> > +		return;
> > +	}
> > +
> > +	if (width < 0 || height < 0) {
> > +		weston_log("%s: illegal args %d, %d, %d, %d\n", __func__,
> > +			   x, y, width, height);
> > +		return;
> > +	}
> > +
> > +	pixman_region32_fini(&view->geometry.scissor);
> > +	pixman_region32_init_rect(&view->geometry.scissor, x, y, width, height);
> 
> Should x, y, width, height be validated/clamped in some way?  If width
> and height are set very large they can affect weston_compositor_pick_view()?

I don't think they should be clamped, because the underlying surface
size may change after the scissor is set, and I want the scissor to
remain what it was set to. Semantic validation is up to the shell
calling this API.

Nothing bad should happen in pick_view, the scissor is just another
condition to match in addition to all the old ones.

Everywhere where the scissor is used, it should be just an additional
limitation. I shouldn't be able to "expand" what was already there,
only limit.

> > +	view->geometry.scissor_enabled = true;
> > +	weston_view_geometry_dirty(view);
> > +	weston_view_schedule_repaint(view);
> > +}
> > +
> > +/** Remove the clip mask from a view
> > + *
> > + * \param view The view to remove the clip mask from.
> > + *
> > + * Removed the clip mask rectangle and schedules a repaint.
> > + *
> > + * \sa weston_view_set_mask
> > + */
> > +WL_EXPORT void
> > +weston_view_set_mask_infinite(struct weston_view *view)
> > +{
> > +	view->geometry.scissor_enabled = false;
> > +	weston_view_geometry_dirty(view);
> > +	weston_view_schedule_repaint(view);
> > +}
> > +
> >  WL_EXPORT bool
> >  weston_view_is_mapped(struct weston_view *view)
> >  {
> > @@ -1561,6 +1699,11 @@ weston_compositor_pick_view(struct weston_compositor *compositor,
> >  						    view_ix, view_iy, NULL))
> >  			continue;
> >  
> > +		if (view->geometry.scissor_enabled &&
> > +		    !pixman_region32_contains_point(&view->geometry.scissor,
> > +						    view_ix, view_iy, NULL))
> > +			continue;

If scissor is set, it is just another condition that a surface need to
pass to be picked. Note, the 'continue' means "failed the hit test".

> > +
> >  		*vx = view_x;
> >  		*vy = view_y;
> >  		return view;

Are you happy with the explanations?
I fixed all the typos, and will push if you are. :-)


Thanks,
pq