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

[Alexandre Janniaux]

Hi,

Ok, I'm fine with this choice then, I +1 the enum for the
future.

Regards,
--
Alexandre Janniaux
Videolabs

On Mon, Oct 07, 2019 at 09:43:25AM +0200, 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.
>
> > > +
> > > +    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