[RFC wayland 1/9] wayland-egl: import libwayland-egl.so frontend library from Mesa

[Daniel Stone]

Hi Emil,

On 15 September 2017 at 15:54, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 15 September 2017 at 14:50, Daniel Stone <daniel at fooishbar.org> wrote:
>> Currently the client-facing libwayland-egl API is defined by a header
>> file shipped by Wayland, but the implementation is left to each
>> vendor. This can cause collisions when multiple implementations are
>> installed on the same system. Importing the implementation into
>> Wayland with a stable and versioned driver-facing ABI allows multiple
>> drivers etc etc
>>
> This sounds really nice. Should I keep the lengthy CC list and/or
> external repository reference?

Yep! That would be good thanks.

>> Apart from the below, this series, when squashed into a single patch
>> for import, is:
>> Reviewed-by: Daniel Stone <daniels at collabora.com>
>
> Err... let's not do that, please. We have three fundamentally
> different things here:
>  - clean copy/import
>  - integration
>  - independent nitpicks
>
> Having clear reproducible steps sounds like a clear win IMHO.
> If you prefer I can address all the nitpics in Mesa first, and then do
> the import?

Sure, that sounds good to me.

>> I realise this is a straight copy/import, so sorry to pick on these, but:
>>
> Ack, will address all the suggestions, but I'll kindly ask you to not
> fold/squash the series.

I'd really prefer to, to be honest; if you take, for instance, the
build system changes, it's quite clear that those changes are separate
from the imported files.

>>> +struct wl_egl_window {
>>> +       const intptr_t version;
>>> +
>>> +       int width;
>>> +       int height;
>>> +       int dx;
>>> +       int dy;
>>> +
>>> +       int attached_width;
>>> +       int attached_height;
>>> +
>>> +       void *private;
>>> +       void (*resize_callback)(struct wl_egl_window *, void *);
>>> +       void (*destroy_window_callback)(void *);
>>> +
>>> +       struct wl_surface *surface;
>>> +};
>>
>> I'd like to see this and the definitions of wayland-egl-abi-check.c
>> not be a complete copy of each other; perhaps in a separate private
>> header?
>>
> Can you elaborate - the believe all the structs (in
> wayland-egl-abi-check.c and this one) all vary in their own, small,
> subtle ways.

I mean that, as written, wl_egl_window_v3 and wl_egl_window are
identical, and written out twice. It makes it easier for them to
accidentally drop out of sync; perhaps there's a way to reuse the
definitions to avoid duplication of the most recent one. I guess the
test does have checks against current though, so it's probably fine.

>>> +WL_EGL_EXPORT struct wl_egl_window *
>>> +wl_egl_window_create(struct wl_surface *surface,
>>> +                    int width, int height)
>>> +{
>>> +       struct wl_egl_window _INIT_ = { .version = WL_EGL_WINDOW_VERSION };
>>> +       struct wl_egl_window *egl_window;
>>> +
>>> +       if (width <= 0 || height <= 0)
>>> +               return NULL;
>>> +
>>> +       egl_window = malloc(sizeof *egl_window);
>>> +       if (!egl_window)
>>> +               return NULL;
>>> +
>>> +       memcpy(egl_window, &_INIT_, sizeof *egl_window);
>>
>> The initialiser is weird; it should be static const if it remains, but
>> why not just use calloc and set it like the rest of the members below?
>>
> Using direct assignment for the version results in build failure.

Oh, because it's const. Ugh, that's incredibly nasty. :( I would
suggest it's worth dropping the const; anyone modifying it is clearly
doing something deeply stupid, and I don't like the gymnastics it
introduces in the struct definition. (Though, when using it, why not
just initialise surface as well, dropping all the other assignments to
the malloced struct?)

> Thinking about it ... we might want to make that bidirectional like
> the ones in Vulkan (and Mesa EGL,GLX <> CL interop).
> Aka - frontend describes the max size/layout it knows and the backend
> min(foo.version, self.version).
>
> This way the frontend/backend can communicate the version they
> support, each one having appropriate code to handle older versions. If
> we opt for that, we want to bump the version to indicate the
> functionality change?

The problem is that we don't know the backend which will be used for a
wl_egl_window, so the negotiation would have to be dynamic at the time
of CreateWindowSurface/etc, which would require driver ->
wl_egl_window callbacks etc. I'd rather not go down that path unless
we have good reason for it.

Cheers,
Daniel