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

[Steve Lhomme]

-1

On 2021-02-11 8:38, Rémi Denis-Courmont wrote:
> 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  <https://mailman.videolan.org/listinfo/vlc-devel>
> 
> 
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser 
> ma brièveté.
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>