[RFC v2] surface crop & scale protocol extension

[Pekka Paalanen]

Argh, reply-to-all had a bad list address. Sorry for double-posting
your personal copies.
- pq

On Mon, 11 Nov 2013 11:13:48 +0200
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> Hi Rafael,
> 
> btw. if anyone thinks any of my replies should be added to the protocol
> specification to clarify it, let me know. If I don't say I'll add or
> change something in the spec, then I probably won't.
> 
> 
> On Fri, 8 Nov 2013 22:52:45 -0200
> Rafael Antognolli <rafael.antognolli at intel.com> wrote:
> 
> > Hi Pekka, following are my comments:
> > 
> > On Fri, Nov 08, 2013 at 01:33:22PM +0200, Pekka Paalanen 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.
> > 
> > So, assuming we are cropping/scaling the main surface of a given
> > application that, for some reason, has some visible subsurfaces inside
> > it. It is not expected that any crop/scale applies to the child
> > subsurfaces, right? It should all be handled by the application in
> > question, I assume.
> 
> Yes, crop & scale is defined for the buffer, not the surface, so it does
> not affect anything particular about surfaces, like clipping of
> (sub-)surfaces. It certainly does not affect other wl_surfaces in any
> way, than one where it is set.
> 
> The crop & scale state is simply a transformation applied to the
> buffer, when it becomes the surface content.
> 
> > > 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 &
> > 
> > I don't agree with this. I think the crop & scale state should be driven
> > by the parent component that is composing the scene, maybe attaching the
> > wl_buffer, but not necessarily the one that is producing the wl_buffer.
> 
> Bad choice of words from me: with producer I mean the one who attaches
> to wl_surface, i.e. the one who uses the wl_surface methods.
> 
> > I'm talking from EFL point of view: the decoder part of a video player
> > application would produce the wl_buffers, with their respective sizes,
> > while Evas (the canvas library, that controls the scene graph) would
> > position, scale and crop the surface as required to fit the scene.
> 
> Yeah, that's one way to do it. The other way I have been thinking of,
> is that the parent toolkit notifies the component that drives the
> wl_surface about crop & scale changes, just like for resizes.
> 
> Think about toolkit agnostic library components, like perhaps a
> gstreamer video sink.
> 
> > > 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
> > 
> > Maybe these input event coordinates should be translated too, by the
> > same parent software component that is forcing the crop/scale state on
> > the subsurface.
> 
> That would introduce one more coordinate system into the protocol. We
> right now have surface coordinates, where practically everything in the
> protocol is given, and buffer coordinates that clients use to draw.
> Using modified input coordinates would add an input coordinate system
> somewhere in between. I think that would complicate the use of the
> protocol, and it then overlooks the opposite case of the parent toolkit
> handling the sub-surface's input events.
> 
> > > described above.
> > > 
> > > 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?
> > 
> > +1 for wl_viewport.
> > 
> > > A minor benefit of keeping crop & scale as a separate interface is that
> > > it could be made optional, a per-backend or renderer thing.
> > 
> > Really cool.
> > 
> > So, in summary, I'm not against using wl_scaler (or wl_viewport), nor
> > adding it directly to wl_surface if necessary. But I do think that
> > putting it into wl_subsurface would work just fine too, except from the
> > fullscreen, without-subsurface, video player case.
> > 
> > All the other use cases that I have in mind, also discussed with other
> > guys from EFL, are related to subsurfaces too.
> > 
> > Just one comment about the protocol description below.
> 
> The video player switching to/from fullscreen question is a really hard
> one, and I still do not have a good solution for that.
> 
> > > 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.
> > 
> > How many pixels to rmeove from the surface size? Does it still mean that
> > the surface is just going to be moved, or will it affect the surface
> > size? Sorry, but it might be just a misunderstanding from me.
> 
> This is exactly the same definition as what is in wl_surface.attach,
> but in different words. "Removed" means compared to the previous
> surface position and size. So yes, it is still just the move.
> 
> Would it be clearer, if it was written like this:
> 
> "They indicate how many pixels to remove from the surface from the left
> and the top. How many pixels are added or removed from the bottom and
> the right are detemined by the combination of x, y, and dst_width and
> dst_height. In other words..."
> 
> Or should I just remove that extra explanation as confusing?
> 
> 
> Thanks,
> pq
> 
> > >       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>
> > 
> > Regards,
> > Rafael
>