[PATCH weston-ivi-shell 04/15] ivi-shell supports a type of shell for In-Vehicle Infotainment system.
Nobuhiko Tanibata
nobuhiko_tanibata at xddp.denso.co.jp
Mon Mar 10 22:51:21 PDT 2014
2014-03-10 19:47 に Pekka Paalanen さんは書きました:
> On Thu, 6 Mar 2014 18:56:58 +0900
> Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp> wrote:
>
>> In-Vehicle Infotainment system traditionally manages surfaces with
>> global
>> identification. A protocol, ivi_application, supports such a feature
>> by
>> implementing a request, ivi_application::surface_creation defined in
>> ivi_application.xml. Additionally, it initialize a library,
>> weston-layout,
>> to manage properties of surfaces and group surfaces in layer. In
>> detail,
>> refer library, weston_layout.
>>
>> Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
>> ---
>> ivi-shell/ivi-shell.c | 333
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 333 insertions(+)
>> create mode 100644 ivi-shell/ivi-shell.c
>>
>> diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
>> new file mode 100644
>> index 0000000..f4c4d25
>> --- /dev/null
>> +++ b/ivi-shell/ivi-shell.c
>> @@ -0,0 +1,333 @@
>> +/*
>> + * Copyright (C) 2013 DENSO CORPORATION
>> + *
>> + * 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.
>> + */
>> +
>> +
>> +/**
>> + * ivi-shell supports a type of shell for In-Vehicle Infotainment
>> system.
>> + * In-Vehicle Infotainment system traditionally manages surfaces with
>> global
>> + * identification. A protocol, ivi_application, supports such a
>> feature
>> + * by implementing a request, ivi_application::surface_creation
>> defined in
>> + * ivi_application.xml.
>> + *
>> + * Additionally, it initialize a library, weston-layout, to manage
>> properties of
>> + * surfaces and group surfaces in layer. In detail, refer
>> weston_layout.
>> + */
>> +
>> +#include <sys/wait.h>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <linux/input.h>
>> +
>> +#include "compositor.h"
>> +#include "ivi-application-server-protocol.h"
>> +#include "weston-layout.h"
>
> weston-layout.h is introduced in the following patch, so this patch
> won't build, breaking bisectability if this series was applied.
>
I agree.
>> +
>> +struct ivi_shell;
>> +
>> +struct ivi_shell_surface
>> +{
>> + struct ivi_shell *shell;
>> + struct weston_layout_surface *layout_surface;
>> +
>> + struct weston_surface *surface;
>> + uint32_t id_surface;
>> +
>> + int32_t width;
>> + int32_t height;
>> +
>> + struct wl_list link;
>> +};
>> +
>> +struct ivi_shell
>> +{
>> + struct wl_resource *resource;
>> + struct wl_listener destroy_listener;
>> +
>> + struct weston_compositor *compositor;
>> + struct weston_seat *seat;
>
> 'compositor' and 'seat' seem pretty unused?
>
>> +
>> + struct wl_list list_surface;
>
> We prefer the convention <something>_list instead. Personally, I've
> also added a comment about what is in this list, like
> /* ivi_shell_surface::link */ because that is an invariant.
I agree.
>
>> +};
>> +
>> +/*
>> ------------------------------------------------------------------------- */
>> +/* common functions
>> */
>> +/*
>> ------------------------------------------------------------------------- */
>> +
>> +static void
>> +configure(struct weston_view *view, float x, float y)
>> +{
>> + if (view != NULL) {
>> + weston_view_set_position(view, x, y);
>> + weston_view_update_transform(view);
>
> Something in this feels odd... the part about calling
> weston_view_update_transform() manually... is that ok, or should it be
> called only when mapping the surface?
>
I move the above code to below.
>> + }
>> +}
>> +
>> +/**
>> + * Implementation of ivi_surface
>> + */
>> +
>> +static void
>> +ivi_shell_surface_configure(struct weston_surface *, int32_t,
>> int32_t);
>> +
>> +static struct ivi_shell_surface *
>> +get_ivi_shell_surface(struct weston_surface *surface)
>> +{
>> + if (surface->configure == ivi_shell_surface_configure) {
>> + return surface->configure_private;
>> + } else {
>> + return NULL;
>> + }
>> +}
>> +
>> +static void
>> +ivi_shell_surface_configure(struct weston_surface *es,
>> + int32_t sx, int32_t sy)
>> +{
>> + struct ivi_shell_surface *ivisurf = get_ivi_shell_surface(es);
>> + struct weston_view *view = NULL;
>> + float from_x = 0.0f;
>> + float from_y = 0.0f;
>> + float to_x = 0.0f;
>> + float to_y = 0.0f;
>> +
>> + if ((es->width == 0) || (es->height == 0) || (ivisurf == NULL)) {
>> + return;
>> + }
>> +
>> + view = weston_layout_get_weston_view(ivisurf->layout_surface);
>> + if (view == NULL) {
>> + return;
>> + }
>> +
>> + if (ivisurf->width != es->width || ivisurf->height != es->height)
>> {
>> +
>> + ivisurf->width = es->width;
>> + ivisurf->height = es->height;
>> +
>> + weston_view_to_global_float(view, 0, 0, &from_x, &from_y);
>> + weston_view_to_global_float(view, sx, sy, &to_x, &to_y);
>> + configure(view,
>> + view->geometry.x + to_x - from_x,
>> + view->geometry.y + to_y - from_y);
>> +
>> + weston_layout_surfaceConfigure(ivisurf->layout_surface,
>> es->width, es->height);
>> + }
>> +}
>> +
>> +static void
>> +surface_destroy(struct wl_client *client,
>> + struct wl_resource *resource)
>> +{
>> + struct ivi_shell_surface *ivisurf =
>> wl_resource_get_user_data(resource);
>> + struct ivi_shell *shell = ivisurf->shell;
>> +
>> + weston_layout_surfaceRemove(ivisurf->layout_surface);
>> +
>> + ivisurf->surface->configure = NULL;
>> + ivisurf->surface->configure_private = NULL;
>> +
>> + if (!wl_list_empty(&ivisurf->link)) {
>> + wl_list_remove(&ivisurf->link);
>> + }
>> + wl_list_init(&shell->list_surface);
>> +
>> + free(ivisurf);
>> + ivisurf = NULL;
>
> Local variable.
>
> I just realized that your protocol spec does not define what should
> happen on destruction.
I will add description to ivi_application.xml.
>
>> +}
>> +
>> +static const struct ivi_surface_interface surface_implementation = {
>> + surface_destroy,
>> +};
>> +
>> +static struct ivi_shell_surface *
>> +is_surf_in_surfaces(struct wl_list *list_surf, uint32_t id_surface)
>> +{
>> + struct ivi_shell_surface *ivisurf;
>> +
>> + wl_list_for_each(ivisurf, list_surf, link) {
>> + if (ivisurf->id_surface == id_surface) {
>> + return ivisurf;
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +/**
>> + * Implementation of ivi_application::surface_create.
>> + * Creating new ivi_shell_surface with identification to identify the
>> surface
>> + * in the system.
>> + */
>> +static void
>> +application_surface_create(struct wl_client *client,
>> + struct wl_resource *resource,
>> + uint32_t id_surface,
>> + struct wl_resource *surface_resource,
>> + uint32_t id)
>> +{
>> + struct ivi_shell *shell = wl_resource_get_user_data(resource);
>> + struct weston_layout_surface *layout_surface = NULL;
>> + struct ivi_shell_surface *ivisurf = NULL;
>> + struct weston_surface *es = NULL;
>> +
>> + es = wl_resource_get_user_data(surface_resource);
>> + if (es == NULL) {
>> + weston_log("application_surface_create: invalid surface\n");
>> + return;
>> + }
>> +
>> + ivisurf = is_surf_in_surfaces(&shell->list_surface, id_surface);
>> +
>> + layout_surface = weston_layout_surfaceCreate(es, id_surface);
>> + if (layout_surface == NULL) {
>> +//FIXME
>
> Forgot something?
> Seems like double-checking the same thing, slightly confusing. Avoid
> calling weston_layout_surfaceCreate() to begin with, if (ivisurf)?
>
>> + weston_log("id_surface is already created\n");
>> +
>> + ivisurf->surface = es;
>> + ivisurf->width = 0;
>> + ivisurf->height = 0;
>> +
>> + es->configure = ivi_shell_surface_configure;
>> + es->configure_private = ivisurf;
>> +
>> + resource = wl_resource_create(client, &ivi_surface_interface,
>> 1, id);
>> + if (resource == NULL) {
>> + weston_log("couldn't get surface object");
>> + return;
>> + }
>> +
>> + wl_resource_set_implementation(resource,
>> + &surface_implementation,
>> + ivisurf, NULL);
>
> Does this mean, that if a client calls ivi_application.surface_create
> with an already used ID, it will get a handle to the existing
> ivi_shell_surface? Is that supposed to work? If yes, then
> ivi_shell_surface needs a list of all wl_resources pointing to it.
>
> IDs are global, aren't they? So it could get a handle to some *other*
> client's ivi_shell_surface. That doesn't sound good.
>
I will realign warning in ivi_application.xml and update the above code.
Basically, I agree with your comments.
>> + return;
>> + }
>> +
>> + ivisurf = calloc(1, sizeof *ivisurf);
>> + if (ivisurf == NULL) {
>> + weston_log("fails to allocate memory\n");
>> + return;
>> + }
>> +
>> + wl_list_init(&ivisurf->link);
>> + ivisurf->shell = shell;
>> + ivisurf->layout_surface = layout_surface;
>> + ivisurf->surface = es;
>> + ivisurf->id_surface = id_surface;
>> + ivisurf->width = 0;
>> + ivisurf->height = 0;
>> +
>> + es->configure = ivi_shell_surface_configure;
>> + es->configure_private = ivisurf;
>> +
>> + wl_list_insert(&shell->list_surface, &ivisurf->link);
>> +
>> + resource = wl_resource_create(client, &ivi_surface_interface, 1,
>> id);
>> + if (resource == NULL) {
>> + weston_log("couldn't get surface object");
>> + return;
>> + }
>> +
>> + wl_resource_set_implementation(resource,
>> + &surface_implementation,
>> + ivisurf, NULL);
>
> Are you sure you don't need a destructor here? If the client
> disconnects without explicitly destroying all its protocol objects, how
> does ivisurf get freed?
>
I agree.
>> +}
>> +
>> +static const struct ivi_application_interface
>> application_implementation = {
>> + application_surface_create
>> +};
>> +
>> +static void
>> +bind_ivi_application(struct wl_client *client,
>> + void *data, uint32_t version, uint32_t id)
>> +{
>> + struct ivi_shell *shell = data;
>> + struct wl_resource *resource = NULL;
>> +
>> + resource = wl_resource_create(client, &ivi_application_interface,
>> 1, id);
>> +
>> + wl_resource_set_implementation(resource,
>> + &application_implementation,
>> + shell, NULL);
>> +}
>> +
>> +/**
>> + * Initialization/destruction method of ivi-shell
>> + */
>> +static void
>> +shell_destroy(struct wl_listener *listener, void *data)
>> +{
>> + struct ivi_shell *shell = NULL;
>> +
>> + shell = container_of(listener, struct ivi_shell,
>> destroy_listener);
>> +
>> + free(shell);
>> + shell = NULL;
>
> Nulling a local variable again.
>
> I think you should do some sanity/leak checks here, like ensuring that
> the surface list is empty, no?
I agree. This is just my habit.
>
>> +}
>> +
>> +static void
>> +init_ivi_shell(struct weston_compositor *ec, struct ivi_shell *shell)
>> +{
>> + shell->compositor = ec;
>> +
>> + wl_list_init(&ec->layer_list);
>> + wl_list_init(&shell->list_surface);
>> +}
>> +
>> +/**
>> + * Initialization of ivi-shell. A library, weston_layout, is also
>> initialized
>> + * here by calling weston_layout_initWithCompositor.
>> + *
>> + */
>> +
>> +WL_EXPORT int
>> +module_init(struct weston_compositor *ec,
>> + int *argc, char *argv[])
>> +{
>> + struct ivi_shell *shell = NULL;
>> + struct weston_seat *seat = NULL;
>> +
>> + shell = calloc(1, sizeof *shell);
>> + if (shell == NULL) {
>> + return -1;
>> + }
>> +
>> + init_ivi_shell(ec, shell);
>> + weston_layout_initWithCompositor(ec);
>
> It's kind of surprising... this module's init requires another module
> to
> have been loaded before, otherwise this module will not load, right?
> I wonder if you get a sensible error message when that happens.
westen_layout is not a module to be loaded by ivi-shell. It is shared
library.
If it is not weston manner, I can link it statically. Please give me
your suggestion?
>
>> +
>> + shell->destroy_listener.notify = shell_destroy;
>> + wl_signal_add(&ec->destroy_signal, &shell->destroy_listener);
>> +
>> + if (wl_global_create(ec->wl_display, &ivi_application_interface,
>> 1,
>> + shell, bind_ivi_application) == NULL) {
>> + return -1;
>> + }
>> +
>> + wl_list_for_each(seat, &ec->seat_list, link) {
>> + if (seat) {
>> + shell->seat = seat;
>
> 'seat' cannot be NULL here. It seems you are assuming that there can be
> only one seat? Why?
>
Ideally, I agree. However I am in automotive software, and I have habit
to avoid NULL access as much as possible in case of the worst scenario.
Thank you,
Nobuhiko
>> + }
>> + }
>> +
>> + return 0;
>> +}
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list