>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 ? >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 _______________________________________________ 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".
