[FFmpeg-devel] [PATCH] add fieldorder video filter

[Mark Himsley]

On 08/04/11 16:36, Stefano Sabatini wrote:
> On date Friday 2011-04-08 12:10:32 +0100, Mark Himsley encoded:
>> On 31/03/11 08:47, Mark Himsley wrote:
>>> Converting to and from interlaced PAL DV files, with their
>>> bottom-field-first interlace field order, can be a pain. Converting tff
>>> files to DV results in tff DV files, which are hard to work with in
>>> editing software.

Thanks for your comments Stefano.

[...]

 >> +    const char        *tff = "tff";
 >> +    const char        *bff = "bff";
 >
 > Here you can skip a confusing level of indirection, by directly using
 > "tff" and "bff" below.

I used "tff" and "bff" twice in that function so I though it would be 
better to define them as const char *s and reuse them.

[...]

>> +static void start_frame(AVFilterLink *inlink, AVFilterBufferRef *inpicref)
>> +{
>> +    AVFilterContext   *ctx        = inlink->dst;
>> +    FieldOrderContext *fieldorder = ctx->priv;
>> +    AVFilterLink      *outlink    = ctx->outputs[0];
>> +
>> +    int               plane;
>> +    AVFilterBufferRef *outpicref;
>> +
>> +    /** only one time, full an array with the number of bytes that the video
>> +     *  data occupies per line for each plane of the input video */
>> +    if(!fieldorder->line_size_set) {
>
> Nit: if_(...)
>
>
>> +        for (plane = 0; plane<  4; plane++) {
>> +            fieldorder->line_size[plane] = av_image_get_linesize(
>> +                    inlink->format,
>> +                    inpicref->video->w,
>> +                    plane);
>> +        }
>> +        fieldorder->line_size_set = 1;
>> +    }
>
> And I'd prefer to put this in config_props, this way you don't need
> line_size_set and the code is slightly more readable.

I tried this code in config_props but inlink->cur_buf is null in 
config_props, where as it isn't in start_frame.

But since you've mentioned it, I see that I should be using inlink->w 
instead; so, yes, it should be in config_props and line_size_set is not 
needed.

> Nit+++:

Sorry about all those. Dyslexia and swapping between preferred standards 
for Java and C are not very good excuses for my poor proof reading.


> As for the bitstream support, basically they are streams for which
> each pixel component can't be expressed as a finite number of bytes
> but only as a finite number of bits.
>
> For example for rgb4_byte you have:
> RGGB____
>
> For monowhite:
> PPPPPPPP
>
> where P can be either 1 (white) or 0 (black) - or the other way.
>
> Note that av_image_get_linesize() returns a number of pixels also for
> bitstream formats, so the logic should be unchanged.
>
> If you're interested you may implement a regression test for the
> filter, this is a little tricky as it needs some knowledge of the
> internal test system (you basically needs to provide the checksum of
> the output issued by the test, the better way is to see how it is done
> for another filter, check for example the patch for the copy or the
> pixdesctest filter), but again no need to delay the application of
> this.

Thanks. I will investigate.

-- 
Mark