[PATCH weston-ivi-shell v5 2/9] The weston-layout library supports
Pekka Paalanen
ppaalanen at gmail.com
Fri Apr 25 00:24:07 PDT 2014
On Thu, 20 Mar 2014 15:59:34 +0900
Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp> wrote:
> API set of controlling properties of surface and layer which groups
> surfaces. An unique ID whose type is integer is required to create
> surface and layer. With the unique ID, surface and layer are identified
> to control them. The API set consists of APIs to control properties of
> surface and layers about followings,
>
> - visibility.
> - opacity.
> - clipping (x,y,width,height).
> - position and size of it to be displayed.
> - orientation per 90 degree.
> - add or remove surfaces to a layer.
> - order of surfaces/layers in layer/screen to be displayed.
> - commit to apply property changes.
> - notifications of property change.
>
> Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at xddp.denso.co.jp>
> ---
>
> Changes for v2:
> - move this patch in front of ivi-shell patch to be compiled successfully.
> - unsupport weston_layout_takeSurfaceScreenshot because implementation needs to
> be discussed more. It is pending.
> - support inherit propoerties of id_surface when client attaches another
> wl_surface with id_surface after destroying ivi_surface once.
> - bug fix of https://bugs.tizen.org/jira/browse/TIVI-2882
>
> Changes for v3 and v4:
> - nothing. Version number aligned to the first patch
>
> Changes for v5:
> - bug fix of https://bugs.tizen.org/jira/browse/TIVI-2881
>
> Makefile.am | 1 +
> configure.ac | 15 +-
> ivi-shell/Makefile.am | 30 +
> ivi-shell/weston-layout.c | 2639 +++++++++++++++++++++++++++++++++++++++++++++
> ivi-shell/weston-layout.h | 934 ++++++++++++++++
> 5 files changed, 3618 insertions(+), 1 deletion(-)
> create mode 100644 ivi-shell/Makefile.am
> create mode 100644 ivi-shell/weston-layout.c
> create mode 100644 ivi-shell/weston-layout.h
>
> diff --git a/Makefile.am b/Makefile.am
> index f22c542..1bc35e2 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -11,6 +11,7 @@ SUBDIRS = \
> src \
> $(xwayland_subdir) \
> desktop-shell \
> + ivi-shell \
> clients \
> data \
> protocol \
Weston recently moved to monolithic Makefile, so the automake stuff
will need porting.
> diff --git a/configure.ac b/configure.ac
> index cce1850..4c0a90f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -409,6 +409,16 @@ if test "x$enable_dbus" != "xno"; then
> fi
> AM_CONDITIONAL(ENABLE_DBUS, test "x$enable_dbus" = "xyes")
>
> +# ivi-shell support
> +AC_ARG_ENABLE(ivi-shell,
> + AS_HELP_STRING([--disable-ivi-shell],
> + [do not build ivi-shell server plugin and client]),,
> + enable_ivi_shell=yes)
> +AM_CONDITIONAL(ENABLE_IVI_SHELL, test "x$enable_ivi_shell" = "xyes")
> +if test x$enable_ivi_shell = xyes; then
> + PKG_CHECK_MODULES(IVI_SHELL, [cairo])
Only cairo? Or maybe cairo-image?
Hmm, I didn't see this patch add any Cairo calls.
> +fi
> +
> AC_ARG_ENABLE(wcap-tools, [ --disable-wcap-tools],, enable_wcap_tools=yes)
> AM_CONDITIONAL(BUILD_WCAP_TOOLS, test x$enable_wcap_tools = xyes)
> if test x$enable_wcap_tools = xyes; then
> @@ -505,7 +515,8 @@ AC_CONFIG_FILES([Makefile
> data/Makefile
> protocol/Makefile
> man/Makefile
> - tests/Makefile])
> + tests/Makefile
> + ivi-shell/Makefile])
> AC_OUTPUT
>
> AC_MSG_RESULT([
> @@ -519,6 +530,8 @@ AC_MSG_RESULT([
> XWayland ${enable_xwayland}
> dbus ${enable_dbus}
>
> + ivi-shell ${enable_ivi_shell}
> +
> Build wcap utility ${enable_wcap_tools}
>
> weston-launch utility ${enable_weston_launch}
> diff --git a/ivi-shell/Makefile.am b/ivi-shell/Makefile.am
> new file mode 100644
> index 0000000..4d54c2d
> --- /dev/null
> +++ b/ivi-shell/Makefile.am
> @@ -0,0 +1,30 @@
> +moduledir = $(libdir)/weston
> +
> +module_LTLIBRARIES = \
> + $(libweston_layout)
> +
> +AM_CPPFLAGS = \
> + -I$(top_srcdir)/shared \
> + -I$(top_srcdir)/src \
> + -I$(top_builddir)/src \
> + -DDATADIR='"$(datadir)"' \
> + -DMODULEDIR='"$(moduledir)"' \
> + -DLIBEXECDIR='"$(libexecdir)"' \
> + -DIN_WESTON
> +
> +westonincludedir = $(includedir)/weston
> +westoninclude_HEADERS =
> +
> +if ENABLE_IVI_SHELL
> +westoninclude_HEADERS += \
> + weston-layout.h
> +
> +libweston_layout = libweston-layout.la
> +libweston_layout_la_LDFLAGS = -avoid-version
This is not a Weston plugin, right?
You intend other stuff to link to this library during build and not by
dlopen(), and you intend to install this library along with weston, by
the name libweston-layout.so?
In that case I think you should version this, and fully commit to the
API/ABI stability as it is public. I'd like to see the guarantees
documented somewhere.
Could you explain again, why this is a separate library?
Who and why would want to replace this in a Weston-IVI system?
If libweston-layout.so is Weston-specific, why build it as a separate
library instead of having ivi-shell.so export the same ABI? A little
like we have the EGL specification and standard headers, but vendors
still make their own libEGL.so exporting the specified ABI; though
looking at the header it seems to be very Weston-specific, so no need
for such standardization.
I mean, something (ivi-shell.so?) needs to load hmi-controller.so and/or
ivi-controller.so into Weston by dlopen(), right? So is there some
problem having ivi-shell.so expose the API they need? Or do you just
need something to link ivi-controller.so against for building?
Looking at the PDF picture about the architecture, there are very many
moving pieces with ABIs between them, so reducing them even by one
should help.
I do not have clear picture on which interfaces have ABI stability
guarantees. Usually all installed libraries must have stable and
properly versioned ABI. Weston plugins are not libraries per se; the ABI
stability guarantees apply to symbols exported from Weston core. The
plugin itself usually exports only a single symbol to initialize it. But
if a Weston plugin like ivi-shell.so was to export symbols to be used by
further dynamically loaded code, those symbols become exported ABI
again.
I'd like to see some clarification here, and how you intend to version
the ABIs. In Weston's case, the Weston core exported ABI towards
plugins is stable in 1.x.*, but is allowed to break between 1.x and
1.x+1. Weston is an executable, so I don't think it can have the
library versioning as .so libraries do, but if you build a lib*.so with
the intention of build-time linking, I believe you should do the proper
library versioning, not '-avoid-version'.
> +libweston_layout_la_LIBADD = $(IVI_SHELL_LIBS) ../shared/libshared.la
> +libweston_layout_la_CFLAGS = $(GCC_CFLAGS) $(IVI_SHELL_CFLAGS)
> +libweston_layout_la_SOURCES = \
> + weston-layout.c \
> + weston-layout.h
> +
> +endif
> diff --git a/ivi-shell/weston-layout.c b/ivi-shell/weston-layout.c
> new file mode 100644
> index 0000000..6fabf48
> --- /dev/null
> +++ b/ivi-shell/weston-layout.c
> @@ -0,0 +1,2639 @@
> +/*
> + * 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.
> + */
> +
> +/**
> + * Implementation of weston-layout library. The actual view on screen is
> + * not updated till calling weston_layout_commitChanges. A overview from
> + * calling API for updating properties of surfaces/layer to asking compositor
> + * to compose them by using weston_compositor_schedule_repaint,
> + * 0/ initialize this library by weston_layout_initWithCompositor
> + * with (struct weston_compositor *ec) from ivi-shell.
> + * 1/ When a API for updating properties of surface/layer, it updates
> + * pending prop of weston_layout_surface/layer/screen which are structure to
> + * store properties.
> + * 2/ Before calling commitChanges, in case of calling a API to get a property,
> + * return current property, not pending property.
> + * 3/ At the timing of calling weston_layout_commitChanges, pending properties
> + * are applied
> + * to properties.
> + * 4/ According properties, set transformation by using weston_matrix and
> + * weston_view per surfaces and layers in while loop.
> + * 5/ Set damage and trigger transform by using weston_view_geometry_dirty and
> + * weston_view_geometry_dirty.
> + * 6/ Notify update of properties.
> + * 7/ Trigger composition by weston_compositor_schedule_repaint.
Ok.
> + *
> + */
> +
> +#include <sys/wait.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <linux/input.h>
> +#include <cairo.h>
> +
> +#include "compositor.h"
> +#include "weston-layout.h"
> +
> +enum weston_layout_surface_orientation {
> + WESTON_LAYOUT_SURFACE_ORIENTATION_0_DEGREES = 0,
> + WESTON_LAYOUT_SURFACE_ORIENTATION_90_DEGREES = 1,
> + WESTON_LAYOUT_SURFACE_ORIENTATION_180_DEGREES = 2,
> + WESTON_LAYOUT_SURFACE_ORIENTATION_270_DEGREES = 3,
> +};
> +
> +enum weston_layout_surface_pixelformat {
> + WESTON_LAYOUT_SURFACE_PIXELFORMAT_R_8 = 0,
> + WESTON_LAYOUT_SURFACE_PIXELFORMAT_RGB_888 = 1,
> + WESTON_LAYOUT_SURFACE_PIXELFORMAT_RGBA_8888 = 2,
> + WESTON_LAYOUT_SURFACE_PIXELFORMAT_RGB_565 = 3,
> + WESTON_LAYOUT_SURFACE_PIXELFORMAT_RGBA_5551 = 4,
> + WESTON_LAYOUT_SURFACE_PIXELFORMAT_RGBA_6661 = 5,
> + WESTON_LAYOUT_SURFACE_PIXELFORMAT_RGBA_4444 = 6,
> + WESTON_LAYOUT_SURFACE_PIXELFORMAT_UNKNOWN = 7,
> +};
> +
> +struct link_layer {
> + struct weston_layout_layer *ivilayer;
> + struct wl_list link;
> +};
> +
> +struct link_screen {
> + struct weston_layout_screen *iviscrn;
> + struct wl_list link;
> +};
Do you have an arbitrary number of lists, where a ivilayer or iviscrn
can be in?
If not, why not embed the wl_list member into weston_layout_* structs?
> +
> +struct link_layerPropertyNotification {
> + layerPropertyNotificationFunc callback;
> + void *userdata;
> + struct wl_list link;
> +};
> +
> +struct link_surfacePropertyNotification {
> + surfacePropertyNotificationFunc callback;
> + void *userdata;
> + struct wl_list link;
> +};
> +
> +struct link_layerCreateNotification {
> + layerCreateNotificationFunc callback;
> + void *userdata;
> + struct wl_list link;
> +};
> +
> +struct link_layerRemoveNotification {
> + layerRemoveNotificationFunc callback;
> + void *userdata;
> + struct wl_list link;
> +};
> +
> +struct link_surfaceCreateNotification {
> + surfaceCreateNotificationFunc callback;
> + void *userdata;
> + struct wl_list link;
> +};
> +
> +struct link_surfaceRemoveNotification {
> + surfaceRemoveNotificationFunc callback;
> + void *userdata;
> + struct wl_list link;
> +};
> +
> +struct link_surfaceConfigureNotification {
> + surfaceConfigureNotificationFunc callback;
> + void *userdata;
> + struct wl_list link;
> +};
Any reason you defined all these different structs, and didn't use the
wl_signal/wl_listener pattern?
> +
> +struct weston_layout;
> +
> +struct weston_layout_surface {
> + struct wl_list link;
For link members, I would personally prefer some indication of which
(kind of) list the link will belong to, if possible and especially if
there are multiple links in a struct.
> + struct wl_list list_notification;
> + struct wl_list list_layer;
The Weston convention is to say "notification_list", "layer_list". I
would also add a comment saying what link the list will hold, e.g.
... foo_list; /* struct weston_layout_layer::link */
That explicitly says what the struct embedding the 'link' member is, as
one list can only ever contain one type of objects, and explains how to
access that object from a link pointer.
Hmm... so one surface can be in multiple layers??
> + uint32_t update_count;
> + uint32_t id_surface;
> +
> + struct weston_layout *layout;
> + struct weston_surface *surface;
> + struct weston_view *view;
Why have the view pointer explicitly here? Can't you access that via
weston_surface?
> +
> + uint32_t buffer_width;
> + uint32_t buffer_height;
Would these perhaps happen to be the same as
weston_surface::width_from_buffer and
weston_surface::height_from_buffer?
They feel a bit out of place here.
> +
> + struct wl_listener surface_destroy_listener;
> + struct weston_transform surface_rotation;
> + struct weston_transform layer_rotation;
> + struct weston_transform surface_pos;
> + struct weston_transform layer_pos;
> + struct weston_transform scaling;
> + struct weston_layout_SurfaceProperties prop;
> + int32_t pixelformat;
> + uint32_t event_mask;
> +
> + struct {
> + struct weston_layout_SurfaceProperties prop;
> + struct wl_list link;
> + } pending;
> +
> + struct {
> + struct wl_list link;
> + struct wl_list list_layer;
> + } order;
> +};
> +
> +struct weston_layout_layer {
> + struct wl_list link;
> + struct wl_list list_notification;
> + struct wl_list list_screen;
> + uint32_t id_layer;
> +
> + struct weston_layout *layout;
> + struct weston_layer el;
> +
> + struct weston_layout_LayerProperties prop;
> + uint32_t event_mask;
> +
> + struct {
> + struct weston_layout_LayerProperties prop;
> + struct wl_list list_surface;
> + struct wl_list link;
> + } pending;
> +
> + struct {
> + struct wl_list list_surface;
> + struct wl_list link;
> + } order;
> +};
> +
> +struct weston_layout_screen {
> + struct wl_list link;
> + uint32_t id_screen;
> +
> + struct weston_layout *layout;
> + struct weston_output *output;
> +
> + uint32_t event_mask;
> +
> + struct {
> + struct wl_list list_layer;
> + struct wl_list link;
> + } pending;
> +
> + struct {
> + struct wl_list list_layer;
> + struct wl_list link;
> + } order;
> +};
> +
> +struct weston_layout {
> + struct weston_compositor *compositor;
> +
> + struct wl_list list_surface;
> + struct wl_list list_layer;
> + struct wl_list list_screen;
> +
> + struct {
> + struct wl_list list_create;
> + struct wl_list list_remove;
> + } layer_notification;
> +
> + struct {
> + struct wl_list list_create;
> + struct wl_list list_remove;
> + struct wl_list list_configure;
> + } surface_notification;
> +
> + /* to enable displaying cursor*/
> + int32_t is_cursor_enabled;
> +};
> +
> +struct weston_layout ivilayout = {0};
A global, non-static variable, what for?
Should it be static at least?
> +
> +static struct weston_layout *
> +get_instance(void)
> +{
> + return &ivilayout;
> +}
> +
...
> +/**
> + * Internal API to initialize screens found from output_list of weston_compositor.
> + * Called by weston_layout_initWithCompositor.
> + */
> +static void
> +create_screen(struct weston_compositor *ec)
> +{
> + struct weston_layout *layout = get_instance();
> + struct weston_layout_screen *iviscrn = NULL;
> + struct weston_output *output = NULL;
> + int32_t count = 0;
> +
> + wl_list_for_each(output, &ec->output_list, link) {
> + iviscrn = calloc(1, sizeof *iviscrn);
> + if (iviscrn == NULL) {
> + weston_log("fails to allocate memory\n");
> + continue;
> + }
> +
> + wl_list_init(&iviscrn->link);
> + iviscrn->layout = layout;
> +
> + iviscrn->id_screen = count;
> + count++;
> +
> + iviscrn->output = output;
> + iviscrn->event_mask = 0;
> +
> + wl_list_init(&iviscrn->pending.list_layer);
> + wl_list_init(&iviscrn->pending.link);
> +
> + wl_list_init(&iviscrn->order.list_layer);
> + wl_list_init(&iviscrn->order.link);
> +
> + wl_list_insert(&layout->list_screen, &iviscrn->link);
> + }
> +}
Do you handle at all the case of hotplugging outputs?
Does the IVI software architecture simply not support any kind of
hotplug?
> +/**
> + * Exported APIs of weston-layout library are implemented from here.
> + * Brief of APIs is described in weston-layout.h.
> + */
> +WL_EXPORT struct weston_view *
> +weston_layout_get_weston_view(struct weston_layout_surface *surface)
> +{
> + return (surface != NULL) ? surface->view : NULL;
> +}
> +
> +WL_EXPORT void
> +weston_layout_initWithCompositor(struct weston_compositor *ec)
> +{
> + struct weston_layout *layout = get_instance();
> +
> + layout->compositor = ec;
> +
> + wl_list_init(&layout->list_surface);
> + wl_list_init(&layout->list_layer);
> + wl_list_init(&layout->list_screen);
> +
> + wl_list_init(&layout->layer_notification.list_create);
> + wl_list_init(&layout->layer_notification.list_remove);
> +
> + wl_list_init(&layout->surface_notification.list_create);
> + wl_list_init(&layout->surface_notification.list_remove);
> + wl_list_init(&layout->surface_notification.list_configure);
> +
> + create_screen(ec);
> +
> + struct weston_config *config = weston_config_parse("weston.ini");
> + struct weston_config_section *s =
> + weston_config_get_section(config, "ivi-shell", NULL, NULL);
> +
> + /*A cursor is configured if weston.ini has keys.*/
> + char* cursor_theme = NULL;
> + weston_config_section_get_string(s, "cursor-theme", &cursor_theme, NULL);
> + layout->is_cursor_enabled = (NULL != cursor_theme);
> + free(cursor_theme);
> + weston_config_destroy(config);
> +}
> +
> +WL_EXPORT int32_t
> +weston_layout_setNotificationCreateLayer(layerCreateNotificationFunc callback,
> + void *userdata)
> +{
> + struct weston_layout *layout = get_instance();
> + struct link_layerCreateNotification *notification = NULL;
> +
> + if (callback == NULL) {
> + weston_log("weston_layout_setNotificationCreateLayer: invalid argument\n");
> + return -1;
> + }
> +
> + notification = malloc(sizeof *notification);
> + if (notification == NULL) {
> + weston_log("fails to allocate memory\n");
> + return -1;
> + }
> +
> + notification->callback = callback;
> + notification->userdata = userdata;
> + wl_list_init(¬ification->link);
> + wl_list_insert(&layout->layer_notification.list_create, ¬ification->link);
> +
> + return 0;
> +}
How does a user of this API remove a notification callback?
Also seems like this is more of an "add" rather than "set", since you
can add many callbacks and they all work.
> +WL_EXPORT struct weston_layout_screen *
> +weston_layout_getScreenFromId(uint32_t id_screen)
> +{
> + struct weston_layout *layout = get_instance();
> + struct weston_layout_screen *iviscrn = NULL;
> + (void)id_screen;
> +
> + wl_list_for_each(iviscrn, &layout->list_screen, link) {
> +//FIXME : select iviscrn from list_screen by id_screen
Ahha ;-)
> + return iviscrn;
> + break;
> + }
> +
> + return NULL;
> +}
...
> +WL_EXPORT int32_t
> +weston_layout_UpdateInputEventAcceptanceOn(struct weston_layout_surface *ivisurf,
> + uint32_t devices, uint32_t acceptance)
> +{
> + /* not supported */
> + (void)ivisurf;
> + (void)devices;
> + (void)acceptance;
> +
> + return 0;
Error, or silently ignored?
Do you plan to implement this function?
There are a lot more similar functions which have the same "not
supported" comment.
...
> diff --git a/ivi-shell/weston-layout.h b/ivi-shell/weston-layout.h
> new file mode 100644
> index 0000000..2667784
> --- /dev/null
> +++ b/ivi-shell/weston-layout.h
> @@ -0,0 +1,934 @@
> +/*
> + * 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.
> + */
> +
> +/**
> + * The weston-layout library supports API set of controlling properties of
> + * surface and layer which groups surfaces. An unique ID whose type is integer
> + * is required to create surface and layer. With the unique ID, surface and
> + * layer are identified to control them. The API set consists of APIs to control
> + * properties of surface and layers about followings,
> + * - visibility.
> + * - opacity.
> + * - clipping (x,y,width,height).
> + * - position and size of it to be displayed.
> + * - orientation per 90 degree.
> + * - add or remove surfaces to a layer.
> + * - order of surfaces/layers in layer/screen to be displayed.
> + * - commit to apply property changes.
> + * - notifications of property change.
> + *
> + * Management of surfaces and layers grouping these surfaces are common way in
> + * In-Vehicle Infotainment system, which integrate several domains in one system.
> + * A layer is allocated to a domain in order to control application surfaces
> + * grouped to the layer all together.
> + */
> +
> +#ifndef _WESTON_LAYOUT_H_
> +#define _WESTON_LAYOUT_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +#include "compositor.h"
> +
> +struct weston_layout_SurfaceProperties
> +{
> + float opacity;
> + uint32_t sourceX;
> + uint32_t sourceY;
> + uint32_t sourceWidth;
> + uint32_t sourceHeight;
> + uint32_t origSourceWidth;
> + uint32_t origSourceHeight;
> + int32_t destX;
> + int32_t destY;
> + uint32_t destWidth;
> + uint32_t destHeight;
All these unsigned members seem like they are calling for mismatching
signed and unsigned variables in computations, which can lead to
surprising bugs. Is there a reason why these coordinate related members
(position and size) are unsigned?
Weston code base and even Wayland protocol categorically uses signed
integers for everything that is computed with, like size which can
never be legally negative. Only flags, bitfields, object ids and such
are unsigned, because they will never be used in computations that may
include signed integers.
> + uint32_t orientation;
> + uint32_t visibility;
> + uint32_t frameCounter;
> + uint32_t drawCounter;
> + uint32_t updateCounter;
> + uint32_t pixelformat;
> + uint32_t nativeSurface;
> + uint32_t inputDevicesAcceptance;
> + uint32_t chromaKeyEnabled;
> + uint32_t chromaKeyRed;
> + uint32_t chromaKeyGreen;
> + uint32_t chromaKeyBlue;
> + int32_t creatorPid;
> +};
> +
> +struct weston_layout_LayerProperties
> +{
> + float opacity;
> + uint32_t sourceX;
> + uint32_t sourceY;
> + uint32_t sourceWidth;
> + uint32_t sourceHeight;
> + uint32_t origSourceWidth;
> + uint32_t origSourceHeight;
> + int32_t destX;
> + int32_t destY;
> + uint32_t destWidth;
> + uint32_t destHeight;
> + uint32_t orientation;
> + uint32_t visibility;
> + uint32_t type;
> + uint32_t chromaKeyEnabled;
> + uint32_t chromaKeyRed;
> + uint32_t chromaKeyGreen;
> + uint32_t chromaKeyBlue;
> + int32_t creatorPid;
> +};
> +
> +struct weston_layout_layer;
> +struct weston_layout_surface;
> +struct weston_layout_screen;
> +
> +typedef struct weston_layout_surface* weston_layout_surface_ptr;
> +typedef struct weston_layout_layer* weston_layout_layer_ptr;
> +typedef struct weston_layout_screen* weston_layout_screen_ptr;
Weston coding style seems to avoid this kind of typedefs, it is the
same rationale as in the kernel coding style.
> +
> +#define IVI_BIT(x) (1 << (x))
> +enum weston_layout_notification_mask {
> + IVI_NOTIFICATION_OPACITY = IVI_BIT(1),
> + IVI_NOTIFICATION_SOURCE_RECT = IVI_BIT(2),
> + IVI_NOTIFICATION_DEST_RECT = IVI_BIT(3),
> + IVI_NOTIFICATION_DIMENSION = IVI_BIT(4),
> + IVI_NOTIFICATION_POSITION = IVI_BIT(5),
> + IVI_NOTIFICATION_ORIENTATION = IVI_BIT(6),
> + IVI_NOTIFICATION_VISIBILITY = IVI_BIT(7),
> + IVI_NOTIFICATION_PIXELFORMAT = IVI_BIT(8),
> + IVI_NOTIFICATION_ADD = IVI_BIT(9),
> + IVI_NOTIFICATION_REMOVE = IVI_BIT(10),
Would probably be shorter to just write = 1 << 7,
;-)
Bit 0 unused?
> + IVI_NOTIFICATION_ALL = 0xFFFF
> +};
> +
> +typedef void(*layerPropertyNotificationFunc)(struct weston_layout_layer *ivilayer,
> + struct weston_layout_LayerProperties*,
> + enum weston_layout_notification_mask mask,
> + void *userdata);
OTOH, function pointer typedefs are useful.
> +
> +typedef void(*surfacePropertyNotificationFunc)(struct weston_layout_surface *ivisurf,
> + struct weston_layout_SurfaceProperties*,
> + enum weston_layout_notification_mask mask,
> + void *userdata);
> +
> +typedef void(*layerCreateNotificationFunc)(struct weston_layout_layer *ivilayer,
> + void *userdata);
> +
> +typedef void(*layerRemoveNotificationFunc)(struct weston_layout_layer *ivilayer,
> + void *userdata);
> +
> +typedef void(*surfaceCreateNotificationFunc)(struct weston_layout_surface *ivisurf,
> + void *userdata);
> +
> +typedef void(*surfaceRemoveNotificationFunc)(struct weston_layout_surface *ivisurf,
> + void *userdata);
> +
> +typedef void(*surfaceConfigureNotificationFunc)(struct weston_layout_surface *ivisurf,
> + void *userdata);
> +
> +/**
> + * \brief to be called by ivi-shell in order to set initail view of
> + * weston_surface.
> + */
> +struct weston_view *
> +weston_layout_get_weston_view(struct weston_layout_surface *surface);
This creates a weston_view? No, it doesn't. What does this have to do
with the initial view?
A side note; If you built weston-layout into ivi-shell.so instead of
making a weston-layout.so, you would not need to export all these
functions that only ivi-shell would use. I think that would be a big
benefit, if you can do that, reducing the API surface.
> +
> +/**
> + * \brief initialize weston-layout
> + */
> +void
> +weston_layout_initWithCompositor(struct weston_compositor *ec);
Mixing two different naming conventions like this looks strange,
especially for a function that I assume will be called by ivi-shell,
not ivi-controller.
> +
> +/**
> + * \brief register for notification when layer is created
> + */
> +int32_t
> +weston_layout_setNotificationCreateLayer(layerCreateNotificationFunc callback,
> + void *userdata);
> +
> +/**
> + * \brief register for notification when layer is removed
> + */
> +int32_t
> +weston_layout_setNotificationRemoveLayer(layerRemoveNotificationFunc callback,
> + void *userdata);
> +
> +/**
> + * \brief register for notification when surface is created
> + */
> +int32_t
> +weston_layout_setNotificationCreateSurface(surfaceCreateNotificationFunc callback,
> + void *userdata);
> +
> +/**
> + * \brief register for notification when surface is removed
> + */
> +int32_t
> +weston_layout_setNotificationRemoveSurface(surfaceRemoveNotificationFunc callback,
> + void *userdata);
> +
> +/**
> + * \brief register for notification when surface is configured
> + */
> +int32_t
> +weston_layout_setNotificationConfigureSurface(surfaceConfigureNotificationFunc callback,
> + void *userdata);
> +
> +/**
> + * \brief get id of surface from weston_layout_surface
> + *
> + * \return 0 if the method call was successful
> + * \return -1 if the client can not call the method on the service.
Where do you actually return the id the caller wanted to get?
> + */
> +uint32_t
> +weston_layout_getIdOfSurface(struct weston_layout_surface *ivisurf);
> +
> +/**
> + * \brief get id of layer from weston_layout_layer
> + *
> + *
> + * \return 0 if the method call was successful
> + * \return -1 if the client can not call the method on the service.
Ditto.
> + */
> +uint32_t
> +weston_layout_getIdOfLayer(struct weston_layout_layer *ivilayer);
> +
> +/**
> + * \brief get weston_layout_layer from id of layer
> + *
> + * \return (struct weston_layout_layer *)
> + * if the method call was successful
> + * \return NULL if the client can not call the method on the service.
What does it mean that "the client can not call the method on the
service"? This is server code, not client code.
> + */
> +struct weston_layout_layer *
> +weston_layout_getLayerFromId(uint32_t id_layer);
...
> +/**
> + * \brief Set from which kind of devices the surface can accept input events.
> + * By default, a surface accept input events from all kind of devices (keyboards, pointer, ...)
> + * By calling this function, you can adjust surface preferences. Note that this function only
> + * adjust the acceptance for the specified devices. Non specified are keept untouched.
> + *
> + * Typicall use case for this function is when dealing with pointer or touch events.
> + * Those are normally dispatched to the first visible surface below the coordinate.
> + * If you want a different behavior (i.e. forward events to an other surface below the coordinate,
> + * you can set all above surfaces to refuse input events)
> + *
> + * \return 0 if the method call was successful
> + * \return -1 if the client can not call the method on the service.
> + */
> +int32_t
> +weston_layout_UpdateInputEventAcceptanceOn(struct weston_layout_surface *ivisurf,
> + uint32_t devices,
> + uint32_t acceptance);
Why this? Isn't the input region enough?
I would probably have lots of questions about all the API defined here,
but it would be too much effort to ask, explain and understand it all,
and I'm not even sure if it matters to Weston that much in the end.
Some of that effort would also we wasted, if you can merge this code
into ivi-shell.so instead of keeping it as a separate library.
Thanks,
pq
More information about the wayland-devel
mailing list