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 <[email protected]> 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 <[email protected]> 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 > _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
