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

[Niklas Haas]

On Thu, 04 Apr 2024 12:01:36 +0000 Nicolas Gaullier <nicolas.gaullier at cji.paris> wrote:
> >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 ?

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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".