On Thu, 04 Apr 2024 12:01:36 +0000 Nicolas Gaullier
<nicolas.gaullier@cji.paris> wrote:
> >De : Niklas Haas <ffm...@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-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
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".
