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

[Juha Jeronen]

Hi,

On Feb 17, 2011, at 21:48 , Laurent Aimar wrote:

> 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.

Mm, it would indeed be clearer to read that way. It's a relatively minor amount of work, so why not. I'll change this.


>> +    /* 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.

This way is to keep the convention consistent with Yadif's history mechanism.

Could change both or leave this as-is? (Leaving as-is for now.)


>> +    /* 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.

True! Thanks for the catch. Will fix.


>> +        /* 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).

Ok. Will change.


>> +    /* 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.

True. Somehow I missed this. Will fix.


>> 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).

Ok. I'll split the patch.


I'll post a fixed version of the patch with the fixes noted above, and as I mentioned before, fixes for what I found myself (posted last week, on the 11th):

- pb_top_field_firsts is not properly initialized, until it fills up
when history entries are written. The uninitialized elements are never
accessed, but for completeness it'd be reasonable to init this, too.

- code duplication instead of for loop for setting b_progressive to true
for all output frames. Should change this for consistency with the rest.

- code duplication instead of for loop for picture_Release() upon
failure. Like above.


 -J