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

[Mark Himsley]

On 03/04/2011 15:21, Stefano Sabatini wrote:
> On date Friday 2011-04-01 19:31:41 +0100, Mark Himsley encoded:
>> On 31/03/11 15:15, Stefano Sabatini wrote:
>> >On date Thursday 2011-03-31 08:47:38 +0100, Mark Himsley encoded:
>> >>Second version of this filter. Renamed and updated from the
>> >>suggestions given for the first version.

[...]

>> diff --git a/libavfilter/vf_fieldorder.c b/libavfilter/vf_fieldorder.c
>> new file mode 100644
>> index 0000000..01ad315
>> --- /dev/null
>> +++ b/libavfilter/vf_fieldorder.c
>> @@ -0,0 +1,231 @@
>> +/*
>> + * video field order filter
>> + * copyright (c) 2011 Mark Himsley
>> + * Heavily influenced by vf_pad.c
>> + *
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +/* #define DEBUG */
>> +
>> +#include "libavutil/pixdesc.h"
>> +#include "avfilter.h"
>> +#include "libavutil/imgutils.h"

Just realised that libavutil/imgutils.h isn't used.

>> +
>> +typedef struct
>> +{
>> +    unsigned int dst_tff;      ///< output bff/tff
>> +    int          line_step[4]; ///< bytes per pixel per each plane
>> +    int          hsub;         ///< chroma subsampling value
>> +} FieldOrderContext;
>> +
>> +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
>> +{
>> +    FieldOrderContext *fieldorder = ctx->priv;
>> +
>> +    if (!args || !sscanf(args, "%u", &fieldorder->dst_tff) == 1) {
>> +        av_log(ctx, AV_LOG_ERROR,
>> +                "Expected 1 argument '#':'%s'\n", args);
> 
> Usability nit:
> 
> Expected 1 argument '#':'(null)'
> 
> I suggest:
> One argument expected, none found in '%s'.
> 
> Alternatively: assumes a default value (whatever is most likely to be
> the default).
> 
>> +        return AVERROR(EINVAL);
>> +    }
>> +    fieldorder->dst_tff = !!fieldorder->dst_tff;
>> +
>> +    av_log(ctx, AV_LOG_INFO, "ttf:%d\n", fieldorder->dst_tff);
> 
> typo: ttf -> tff
> 
> Nit: maybe more useful when debugging:
> field:tff
> field:bff
>
> Also maybe we could support accepting string values in input, e.g.
> fieldorder=tff|bff
> 
> which should be easier on the reader if she hasn't the manual at her
> hand.

Yes. That sounds like a nice feature.

>> +
>> +    return 0;
>> +}
>> +
>> +static int query_formats(AVFilterContext *ctx)
>> +{
>> +    AVFilterFormats *formats;
>> +    enum PixelFormat pix_fmt;
>> +    int ret;
>> +
>> +    /** accept any input pixel format that is not hardware accelerated
>> +     *  and does not have vertically sub-sampled chroma */
>> +    if (ctx->inputs[0]) {
>> +        formats = NULL;
>> +        for (pix_fmt = 0; pix_fmt < PIX_FMT_NB; pix_fmt++)
>> +            if (   !(av_pix_fmt_descriptors[pix_fmt].flags & PIX_FMT_HWACCEL)
>> +                && av_pix_fmt_descriptors[pix_fmt].nb_components
>> +                && !av_pix_fmt_descriptors[pix_fmt].log2_chroma_h
>> +                && (ret = avfilter_add_format(&formats, pix_fmt)) < 0) {
>> +                avfilter_formats_unref(&formats);
>> +                return ret;
>> +            }
>> +        avfilter_formats_ref(formats, &ctx->inputs[0]->out_formats);
>> +        avfilter_formats_ref(formats, &ctx->outputs[0]->in_formats);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int config_input(AVFilterLink *inlink)
>> +{
>> +    AVFilterContext   *ctx        = inlink->dst;
>> +    FieldOrderContext *fieldorder = ctx->priv;
>> +
>> +    int is_packed, component, plane;
>> +
>> +    /** discover if the pixel format is packed or planar */
>> +    is_packed = 1;
>> +    for (component = 0; component < av_pix_fmt_descriptors[inlink->format].nb_components; component++) {
>> +        if (av_pix_fmt_descriptors[inlink->format].comp[component].plane != 0) {
>> +            is_packed = 0;
>> +        }
>> +    }
>> +
>> +    /** discover the bytes per pixel for each plane */
>> +    if (is_packed) {
>> +        fieldorder->line_step[0] = av_get_bits_per_pixel(&av_pix_fmt_descriptors[inlink->format])>>3;
>> +    } else {
> 
>> +        for (plane = 0; plane < 4; plane++) {
>> +            fieldorder->line_step[plane] = 1 + (av_pix_fmt_descriptors[inlink->format].comp->depth_minus1 >> 3 );
> 
> Uhm, just checked with the command:
> 
> $ tools/lavfi-showfiltfmts fieldorder 1
> [fieldorder @ 0xaa30d20] ttf:1
> INPUT[0] default: yuyv422
> INPUT[0] default: rgb24
> INPUT[0] default: bgr24
> INPUT[0] default: yuv422p
> INPUT[0] default: yuv444p
> INPUT[0] default: yuv411p
> INPUT[0] default: gray
> INPUT[0] default: monow
> INPUT[0] default: monob
> INPUT[0] default: pal8
> INPUT[0] default: yuvj422p
> INPUT[0] default: yuvj444p
> INPUT[0] default: uyvy422
> INPUT[0] default: uyyvyy411
> INPUT[0] default: bgr8
> INPUT[0] default: bgr4
> INPUT[0] default: bgr4_byte
> INPUT[0] default: rgb8
> INPUT[0] default: rgb4
> INPUT[0] default: rgb4_byte
> INPUT[0] default: argb
> INPUT[0] default: rgba
> INPUT[0] default: abgr
> INPUT[0] default: bgra
> INPUT[0] default: gray16be
> INPUT[0] default: gray16le
> INPUT[0] default: rgb48be
> INPUT[0] default: rgb48le
> INPUT[0] default: rgb565be
> INPUT[0] default: rgb565le
> INPUT[0] default: rgb555be
> INPUT[0] default: rgb555le
> INPUT[0] default: bgr565be
> INPUT[0] default: bgr565le
> INPUT[0] default: bgr555be
> INPUT[0] default: bgr555le
> INPUT[0] default: yuv422p16le
> INPUT[0] default: yuv422p16be
> INPUT[0] default: yuv444p16le
> INPUT[0] default: yuv444p16be
> INPUT[0] default: rgb444be
> INPUT[0] default: rgb444le
> INPUT[0] default: bgr444be
> INPUT[0] default: bgr444le
> INPUT[0] default: y400a
> INPUT[0] default: bgr48le
> INPUT[0] default: bgr48be
> [...]
> 
> I see that this should work with all the listed formats, but may break
> if we add support for formats of the kind:
> * bitstream formats with components on plane 2/3
> * formats which have more than one component on planes 2/3 with log2_chroma_h != 0
>   (e.g. a freak variant of NV12, maybe Bayer formats?)

I hadn't thought about bitstream formats. Perhaps this filter should
also not accept PIX_FMT_BITSTREAM formats as well as PIX_FMT_HWACCEL
formats?

Personally, I prefer the programmatic determining if allowed formats
than having a fixed table that can get out of date - but I do see the
issues that could lead to if bad assumptions are made in the determining
logic. But I'm just a rookie around here.

Oh - and thanks for pointing out lavfi-showfiltfmts.


> On the other hand I see that it is not very likely to happen, but I'd
> still prefer to explicitely list the formats in query_formats().
> Other opinions, Michael?
> 
> [...]
> 
> Apart from this, no other comments from me, I can fix the usability
> issues myself with further commits, but I'm still a bit uneasy with
> the formats issue.

I can send a new version on Monday evening (GMT) removing the extra
include, tidying up init() and removing the bitstream formats from the
allowed formats.

-- 
Mark