Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
On 11/11/2018 7:20 PM, James Almer wrote: > On 11/8/2018 6:31 PM, Martin Vignali wrote: >> Le dim. 28 oct. 2018 à 12:59, Martin Vignali a >> écrit : >> >>> Thanks for the comments >>> >>> New patch in attach >>> >>> - Change the options, to only authorized few value in prores frame (based >>> on rdd36). >>> - Change option name to match setparams filter >>> - Include intreadwrite instead of bytestream >>> - Do not modify the property if set to -1 >>> - Move av_packet_make_writable at the start of the func >>> >>> Martin >>> >>> >>> >> i will pushed in few days, if no one is against. >> >> Martin > > This broke fate. > > --- /home/fate/ffmpeg/tests/ref/fate/prores-metadata 2018-11-11 > 20:20:19.486707760 + > +++ tests/data/fate/prores-metadata 2018-11-11 20:23:35.356712028 + > @@ -1 +1 @@ > -84814393e25b673e8ebd6a94f3d07349 > +b4e2c801d594b9614433b284b886be0d Ah, nevermind, i missed you pushed a fix. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
On 11/8/2018 6:31 PM, Martin Vignali wrote: > Le dim. 28 oct. 2018 à 12:59, Martin Vignali a > écrit : > >> Thanks for the comments >> >> New patch in attach >> >> - Change the options, to only authorized few value in prores frame (based >> on rdd36). >> - Change option name to match setparams filter >> - Include intreadwrite instead of bytestream >> - Do not modify the property if set to -1 >> - Move av_packet_make_writable at the start of the func >> >> Martin >> >> >> > i will pushed in few days, if no one is against. > > Martin This broke fate. --- /home/fate/ffmpeg/tests/ref/fate/prores-metadata2018-11-11 20:20:19.486707760 + +++ tests/data/fate/prores-metadata 2018-11-11 20:23:35.356712028 + @@ -1 +1 @@ -84814393e25b673e8ebd6a94f3d07349 +b4e2c801d594b9614433b284b886be0d ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
Le dim. 28 oct. 2018 à 12:59, Martin Vignali a écrit : > Thanks for the comments > > New patch in attach > > - Change the options, to only authorized few value in prores frame (based > on rdd36). > - Change option name to match setparams filter > - Include intreadwrite instead of bytestream > - Do not modify the property if set to -1 > - Move av_packet_make_writable at the start of the func > > Martin > > > i will pushed in few days, if no one is against. Martin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
Thanks for the comments New patch in attach - Change the options, to only authorized few value in prores frame (based on rdd36). - Change option name to match setparams filter - Include intreadwrite instead of bytestream - Do not modify the property if set to -1 - Move av_packet_make_writable at the start of the func Martin 0002-fate-prores_metadata_bsf-add-test-for-setting-color.patch Description: Binary data 0001-avcodec-add-prores_metadata-bsf-for-set-the-color.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
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 00..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
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
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 ? Martin 0002-fate-prores_metadata_bsf-add-test-for-setting-color.patch Description: Binary data 0001-avcodec-add-prores_metadata-bsf-for-set-the-color.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
On 22/10/18 21:53, Martin Vignali wrote:
> From 312b153d2b92012cacd8a189d34738de9bdfa615 Mon Sep 17 00:00:00 2001
> From: Martin Vignali
> 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 00..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
> 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/prores_with_transparency.mov -pix_fmt
> yuva444p10le
> fate-prores-transparency_skip: CMD = framecrc -flags +bitexact -skip_alpha 1
> -i $(TARGET_SAMPLES)/prores/prores_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
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
On 10/22/2018 6:44 PM, Martin Vignali wrote: >> >> Could you look into adding ProRes bitstream parsing support to the CBS >> framework? It would be ideal for a filter like this. >> >> Hello, > > The only metadata, which seems to have an interest to be edited (color > property), is at the start of every prores frame, no need to go deeper to > edit these three bytes. > > Seems much more complicated to edit prores color property using cbs > (considering the amount of work to add prores to cbs). > > Martin Alright. Adding support to CBS would still be nice, but it's not a blocker for this filter. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
> > Could you look into adding ProRes bitstream parsing support to the CBS > framework? It would be ideal for a filter like this. > > Hello, The only metadata, which seems to have an interest to be edited (color property), is at the start of every prores frame, no need to go deeper to edit these three bytes. Seems much more complicated to edit prores color property using cbs (considering the amount of work to add prores to cbs). Martin ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
On 10/20/2018 9:32 AM, Martin Vignali wrote: > Hello, > > Patch in attach add a bsf filter to set color property of a prores frame > > Can be used, to set all frames of prores file to the same value > (avoid color shift on some frame depending of the software used to decode > the file) > or fix the value, for file created with incorrect metadata. > > Martin Could you look into adding ProRes bitstream parsing support to the CBS framework? It would be ideal for a filter like this. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
> > fails on mips qemu > > Thanks for testing. New patch in attach, 001 : unchanged 002 : limit frame entries for ffprobe part, in order to not have pix_fmt in the output. Martin 0001-avcodec-add-prores_metadata-bsf-for-set-the-color.patch Description: Binary data 0002-fate-prores_metadata_bsf-add-test-for-setting-color.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] avcodec : add prores_metadata bsf and fate test
On Sat, Oct 20, 2018 at 02:32:20PM +0200, Martin Vignali wrote: > Hello, > > Patch in attach add a bsf filter to set color property of a prores frame > > Can be used, to set all frames of prores file to the same value > (avoid color shift on some frame depending of the software used to decode > the file) > or fix the value, for file created with incorrect metadata. > > Martin > fate/prores.mak| 18 +++ > ref/fate/prores-metadata-to-rec709 | 60 > + > 2 files changed, 78 insertions(+) > 90437f4c4dd9d6af5cfa40ce907f81e9f86805df > 0002-fate-prores_metadata_bsf-add-test-for-setting-color-.patch > From 9f7bd41c23fd0f01994579cdcaf478a9b922aa79 Mon Sep 17 00:00:00 2001 > From: Martin Vignali > Date: Sat, 20 Oct 2018 14:13:25 +0200 > Subject: [PATCH 2/2] fate/prores_metadata_bsf : add test for setting color > property fails on mips qemu @@ -14,7 +14,7 @@ pkt_size=57296 width=1920 height=1080 -pix_fmt=yuv422p10le +pix_fmt=yuv422p10be sample_aspect_ratio=N/A pict_type=I coded_picture_number=0 @@ -44,7 +44,7 @@ pkt_size=57296 width=1920 height=1080 -pix_fmt=yuv422p10le +pix_fmt=yuv422p10be sample_aspect_ratio=N/A pict_type=I coded_picture_number=0 Test prores-metadata-to-rec709 failed. Look at tests/data/fate/prores-metadata-to-rec709.err for details. make: *** [fate-prores-metadata-to-rec709] Error 1 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you drop bombs on a foreign country and kill a hundred thousand innocent people, expect your government to call the consequence "unprovoked inhuman terrorist attacks" and use it to justify dropping more bombs and killing more people. The technology changed, the idea is old. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
