[FFmpeg-devel] [PATCH v3 2/2] avfilter/vf_colorspace: Use colorspace negotiation API

[Nicolas Gaullier]

>De : ffmpeg-devel <ffmpeg-devel-bounces at 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