From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 3/9] avformat/avisynth: add read_frameprops option
Date: Mon, 29 Aug 2022 02:41:04 +0200
Message-ID: <AS8P250MB0744C7D66F80097C334639ED8F769@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20220829000244.71123-4-qyot27@gmail.com>
Stephen Hutchinson:
> Allows turning the reading of frame properties entirely on and off.
> Defaults to reading frame properties.
>
> Signed-off-by: Stephen Hutchinson <qyot27@gmail.com>
> ---
> libavformat/avisynth.c | 355 +++++++++++++++++++++--------------------
> 1 file changed, 179 insertions(+), 176 deletions(-)
>
> diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c
> index d503c7ed40..5d726d70a5 100644
> --- a/libavformat/avisynth.c
> +++ b/libavformat/avisynth.c
> @@ -103,6 +103,7 @@ typedef struct AviSynthContext {
> int error;
>
> /* (de)activate reading frame properties */
> + int frameprops;
> int frameprop_sar;
>
> /* Linked list pointers. */
> @@ -522,227 +523,228 @@ static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st)
> frame = avs_library.avs_get_frame(avs->clip, 0);
> avsmap = avs_library.avs_get_frame_props_ro(avs->env, frame);
>
> - /* Field order */
> - if(avs_library.avs_prop_get_type(avs->env, avsmap, "_FieldBased") == AVS_PROPTYPE_UNSET) {
> - st->codecpar->field_order = AV_FIELD_UNKNOWN;
> - } else {
> - switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_FieldBased", 0, &error)) {
> - case 0:
> - st->codecpar->field_order = AV_FIELD_PROGRESSIVE;
> - break;
> - case 1:
> - st->codecpar->field_order = AV_FIELD_BB;
> - break;
> - case 2:
> - st->codecpar->field_order = AV_FIELD_TT;
> - break;
> - default:
> + if(avs->frameprops) {
This will make frameprops a global on-off which overrides everything
else even if some of the "else" stuff has been enabled explicitly. Worse
yet, if you want to disable everything except exactly one field, you
have to leave frameprops enabled and disable everything else explicitly.
Why not use a bitfield (with AV_OPT_TYPE_FLAGS and AV_OPT_TYPE_CONST)?
These properties certainly seem like a bitfield and the above would be
easily achievable with it.
> + /* Field order */
> + if(avs_library.avs_prop_get_type(avs->env, avsmap, "_FieldBased") == AVS_PROPTYPE_UNSET) {
> st->codecpar->field_order = AV_FIELD_UNKNOWN;
> + } else {
> + switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_FieldBased", 0, &error)) {
> + case 0:
> + st->codecpar->field_order = AV_FIELD_PROGRESSIVE;
> + break;
> + case 1:
> + st->codecpar->field_order = AV_FIELD_BB;
> + break;
> + case 2:
> + st->codecpar->field_order = AV_FIELD_TT;
> + break;
> + default:
> + st->codecpar->field_order = AV_FIELD_UNKNOWN;
> + }
> }
> - }
>
> - /* Color Range */
> - if(avs_library.avs_prop_get_type(avs->env, avsmap, "_ColorRange") == AVS_PROPTYPE_UNSET) {
> - st->codecpar->color_range = AVCOL_RANGE_UNSPECIFIED;
> - } else {
> - switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_ColorRange", 0, &error)) {
> - case 0:
> - st->codecpar->color_range = AVCOL_RANGE_JPEG;
> - break;
> - case 1:
> - st->codecpar->color_range = AVCOL_RANGE_MPEG;
> - break;
> - default:
> + /* Color Range */
> + if(avs_library.avs_prop_get_type(avs->env, avsmap, "_ColorRange") == AVS_PROPTYPE_UNSET) {
> st->codecpar->color_range = AVCOL_RANGE_UNSPECIFIED;
> + } else {
> + switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_ColorRange", 0, &error)) {
> + case 0:
> + st->codecpar->color_range = AVCOL_RANGE_JPEG;
> + break;
> + case 1:
> + st->codecpar->color_range = AVCOL_RANGE_MPEG;
> + break;
> + default:
> + st->codecpar->color_range = AVCOL_RANGE_UNSPECIFIED;
> + }
> }
> - }
>
> - /* Color Primaries */
> - switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_Primaries", 0, &error)) {
> - case 1:
> - st->codecpar->color_primaries = AVCOL_PRI_BT709;
> - break;
> - case 2:
> - st->codecpar->color_primaries = AVCOL_PRI_UNSPECIFIED;
> - break;
> - case 4:
> - st->codecpar->color_primaries = AVCOL_PRI_BT470M;
> - break;
> - case 5:
> - st->codecpar->color_primaries = AVCOL_PRI_BT470BG;
> - break;
> - case 6:
> - st->codecpar->color_primaries = AVCOL_PRI_SMPTE170M;
> - break;
> - case 7:
> - st->codecpar->color_primaries = AVCOL_PRI_SMPTE240M;
> - break;
> - case 8:
> - st->codecpar->color_primaries = AVCOL_PRI_FILM;
> - break;
> - case 9:
> - st->codecpar->color_primaries = AVCOL_PRI_BT2020;
> - break;
> - case 10:
> - st->codecpar->color_primaries = AVCOL_PRI_SMPTE428;
> - break;
> - case 11:
> - st->codecpar->color_primaries = AVCOL_PRI_SMPTE431;
> - break;
> - case 12:
> - st->codecpar->color_primaries = AVCOL_PRI_SMPTE432;
> - break;
> - case 22:
> - st->codecpar->color_primaries = AVCOL_PRI_EBU3213;
> - break;
> - default:
> - st->codecpar->color_primaries = AVCOL_PRI_UNSPECIFIED;
> - }
> -
> - /* Color Transfer Characteristics */
> - switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_Transfer", 0, &error)) {
> - case 1:
> - st->codecpar->color_trc = AVCOL_TRC_BT709;
> - break;
> - case 2:
> - st->codecpar->color_trc = AVCOL_TRC_UNSPECIFIED;
> - break;
> - case 4:
> - st->codecpar->color_trc = AVCOL_TRC_GAMMA22;
> - break;
> - case 5:
> - st->codecpar->color_trc = AVCOL_TRC_GAMMA28;
> - break;
> - case 6:
> - st->codecpar->color_trc = AVCOL_TRC_SMPTE170M;
> - break;
> - case 7:
> - st->codecpar->color_trc = AVCOL_TRC_SMPTE240M;
> - break;
> - case 8:
> - st->codecpar->color_trc = AVCOL_TRC_LINEAR;
> - break;
> - case 9:
> - st->codecpar->color_trc = AVCOL_TRC_LOG;
> - break;
> - case 10:
> - st->codecpar->color_trc = AVCOL_TRC_LOG_SQRT;
> - break;
> - case 11:
> - st->codecpar->color_trc = AVCOL_TRC_IEC61966_2_4;
> - break;
> - case 12:
> - st->codecpar->color_trc = AVCOL_TRC_BT1361_ECG;
> - break;
> - case 13:
> - st->codecpar->color_trc = AVCOL_TRC_IEC61966_2_1;
> - break;
> - case 14:
> - st->codecpar->color_trc = AVCOL_TRC_BT2020_10;
> - break;
> - case 15:
> - st->codecpar->color_trc = AVCOL_TRC_BT2020_12;
> - break;
> - case 16:
> - st->codecpar->color_trc = AVCOL_TRC_SMPTE2084;
> - break;
> - case 17:
> - st->codecpar->color_trc = AVCOL_TRC_SMPTE428;
> - break;
> - case 18:
> - st->codecpar->color_trc = AVCOL_TRC_ARIB_STD_B67;
> - break;
> - default:
> - st->codecpar->color_trc = AVCOL_TRC_UNSPECIFIED;
> - }
> -
> - /* Matrix coefficients */
> - if(avs_library.avs_prop_get_type(avs->env, avsmap, "_Matrix") == AVS_PROPTYPE_UNSET) {
> - st->codecpar->color_space = AVCOL_SPC_UNSPECIFIED;
> - } else {
> - switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_Matrix", 0, &error)) {
> - case 0:
> - st->codecpar->color_space = AVCOL_SPC_RGB;
> - break;
> + /* Color Primaries */
> + switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_Primaries", 0, &error)) {
> case 1:
> - st->codecpar->color_space = AVCOL_SPC_BT709;
> + st->codecpar->color_primaries = AVCOL_PRI_BT709;
You reindent everything once in this patch and then in every
switch/block once more when you add the relevant option. This is
unnecessary; it also makes this patch harder to read. Just take a look
at the above: It seems as if you were replacing setting color_space with
color_primaries, but it is just the git diff algorithm trying to
minimize the diff. For that reason the project policy states that
reindentation should be done in a separate commit (one commit at the end
of this patchset is enough).
> break;
> case 2:
> - st->codecpar->color_space = AVCOL_SPC_UNSPECIFIED;
> + st->codecpar->color_primaries = AVCOL_PRI_UNSPECIFIED;
> break;
> case 4:
> - st->codecpar->color_space = AVCOL_SPC_FCC;
> + st->codecpar->color_primaries = AVCOL_PRI_BT470M;
> break;
> case 5:
> - st->codecpar->color_space = AVCOL_SPC_BT470BG;
> + st->codecpar->color_primaries = AVCOL_PRI_BT470BG;
> break;
> case 6:
> - st->codecpar->color_space = AVCOL_SPC_SMPTE170M;
> + st->codecpar->color_primaries = AVCOL_PRI_SMPTE170M;
> break;
> case 7:
> - st->codecpar->color_space = AVCOL_SPC_SMPTE240M;
> + st->codecpar->color_primaries = AVCOL_PRI_SMPTE240M;
> break;
> case 8:
> - st->codecpar->color_space = AVCOL_SPC_YCGCO;
> + st->codecpar->color_primaries = AVCOL_PRI_FILM;
> break;
> case 9:
> - st->codecpar->color_space = AVCOL_SPC_BT2020_NCL;
> + st->codecpar->color_primaries = AVCOL_PRI_BT2020;
> break;
> case 10:
> - st->codecpar->color_space = AVCOL_SPC_BT2020_CL;
> + st->codecpar->color_primaries = AVCOL_PRI_SMPTE428;
> break;
> case 11:
> - st->codecpar->color_space = AVCOL_SPC_SMPTE2085;
> + st->codecpar->color_primaries = AVCOL_PRI_SMPTE431;
> break;
> case 12:
> - st->codecpar->color_space = AVCOL_SPC_CHROMA_DERIVED_NCL;
> + st->codecpar->color_primaries = AVCOL_PRI_SMPTE432;
> break;
> - case 13:
> - st->codecpar->color_space = AVCOL_SPC_CHROMA_DERIVED_CL;
> - break;
> - case 14:
> - st->codecpar->color_space = AVCOL_SPC_ICTCP;
> + case 22:
> + st->codecpar->color_primaries = AVCOL_PRI_EBU3213;
> break;
> default:
> - st->codecpar->color_space = AVCOL_SPC_UNSPECIFIED;
> + st->codecpar->color_primaries = AVCOL_PRI_UNSPECIFIED;
> }
> - }
>
> - /* Chroma Location */
> - if(avs_library.avs_prop_get_type(avs->env, avsmap, "_ChromaLocation") == AVS_PROPTYPE_UNSET) {
> - st->codecpar->chroma_location = AVCHROMA_LOC_UNSPECIFIED;
> - } else {
> - switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_ChromaLocation", 0, &error)) {
> - case 0:
> - st->codecpar->chroma_location = AVCHROMA_LOC_LEFT;
> - break;
> + /* Color Transfer Characteristics */
> + switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_Transfer", 0, &error)) {
> case 1:
> - st->codecpar->chroma_location = AVCHROMA_LOC_CENTER;
> + st->codecpar->color_trc = AVCOL_TRC_BT709;
> break;
> case 2:
> - st->codecpar->chroma_location = AVCHROMA_LOC_TOPLEFT;
> - break;
> - case 3:
> - st->codecpar->chroma_location = AVCHROMA_LOC_TOP;
> + st->codecpar->color_trc = AVCOL_TRC_UNSPECIFIED;
> break;
> case 4:
> - st->codecpar->chroma_location = AVCHROMA_LOC_BOTTOMLEFT;
> + st->codecpar->color_trc = AVCOL_TRC_GAMMA22;
> break;
> case 5:
> - st->codecpar->chroma_location = AVCHROMA_LOC_BOTTOM;
> + st->codecpar->color_trc = AVCOL_TRC_GAMMA28;
> + break;
> + case 6:
> + st->codecpar->color_trc = AVCOL_TRC_SMPTE170M;
> + break;
> + case 7:
> + st->codecpar->color_trc = AVCOL_TRC_SMPTE240M;
> + break;
> + case 8:
> + st->codecpar->color_trc = AVCOL_TRC_LINEAR;
> + break;
> + case 9:
> + st->codecpar->color_trc = AVCOL_TRC_LOG;
> + break;
> + case 10:
> + st->codecpar->color_trc = AVCOL_TRC_LOG_SQRT;
> + break;
> + case 11:
> + st->codecpar->color_trc = AVCOL_TRC_IEC61966_2_4;
> + break;
> + case 12:
> + st->codecpar->color_trc = AVCOL_TRC_BT1361_ECG;
> + break;
> + case 13:
> + st->codecpar->color_trc = AVCOL_TRC_IEC61966_2_1;
> + break;
> + case 14:
> + st->codecpar->color_trc = AVCOL_TRC_BT2020_10;
> + break;
> + case 15:
> + st->codecpar->color_trc = AVCOL_TRC_BT2020_12;
> + break;
> + case 16:
> + st->codecpar->color_trc = AVCOL_TRC_SMPTE2084;
> + break;
> + case 17:
> + st->codecpar->color_trc = AVCOL_TRC_SMPTE428;
> + break;
> + case 18:
> + st->codecpar->color_trc = AVCOL_TRC_ARIB_STD_B67;
> break;
> default:
> + st->codecpar->color_trc = AVCOL_TRC_UNSPECIFIED;
> + }
> +
> + /* Matrix coefficients */
> + if(avs_library.avs_prop_get_type(avs->env, avsmap, "_Matrix") == AVS_PROPTYPE_UNSET) {
> + st->codecpar->color_space = AVCOL_SPC_UNSPECIFIED;
> + } else {
> + switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_Matrix", 0, &error)) {
> + case 0:
> + st->codecpar->color_space = AVCOL_SPC_RGB;
> + break;
> + case 1:
> + st->codecpar->color_space = AVCOL_SPC_BT709;
> + break;
> + case 2:
> + st->codecpar->color_space = AVCOL_SPC_UNSPECIFIED;
> + break;
> + case 4:
> + st->codecpar->color_space = AVCOL_SPC_FCC;
> + break;
> + case 5:
> + st->codecpar->color_space = AVCOL_SPC_BT470BG;
> + break;
> + case 6:
> + st->codecpar->color_space = AVCOL_SPC_SMPTE170M;
> + break;
> + case 7:
> + st->codecpar->color_space = AVCOL_SPC_SMPTE240M;
> + break;
> + case 8:
> + st->codecpar->color_space = AVCOL_SPC_YCGCO;
> + break;
> + case 9:
> + st->codecpar->color_space = AVCOL_SPC_BT2020_NCL;
> + break;
> + case 10:
> + st->codecpar->color_space = AVCOL_SPC_BT2020_CL;
> + break;
> + case 11:
> + st->codecpar->color_space = AVCOL_SPC_SMPTE2085;
> + break;
> + case 12:
> + st->codecpar->color_space = AVCOL_SPC_CHROMA_DERIVED_NCL;
> + break;
> + case 13:
> + st->codecpar->color_space = AVCOL_SPC_CHROMA_DERIVED_CL;
> + break;
> + case 14:
> + st->codecpar->color_space = AVCOL_SPC_ICTCP;
> + break;
> + default:
> + st->codecpar->color_space = AVCOL_SPC_UNSPECIFIED;
> + }
> + }
> +
> + /* Chroma Location */
> + if(avs_library.avs_prop_get_type(avs->env, avsmap, "_ChromaLocation") == AVS_PROPTYPE_UNSET) {
> st->codecpar->chroma_location = AVCHROMA_LOC_UNSPECIFIED;
> + } else {
> + switch (avs_library.avs_prop_get_int(avs->env, avsmap, "_ChromaLocation", 0, &error)) {
> + case 0:
> + st->codecpar->chroma_location = AVCHROMA_LOC_LEFT;
> + break;
> + case 1:
> + st->codecpar->chroma_location = AVCHROMA_LOC_CENTER;
> + break;
> + case 2:
> + st->codecpar->chroma_location = AVCHROMA_LOC_TOPLEFT;
> + break;
> + case 3:
> + st->codecpar->chroma_location = AVCHROMA_LOC_TOP;
> + break;
> + case 4:
> + st->codecpar->chroma_location = AVCHROMA_LOC_BOTTOMLEFT;
> + break;
> + case 5:
> + st->codecpar->chroma_location = AVCHROMA_LOC_BOTTOM;
> + break;
> + default:
> + st->codecpar->chroma_location = AVCHROMA_LOC_UNSPECIFIED;
> + }
> }
> - }
>
> - /* Sample aspect ratio */
> - if (avs->frameprop_sar) {
> - sar_num = avs_library.avs_prop_get_int(avs->env, avsmap, "_SARNum", 0, &error);
> - sar_den = avs_library.avs_prop_get_int(avs->env, avsmap, "_SARDen", 0, &error);
> - st->sample_aspect_ratio = (AVRational){ sar_num, sar_den };
> + /* Sample aspect ratio */
> + if (avs->frameprop_sar) {
> + sar_num = avs_library.avs_prop_get_int(avs->env, avsmap, "_SARNum", 0, &error);
> + sar_den = avs_library.avs_prop_get_int(avs->env, avsmap, "_SARDen", 0, &error);
> + st->sample_aspect_ratio = (AVRational){ sar_num, sar_den };
> + }
> }
> -
> avs_library.avs_release_video_frame(frame);
> } else {
> st->codecpar->field_order = AV_FIELD_UNKNOWN;
> @@ -1149,6 +1151,7 @@ static int avisynth_read_seek(AVFormatContext *s, int stream_index,
>
> #define OFFSET(x) offsetof(AviSynthContext, x)
> static const AVOption avisynth_options[] = {
> + { "read_frameprops", "Read frame properties from script (AviSynth+ v3.7.1+).", OFFSET(frameprops), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, AV_OPT_FLAG_DECODING_PARAM },
> { "read_frameprop_sar", "Read SAR from script's frame properties (AviSynth+ v3.7.1+).", OFFSET(frameprop_sar), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, AV_OPT_FLAG_DECODING_PARAM },
> { NULL },
> };
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
next prev parent reply other threads:[~2022-08-29 0:41 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-29 0:02 [FFmpeg-devel] [PATCH 0/9] avisynth: add user options Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 1/9] avformat/avisynth: read _SARNum/_SARDen from frame properties Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 2/9] avformat/avisynth: add read_frameprop_sar option Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 3/9] avformat/avisynth: add read_frameprops option Stephen Hutchinson
2022-08-29 0:41 ` Andreas Rheinhardt [this message]
2022-08-30 23:04 ` Stephen Hutchinson
2022-08-30 23:17 ` Andreas Rheinhardt
2022-08-31 0:07 ` Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 4/9] avformat/avisynth: add read_frameprop_field_order option Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 5/9] avformat/avisynth: add read_frameprop_range option Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 6/9] avformat/avisynth: add read_frameprop_primaries option Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 7/9] avformat/avisynth: add read_frameprop_transfer option Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 8/9] avformat/avisynth: add read_frameprop_matrix option Stephen Hutchinson
2022-08-29 0:02 ` [FFmpeg-devel] [PATCH 9/9] avformat/avisynth: add read_frameprop_chroma_location option Stephen Hutchinson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AS8P250MB0744C7D66F80097C334639ED8F769@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM \
--to=andreas.rheinhardt@outlook.com \
--cc=ffmpeg-devel@ffmpeg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git