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

[Steve Lhomme]

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.