[vlc-devel] [PATCH 1/2] vout: add a resize acknowledgement callback
Thomas Guillem
thomas at gllm.fr
Thu Feb 11 08:51:34 UTC 2021
+1 for the set.
On Thu, Feb 11, 2021, at 09:33, Alexandre Janniaux wrote:
> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list