>De : ffmpeg-devel <ffmpeg-devel-boun...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to