[vlc-devel] [PATCH] Soft field repeat support for deinterlacer
Juha Jeronen
juha.jeronen at jyu.fi
Thu Feb 17 22:11:43 CET 2011
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
More information about the vlc-devel
mailing list