[vlc-devel] [PATCH v2 08/20] fps: implement draining of extra pictures

[Steve Lhomme]

On 2020-10-15 21:43, Romain Vimont wrote:
> On Wed, Oct 14, 2020 at 02:39:08PM +0200, Steve Lhomme wrote:
>> The p_previous_pic keeps the picture that was returned by Filter(), which is
>> the basis for the extra pictures created during the drain, by incrementing by
>> one "framerate tick" each output. We need to keep a reference as the output
>> picture might be released by the caller, before the drain is called.
>>
>> We keep the original date of the incoming picture so we can compare to the
>> "framerate" ticks to know if we need extra pictures or not.
>>
>> When expanding the frame rate, we have the following output:
>> - pic0 -> original date
>>         -> original date + tick 1
>> - pic1 -> original date + tick 2
>>         -> original date + tick 3
>> - pic2 -> original date + tick 4
>>         -> original date + tick 5
>>
>> The date of the incoming picture is always included in the last output
>> frame date + duration.
>>
>> The code assumes that enough drain calls will be done until the next filter
>> call. (A flush in between is good as well)
>>
>> We no longer return pictures chained using vlc_picture_chain_AppendChain().
>> ---
>>   modules/video_filter/fps.c | 53 ++++++++++++++++++++------------------
>>   1 file changed, 28 insertions(+), 25 deletions(-)
>>
>> diff --git a/modules/video_filter/fps.c b/modules/video_filter/fps.c
>> index 67c1bd86e23..cfc711e60bf 100644
>> --- a/modules/video_filter/fps.c
>> +++ b/modules/video_filter/fps.c
>> @@ -61,24 +61,27 @@ static const char *const ppsz_filter_options[] = {
>>   typedef struct
>>   {
>>       date_t          next_output_pts; /**< output calculated PTS */
>> -    picture_t       *p_previous_pic; /**< kept source picture used to produce filter output */
>> +    picture_t       *p_previous_pic; /**< picture used to produce drain outputs */
>> +    vlc_tick_t      src_date;
>>       vlc_tick_t      i_output_frame_interval;
>>   } filter_sys_t;
>>   
>> -static void SetOutputDate(filter_sys_t *p_sys, picture_t *pic)
>> +static picture_t *SetOutputDate(filter_sys_t *p_sys, picture_t *pic)
> 
> nit: This is odd, the caller knows the picture (it passed it by
> argument, and it's always the same), there is no need to return it.
> And semantically, it's confusing that SetOutputDate() returns a picture.

It simplifies the code a bit. It doesn't force the caller to use the 
return value. But when it can, it's convenient. Just like picture_Hold() 
returns the picture it added a reference to.

>>   {
>>       pic->date = date_Get( &p_sys->next_output_pts );
>>       date_Increment( &p_sys->next_output_pts, 1 );
>> +    return pic;
>>   }
>>   
>>   static picture_t *Filter( filter_t *p_filter, picture_t *p_picture)
>>   {
>>       filter_sys_t *p_sys = p_filter->p_sys;
>> -    const vlc_tick_t src_date = p_picture->date;
>> +    assert(p_sys->p_previous_pic == NULL); // missing drain ?
>> +    p_sys->src_date = p_picture->date;
>>       /* If input picture doesn't have actual valid timestamp,
>>           we don't really have currently a way to know what else
>>           to do with it other than drop it for now*/
>> -    if( unlikely( src_date == VLC_TICK_INVALID) )
>> +    if( unlikely( p_sys->src_date == VLC_TICK_INVALID) )
>>       {
>>           msg_Dbg( p_filter, "skipping non-dated picture");
>>           picture_Release( p_picture );
>> @@ -93,43 +96,43 @@ static picture_t *Filter( filter_t *p_filter, picture_t *p_picture)
>>       if( unlikely( date_Get( &p_sys->next_output_pts ) == VLC_TICK_INVALID ) )
>>       {
>>           msg_Dbg( p_filter, "Resetting timestamps" );
>> -        date_Set( &p_sys->next_output_pts, src_date );
>> -        if( p_sys->p_previous_pic )
>> -            picture_Release( p_sys->p_previous_pic );
>> -        // p_picture will be returned so we need a reference
>> -        p_sys->p_previous_pic = picture_Hold( p_picture );
>> +        date_Set( &p_sys->next_output_pts, p_sys->src_date );
>>       }
>>   
>>       /* Check if we can skip input as better should follow */
>> -    else if( src_date <
>> +    else if( p_sys->src_date <
>>           ( date_Get( &p_sys->next_output_pts ) - p_sys->i_output_frame_interval ) )
>>       {
>> -        if( p_sys->p_previous_pic )
>> -            picture_Release( p_sys->p_previous_pic );
>> -        p_sys->p_previous_pic = p_picture;
>> +        picture_Release( p_picture );
>>           return NULL;
>>       }
>>   
>> -    SetOutputDate( p_sys, p_sys->p_previous_pic );
>> +    p_sys->p_previous_pic = picture_Hold( p_picture );
>> +    return SetOutputDate( p_sys, p_picture );
>> +}
>> +
>> +static picture_t *Drain( filter_t *p_filter )
>> +{
>> +    filter_sys_t *p_sys = p_filter->p_sys;
>> +
>> +    if ( p_sys->p_previous_pic == NULL )
>> +        return NULL;
>>   
>> -    picture_t *last_pic = p_sys->p_previous_pic;
>>       /* Duplicating pictures are not that effective and framerate increase
>>           should be avoided, it's only here as filter should work in that direction too*/
>> -    while( unlikely( (date_Get( &p_sys->next_output_pts ) + p_sys->i_output_frame_interval ) < src_date ) )
>> +    if ( unlikely( date_Get( &p_sys->next_output_pts ) < p_sys->src_date ) )
>>       {
>>           picture_t *p_tmp = NULL;
>>           p_tmp = picture_NewFromFormat( &p_filter->fmt_out.video );
>>   
>> -        picture_Copy( p_tmp, p_sys->p_previous_pic);
>> -        SetOutputDate( p_sys, p_tmp );
>> -
>> -        vlc_picture_chain_AppendChain( last_pic, p_tmp );
>> -        last_pic = p_tmp;
>> +        picture_Copy( p_tmp, p_sys->p_previous_pic );
>> +        return SetOutputDate( p_sys, p_tmp );
>>       }
>>   
>> -    last_pic = p_sys->p_previous_pic;
>> -    p_sys->p_previous_pic = p_picture;
>> -    return last_pic;
>> +    // no more draining possible
>> +    picture_Release( p_sys->p_previous_pic );
>> +    p_sys->p_previous_pic = NULL;
>> +    return NULL;
>>   }
>>   
>>   static void Flush( filter_t *p_filter )
>> @@ -153,7 +156,7 @@ static void Close( filter_t *p_filter )
>>   
>>   static const struct vlc_filter_operations filter_ops =
>>   {
>> -    .filter_video = Filter, .close = Close,
>> +    .filter_video = Filter, .drain_video = Drain, .close = Close,
>>       .flush = Flush,
>>   };
>>   
>> -- 
>> 2.26.2
>>
>> _______________________________________________
>> 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
>