| Message ID | 20240402130559.382049-3-nicolas.gaullier@cji.paris |
|---|---|
| State | New |
| Headers | show |
| Series | avfilter/vf_colorspace: Use colorspace negotiation API | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Tue, 02 Apr 2024 15:05:59 +0200 Nicolas Gaullier <nicolas.gaullier@cji.paris> 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 <nicolas.gaullier@cji.paris> > --- > 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".
>De : Niklas Haas <ffmpeg@haasn.xyz> >Envoyé : jeudi 4 avril 2024 12:18 >> --- 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? This is how I understand things: the output colorspace is a mandatory parameter of the filter, so it can be set early and negotiated, So I lifted some part of the code from the "dynamic" part (filter_frame/create_filtergraph) to upstream "init/query_formats". out_lumacoef depends on colorspace solely, so it seems there is no point to unset it and re-set it again. rgb2yuv is dynamic, dependent on the range, so this is the new trigger, the pointer that has to be updated. >> @@ -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. This is the kind of questioning I had when working on this issue. This seems very annoying and overly complex for a very uncommon scenario; is it even possible within the filter to ask for a reconfiguration of the filter graph ? >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. This is the kind of easy logic I tried at the beginning. Some water has flowed under the bridge, notably b89ee26539, but I just tried at the moment (with current code) an easy patch with just these two lines, and it still does not work as "I" expected. More specifically: When running: ./ffprobe -f lavfi -i yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale,showinfo At the entry of filter_frame(), the outlink values are incorrect: colorspace = AVCOL_SPC_BT470BG And color_range = AVCOL_RANGE_UNSPECIFIED So, this is why I introduced the negotiation for the colorspace to early set and persist this outlink property. But for the range, I am bit confused with the documentation, it is not clear to me, but possibly it is expected to pass through? so it cannot be negotiated, so I am sticked if the outlink range cannot be changed dynamically... >Nit: Why introduce a new result variable instead of re-using the one that's already declared? >IMO this logic would look cleaner if you assigned ret before testing it, especially since it's nested. Thanks, ok, will fix this in the next version. Thank you for your review; as you can see, I have no certainty, rather many questions... Nicolas
>De : Nicolas Gaullier >Envoyé : jeudi 4 avril 2024 14:02 > >>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. >This is the kind of easy logic I tried at the beginning. Some water has flowed under the bridge, notably b89ee26539, but I just tried at the moment (with current code) an easy patch with just these two lines, and it still does not work >as "I" expected. >More specifically: >When running: ./ffprobe -f lavfi -i yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale,showinfo >At the entry of filter_frame(), the outlink values are incorrect: >colorspace = AVCOL_SPC_BT470BG >And color_range = AVCOL_RANGE_UNSPECIFIED So, this is why I introduced the negotiation for the colorspace to early set and persist this outlink property. >But for the range, I am bit confused with the documentation, it is not clear to me, but possibly it is expected to pass through? so it cannot be negotiated, so I am sticked if the outlink range cannot be changed dynamically... I was too hasty, sorry, let me precise what is running wrong: the "out" frame colorspace/range is fine And when running: /ffprobe -f lavfi -i yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,showinfo 2>&1|grep color_space you actually get, as expected: color_range:tv color_space:bt709 The issue is the link, and it is raised when chaining with the scale filter for example here: ./ffprobe -f lavfi -i yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale,showinfo 2>&1|grep color_space color_range:unknown color_space:bt470bg So, I don't know, maybe my approach (fixing vf_colorspace) is wrong ? Anyway, the "out->color_range = outlink->color_range" logic does not seem to be the kind of things to do here. Thank you again Nicolas
On Thu, 04 Apr 2024 12:01:36 +0000 Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote: > >De : Niklas Haas <ffmpeg@haasn.xyz> > >Envoyé : jeudi 4 avril 2024 12:18 > >> --- 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? > > This is how I understand things: the output colorspace is a mandatory parameter of the filter, so it can be set early and negotiated, > So I lifted some part of the code from the "dynamic" part (filter_frame/create_filtergraph) to upstream "init/query_formats". > out_lumacoef depends on colorspace solely, so it seems there is no point to unset it and re-set it again. > rgb2yuv is dynamic, dependent on the range, so this is the new trigger, the pointer that has to be updated. > > >> @@ -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. > This is the kind of questioning I had when working on this issue. This seems very annoying and overly complex for a very uncommon scenario; is it even possible within the filter to ask for a reconfiguration of the filter graph ? No, reconfiguring the filter graph is (currently) the API user's responsibility. (See fftools/ffmpeg_filter.c for an example) vf_buffersrc even warns you if you try and change colorspace properties mid-stream, and for good reason - IMO there is no general expectation that filters should be able to handle dynamically changing colorspace properties. (This is a feature, not a bug) There *are* some filters that handle dynamically changing input properties (e.g. scale, zscale, libplacebo), but these are the exception rather than the norm, and it's only because they're already written in a way that can trivially handle dynamic conversions. If it's difficult to do from within vf_colorspace, there's no need to do so. Feel free to assume that frame->colorspace == inlink->colorspace == constant. (Ditto color_range) > > >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. > This is the kind of easy logic I tried at the beginning. Some water has flowed under the bridge, notably b89ee26539, but I just tried at the moment (with current code) an easy patch with just these two lines, and it still does not work as "I" expected. > More specifically: > When running: ./ffprobe -f lavfi -i yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale,showinfo > At the entry of filter_frame(), the outlink values are incorrect: > colorspace = AVCOL_SPC_BT470BG > And color_range = AVCOL_RANGE_UNSPECIFIED > So, this is why I introduced the negotiation for the colorspace to early set and persist this outlink property. > But for the range, I am bit confused with the documentation, it is not clear to me, but possibly it is expected to pass through? so it cannot be negotiated, so I am sticked if the outlink range cannot be changed dynamically... Note: passing through the range untouched *is* the default behavior (via ff_default_query_formats). So I think the correct logic can be condensed into just: if (out_rng != UNSPEC) { ret = set_common_color_ranges(make_singleton(out_rng)); if (ret < 0) return ret; } That way, if the user passes unspec, it's guaranteed that the input range == output_range (but with no preference for any specific range); but if the user passes a specific range, both the input and output of the filter are forced to be this range. Hopefully that clears up some confusion? > > >Nit: Why introduce a new result variable instead of re-using the one that's already declared? > >IMO this logic would look cleaner if you assigned ret before testing it, especially since it's nested. > Thanks, ok, will fix this in the next version. > > Thank you for your review; as you can see, I have no certainty, rather many questions... > > Nicolas > _______________________________________________ > 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".
>De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Niklas Haas >Envoyé : jeudi 4 avril 2024 14:44 > >> >> @@ -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. >> This is the kind of questioning I had when working on this issue. This seems very annoying and overly complex for a very uncommon scenario; is it even possible within the filter to ask for a reconfiguration of the filter graph ? > >No, reconfiguring the filter graph is (currently) the API user's responsibility. (See fftools/ffmpeg_filter.c for an example) > >vf_buffersrc even warns you if you try and change colorspace properties mid-stream, and for good reason - IMO there is no general expectation that filters should be able to handle dynamically changing colorspace properties. (This is >a feature, not a bug) > >There *are* some filters that handle dynamically changing input properties (e.g. scale, zscale, libplacebo), but these are the exception rather than the norm, and it's only because they're already written in a way that can trivially >handle dynamic conversions. > >If it's difficult to do from within vf_colorspace, there's no need to do so. Feel free to assume that frame->colorspace == inlink->colorspace == constant. (Ditto color_range) Thank you, this is pretty clear. I agree dynamic change of color range sounds weird and I am pleased it can be assumed constant. In my patch, it means the problematic "outlink->color_range = xxx" you pointed at can be dropped. Nice. >> >> >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. >> This is the kind of easy logic I tried at the beginning. Some water has flowed under the bridge, notably b89ee26539, but I just tried at the moment (with current code) an easy patch with just these two lines, and it still does not >work as "I" expected. >> More specifically: >> When running: ./ffprobe -f lavfi -i >> yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:color >> space=bt470bg,colorspace=bt709:range=tv,scale,showinfo >> At the entry of filter_frame(), the outlink values are incorrect: >> colorspace = AVCOL_SPC_BT470BG >> And color_range = AVCOL_RANGE_UNSPECIFIED So, this is why I introduced >> the negotiation for the colorspace to early set and persist this outlink property. >> But for the range, I am bit confused with the documentation, it is not clear to me, but possibly it is expected to pass through? so it cannot be negotiated, so I am sticked if the outlink range cannot be changed dynamically... > >Note: passing through the range untouched *is* the default behavior (via ff_default_query_formats). So I think the correct logic can be condensed into just: > >if (out_rng != UNSPEC) { > ret = set_common_color_ranges(make_singleton(out_rng)); > if (ret < 0) > return ret; >} > >That way, if the user passes unspec, it's guaranteed that the input range == output_range (but with no preference for any specific range); but if the user passes a specific range, both the input and output of the filter are forced to be >this range. Well, this is where I still not feel confident. Dynamic input range is not expected but somewhat still supported. First thing: in my understanding, the colorspace filter is aimed at converting from any input range to the desired output range (if specified), and I think my patch is ok with the current "ff_formats_ref(ff_make_formats_list_singleton(s->user_rng), &outlink->incfg.color_ranges)". I don't think they are issues against it, so I will keep it that way unless you object. Second thing: I understand the default behaviour is to pass through (I mean/guess dynamically) the range, but it is not what I experience. To test this, my patch serie includes a first patch to make setparams support timeline and here it is when running a "dynamic input range input": 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}" Current master (solely patched for timeline support in setparams): color_range:tv color_space:bt470bg color_range:tv color_space:bt470bg color_range:tv color_space:bt470bg My current patch: color_range:unknown color_space:bt709 color_range:pc color_space:bt709 color_range:tv color_space:bt709 Release tag 6.1 (solely patched for timeline support in setparams): color_range:unknown color_space:bt709 color_range:pc color_space:bt709 color_range:tv color_space:bt709 My current patch without the problematic "outlink->color_range = xxx" color_range:tv color_space:bt709 color_range:tv color_space:bt709 color_range:tv color_space:bt709 So, I can remove the problematic outlink change, but it causes more or less a subtle <<regression>>; I don't think it is the good word for it since what has been said above... => If I read you correctly, I really have to drop this problematic outlink setting, and the resulting slight change in the colorspace filter behaviour is okay. => At the end, if I drop the outlink setting, clear the nits and update the commit msg to remove the "dynamic input range scenario", I would be on the right track, so this is to be my next version. >Hopefully that clears up some confusion? Sure! Some definite confirmations remain but thank you already for all this. Nicolas
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; 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; 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; 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; + 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)
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 <nicolas.gaullier@cji.paris> --- libavfilter/vf_colorspace.c | 63 +++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 27 deletions(-)