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

[Steve Lhomme]

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
>