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

Mark Thompson sw at jkqxz.net
Tue Oct 23 02:48:53 EEST 2018


On 22/10/18 21:53, Martin Vignali wrote:
> From 312b153d2b92012cacd8a189d34738de9bdfa615 Mon Sep 17 00:00:00 2001
> From: Martin Vignali <martin.vignali at gmail.com>
> Date: Mon, 22 Oct 2018 22:46:05 +0200
> Subject: [PATCH 1/2] avcodec : add prores_metadata bsf for set the color 
>  property of each prores file
> 
> ---
>  doc/bitstream_filters.texi       |  16 ++++++
>  libavcodec/Makefile              |   1 +
>  libavcodec/bitstream_filters.c   |   1 +
>  libavcodec/prores_metadata_bsf.c | 120 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 138 insertions(+)
>  create mode 100644 libavcodec/prores_metadata_bsf.c
> 
> ...
> diff --git a/libavcodec/prores_metadata_bsf.c b/libavcodec/prores_metadata_bsf.c
> new file mode 100644
> index 0000000000..1943cdce18
> --- /dev/null
> +++ b/libavcodec/prores_metadata_bsf.c
> @@ -0,0 +1,120 @@
> +/*
> + * 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 "avcodec.h"

Dead comment.

> +#include "bsf.h"
> +#include "bytestream.h"
> +
> +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;
> +    }
> +
> +    /* set the new values */
> +    buf[14] = ctx->colour_primaries;
> +    buf[15] = ctx->transfer_characteristics;
> +    buf[16] = ctx->matrix_coefficients;

Since you are working in-place I think this needs a call to av_packet_make_writable() before writing.

> +
> +fail:
> +    if (ret < 0)
> +        av_packet_unref(pkt);
> +    return ret;
> +}
> +
> ...
> 
> From 3a5c353124cfc18ca24fda50bc5ea7c195ebcfad Mon Sep 17 00:00:00 2001
> From: Martin Vignali <martin.vignali at gmail.com>
> Date: Mon, 22 Oct 2018 22:48:57 +0200
> Subject: [PATCH 2/2] fate/prores_metadata_bsf : add test for setting color 
>  property
> 
> ---
>  tests/fate/prores.mak                    | 18 ++++++++++++++++++
>  tests/ref/fate/prores-metadata-to-rec709 | 20 ++++++++++++++++++++
>  2 files changed, 38 insertions(+)
>  create mode 100644 tests/ref/fate/prores-metadata-to-rec709
> 
> diff --git a/tests/fate/prores.mak b/tests/fate/prores.mak
> index f7f52ca7fc..1a561d123a 100644
> --- a/tests/fate/prores.mak
> +++ b/tests/fate/prores.mak
> @@ -20,3 +20,21 @@ fate-prores-alpha_skip: CMD = framecrc -flags +bitexact -skip_alpha 1 -i $(TARGE
>  fate-prores-transparency: CMD = framecrc -flags +bitexact -i $(TARGET_SAMPLES)/prores/prores4444_with_transparency.mov -pix_fmt yuva444p10le
>  fate-prores-transparency_skip: CMD = framecrc -flags +bitexact -skip_alpha 1 -i $(TARGET_SAMPLES)/prores/prores4444_with_transparency.mov -pix_fmt yuv444p10le
>  fate-prores-gray:      CMD = framecrc -flags +bitexact -c:a aac_fixed -i $(TARGET_SAMPLES)/prores/gray.mov -pix_fmt yuv422p10le
> +
> +
> +#Test bsf prores-metadata
> +tests/data/prores_proxy_rec709_metadata.mov: TAG = GEN
> +tests/data/prores_proxy_rec709_metadata.mov: ffmpeg$(PROGSSUF)$(EXESUF) | tests/data
> +	$(M)$(TARGET_EXEC) $(TARGET_PATH)/$< \
> +	-i $(TARGET_SAMPLES)/prores/Sequence_1-Apple_ProRes_422_Proxy.mov -nostdin -c:v copy -an -bsf:v \

Perhaps irrelevant or intended, but this input sample already has BT.709 matrix coefficients.

> +	prores_metadata=colour_primaries=1:transfer_characteristics=1:matrix_coefficients=1 \
> +        $(TARGET_PATH)/$@ -y 2>/dev/null
> +
> +
> +FATE_PRORES_METADATA_BSF_FFPROBE += fate-prores-metadata-to-rec709
> +fate-prores-metadata-to-rec709: tests/data/prores_proxy_rec709_metadata.mov
> +fate-prores-metadata-to-rec709: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_frames -show_entries frame=pict_type,interlaced_frame,top_field_first,color_range,color_space,color_primaries,color_transfer,pkt_size  -v 0 $(TARGET_PATH)/tests/data/prores_proxy_rec709_metadata.mov
> +
> +FATE_SAMPLES_FFPROBE += $(FATE_PRORES_METADATA_BSF_FFPROBE)

This is missing the dep on prores_metadata actually being enabled (try --disable-bsf=prores_metadata).

I wonder whether it might be more direct to verify the output once and then just compare against the known-good hash?  (That's how the CBS metadata tests work, with CMD = md5.)

> +
> +fate-prores-metadata-bsf: $(FATE_PRORES_METADATA_BSF_FFPROBE)
> \ No newline at end of file

git also has a comment for you.

> ...

Looks good, though +1 to the suggestion from James to write it with cbs_prores so it fits in with the other metadata BSFs (trace support for free!).

Thanks,

- Mark


More information about the ffmpeg-devel mailing list