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

[Rémi Denis-Courmont]

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.

> >   {
> >   
> >       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.

-- 
Rémi Denis-Courmont
http://www.remlab.net/