[vlc-devel] [PATCH 05/39] decoder: move the display creation in a separate function

[Alexandre Janniaux]

Hi,

Comments inline,

On Wed, Oct 02, 2019 at 04:23:20PM +0200, Steve Lhomme wrote:
> To create the vout and decoder device we don't need to know the DPB.
>
> For now the display still holds the picture pool so the display still needs the
> DBP size.
> ---
>  src/input/decoder.c | 52 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/src/input/decoder.c b/src/input/decoder.c
> index cfa3fd237eb..c058779deb0 100644
> --- a/src/input/decoder.c
> +++ b/src/input/decoder.c
> @@ -459,11 +459,10 @@ static void FixDisplayFormat(decoder_t *p_dec, video_format_t *fmt)
>      video_format_AdjustColorSpace( fmt );
>  }
>
> -static int ModuleThread_UpdateVideoFormat( decoder_t *p_dec )
> +static int CreateVoutIfNeeded(struct decoder_owner *p_owner)
>  {
> -    struct decoder_owner *p_owner = dec_get_owner( p_dec );
> +    decoder_t *p_dec = &p_owner->dec;
>      bool need_vout = false;
> -    bool need_format_update = false;
>
>      if( p_owner->p_vout == NULL )
>      {
> @@ -506,22 +505,6 @@ static int ModuleThread_UpdateVideoFormat( decoder_t *p_dec )
>          need_vout = true;
>      }
>
> -    if ( memcmp( &p_dec->fmt_out.video.mastering,
> -                 &p_owner->fmt.video.mastering,
> -                 sizeof(p_owner->fmt.video.mastering)) )
> -    {
> -        msg_Dbg(p_dec, "vout update: mastering data");
> -        need_format_update = true;
> -    }
> -    if ( p_dec->fmt_out.video.lighting.MaxCLL !=
> -         p_owner->fmt.video.lighting.MaxCLL ||
> -         p_dec->fmt_out.video.lighting.MaxFALL !=
> -         p_owner->fmt.video.lighting.MaxFALL )
> -    {
> -        msg_Dbg(p_dec, "vout update: lighting data");
> -        need_format_update = true;
> -    }
> -
>      if( need_vout )
>      {
>          vout_thread_t *p_vout;
> @@ -591,8 +574,37 @@ static int ModuleThread_UpdateVideoFormat( decoder_t *p_dec )
>          vlc_fifo_Lock( p_owner->p_fifo );
>          p_owner->reset_out_state = true;
>          vlc_fifo_Unlock( p_owner->p_fifo );
> +
> +        return 1; // new vout was created
>      }
> -    else
> +    return 0; // vout unchanged
> +}
> +
> +static int ModuleThread_UpdateVideoFormat( decoder_t *p_dec )
> +{
> +    struct decoder_owner *p_owner = dec_get_owner( p_dec );
> +
> +    int created_vout = CreateVoutIfNeeded(p_owner);
> +    if (created_vout != 0)
> +        return created_vout == -1 ? -1 : 0; // error or new vout was created

To be honest with myself, this is becoming a bit hard to parse
and understand without reading both functions, as
`0 == VLC_SUCCESS` everywhere else in the code base so it looks
like an error checking, but that's also coming from the usual -1
or 0 return value convention issue.

I'm not sure of what you could do though, except trying to return
VLC_EGENERIC/VLC_SUCCESS here and convert the result back in
decoder_UpdateVideoFormat or changing its documentation.

> +
> +    bool need_format_update = false;
> +    if ( memcmp( &p_dec->fmt_out.video.mastering,
> +                 &p_owner->fmt.video.mastering,
> +                 sizeof(p_owner->fmt.video.mastering)) )
> +    {
> +        msg_Dbg(p_dec, "vout update: mastering data");
> +        need_format_update = true;
> +    }
> +    if ( p_dec->fmt_out.video.lighting.MaxCLL !=
> +         p_owner->fmt.video.lighting.MaxCLL ||
> +         p_dec->fmt_out.video.lighting.MaxFALL !=
> +         p_owner->fmt.video.lighting.MaxFALL )
> +    {
> +        msg_Dbg(p_dec, "vout update: lighting data");
> +        need_format_update = true;
> +    }
> +
>      if ( need_format_update )
>      {
>          /* the format has changed but we don't need a new vout */
> --
> 2.17.1
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

Regards,

--
Alexandre Janniaux
Videolabs