[vlc-devel] [PATCH] Soft field repeat support for deinterlacer

[Laurent Aimar]

Hi,

On Wed, Feb 09, 2011 at 10:00:05PM +0200, Juha Jeronen wrote:
> +    /* Used for calculating field duration for framerate doublers */
> +    mtime_t pi_last_dates[LASTDATES_SIZE];
> +    int pi_last_nb_fields[LASTDATES_SIZE];
> +    bool pb_top_field_firsts[LASTDATES_SIZE];
 A structure to group the fields may ease a bit reading and/or the code. But
it's not important.

> +    /* The last element corresponds to the current input frame. */
> +    p_sys->pi_last_dates[LASTDATES_SIZE-1]       = p_pic->date;
> +    p_sys->pi_last_nb_fields[LASTDATES_SIZE-1]   = p_pic->i_nb_fields;
> +    p_sys->pb_top_field_firsts[LASTDATES_SIZE-1] = p_pic->b_top_field_first;
 I would have put the current frame first, I think it would simplify a bit, but I
am not sure. Not important at all.

> +    /* For framerate doublers, determine field duration and allocate output frames. */
> +    mtime_t i_field_dur = 0;
> +    if( p_sys->b_double_rate )
> +    {
> +        /* Calculate one field duration. */
> +        int i = 0;
> +        int iend = LASTDATES_SIZE-1;
> +        /* Find oldest valid logged date. Note: the current input frame doesn't count. */
> +        for( ; i < iend; i++ )
> +            if( p_sys->pi_last_dates[i] > VLC_TS_INVALID )
> +                break;
> +        if( i < iend )
> +        {
> +            /* Count how many fields the valid history entries (except the new frame) represent. */
> +            int i_fields_total = 0;
> +            for( int j = i ; j < iend; j++ )
> +                i_fields_total += p_sys->pi_last_nb_fields[j];
> +            /* One field took this long. */
> +            i_field_dur = (p_pic->date - p_sys->pi_last_dates[i]) / i_fields_total;
> +        }
> +        /* Note that we default to field duration 0 if it could not be determined.
> +           This behaves the same as the old code - leaving the extra output frame
> +           dates the same as p_pic->date if the last cached date was not valid.
> +        */
> +
> +        /* Allocate output frames. */
> +        iend = i_nb_fields;
 Reusing iend with a different meaning is not great.
> +        if( i_nb_fields > DEINTERLACE_DST_SIZE )
> +        {
> +            /* Note that the effective buffer size depends also on the constant private_picture in vout_wrapper.c,
> +               as that determines the maximum number of output pictures filter_NewPicture() will successfully
> +               allocate for one input frame.
> +            */
> +            msg_Err( p_filter, "Framerate doubler: output buffer too small; fields = %d, buffer size = %d. Dropping the remaining fields.", i_nb_fields, DEINTERLACE_DST_SIZE );
> +            iend = DEINTERLACE_DST_SIZE;
> +        }
> +        bool b_alloc_success = true;
> +        for( int i = 1; i < iend ; ++i )
> +        {
> +            p_dst[i-1]->p_next =
> +            p_dst[i]           = filter_NewPicture( p_filter );
> +            if( p_dst[i] )
> +            {
> +                picture_CopyProperties( p_dst[i], p_pic );
> +            }
> +            else
> +            {
> +                msg_Err( p_filter, "Framerate doubler: could not allocate output frame %d", i+1 );
> +                b_alloc_success = false;
> +                break; /* If this happens, the rest of the allocations aren't likely to work, either... */
> +            }
> +        }
> +        /* Clean up if alloc failed */
> +        if( !b_alloc_success )
> +        {
> +            for( int i = 1; i < iend; ++i )
> +            {
> +                /* Here we need the fact that any unused p_dst have been initialized to NULL. */
> +                if( p_dst[i] )
> +                {
> +                    picture_Release( p_dst[i] );
> +                    p_dst[i] = NULL;
> +                }
> +            }
> +        }
 I don't think it is worth it to release successfully allocated picture. Because
if you don't, then you output as much as possible, which I think is better (and
simpler).

> +    /* Set output timestamps, if the algorithm didn't request CUSTOM_PTS for this frame */
> +    assert( i_frame_offset <= LASTDATES_SIZE  ||  i_frame_offset == CUSTOM_PTS );
> +    if( i_frame_offset != CUSTOM_PTS )
> +    {
> +        mtime_t i_base_pts = p_sys->pi_last_dates[i_idx_to_lasts];
> +
> +        p_dst[0]->date = i_base_pts;
> +        if( p_sys->b_double_rate )
> +        {
> +            for( int i = 1; i < i_nb_fields; ++i )
> +            {
> +                /* XXX it's not really good especially for the first picture, but
> +                    * I don't think that delaying by one frame is worth it */
> +                if( i_base_pts > VLC_TS_INVALID )
> +                    p_dst[i]->date = i_base_pts + i * i_field_dur;
> +                else
> +                    p_dst[i]->date = VLC_TS_INVALID;
 I think p_dst[i] can be NULL in case of allocation failures.
> +            }
> +        }
> +    }
> +
>      p_dst[0]->b_progressive = true;
>      if( p_dst[1] )
>          p_dst[1]->b_progressive = true;
> +    if( p_dst[2] )
> +        p_dst[2]->b_progressive = true;
>  
>      picture_Release( p_pic );
>      return p_dst[0];
> @@ -1622,6 +1828,8 @@ drop:
>      picture_Release( p_dst[0] );
>      if( p_dst[1] )
>          picture_Release( p_dst[1] );
> +    if( p_dst[2] )
> +        picture_Release( p_dst[2] );
>      picture_Release( p_pic );
>      return NULL;
>  }
> @@ -1630,7 +1838,11 @@ static void Flush( filter_t *p_filter )
>  {
>      filter_sys_t *p_sys = p_filter->p_sys;
>  
> -    p_sys->i_last_date = VLC_TS_INVALID;
> +    for( int i = 0; i < LASTDATES_SIZE; i++ )
> +    {
> +        p_sys->pi_last_dates[i] = VLC_TS_INVALID;
> +        p_sys->pi_last_nb_fields[i] = 2;
> +    }
>      for( int i = 0; i < HISTORY_SIZE; i++ )
>      {
>          if( p_sys->pp_history[i] )
> @@ -1669,7 +1881,12 @@ static int Open( vlc_object_t *p_this )
>      p_sys->i_mode = DEINTERLACE_BLEND;
>      p_sys->b_double_rate = false;
>      p_sys->b_half_height = true;
> -    p_sys->i_last_date = VLC_TS_INVALID;
> +    for( int i = 0; i < LASTDATES_SIZE; i++ )
> +    {
> +        p_sys->pi_last_dates[i] = VLC_TS_INVALID;
> +        p_sys->pi_last_nb_fields[i] = 2;
> +    }
> +    p_sys->i_frame_offset = 0; /* start with default value (first-ever frame cannot have offset) */
>      for( int i = 0; i < HISTORY_SIZE; i++ )
>          p_sys->pp_history[i] = NULL;

> diff --git a/src/video_output/vout_wrapper.c b/src/video_output/vout_wrapper.c
> index 8ff36b8..2015dc4 100644
> --- a/src/video_output/vout_wrapper.c
> +++ b/src/video_output/vout_wrapper.c
> @@ -134,7 +134,7 @@ int vout_InitWrapper(vout_thread_t *vout)
>  
>      sys->display.use_dr = !vout_IsDisplayFiltered(vd);
>      const bool allow_dr = !vd->info.has_pictures_invalid && sys->display.use_dr;
> -    const unsigned private_picture  = 3; /* XXX 2 for filter, 1 for SPU */
> +    const unsigned private_picture  = 4; /* XXX 3 for filter, 1 for SPU */
>      const unsigned decoder_picture  = 1 + sys->dpb_size;
>      const unsigned kept_picture     = 1; /* last displayed picture */
>      const unsigned reserved_picture = DISPLAY_PICTURE_COUNT +
 I think I would prefer it to be in an independant patch. There are related
but it can be used on its own (I'm toying on a filter which need it too
for example).

Regards,

-- 
fenrir