[RFC v2] surface crop & scale protocol extension

Fri Nov 8 20:33:18 PST 2013

Pekka,
I've got just a comple of general comments below.

On Nov 8, 2013 8:46 AM, "Pekka Paalanen" <ppaalanen at gmail.com> wrote:
>
> Hi all,
>
> this is the v2 of the crop and scale extension, as RFC.
>
> The v1 was in:
> http://lists.freedesktop.org/archives/wayland-devel/2013-April/008927.html
>
> Based on v1, Jonny Lamb has been working on a Weston implementation,
> starting from the protocol side, and working towards the renderer
> implementations. That work is still very much in progress.
>
>
> Introduction:
>
> The primary use case for the crop & scale extension is for hardware
> accelerated, zero-copy video display. Hardware video decoders produce
> complete frames or ultimately wl_buffers that are attached to
> a wl_surface. The crop & scale extension allows the client to
> request the compositor to crop and scale the frame so it fits the
> client's purposes. Optimally, a compositor implements this by
> programming a hardware overlay unit to do the requested cropping and
> scaling. The major benefit is that the CPU never has to even see the
> video pixels.
>
> Probably a common case is to have a video in a sub-surface, so that
> window decorations and overlaid GUI elements can be in other
> (sub-)surfaces, and avoid touching the pixels in the video frame
> buffers. Video as a browser element will need cropping when the element
> is partially off-view. Scaling should be useful in general, e.g.
> showing a video scaled up (or down) but still in a window.
>
> However, I also see that crop & scale can be used to present videos
> with non-square pixels in fullscreen (e.g. without sub-surfaces). Crop
> & scale is needed in this case, because the fullscreening methods in
> wl_shell do not allow changing the aspect ratio, or cropping parts of
> the video out to fill the whole output area when video and output
> aspect ratios differ. The fullscreening methods support only adding
> black borders, but not scale-to-fill-and-crop.
>
>
> Changes since v1:
>
> The changes I have made are very small, and can be seen in patch form
> at:
> http://cgit.collabora.com/git/user/pq/weston.git/log/?h=clipscale-wip
>
> The changes are:
> - improve wording, add missing details
> - take buffer_scale into account
> - rewrite the coordinate transformations more clearly
>
> In the end, I did not get much else out from the discussions of v1.
>
> I think some people do not like the structure of the protocol
> additions, namely adding yet another global factory interface and a
> sub-interface to wl_surface. If the concensus really is to move this
> into a single wl_surface request, that is fine.
>
> But, to me this would not make sense as a request in wl_subsurface. The
> crop & scale state is supposed to be driven by the application
> component that produces the wl_buffers and attaches them to a
> wl_surface. The wl_subsurface interface OTOH is to be used by the
> parent component, for e.g. setting the sub-surface position. The
> situation is similar to a compositor vs. clients: clients define the
> surface size and content, compositor the position. Also, the crop &
> scale state is not well suited to be "forced on" by a parent software
> component, as it changes how e.g. input event coordinates relate to the
> wl_buffer contents. Finally, there is the fullscreen video use case I
> described above.

In the design of both the subsurfaces and crop&scale, I fear that you may
be trying too hard to avoid out-of-band communication between components
with little to no benefit.  I don't want to start a bikeshedding landslide
here and you are free to disagree.  However, I'm concerned that we're
overcomplicating things without gaining anything.

In some simple use-cases, you will be able to write one component that has
runs on a subsurface and another that uses it and the two components are
basically unaware of each other beyond the initial handshake.  However, I
think in practice I feel that what is more likely to happen is that the
primary tookit will handle everything.  If this is the case, it makes no
difference whether it's separate or inside wl_subsurface except that the
toolkit has more extensions to wrangle.

Concerning full-screen video:  After thinking about it a bit more, I think
this is a failure of wl_subsurface more than a need for scaling regular
surfaces.  I think the primary issue there is that wl_subsurface provides
no real way to make a surface that, itself, is empty but its subsurfaces
have content.  If this were possible, then it would be no problem putting
crop&scale in the wl_subsurface protocol.  It's worth noting that this same
issue makes putting a video (from an external source) in a SHM subsurface
frame really awkward.  See also this e-mail from April:

http://lists.freedesktop.org/archives/wayland-devel/2013-April/008875.html

Hope it's useful.  Thanks,
--Jason Ekstrand

>
> The interface names are still so-and-so, I haven't come up with
> anything better. wl_scaler is pretty ok, but I feel something better
> would be in place for wl_surface_scaler. How would wl_viewport sound
> like?
>
> A minor benefit of keeping crop & scale as a separate interface is that
> it could be made optional, a per-backend or renderer thing.
>
>
> For your reviewing pleasure, here is the v2:
>
> <?xml version="1.0" encoding="UTF-8"?>
> <protocol name="scaler">
>
>   <copyright>
>     Copyright Â© 2013 Collabora, Ltd.
>
>     Permission to use, copy, modify, distribute, and sell this
>     software and its documentation for any purpose is hereby granted
>     without fee, provided that the above copyright notice appear in
>     all copies and that both that copyright notice and this permission
>     notice appear in supporting documentation, and that the name of
>     the copyright holders not be used in advertising or publicity
>     pertaining to distribution of the software without specific,
>     written prior permission.  The copyright holders make no
>     representations about the suitability of this software for any
>     purpose.  It is provided "as is" without express or implied
>     warranty.
>
>     THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
>     SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
>     FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
>     SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
>     WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
>     AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
>     ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
>     THIS SOFTWARE.
>   </copyright>
>
>   <interface name="wl_scaler" version="1">
>     <description summary="surface cropping and scaling">
>       The global interface exposing surface cropping and scaling
>       capabilities is used to instantiate an interface extension for a
>       wl_surface object. This extended interface will then allow
>       cropping and scaling the surface contents, effectively
>       disconnecting the direct relationship between the buffer and the
>       surface size.
>     </description>
>
>     <request name="destroy" type="destructor">
>       <description summary="unbind from the cropping and scaling
interface">
>         Informs the server that the client will not be using this
>         protocol object anymore. This does not affect any other objects,
>         wl_surface_scaler objects included.
>       </description>
>     </request>
>
>     <enum name="error">
>       <entry name="scaler_exists" value="0"
>              summary="the surface already has a scaler object
associated"/>
>     </enum>
>
>     <request name="get_surface_scaler">
>       <description summary="extend surface interface for crop and scale">
>         Instantiate an interface extension for the given wl_surface to
>         crop and scale its content. If the given wl_surface already has
>         a wl_surface_scaler object associated, the scaler_exists
>         protocol error is raised.
>       </description>
>
>       <arg name="id" type="new_id" interface="wl_surface_scaler"
>            summary="the new scaler interface id"/>
>       <arg name="surface" type="object" interface="wl_surface"
>            summary="the surface"/>
>     </request>
>   </interface>
>
>   <interface name="wl_surface_scaler" version="1">
>     <description summary="crop and scale interface to a wl_surface">
>       An additional interface to a wl_surface object, which allows the
>       client to specify the cropping and scaling of the surface
>       contents.
>
>       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.
>
>       Before the first set request, the wl_surface still behaves as if
>       there was no crop and scale state. That is, no scaling is applied,
>       and the surface size is as defined in wl_surface.attach.
>
>       The crop and scale state causes the surface size to become
>       dst_width, dst_height. This overrides whatever the attached
>       wl_buffer size is, unless the wl_buffer is NULL. If the wl_buffer is
>       NULL, the surface has no content and therefore no size.
>
>       The coordinate transformations from buffer pixel coordinates up to
>       the surface-local coordinates happen in the following order:
>         1. buffer_transform (wl_surface.set_buffer_transform)
>         2. buffer_scale (wl_surface.set_buffer_scale)
>         3. crop and scale (wl_surface_scaler.set)
>       This means, that the source rectangle coordinates of crop and scale
>       are given in the coordinates after the buffer transform and scale,
>       i.e. in the coordinates that would be the surface-local coordinates
>       if the crop and scale was not applied.
>
>       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.
>
>       The x, y arguments of wl_surface.attach are applied as normal to
>       the surface. They indicate how many pixels to remove from the
>       surface size from the left and the top. In other words, they are
>       still in the surface-local coordinate system, just like dst_width
>       and dst_height are.
>
>       If the wl_surface associated with the wl_surface_scaler is
>       destroyed, the wl_surface_scaler object becomes inert.
>
>       If the wl_surface_scaler object is destroyed, the crop and scale
>       state is removed from the wl_surface. The change will be applied
>       on the next wl_surface.commit.
>     </description>
>
>     <request name="destroy" type="destructor">
>       <description summary="remove scaling and cropping from the surface">
>         The associated wl_surface's crop and scale state is removed.
>         The change is applied on the next wl_surface.commit.
>       </description>
>     </request>
>
>     <enum name="error">
>       <entry name="bad_value" value="0"
>              summary="negative values in width or height"/>
>     </enum>
>
>     <request name="set">
>       <description summary="set the crop and scale state">
>         Set the crop and scale state of the associated wl_surface. See
>         wl_surface_scaler for the description, and relation to the
>         wl_buffer size.
>
>         If any of src_width, src_height, dst_width, and dst_height is
>         negative, the bad_value protocol error is raised.
>
>         The crop and scale state is double-buffered state, and will be
>         applied on the next wl_surface.commit.
>
>         Arguments dst_x and dst_y do not exist here, use the x and y
>         arguments to wl_surface.attach. The x, y, dst_width, and
dst_height
>         define the surface-local coordinate system irrespective of the
>         attached wl_buffer size.
>       </description>
>
>       <arg name="src_x" type="fixed" summary="source rectangle x"/>
>       <arg name="src_y" type="fixed" summary="source rectangle y"/>
>       <arg name="src_width" type="fixed" summary="source rectangle
width"/>
>       <arg name="src_height" type="fixed" summary="source rectangle
height"/>
>       <arg name="dst_width" type="int" summary="surface width"/>
>       <arg name="dst_height" type="int" summary="surface height"/>
>     </request>
>
>   </interface>
> </protocol>
>
>
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20131108/f5474064/attachment-0001.html>