[vlc-devel] [PATCH 1/4] vout: don't drop window size when there is no display plugins yet

[Thomas Guillem]

Hello,

Sorry for the very long delay to answer this review,

On Tue, Dec 10, 2019, at 20:02, Rémi Denis-Courmont wrote:
> Le tiistaina 10. joulukuuta 2019, 15.50.53 EET Thomas Guillem a écrit :
> > This fixes the window size that was ignored when the size was updated from
> > the open callback of the window plugin.
> > 
> > Fixes #22674
> > ---
> >  src/video_output/video_output.c  | 12 ++++++++++++
> >  src/video_output/vout_internal.h |  5 +++++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/src/video_output/video_output.c
> > b/src/video_output/video_output.c index 659ede546ba..f05f19f4bb7 100644
> > --- a/src/video_output/video_output.c
> > +++ b/src/video_output/video_output.c
> > @@ -425,6 +425,10 @@ void vout_ChangeDisplaySize(vout_thread_t *vout,
> > 
> >      /* DO NOT call this outside the vout window callbacks */
> >      vlc_mutex_lock(&sys->display_lock);
> > +
> > +    sys->window_width = width;
> > +    sys->window_height = height;
> > +
> >      if (sys->display != NULL)
> >          vout_display_SetSize(sys->display, width, height);
> >      vlc_mutex_unlock(&sys->display_lock);
> > @@ -1606,6 +1610,13 @@ static int vout_Start(vout_thread_t *vout,
> > vlc_video_context *vctx, const vout_c vlc_mutex_lock(&sys->display_lock);
> >      vlc_mutex_unlock(&sys->window_lock);
> > 
> > +    /* Setup the initial size of the window, if not already forced by the
> > user */
> > +    if (dcfg.display.width == 0 && dcfg.display.height == 0)
> 
> That's inconsistent. Indeed, I would expect that the bug you are trying to fix 
> here exists regardless of --width and/or --height.
> 
> In the core vout internals, we need to retain both the requested dimensions to 
> (try to) size the window, and the actual dimensions to pass onto the display.
> 
> But here, it should be one or the other. Presumably the later.

OK with that. Will send new patches.

> 
> > +    {
> > +        dcfg.display.width = sys->window_width;
> > +        dcfg.display.height = sys->window_height;
> > +    }
> > +
> >      sys->display = vout_OpenWrapper(vout, sys->splitter_name, &dcfg, vctx);
> > if (sys->display == NULL) {
> >          vlc_mutex_unlock(&sys->display_lock);
> > @@ -1945,6 +1956,7 @@ vout_thread_t *vout_Create(vlc_object_t *object)
> > 
> >      /* Display */
> >      sys->display = NULL;
> > +    sys->window_width = sys->window_height = 0;
> >      vlc_mutex_init(&sys->display_lock);
> > 
> >      /* Window */
> > diff --git a/src/video_output/vout_internal.h
> > b/src/video_output/vout_internal.h index 0ca81871fee..d0c6524ecf9 100644
> > --- a/src/video_output/vout_internal.h
> > +++ b/src/video_output/vout_internal.h
> > @@ -184,6 +184,11 @@ struct vout_thread_sys_t
> >      vout_display_t *display;
> >      vlc_mutex_t     display_lock;
> > 
> > +    /* Protected by display_lock, window size that will be used by as the
> > +     * default display size when creating a new vd plugin. */
> > +    unsigned        window_width;
> > +    unsigned        window_height;
> 
> I have not checked, but it might be simpler to move the requested size from 
> display_cfg to new variables, and the actual size in display_cfg.

I tried to do like you said, but this display_cfg struct is very tied to user requested configuration.
cf. :
    /** Display properties */
    struct {
        unsigned width; /**< Requested display pixel width (0 by default). */
        unsigned height; /**< Requested display pixel height (0 by default). */
        vlc_rational_t sar; /**< Requested sample aspect ratio */
    } display;

This struct is filled by VoutGetDisplayCfg(), with values only coming from user variables.

Therefore, I propose to keep it this way: display_cfg contains requested configuration.

Few points of improvements:
 - rename the struct to vout_window_cfg
 - Move out the "struct vout_window_t *window" from this struct since this variable doesn't come from user configuration (but it need modifications on all modules) 

> 
> > +
> >      picture_pool_t  *private_pool;
> >      picture_pool_t  *display_pool;
> >      picture_fifo_t  *decoder_fifo;
> 
> 
> -- 
> Реми Дёни-Курмон
> http://www.remlab.net/
> 
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel