From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 66C5B49C09 for ; Thu, 4 Apr 2024 10:18:32 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id DB6C468CE03; Thu, 4 Apr 2024 13:18:29 +0300 (EEST) Received: from haasn.dev (haasn.dev [78.46.187.166]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id C04C868C14A for ; Thu, 4 Apr 2024 13:18:23 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=haasn.xyz; s=mail; t=1712225903; bh=6a9RvgF9M2IXL0Hv+lPQItfYIgDCtCChEUs4vgaLZdM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=SwhUtvFRAztxMOvaAXg3f/t4UGEmPLbYrQuOworXPvQwovMQ97ZLa43E2rDIQqpVU CWWXn5m9mRKZv8fvQ3GI9iasZUJq4EcJNgnJJ6aurO2DQIeFYwM3UorbEX1icV1kvK 3pixLzHcbOvquACZN2rgnxXdf5FnXCdZxiZtBV/4= Received: from haasn.dev (unknown [10.30.0.2]) by haasn.dev (Postfix) with ESMTP id 6BD5B4223F; Thu, 4 Apr 2024 12:18:23 +0200 (CEST) Date: Thu, 4 Apr 2024 12:18:23 +0200 Message-ID: <20240404121823.GC8505@haasn.xyz> From: Niklas Haas To: ffmpeg-devel@ffmpeg.org In-Reply-To: <20240402130559.382049-3-nicolas.gaullier@cji.paris> References: <20240402130559.382049-1-nicolas.gaullier@cji.paris> <20240402130559.382049-3-nicolas.gaullier@cji.paris> MIME-Version: 1.0 Content-Disposition: inline Subject: Re: [FFmpeg-devel] [PATCH v3 2/2] avfilter/vf_colorspace: Use colorspace negotiation API X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Nicolas Gaullier Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Tue, 02 Apr 2024 15:05:59 +0200 Nicolas Gaullier wrote: > Fixes a regression due to the fact that the colorspace filter does > not use the new API introduced by 8c7934f73ab6c568acaa. > The scale filter uses it since 45e09a30419cc2a7251e, and the setparams > filter since 3bf80df3ccd32aed23f0. > > Example 1 - color_range specified: > ffmpeg -f lavfi -i yuvtestsrc -vf setparams=color_primaries=bt470bg: > color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale > ,showinfo -f null -frames 1 - > > Before: > color_range:unknown color_space:bt470bg ... > After: > color_range:tv color_space:bt709 ... > > Example 2 - color_range pass-through: > ffmpeg -f lavfi -i yuvtestsrc -vf "setparams=color_primaries=bt470bg: > color_trc=smpte170m:colorspace=bt470bg:range=unknown, > setparams=range=pc:enable='between(n,1,2)', > setparams=range=tv:enable='between(n,2,3)', > colorspace=bt709,scale,showinfo" > -f null -frames 3 - 2>&1|awk "/color_/ {print \$4 \" \" \$5}" > > Before: > color_range:tv color_space:bt470bg > color_range:tv color_space:bt470bg > color_range:tv color_space:bt470bg > After: > color_range:unknown color_space:bt709 > color_range:pc color_space:bt709 > color_range:tv color_space:bt709 > > Signed-off-by: Nicolas Gaullier > --- > libavfilter/vf_colorspace.c | 63 +++++++++++++++++++++---------------- > 1 file changed, 36 insertions(+), 27 deletions(-) > > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c > index d181e81ace..12a571172b 100644 > --- a/libavfilter/vf_colorspace.c > +++ b/libavfilter/vf_colorspace.c > @@ -433,8 +433,7 @@ static int create_filtergraph(AVFilterContext *ctx, > if (out->color_trc != s->out_trc) s->out_txchr = NULL; > if (in->colorspace != s->in_csp || > in->color_range != s->in_rng) s->in_lumacoef = NULL; > - if (out->colorspace != s->out_csp || > - out->color_range != s->out_rng) s->out_lumacoef = NULL; > + if (out->color_range != s->out_rng) s->rgb2yuv = NULL; Can you explain this change to me? > > if (!s->out_primaries || !s->in_primaries) { > s->in_prm = in->color_primaries; > @@ -563,26 +562,8 @@ static int create_filtergraph(AVFilterContext *ctx, > redo_yuv2rgb = 1; > } > > - if (!s->out_lumacoef) { > - s->out_csp = out->colorspace; > + if (!s->rgb2yuv) { > s->out_rng = out->color_range; > - s->out_lumacoef = av_csp_luma_coeffs_from_avcsp(s->out_csp); > - if (!s->out_lumacoef) { > - if (s->out_csp == AVCOL_SPC_UNSPECIFIED) { > - if (s->user_all == CS_UNSPECIFIED) { > - av_log(ctx, AV_LOG_ERROR, > - "Please specify output colorspace\n"); > - } else { > - av_log(ctx, AV_LOG_ERROR, > - "Unsupported output color property %d\n", s->user_all); > - } > - } else { > - av_log(ctx, AV_LOG_ERROR, > - "Unsupported output colorspace %d (%s)\n", s->out_csp, > - av_color_space_name(s->out_csp)); > - } > - return AVERROR(EINVAL); > - } > redo_rgb2yuv = 1; > } > > @@ -687,6 +668,26 @@ static av_cold int init(AVFilterContext *ctx) > { > ColorSpaceContext *s = ctx->priv; > > + s->out_csp = s->user_csp == AVCOL_SPC_UNSPECIFIED ? > + default_csp[FFMIN(s->user_all, CS_NB)] : s->user_csp; > + s->out_lumacoef = av_csp_luma_coeffs_from_avcsp(s->out_csp); > + if (!s->out_lumacoef) { > + if (s->out_csp == AVCOL_SPC_UNSPECIFIED) { > + if (s->user_all == CS_UNSPECIFIED) { > + av_log(ctx, AV_LOG_ERROR, > + "Please specify output colorspace\n"); > + } else { > + av_log(ctx, AV_LOG_ERROR, > + "Unsupported output color property %d\n", s->user_all); > + } > + } else { > + av_log(ctx, AV_LOG_ERROR, > + "Unsupported output colorspace %d (%s)\n", s->out_csp, > + av_color_space_name(s->out_csp)); > + } > + return AVERROR(EINVAL); > + } > + > ff_colorspacedsp_init(&s->dsp); > > return 0; > @@ -735,6 +736,9 @@ static int filter_frame(AVFilterLink *link, AVFrame *in) > return res; > } > > + out->colorspace = s->out_csp; > + outlink->color_range = s->user_rng != AVCOL_RANGE_UNSPECIFIED ? s->user_rng : in->color_range; > + out->color_range = outlink->color_range; Changing outlink dynamically like this seems not correct to me (what if the next filter in the chain only supports one color range?). Changing the range on the fly would at the minimum require reconfiguring the filter graph, and possibly insertion of more auto-scale filters. The logic that is used in the other YUV negotiation aware filters is to just set `out->colorspace = outlink->colorspace` and `out->color_range = outlink->color_range`, since the link properties are authoritative. > out->color_primaries = s->user_prm == AVCOL_PRI_UNSPECIFIED ? > default_prm[FFMIN(s->user_all, CS_NB)] : s->user_prm; > if (s->user_trc == AVCOL_TRC_UNSPECIFIED) { > @@ -746,10 +750,6 @@ static int filter_frame(AVFilterLink *link, AVFrame *in) > } else { > out->color_trc = s->user_trc; > } > - out->colorspace = s->user_csp == AVCOL_SPC_UNSPECIFIED ? > - default_csp[FFMIN(s->user_all, CS_NB)] : s->user_csp; > - out->color_range = s->user_rng == AVCOL_RANGE_UNSPECIFIED ? > - in->color_range : s->user_rng; > if (rgb_sz != s->rgb_sz) { > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(out->format); > int uvw = in->width >> desc->log2_chroma_w; > @@ -839,10 +839,19 @@ static int query_formats(AVFilterContext *ctx) > AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUVJ422P, AV_PIX_FMT_YUVJ444P, > AV_PIX_FMT_NONE > }; > - int res; > + int res, ret; Nit: Why introduce a new result variable instead of re-using the one that's already declared? > ColorSpaceContext *s = ctx->priv; > + AVFilterLink *outlink = ctx->outputs[0]; > AVFilterFormats *formats = ff_make_format_list(pix_fmts); > > + if (ret = ff_formats_ref(ff_make_formats_list_singleton(s->out_csp), > + &outlink->incfg.color_spaces) < 0) > + return ret; > + if (s->user_rng != AVCOL_RANGE_UNSPECIFIED > + && (ret = ff_formats_ref(ff_make_formats_list_singleton(s->user_rng), > + &outlink->incfg.color_ranges) < 0)) > + return ret; > + IMO this logic would look cleaner if you assigned ret before testing it, especially since it's nested. > if (!formats) > return AVERROR(ENOMEM); > if (s->user_format == AV_PIX_FMT_NONE) > @@ -855,7 +864,7 @@ static int query_formats(AVFilterContext *ctx) > if (res < 0) > return res; > > - return ff_formats_ref(formats, &ctx->outputs[0]->incfg.formats); > + return ff_formats_ref(formats, &outlink->incfg.formats); > } > > static int config_props(AVFilterLink *outlink) > -- > 2.30.2 > > _______________________________________________ > 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". _______________________________________________ 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".