[vlc-devel] [PATCH 1/2] vout: add a resize acknowledgement callback

[Alexandre Janniaux]

Hi,

On Thu, Feb 11, 2021 at 08:31:33AM +0100, Steve Lhomme wrote:
> On 2021-02-10 16:53, Rémi Denis-Courmont wrote:
> > > > diff --git a/src/video_output/video_output.c
> > > > b/src/video_output/video_output.c index d04d3f4d11..fb7f54e385 100644
> > > > --- a/src/video_output/video_output.c
> > > > +++ b/src/video_output/video_output.c
> > > > @@ -543,7 +543,8 @@ void vout_ChangeWindowState(vout_thread_t *vout,
> > > > unsigned st)>
> > > >    }
> > > >    void vout_ChangeDisplaySize(vout_thread_t *vout,
> > > >
> > > > -                            unsigned width, unsigned height)
> > > > +                            unsigned width, unsigned height,
> > > > +                            void (*cb)(void *), void *opaque)
> > >
> > > There's no guarantee this callback signature will match
> > > vout_window_ack_cb if it changes.
> >
> > It does not match in the first place.
> >
> > > There should be a cast to that type.
> >
> > What? You can't cast function types. That is UB in theory, and breaks CFI in
> > practice.
>
> Similar to what we do with some module callback:
>         vlc_decoder_device_Open open__ = activate;
>         (void) open__;
>         set_callback(activate)

Here it's not used behind a macro, so the module callback
method makes no sense, and it's actually NOT the same as
the vout_window_ack_cb callback, like explained by Rémi
right above.

As read in the patch, it's actually used to trigger this
function, using the vout_window_ack_data which has the
vout_window_ack_cb as one of its member and would correctly
triggers warning/error if its pointer callback were to be
called with invalid parameters because of a funtion change.

    +struct vout_window_ack_data {
    +    vout_window_t *window;
    +    vout_window_ack_cb callback;
    +    void *opaque;
    +};
    +
    +static void vout_window_Ack(void *data)
    +{
    +    struct vout_window_ack_data *cb_data = data;
    +
    +    if (cb_data->callback != NULL)
    +        cb_data->callback(cb_data->window, cb_data->opaque);
    +}

So I really don't get where your point is going too. Maybe
you confused the two part of the patch, between the vout_window
reporting and the reporting from the vout_display_window to the
vout thread?

> > > >    {
> > > >        vout_thread_sys_t *sys = VOUT_THREAD_TO_SYS(vout);
> > > >
> > > > diff --git a/src/video_output/window.c b/src/video_output/window.c
> > > > index 9c2df5c1e6..c866fe7af4 100644
> > > > --- a/src/video_output/window.c
> > > > +++ b/src/video_output/window.c
> > > > @@ -214,13 +228,16 @@ typedef struct vout_display_window
> > > >
> > > >    } vout_display_window_t;
> > > >    static void vout_display_window_ResizeNotify(vout_window_t *window,
> > > >
> > > > -                                             unsigned width, unsigned
> > > > height) +                                             unsigned width,
> > > > unsigned height, +
> > > > vout_window_ack_cb cb, +                                             void
> > > > *opaque)
> > > >
> > > >    {
> > > >        vout_display_window_t *state = window->owner.sys;
> > > >        vout_thread_t *vout = state->vout;
> > > >
> > > > +    struct vout_window_ack_data data = { window, cb, opaque };
> > > >
> > > >        msg_Dbg(window, "resized to %ux%u", width, height);
> > > >
> > > > -    vout_ChangeDisplaySize(vout, width, height);
> > > > +    vout_ChangeDisplaySize(vout, width, height, vout_window_Ack, &data);
> > >
> > > Using a local stack structure forces vout_ChangeDisplaySize() to be
> > > synchronous.
> >
> > The whole point of the callback is that it's synchronous.
> >
> > > Which is kind of contradictory to using a callback that
> > > look asynchronous (but is not).
> >
> > No, you're contradictory. The callback *is* synchronous therefore it *can* use
> > stack objects. (Like many here) I prefer not to call xmalloc() if I don't have
> > to.
>
> My point is that reading this code there is zero indication that it's
> synchronous. Not in the type, not in the name, not in the documentation.
> Passing a callback to a function is usually a hint that it's asynchronous.
> Otherwise you'd just get a return value.

Being synchronous or asynchronous is meaningless for the end-user here.
I also disagree that callback is meant to be asynchronous. Wayland
typically only have callback synchronous to the event_queue you're
currently processing, and is an asynchronous system because of its
ability to actually create new event_queue and dispatch them.

Here the callback is meant as an interception point within the display
lock to ack the sizing change, just like callbacks in an asynchronous
system are an interception point to signal the end of a processing.

In the current state, the callback is _really_ meant to be synchronous,
because it allows simpler implementation where the window implementation
doesn't have to lock to ack the resizing, which would be a much bigger
issue (potentially harder to track) than a use-after-free. What you might
suggest is a way to queue resizing request and only use the last one when
we get the display lock, which might not be useful with almost all other
WSI but can improve resizing on Wayland, but that's yet another design and
you cannot just provide a single callback with only the opaque since you
potentially need to notify the app about which resizing request has been
cancelled.

But honestly, the current state is broken, I'd rather have any solution
merged, even non-optimal, as long as it fixes the issue in 99% of the
situation rather than spend time on another design. And given there is
no doc on this internal code, it's more likely expected to be synchronous
than asynchronous, just like functions are expected to not be thread-safe
except if mentioned otherwise.

Regards,
--
Alexandre Janniaux
Videolabs