[vlc-devel] [PATCH] Phosphor deinterlacer, reworked += 3

[Juha Jeronen]

Hi,

On Mar 7, 2011, at 21:26 , Laurent Aimar wrote:

> Hi,
> 
> On Sun, Mar 06, 2011 at 01:26:57AM +0200, Juha Jeronen wrote:
>> -void plane_CopyPixels( plane_t *p_dst, const plane_t *p_src )
>> +static inline void plane_CopyPixelsInternal( plane_t *p_dst, const plane_t *p_src, bool b_allow_block_copy )
>> {
>>     const unsigned i_width  = __MIN( p_dst->i_visible_pitch,
>>                                      p_src->i_visible_pitch );
>>     const unsigned i_height = __MIN( p_dst->i_visible_lines,
>>                                      p_src->i_visible_lines );
>> 
>> -    if( p_src->i_pitch == p_dst->i_pitch )
>> +    /* Disallowing the direct copy (including margins) can be useful for field plane_t's,
>> +       because a block copy in that case would overwrite the other field.
>> +       See plane_ToFieldPlane() and the deinterlacer module. */
>> +    if( b_allow_block_copy  &&  p_src->i_pitch == p_dst->i_pitch )
>>     {
>>         /* There are margins, but with the same width : perfect ! */
>>         vlc_memcpy( p_dst->p_pixels, p_src->p_pixels,
>> @@ -315,6 +318,38 @@ void plane_CopyPixels( plane_t *p_dst, const plane_t *p_src )
>>     }
>> }
>> 
>> +void plane_CopyPixels( plane_t *p_dst, const plane_t *p_src )
>> +{
>> +    plane_CopyPixelsInternal( p_dst, p_src, true );
>> +}
>> +
>> +void plane_CopyPixelsLineByLine( plane_t *p_dst, const plane_t *p_src )
>> +{
>> +    plane_CopyPixelsInternal( p_dst, p_src, false );
>> +}
> 
> I think that plane_Copy() could simply be fixed to detect such cases.
> It would also improves performances of plane_Copy() when the number
> of pixels to copy and the strides are very different.

When I was coding that, I couldn't think of a solution that would do this in a general manner. I was also wary of breaking the ABI and the existing API, hence the (very unelegant) new export.

But now that you said that, it occurs to me that one could check whether the pitch >= 2*visible_pitch. This is simple and solves the problem.

Whether that is passing control information in the data band (implicitly, even), or a clever performance optimization that also happens to solve the problem at hand, is up to interpretation...

So I'll leave the question up to you and Rémi - do this, or go the other route and keep all field processing internal to the deinterlacer?

I'm all for elegant solutions either way - in general the VLC codebase seems well-engineered, so let's keep it that way :)

 -J