[FFmpeg-devel] avcodec : add prores_metadata bsf and fate test

[James Almer]

On 10/24/2018 5:25 PM, Martin Vignali wrote:
> New patch in attach
> 
> 001 : remove dead comment, add av_packet_make_writable
> 002 : change the test to use md5 and didn't use ffprobe. Also change color
> property
> 
> I will try to add prores to cbs later. Any tips for this ?

Grab one of the simplest implementations (mpeg2 and vp9, probably) and
copy what they do. The glue is pretty much the same for all of them,
what varies between codecs is the actual bitstream parsing.

> diff --git a/libavcodec/prores_metadata_bsf.c b/libavcodec/prores_metadata_bsf.c
> new file mode 100644
> index 0000000000..7831149471
> --- /dev/null
> +++ b/libavcodec/prores_metadata_bsf.c
> @@ -0,0 +1,122 @@
> +/*
> + * Prores Metadata bitstream filter
> + * Copyright (c) 2018 Jokyo Images
> + *
> + * 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
> + */
> +
> +/**
> + * @file
> + * Prores Metadata bitstream filter
> + * set frame colorspace property
> + */
> +
> +#include "libavutil/common.h"
> +#include "libavutil/opt.h"
> +#include "bsf.h"
> +#include "bytestream.h"

You're not using this API anywhere.

> +
> +typedef struct ProresMetadataContext {
> +    const AVClass *class;
> +
> +    int colour_primaries;
> +    int transfer_characteristics;
> +    int matrix_coefficients;
> +} ProresMetadataContext;
> +
> +static int prores_metadata(AVBSFContext *bsf, AVPacket *pkt)
> +{
> +    ProresMetadataContext *ctx = bsf->priv_data;
> +    int ret = 0;
> +    int buf_size;
> +    uint8_t *buf;
> +
> +    ret = ff_bsf_get_packet_ref(bsf, pkt);
> +    if (ret < 0)
> +        return ret;
> +
> +    buf = pkt->data;
> +    buf_size = pkt->size;
> +
> +    /* check start of the prores frame */
> +    if (buf_size < 28) {
> +        av_log(bsf, AV_LOG_ERROR, "not enough data in prores frame\n");
> +        ret = AVERROR_INVALIDDATA;
> +        goto fail;
> +    }
> +
> +    if (AV_RL32(buf + 4) != AV_RL32("icpf")) {
> +        av_log(bsf, AV_LOG_ERROR, "invalid frame header\n");
> +        ret = AVERROR_INVALIDDATA;
> +        goto fail;
> +    }
> +    buf += 8;
> +
> +    if (AV_RB16(buf) < 28) {
> +        av_log(bsf, AV_LOG_ERROR, "invalid frame header size\n");
> +        ret = AVERROR_INVALIDDATA;
> +        goto fail;
> +    }
> +
> +    ret = av_packet_make_writable(pkt);
> +    if (ret < 0)
> +        goto fail;
> +
> +    /* set the new values */
> +    buf[14] = ctx->colour_primaries;

buf is still pointing to pkt->data as it was before the call to
av_packet_make_writable(). Nothing guarantees that pointer is still
valid, and if it is, it may not be the one you want to use.

You should also drop pointer arithmetic for this and use exact offsets
everywhere.

> +    buf[15] = ctx->transfer_characteristics;
> +    buf[16] = ctx->matrix_coefficients;

What if any of these three are -1, the default value from the AVOptions?
Is that a valid value? And are all values up to 255 valid as well for
that matter?

You may want to use AVColorSpace, AVColorTransferCharacteristic and
AVColorPrimaries if they map to what ProRes uses. Otherwise make sure
the range of valid values is the correct one.

> +
> +fail:
> +    if (ret < 0)
> +        av_packet_unref(pkt);
> +    return ret;
> +}
> +
> +static const enum AVCodecID codec_ids[] = {
> +    AV_CODEC_ID_PRORES, AV_CODEC_ID_NONE,
> +};
> +
> +#define OFFSET(x) offsetof(ProresMetadataContext, x)
> +#define FLAGS (AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_BSF_PARAM)
> +static const AVOption options[] = {
> +    { "colour_primaries", "Set colour primaries",
> +        OFFSET(colour_primaries), AV_OPT_TYPE_INT,
> +        { .i64 = -1 }, -1, 255, FLAGS },
> +    { "transfer_characteristics", "Set transfer characteristics",
> +        OFFSET(transfer_characteristics), AV_OPT_TYPE_INT,
> +        { .i64 = -1 }, -1, 255, FLAGS },
> +    { "matrix_coefficients", "Set matrix coefficients",
> +        OFFSET(matrix_coefficients), AV_OPT_TYPE_INT,
> +        { .i64 = -1 }, -1, 255, FLAGS },
> +    { NULL },
> +};
> +
> +static const AVClass prores_metadata_class = {
> +    .class_name = "prores_metadata_bsf",
> +    .item_name  = av_default_item_name,
> +    .option     = options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +};
> +
> +const AVBitStreamFilter ff_prores_metadata_bsf = {
> +    .name       = "prores_metadata",
> +    .filter     = prores_metadata,
> +    .priv_data_size = sizeof(ProresMetadataContext),
> +    .priv_class = &prores_metadata_class,
> +    .codec_ids  = codec_ids,
> +};
> -- 
> 2.14.3 (Apple Git-98)
>