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

[Nicolas Gaullier]

>De : Niklas Haas <ffmpeg at 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