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

[Rémi Denis-Courmont]

No and no.

Le 11 février 2021 09:31:33 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>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.
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20210211/6b3df725/attachment.html>