[vlc-devel] [PATCH 1/2] vout: add a resize acknowledgement callback
Steve Lhomme
robux4 at ycbcr.xyz
Thu Feb 11 07:31:33 UTC 2021
On 2021-02-10 16:53, Rémi Denis-Courmont wrote:
> Le keskiviikkona 10. helmikuuta 2021, 8.38.11 EET Steve Lhomme a écrit :
>>> @@ -543,7 +546,7 @@ void vout_window_Disable(vout_window_t *window);
>>>
>>> static inline void vout_window_ReportSize(vout_window_t *window,
>>>
>>> unsigned width, unsigned
>>> height)
>>>
>>> {
>>>
>>> - window->owner.cbs->resized(window, width, height);
>>> + window->owner.cbs->resized(window, width, height, NULL, NULL);
>>>
>>> }
>>
>> IMO this function should always expose the new parameter or have a new
>> variant with the extra params. Just calling the callback manually (as
>> done in 2/2) defeats the purpose of having a helper in the first place.
>
> The purpose of the helper is to help, which is what it does, including filling
> default parameters.
>
> Providing default values for rarely used parameters is possible, but it's
> really ugly in pure C (as opposed to C++).
>
>>> /**
>>>
>>> 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)
>>> {
>>>
>>> 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.
More information about the vlc-devel
mailing list