[vlc-devel] [PATCH] Added IVTC deinterlacer (NTSC film mode)

[Juha Jeronen]

Hi,


On Dec 31, 2010, at 12:52 , Jean-Baptiste Kempf wrote:

> Hello,
> 
> Thanks a lot for the work.
> 
> Here are rough remarks on a first analysis.

Thanks for the comments.


> On Fri, Dec 31, 2010 at 03:41:39AM +0200, Juha Jeronen wrote :
>> +#define DEINTERLACE_FILM    9
> Here and all the following, you use "FILM" as a name, isn't that too
> generic for the user to understand?
> And in the code, maybe it should be IVTC?

The reason for choosing that name was that in hardware DVD players, projectors etc. IVTC is often called "film mode" or just "film" (if it's in a deinterlacer menu), named after the fact that material for which IVTC is applicable usually originates from film. The technical name IVTC is less known, although more descriptive for advanced users.

You're right in that "film mode" as a generic term may refer either to 3:2 pulldown removal or 2:2 pulldown removal (fixing half-frame cuts in PAL telecines), or to a system that does both. In the code IVTC is definitely more descriptive, so I'll change that.

What should we do about the name shown to the user? Relabel as "IVTC (NTSC film)" to reach both camps of users?


>> +    /* Film. This one needs a lot of state. */
> What about having a sub-structure or a separate one?

A sub-structure sounds acceptable. Will change.


>> -    return fc < 1 ? false : true;
>> +    /* Changed for Film deinterlacer - return the raw fc value.
>> +       RenderX() treats the result as a logical, so it won't break. */
>> +/*    return fc < 1 ? false : true; */
>> +    return fc;
> 
> Maybe this should be in a separate patch

Oops... that change is not used by the current version, so it shouldn't have been there at all :)

Will remove.


>> #ifdef CAN_COMPILE_MMXEXT
>> static inline int XDeint8x8DetectMMXEXT( uint8_t *src, int i_src )
>> {
>> -
>>     int y, x;
>>     int32_t ff, fr;
>>     int fc;
>> @@ -1182,7 +1213,6 @@ static inline int XDeintNxNDetect( uint8_t *src,  
>> int i_src,
>>     int ff, fr;
>>     int fc;
>> 
>> -
>>     /* Detect interlacing */
>>     /* FIXME way too simple, need to be more like XDeint8x8Detect */
>>     ff = fr = 0;
>> @@ -1199,7 +1229,10 @@ static inline int XDeintNxNDetect( uint8_t *src,  
>> int i_src,
>>             fc++;
>>     }
> 
> Cosmetics should not be in this patch

Thanks for catching. I removed the blank lines on instinct and missed that when I read through the patch. Will remove.


>> -    return fc < 2 ? false : true;
>> +    /* Changed for Film deinterlacer - return the raw fc value.
>> +       RenderX() treats the result as a logical, so it won't break. */
>> +/*    return fc < 2 ? false : true; */
>> +    return fc < 2 ? 0 : fc;
>> }
> 
> as above?

Yes.


>> +static void ComposeFrame( filter_t *p_filter, picture_t *p_outpic,  
>> picture_t *p_inpic_top, picture_t *p_inpic_bottom )
>> +{
>> +    assert( p_filter != NULL );
>> +    assert( p_outpic != NULL );
>> +    assert( p_inpic_top != NULL );
>> +    assert( p_inpic_bottom != NULL );
>> +
>> +    int i_plane;
>> +    for( i_plane = 0 ; i_plane < p_inpic_top->i_planes ; i_plane++ )
> You can defined int directly in the for, because vlc is C99

Ok. I did it this way to stay consistent with the other deinterlacers in the module, but I can do it the modern way if desired.


>> +    for( i_plane = 0 ; i_plane < p_pic->i_planes ; i_plane++ )
> same here
> 
>> +        for( y = 1; y < i_lasty; y+=2 )
> and there

Ok.


>> +                for( x = 0; x < w; ++x )
>> +                {
>> +                    long C = *src_c;
>> +                    long P = *src_p;
>> +                    long N = *src_n;
> Are you sure long is the right type? I kind of doubt it, seeing how long
> behaves differently in LP64 and LLP64.

Mm, maybe I should use int_fast32_t instead. What I basically want there is simply enough space to handle (P - C)*(N - C) in all cases, and keep it as "native" as possible so that the speed doesn't suffer unnecessarily. This requires at least 17 bits (worst case: both (P - C) and (N - C) are plus or minus 255), so 32 is the smallest size that will work.

Also now that you mentioned it, in the MMX version, when moving the results to memory, the receiving variables should be int32_t (specifically), so that we can be sure that their sign bits hit the right position.

The long type came from how it was done in Transcode - it bothered me a bit, but not enough to actually do anything about it ;)

Will fix both the C and MMX versions.


>> +#define FIELD_PAIR_TPBP 0
>> +#define FIELD_PAIR_TPBC 1
>> +#define FIELD_PAIR_TCBP 2
>> +#define FIELD_PAIR_TCBC 3
>> +#define FIELD_PAIR_TCBN 4
>> +#define FIELD_PAIR_TNBC 5
>> +#define FIELD_PAIR_TNBN 6
> enum maybe?

Yes. WIll fix.


>> +#define CADENCE_POS_PROGRESSIVE 0
>> +#define CADENCE_POS_TFF_ABC 1
>> +#define CADENCE_POS_TFF_BCD 2
>> +#define CADENCE_POS_TFF_CDE 3
>> +#define CADENCE_POS_TFF_EAB 4
>> +#define CADENCE_POS_BFF_ABC 5
>> +#define CADENCE_POS_BFF_BCD 6
>> +#define CADENCE_POS_BFF_CDE 7
>> +#define CADENCE_POS_BFF_EAB 8
> idem

Ok.


>> +/* Operations for film frame reconstruction.
>> +   Indices: [TFD][i_cadence_pos] */
>> +#define IVTC_OP_DROP_FRAME 0
>> +#define IVTC_OP_COPY_N 1
>> +#define IVTC_OP_COPY_C 2
>> +#define IVTC_OP_COMPOSE_TNBC 3
>> +#define IVTC_OP_COMPOSE_TCBN 4
> idem

Ok.


>> +            /* allocate new */
>> +            p_sys->pp_ivtc_working_area[i] = picture_NewFromFormat(  
>> &p_next->format );
>> +
>> +            assert( p_sys->pp_ivtc_working_area[i] != NULL );
> Are you sure you have to assert here? If mem is full, wouldn't
> picture_NewFromFormat return NULL ? I would quit, nicely...

True. Needs to be fixed.

How to quit a filter nicely in VLC? Any suggestions where I should look? None of the other deinterlacers quit, so in this module there was no example of that.

My concern is that if I just call Close(), the rest of the core might not receive the information that there is no longer a deinterlacer in the filter chain.


By the way, I found a bug reading through my changes again. If the video size changes and the working area needs to be reallocated, the filter should be restarted (releasing p_prev and p_curr), because otherwise the input picture sizes in the buffer will fail to match and ComposeFrame will fail spectacularly. I'll fix this while I'm at it.

And on another side note, if anyone wants the parts I've commented as "tested during development and removed", I have kept a local copy. I didn't include them in the patch, because I think the lengthier ones don't belong there.


Please let me know what you think about the name issue, whether the loop counters should be defined in the for or outside it, and the quit issue. Once these are decided, I'll get to work :)

Thanks for the analysis!

 -J