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

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

[Nicolas Gaullier]

v3:
- Fixes case where colorspace is the first filter (no inlink)
- Illustrates with proper examples in commit msg (use yuvtestsrc instead of testsrc)

Please note that it is a regression compared to the previous release:
both examples (see commit msg) behave the same way as 6.1 after this patch.


Nicolas Gaullier (2):
  avfilter/vf_setparams: Add timeline support
  avfilter/vf_colorspace: Use colorspace negotiation API

 libavfilter/vf_colorspace.c | 63 +++++++++++++++++++++----------------
 libavfilter/vf_setparams.c  |  2 +-
 2 files changed, 37 insertions(+), 28 deletions(-)

-- 
2.30.2

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[FFmpeg-devel] [PATCH v3 1/2] avfilter/vf_setparams: Add timeline support

[Nicolas Gaullier]

This is helpful at least for test purposes.

Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
---
 libavfilter/vf_setparams.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavfilter/vf_setparams.c b/libavfilter/vf_setparams.c
index c96f4d314b..1b5eb70344 100644
--- a/libavfilter/vf_setparams.c
+++ b/libavfilter/vf_setparams.c
@@ -198,7 +198,7 @@ const AVFilter ff_vf_setparams = {
     .description = NULL_IF_CONFIG_SMALL("Force field, or color property for the output video frame."),
     .priv_size   = sizeof(SetParamsContext),
     .priv_class  = &setparams_class,
-    .flags       = AVFILTER_FLAG_METADATA_ONLY,
+    .flags       = AVFILTER_FLAG_SUPPORT_TIMELINE_GENERIC | AVFILTER_FLAG_METADATA_ONLY,
     FILTER_INPUTS(inputs),
     FILTER_OUTPUTS(ff_video_default_filterpad),
     FILTER_QUERY_FUNC(query_formats),
-- 
2.30.2

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

[Nicolas Gaullier]

Fixes a regression due to the fact that the colorspace filter does
not use the new API introduced by 8c7934f73ab6c568acaa.
The scale filter uses it since 45e09a30419cc2a7251e, and the setparams
filter since 3bf80df3ccd32aed23f0.

Example 1 - color_range specified:
ffmpeg -f lavfi -i yuvtestsrc -vf setparams=color_primaries=bt470bg:
color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale
,showinfo -f null -frames 1 -

Before:
  color_range:unknown color_space:bt470bg ...
After:
  color_range:tv color_space:bt709 ...

Example 2 - color_range pass-through:
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}"

Before:
color_range:tv color_space:bt470bg
color_range:tv color_space:bt470bg
color_range:tv color_space:bt470bg
After:
color_range:unknown color_space:bt709
color_range:pc color_space:bt709
color_range:tv color_space:bt709

Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
---
 libavfilter/vf_colorspace.c | 63 +++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index d181e81ace..12a571172b 100644
--- 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;
 
     if (!s->out_primaries || !s->in_primaries) {
         s->in_prm = in->color_primaries;
@@ -563,26 +562,8 @@ static int create_filtergraph(AVFilterContext *ctx,
         redo_yuv2rgb = 1;
     }
 
-    if (!s->out_lumacoef) {
-        s->out_csp = out->colorspace;
+    if (!s->rgb2yuv) {
         s->out_rng = out->color_range;
-        s->out_lumacoef = av_csp_luma_coeffs_from_avcsp(s->out_csp);
-        if (!s->out_lumacoef) {
-            if (s->out_csp == AVCOL_SPC_UNSPECIFIED) {
-                if (s->user_all == CS_UNSPECIFIED) {
-                    av_log(ctx, AV_LOG_ERROR,
-                           "Please specify output colorspace\n");
-                } else {
-                    av_log(ctx, AV_LOG_ERROR,
-                           "Unsupported output color property %d\n", s->user_all);
-                }
-            } else {
-                av_log(ctx, AV_LOG_ERROR,
-                       "Unsupported output colorspace %d (%s)\n", s->out_csp,
-                       av_color_space_name(s->out_csp));
-            }
-            return AVERROR(EINVAL);
-        }
         redo_rgb2yuv = 1;
     }
 
@@ -687,6 +668,26 @@ static av_cold int init(AVFilterContext *ctx)
 {
     ColorSpaceContext *s = ctx->priv;
 
+    s->out_csp  = s->user_csp == AVCOL_SPC_UNSPECIFIED ?
+                  default_csp[FFMIN(s->user_all, CS_NB)] : s->user_csp;
+    s->out_lumacoef = av_csp_luma_coeffs_from_avcsp(s->out_csp);
+    if (!s->out_lumacoef) {
+        if (s->out_csp == AVCOL_SPC_UNSPECIFIED) {
+            if (s->user_all == CS_UNSPECIFIED) {
+                av_log(ctx, AV_LOG_ERROR,
+                       "Please specify output colorspace\n");
+            } else {
+                av_log(ctx, AV_LOG_ERROR,
+                       "Unsupported output color property %d\n", s->user_all);
+            }
+        } else {
+            av_log(ctx, AV_LOG_ERROR,
+                   "Unsupported output colorspace %d (%s)\n", s->out_csp,
+                   av_color_space_name(s->out_csp));
+        }
+        return AVERROR(EINVAL);
+    }
+
     ff_colorspacedsp_init(&s->dsp);
 
     return 0;
@@ -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;
     out->color_primaries = s->user_prm == AVCOL_PRI_UNSPECIFIED ?
                            default_prm[FFMIN(s->user_all, CS_NB)] : s->user_prm;
     if (s->user_trc == AVCOL_TRC_UNSPECIFIED) {
@@ -746,10 +750,6 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
     } else {
         out->color_trc   = s->user_trc;
     }
-    out->colorspace      = s->user_csp == AVCOL_SPC_UNSPECIFIED ?
-                           default_csp[FFMIN(s->user_all, CS_NB)] : s->user_csp;
-    out->color_range     = s->user_rng == AVCOL_RANGE_UNSPECIFIED ?
-                           in->color_range : s->user_rng;
     if (rgb_sz != s->rgb_sz) {
         const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(out->format);
         int uvw = in->width >> desc->log2_chroma_w;
@@ -839,10 +839,19 @@ static int query_formats(AVFilterContext *ctx)
         AV_PIX_FMT_YUVJ420P,  AV_PIX_FMT_YUVJ422P,  AV_PIX_FMT_YUVJ444P,
         AV_PIX_FMT_NONE
     };
-    int res;
+    int res, ret;
     ColorSpaceContext *s = ctx->priv;
+    AVFilterLink *outlink = ctx->outputs[0];
     AVFilterFormats *formats = ff_make_format_list(pix_fmts);
 
+    if (ret = ff_formats_ref(ff_make_formats_list_singleton(s->out_csp),
+                             &outlink->incfg.color_spaces) < 0)
+        return ret;
+    if (s->user_rng != AVCOL_RANGE_UNSPECIFIED
+        && (ret = ff_formats_ref(ff_make_formats_list_singleton(s->user_rng),
+                             &outlink->incfg.color_ranges) < 0))
+        return ret;
+
     if (!formats)
         return AVERROR(ENOMEM);
     if (s->user_format == AV_PIX_FMT_NONE)
@@ -855,7 +864,7 @@ static int query_formats(AVFilterContext *ctx)
     if (res < 0)
         return res;
 
-    return ff_formats_ref(formats, &ctx->outputs[0]->incfg.formats);
+    return ff_formats_ref(formats, &outlink->incfg.formats);
 }
 
 static int config_props(AVFilterLink *outlink)
-- 
2.30.2

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

[Niklas Haas]

On Tue, 02 Apr 2024 15:05:59 +0200 Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
> Fixes a regression due to the fact that the colorspace filter does
> not use the new API introduced by 8c7934f73ab6c568acaa.
> The scale filter uses it since 45e09a30419cc2a7251e, and the setparams
> filter since 3bf80df3ccd32aed23f0.
> 
> Example 1 - color_range specified:
> ffmpeg -f lavfi -i yuvtestsrc -vf setparams=color_primaries=bt470bg:
> color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale
> ,showinfo -f null -frames 1 -
> 
> Before:
>   color_range:unknown color_space:bt470bg ...
> After:
>   color_range:tv color_space:bt709 ...
> 
> Example 2 - color_range pass-through:
> 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}"
> 
> Before:
> color_range:tv color_space:bt470bg
> color_range:tv color_space:bt470bg
> color_range:tv color_space:bt470bg
> After:
> color_range:unknown color_space:bt709
> color_range:pc color_space:bt709
> color_range:tv color_space:bt709
> 
> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
> ---
>  libavfilter/vf_colorspace.c | 63 +++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 27 deletions(-)
> 
> diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
> index d181e81ace..12a571172b 100644
> --- 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?

>  
>      if (!s->out_primaries || !s->in_primaries) {
>          s->in_prm = in->color_primaries;
> @@ -563,26 +562,8 @@ static int create_filtergraph(AVFilterContext *ctx,
>          redo_yuv2rgb = 1;
>      }
>  
> -    if (!s->out_lumacoef) {
> -        s->out_csp = out->colorspace;
> +    if (!s->rgb2yuv) {
>          s->out_rng = out->color_range;
> -        s->out_lumacoef = av_csp_luma_coeffs_from_avcsp(s->out_csp);
> -        if (!s->out_lumacoef) {
> -            if (s->out_csp == AVCOL_SPC_UNSPECIFIED) {
> -                if (s->user_all == CS_UNSPECIFIED) {
> -                    av_log(ctx, AV_LOG_ERROR,
> -                           "Please specify output colorspace\n");
> -                } else {
> -                    av_log(ctx, AV_LOG_ERROR,
> -                           "Unsupported output color property %d\n", s->user_all);
> -                }
> -            } else {
> -                av_log(ctx, AV_LOG_ERROR,
> -                       "Unsupported output colorspace %d (%s)\n", s->out_csp,
> -                       av_color_space_name(s->out_csp));
> -            }
> -            return AVERROR(EINVAL);
> -        }
>          redo_rgb2yuv = 1;
>      }
>  
> @@ -687,6 +668,26 @@ static av_cold int init(AVFilterContext *ctx)
>  {
>      ColorSpaceContext *s = ctx->priv;
>  
> +    s->out_csp  = s->user_csp == AVCOL_SPC_UNSPECIFIED ?
> +                  default_csp[FFMIN(s->user_all, CS_NB)] : s->user_csp;
> +    s->out_lumacoef = av_csp_luma_coeffs_from_avcsp(s->out_csp);
> +    if (!s->out_lumacoef) {
> +        if (s->out_csp == AVCOL_SPC_UNSPECIFIED) {
> +            if (s->user_all == CS_UNSPECIFIED) {
> +                av_log(ctx, AV_LOG_ERROR,
> +                       "Please specify output colorspace\n");
> +            } else {
> +                av_log(ctx, AV_LOG_ERROR,
> +                       "Unsupported output color property %d\n", s->user_all);
> +            }
> +        } else {
> +            av_log(ctx, AV_LOG_ERROR,
> +                   "Unsupported output colorspace %d (%s)\n", s->out_csp,
> +                   av_color_space_name(s->out_csp));
> +        }
> +        return AVERROR(EINVAL);
> +    }
> +
>      ff_colorspacedsp_init(&s->dsp);
>  
>      return 0;
> @@ -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.

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.

>      out->color_primaries = s->user_prm == AVCOL_PRI_UNSPECIFIED ?
>                             default_prm[FFMIN(s->user_all, CS_NB)] : s->user_prm;
>      if (s->user_trc == AVCOL_TRC_UNSPECIFIED) {
> @@ -746,10 +750,6 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
>      } else {
>          out->color_trc   = s->user_trc;
>      }
> -    out->colorspace      = s->user_csp == AVCOL_SPC_UNSPECIFIED ?
> -                           default_csp[FFMIN(s->user_all, CS_NB)] : s->user_csp;
> -    out->color_range     = s->user_rng == AVCOL_RANGE_UNSPECIFIED ?
> -                           in->color_range : s->user_rng;
>      if (rgb_sz != s->rgb_sz) {
>          const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(out->format);
>          int uvw = in->width >> desc->log2_chroma_w;
> @@ -839,10 +839,19 @@ static int query_formats(AVFilterContext *ctx)
>          AV_PIX_FMT_YUVJ420P,  AV_PIX_FMT_YUVJ422P,  AV_PIX_FMT_YUVJ444P,
>          AV_PIX_FMT_NONE
>      };
> -    int res;
> +    int res, ret;

Nit: Why introduce a new result variable instead of re-using the one
that's already declared?

>      ColorSpaceContext *s = ctx->priv;
> +    AVFilterLink *outlink = ctx->outputs[0];
>      AVFilterFormats *formats = ff_make_format_list(pix_fmts);
>  
> +    if (ret = ff_formats_ref(ff_make_formats_list_singleton(s->out_csp),
> +                             &outlink->incfg.color_spaces) < 0)
> +        return ret;
> +    if (s->user_rng != AVCOL_RANGE_UNSPECIFIED
> +        && (ret = ff_formats_ref(ff_make_formats_list_singleton(s->user_rng),
> +                             &outlink->incfg.color_ranges) < 0))
> +        return ret;
> +

IMO this logic would look cleaner if you assigned ret before testing it,
especially since it's nested.

>      if (!formats)
>          return AVERROR(ENOMEM);
>      if (s->user_format == AV_PIX_FMT_NONE)
> @@ -855,7 +864,7 @@ static int query_formats(AVFilterContext *ctx)
>      if (res < 0)
>          return res;
>  
> -    return ff_formats_ref(formats, &ctx->outputs[0]->incfg.formats);
> +    return ff_formats_ref(formats, &outlink->incfg.formats);
>  }
>  
>  static int config_props(AVFilterLink *outlink)
> -- 
> 2.30.2
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".

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

[Nicolas Gaullier]

>De : Niklas Haas <ffmpeg@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-request@ffmpeg.org with subject "unsubscribe".

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

[Nicolas Gaullier]

>De : Nicolas Gaullier 
>Envoyé : jeudi 4 avril 2024 14:02
>
>>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...

I was too hasty, sorry, let me precise what is running wrong:
the "out" frame colorspace/range is fine
And when running:
/ffprobe -f lavfi -i yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,showinfo 2>&1|grep color_space
you actually get, as expected:
color_range:tv color_space:bt709

The issue is the link, and it is raised when chaining with the scale filter for example here:
./ffprobe -f lavfi -i yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale,showinfo 2>&1|grep color_space
color_range:unknown color_space:bt470bg

So, I don't know, maybe my approach (fixing vf_colorspace) is wrong ?
Anyway, the "out->color_range = outlink->color_range" logic does not seem to be the kind of things to do here.

Thank you again
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-request@ffmpeg.org with subject "unsubscribe".

Re: [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@cji.paris> wrote:
> >De : Niklas Haas <ffmpeg@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-request@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-request@ffmpeg.org with subject "unsubscribe".

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

[Nicolas Gaullier]

>De : ffmpeg-devel <ffmpeg-devel-bounces@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-request@ffmpeg.org with subject "unsubscribe".

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

[Nicolas Gaullier]

>Envoyé : mardi 2 avril 2024 15:06
>
>v3:
>- Fixes case where colorspace is the first filter (no inlink)
>- Illustrates with proper examples in commit msg (use yuvtestsrc instead of testsrc)
>
>Please note that it is a regression compared to the previous release:
>both examples (see commit msg) behave the same way as 6.1 after this patch.
>
>Nicolas Gaullier (2):
>  avfilter/vf_setparams: Add timeline support
>  avfilter/vf_colorspace: Use colorspace negotiation API

Any chance to get this regression fixed for 7.0 one way or another?
For remembering:
./ffprobe -f lavfi -i yuvtestsrc,setparams=color_primaries=bt470bg:color_trc=smpte170m:colorspace=bt470bg,colorspace=bt709:range=tv,scale,showinfo 2>&1|grep color_space
Current code:
color_range:unknown color_space:bt470bg ...
Release 6.1: (expected values) [or current code with my proposed patch]
color_range:tv color_space:bt709 ...

Thanks
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-request@ffmpeg.org with subject "unsubscribe".