[PATCH wayland-protocols 1/2] stable: add viewporter draft

[Daniel Stone]

Hi,

On 15 April 2016 at 15:53, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> +      This interface allows to define the source rectangle (src_x,
> +      src_y, src_width, src_height) from where to take the wl_buffer
> +      contents, and scale that to destination size (dst_width,
> +      dst_height). This state is double-buffered, and is applied on the
> +      next wl_surface.commit.

I like Yong's rewording, except with an additional bikeshed of 'allows
the client to control the [...]' perhaps.

> +      If the source rectangle is set, it defines what area of the
> +      wl_buffer is taken as the source. If the source rectangle is set and
> +      the destination size is not set, the surface size becomes the source
> +      rectangle size rounded up to the nearest integer. If the source size
> +      is already exactly integers, this results in cropping without scaling.

Ugh, I really intensely dislike that middle section. The cropping and
scaling are completely independent ... except where they aren't,
because sometimes we'll magically do it for you. I'd very much prefer
to see fractional crop + no scale defined as a protocol error, i.e.
the user must always explicitly set a configuration that results in an
integral destination size.

> +      If the source rectangle is partially or completely outside of the
> +      wl_buffer, then the surface contents are undefined (not void), and
> +      the surface size is still dst_width, dst_height.

Again, I'd rather see this defined as a protocol error. Without
explicit border-colour/mode controls (e.g. 'sample this colour for
anything outside buffer bounds'), the result of sampling outside the
source image can never be useful (at least AFAICT). So, why would you
ever want to do so? To achieve more perfect scaling factors, at the
expense of having to display some fractional garbage?

> +      If the wl_surface associated with the wl_viewport is destroyed,
> +      the wl_viewport object becomes inert.

Should attempting to call wl_viewport::set_* then result in an error?

> +       Arguments dst_x and dst_y do not exist here, use the x and y
> +       arguments to wl_surface.attach.

The x and y arguments perform relative translation of the surface, and
as such I don't think are really analagous. I'd suggest just deleting
this to avoid confusion.

> +    <request name="set_source" since="2">
> +      <description summary="set the source rectangle for cropping">
> +       Set the source rectangle of the associated wl_surface. See
> +       wl_viewport for the description, and relation to the wl_buffer
> +       size.
> +
> +       If width is -1.0 and height is -1.0, the source rectangle is unset
> +       instead. Any other pair of values for width and height that
> +       contains zero or negative values raises the bad_value protocol
> +       error.

This is different to the bare 'set' request, which doesn't (at least
in its documentation) allow for this. But that's being removed anyway,
so no problem. Should we require a pair of fixed values for x and y
here, either (0,0) as the state they'll be fixed to, or (-1,-1) for
symmetry?

Cheers,
Daniel