[PATCH weston-ivi-shell v4 1/9] ivi application protocol:
Nobuhiko Tanibata
nobuhiko_tanibata at xddp.denso.co.jp
Sun Apr 27 22:47:26 PDT 2014
2014-04-23 19:40 に Pekka Paalanen さんは書きました:
> Hi,
>
> it's been a long while since I have looked at this, but I got a bit of
> time to come back. I hope you haven't abandoned this effort yet. :-)
>
> I looked at the PDF from your post on March 6th, 2014, and some of my
> own comments I gave at that time to recall what this was about, but I
> probably still forgot something.
>
> New comments below. I started by reading the global interface, and
> moved on to ivi_surface after it, so the comments might seem
> temporally strange if you read just top-down.
>
>
> On Mon, 17 Mar 2014 15:23:22 +0900
> Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp> wrote:
>
>> Add interface ivi_application, which creates ivi_surface objects tied
>> to a given wl_surface with a given id. The given id can be used in a
>> shell to identify which application is assigned to a wl_surface and
>> layout the surface wherever the shell wants. ivi_surface objects can
>> be used to receive status of wl_surface in the scenegraph of the
>> compositor.
>>
>> Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
>> ---
>>
>> Changes for v2:
>> - Rename "error" to "warning" because meaning of "error" in wayland
>> is fatal.
>>
>> Changes for v3:
>> - Move "warning" from ivi_application to ivi_surface.
>> - Squash Makefile.
>> - Add description to ivi_surface:destroy.
>> - Update description of ivi_application:surface_create.
>>
>> Changes for v4:
>> - Remove detail description of server side from
>> ivi_surface::destroy
>> - Add clear discripton what client shall do if it encounters
>> warning in ivi_surface
>> ::warning.
>> - Add decription what happens when client tries to tie a wl_surface
>> to multiple
>> ivi_surfaces.
>>
>> protocol/Makefile.am | 3 +-
>> protocol/ivi-application.xml | 99
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 101 insertions(+), 1 deletion(-)
>> create mode 100644 protocol/ivi-application.xml
>>
>> diff --git a/protocol/Makefile.am b/protocol/Makefile.am
>> index 5e331a7..9913f16 100644
>> --- a/protocol/Makefile.am
>> +++ b/protocol/Makefile.am
>> @@ -8,7 +8,8 @@ protocol_sources = \
>> text-cursor-position.xml \
>> wayland-test.xml \
>> xdg-shell.xml \
>> - scaler.xml
>> + scaler.xml \
>> + ivi-application.xml
>>
>> if HAVE_XMLLINT
>> .PHONY: validate
>> diff --git a/protocol/ivi-application.xml
>> b/protocol/ivi-application.xml
>> new file mode 100644
>> index 0000000..37ad489
>> --- /dev/null
>> +++ b/protocol/ivi-application.xml
>> @@ -0,0 +1,99 @@
>> +<?xml version="1.0" encoding="UTF-8"?>
>> +<protocol name="ivi_application">
>> +
>> + <copyright>
>> + Copyright (C) 2013 DENSO CORPORATION
>> + Copyright (c) 2013 BMW Car IT GmbH
>> +
>> + Permission is hereby granted, free of charge, to any person
>> obtaining a copy
>> + of this software and associated documentation files (the
>> "Software"), to deal
>> + in the Software without restriction, including without limitation
>> the rights
>> + to use, copy, modify, merge, publish, distribute, sublicense,
>> and/or sell
>> + copies of the Software, and to permit persons to whom the
>> Software is
>> + furnished to do so, subject to the following conditions:
>> +
>> + The above copyright notice and this permission notice shall be
>> included in
>> + all copies or substantial portions of the Software.
>> +
>> + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
>> SHALL THE
>> + AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> OTHER
>> + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING FROM,
>> + OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> DEALINGS IN
>> + THE SOFTWARE.
>> + </copyright>
>> +
>> + <interface name="ivi_surface" version="1">
>> + <description summary="application interface to surface in ivi
>> compositor"/>
>> +
>> + <request name="destroy" type="destructor">
>> + <description summary="destroy ivi_surface">
>> + This removes link from surface_id to wl_surface and
>> destroys ivi_surface.
>> + </description>
>> + </request>
>> +
>> + <event name="visibility">
>> + <description summary="visibility of surface in ivi
>> compositor has changed">
>> + The new visibility state is provided in argument
>> visibility.
>> + If visibility is 0, the surface has become invisible.
>> + If visibility is not 0, the surface has become
>> visible.
>> + </description>
>> + <arg name="visibility" type="int"/>
>> + </event>
>> +
>> + <enum name="warning_code">
>> + <description summary="possible warning codes returned by
>> ivi compositor">
>> + These warning codes define all possible warning codes
>> returned by ivi compositor
>
> Codes define codes? :-)
Thanks. I fix it.
>
>> + on server-side warnings.
>> + invalid_wl_surface: invalid wl_surface is set. This
>> happens if wl_surface is destroyed before this.
>
> Ooh, does this mean that invalid_wl_surface is emitted if wl_surface is
> destroyed before the ivi_surface? Try to use more nouns and less
> pronouns in specification language to keep things explicit.
>
> I was about ask what happens if the wl_surface gets destroyed first.
I agree.
>
>> + surface_id_in_use: surface_id is already assigned by
>> another application.
>> + </description>
>> + <entry name="invalid_wl_surface" value="1"
>> summary="wl_surface is invalid"/>
>> + <entry name="surface_id_in_use" value="2"
>> summary="surface_id is in use and can not be shared"/>
>> + </enum>
>> +
>> + <event name="warning">
>> + <description summary="server-side warning detected">
>> + The ivi compositor encountered warning while
>> processing a request by this
>> + application. The warning is defined by argument
>> warning_code and optional
>> + warning_text. If the warning is detected, client
>> shall destroy the ivi_surface
>> + object.
>> +
>> + When ivi compositor encounters warnings, a request is
>> canceled and there is no
>> + mapping from id_surface/wl_surface to this
>> ivi_surface. Even if there is no
>> + mapping but destroying this ivi_surface is
>> recommended.
>
> I would propose a slightly different wording here, which may or may not
> be what you intended above.
>
> "When a warning event is sent, the compositor turns the ivi_surface
> object inert. The ivi_surface will not deliver further events, all
> requests on it are ignored except 'destroy', and the association to the
> surface_id is removed. The client should destroy the ivi_surface
> object. If an inert ivi_surface object is used as an argument to any
> other object's request, that request will [produce a fatal error /
> produce a warning / be ignored]."
>
> There are no requests other than 'destroy' on ivi_surface, but it is
> better to be explicit just in case later someone adds a new request. On
> the final part, you can pick what suits best, or say that the effect is
> undefined.
I agree.
>
>> + </description>
>> + <arg name="warning_code" type="int"/>
>> + <arg name="warning_text" type="string"
>> allow-null="true"/>
>> + </event>
>> +
>> + </interface>
>> +
>> + <interface name="ivi_application" version="1">
>> + <description summary="interface for ivi applications to use
>> ivi compositor features"/>
>
> Could mention that this is a global interface, isn't it?
Yes. I will fix it.
>
>> + <request name="surface_create">
>> + <description summary="create ivi_surface with numeric ID
>> in ivi compositor">
>> + surface_create will create a interface:ivi_surface
>> with numeric ID; surface_id in
>> + ivi compositor. These surface_ids are defined as
>> unique in the system to identify
>> + it inside of ivi compositor. The ivi compositor
>> implements business logic how to
>> + set properties of the surface with surface_id
>> according to status of the system.
>> + E.g. a unique ID for Car Navigation application is
>> used for implementing special
>> + logic of the application about where it shall be
>> located.
>> +
>> + Created ivi_surface notifies warnings when following
>> situation happens,
>> + - Invalid wl_surface is set. This happen if
>> wl_surface is destroy before this.
>
> How is it possible to destroy the wl_surface before issuing a
> ivi_application.surface_create request? IOW, I cannot understand when a
> wl_surface could be invalid due to "destroy". But it could be if the
> wl_surface already has a role.
>
>> + - surface_id is already assigned by another
>> application.
>> + If a client sets a wl_surface to multiple
>> id_surfaces, no warning is issued. Property
>> + of the surface can be controlled by multiple
>> ivi_surface. However this case shall
>> + be avoided by a client.
>
> By id_surface, do you mean creating multiple ivi_surface objects with
> the same or different surface_id for a single wl_surface? Would it ever
> make any sense to do that?
>
> Is it legal or illegal to do that? If it is illegal, there should be a
> fatal protocol error. I know you prefer non-fatal warnings, but even
> Wayland core has fatal errors like for invalid object id. I would think
> that creating several ivi_surfaces for the same wl_surface would be a
> similar problem as use-after-free producing an invalid object id, if it
> really is illegal.
>
> Also, if ivi_application.surface_create assigns a role to a wl_surface,
> then you cannot have several ivi_surfaces referencing the same
> wl_surface.
>
> In Weston, a role is identified by weston_surface::configure function
> pointer. More conceptually, a role defines how the surface behaves on
> screen: cursor surface moves with the pointer, ivi_surface is
> controlled by your... ivi-controller or something. In principle,
> without a role, a surface cannot be shown at all because the compositor
> does not know how or when to show it.
I agree. I will take care of it as fatal.
>
>> + </description>
>> + <arg name="id_surface" type="uint"/>
>
> id_surface here, surface_id in the doc; maybe pick one form, and use
> that everywhere?
>
> I wonder, could we use a more specific term for the ID? Would "ivi_id"
> be unambiguous? So we could talk about surface's ivi_id, as opposed to
> surface_id which is not obviously an IVI concept. Every Wayland
> protocol
> object has an id, and I would like avoid any chances of confusing
> wl_surface id with ivi_id.
I agree.
>
>> + <arg name="surface" type="object"
>> interface="wl_surface"/>
>> + <arg name="id" type="new_id" interface="ivi_surface"/>
>> + </request>
>> +
>> + </interface>
>> +
>> +</protocol>
>
> This is looking good, mostly just some details in the wording to be
> tuned. :-)
Thank you for reviewing. I will take care of it as your suggestion.
BR,
Nobuhiko
>
> I will see if I can review more of the patches, but I would also
> suggest the following, in case you are still interested in pushing this
> upstream.
>
> Wait for Weston 1.5 to be released. We are currently in freeze, so
> there is no point in re-sending until that is done. Check that the 1.5
> stable branch has been created, or at least the release has been made,
> before you rebase and re-send this series.
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list