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

[Thomas Guillem]

On Mon, Oct 7, 2019, at 09:43, Steve Lhomme wrote:
> On 2019-10-06 20:45, Alexandre Janniaux wrote:
> > 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.
> 
> Again, I'm trying to make the least amount of changes to get to have 
> proper push. The 0/-1 is the usual way of handling things in that file 
> so I keep using that. If I'd change it I would use an enum.

I agree with steve. I generally use the "error return style" of the file/API.

> 
> >> +
> >> +    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
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel