[vlc-devel] [PATCH 1/2] vout: add a resize acknowledgement callback
Steve Lhomme
robux4 at ycbcr.xyz
Thu Feb 11 08:54:23 UTC 2021
On 2021-02-11 9: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.
OK, after a closer look I see the "internal" callback is not the same as
vout_window_ack_cb. It hides the fact it calls the window from the
display module side.
> 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.
The fact there are exception to the (unwritten) rule doesn't mean that's
what people usually do. Nor how *I* actually read this code absence of
any hint of how it should be understood. If I read it wrong others can too.
> 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