diff mbox series

[FFmpeg-devel] avfilter/vf_bwdif: consider chroma subsampling when enforcing minimum dimensions

Message ID 0101018c0835aafa-0a570ead-e0fc-4e1f-98c1-cf44ae96b7c3-000000@us-west-2.amazonses.com
State New
Headers show
Series [FFmpeg-devel] avfilter/vf_bwdif: consider chroma subsampling when enforcing minimum dimensions | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Cosmin Stejerean Nov. 25, 2023, 8:39 p.m. UTC 
Fixes #10688

Signed-off-by: Cosmin Stejerean <cosmin@cosmin.at>
---
 libavfilter/vf_bwdif.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Thomas Mundt Nov. 28, 2023, 1:30 p.m. UTC | #1 
Hi Cosmin,

Cosmin Stejerean via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> schrieb am Sa.,
25. Nov. 2023, 21:39:

> Fixes #10688
>
> Signed-off-by: Cosmin Stejerean <cosmin@cosmin.at>
> ---
>  libavfilter/vf_bwdif.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
> index 137cd5ef13..bce11c39f7 100644
> --- a/libavfilter/vf_bwdif.c
> +++ b/libavfilter/vf_bwdif.c
> @@ -197,6 +197,18 @@ static int config_props(AVFilterLink *link)
>      }
>
>      yadif->csp = av_pix_fmt_desc_get(link->format);
> +
> +    if (yadif->csp->nb_components > 1) {
> +        int w_chroma, h_chroma;
> +        h_chroma = AV_CEIL_RSHIFT(link->h, yadif->csp->log2_chroma_h);
> +        w_chroma = AV_CEIL_RSHIFT(link->w, yadif->csp->log2_chroma_w);
> +
> +        if (w_chroma < 3 || h_chroma < 4) {
> +            av_log(ctx, AV_LOG_ERROR, "Video with planes less than 3
> columns or 4 lines is not supported\n");
> +            return AVERROR(EINVAL);
> +        }
> +    }
> +
>

Thanks for your quick patch.
Could you please make the size check for all components and remove the old
one to avoid having two size checks in a row?

Regards,
Thomas
Cosmin Stejerean Nov. 29, 2023, 7:26 p.m. UTC | #2 
> On Nov 28, 2023, at 5:30 AM, Thomas Mundt <tmundt75@gmail.com> wrote:
> 
> Hi Cosmin,
> 
> Cosmin Stejerean via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> schrieb am Sa.,
> 25. Nov. 2023, 21:39:
> 
>> Fixes #10688
>> 
>> Signed-off-by: Cosmin Stejerean <cosmin@cosmin.at>
>> ---
>> libavfilter/vf_bwdif.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>> 
>> diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
>> index 137cd5ef13..bce11c39f7 100644
>> --- a/libavfilter/vf_bwdif.c
>> +++ b/libavfilter/vf_bwdif.c
>> @@ -197,6 +197,18 @@ static int config_props(AVFilterLink *link)
>>     }
>> 
>>     yadif->csp = av_pix_fmt_desc_get(link->format);
>> +
>> +    if (yadif->csp->nb_components > 1) {
>> +        int w_chroma, h_chroma;
>> +        h_chroma = AV_CEIL_RSHIFT(link->h, yadif->csp->log2_chroma_h);
>> +        w_chroma = AV_CEIL_RSHIFT(link->w, yadif->csp->log2_chroma_w);
>> +
>> +        if (w_chroma < 3 || h_chroma < 4) {
>> +            av_log(ctx, AV_LOG_ERROR, "Video with planes less than 3
>> columns or 4 lines is not supported\n");
>> +            return AVERROR(EINVAL);
>> +        }
>> +    }
>> +
>> 
> 
> Thanks for your quick patch.
> Could you please make the size check for all components and remove the old
> one to avoid having two size checks in a row?
> 

Certainly, will send a v2 shortly.

- Cosmin
Dennis Mungai Nov. 29, 2023, 7:36 p.m. UTC | #3 
On Wed, 29 Nov 2023 at 22:26, Cosmin Stejerean via ffmpeg-devel <
ffmpeg-devel@ffmpeg.org> wrote:

>
>
> > On Nov 28, 2023, at 5:30 AM, Thomas Mundt <tmundt75@gmail.com> wrote:
> >
> > Hi Cosmin,
> >
> > Cosmin Stejerean via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> schrieb am
> Sa.,
> > 25. Nov. 2023, 21:39:
> >
> >> Fixes #10688
> >>
> >> Signed-off-by: Cosmin Stejerean <cosmin@cosmin.at>
> >> ---
> >> libavfilter/vf_bwdif.c | 12 ++++++++++++
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
> >> index 137cd5ef13..bce11c39f7 100644
> >> --- a/libavfilter/vf_bwdif.c
> >> +++ b/libavfilter/vf_bwdif.c
> >> @@ -197,6 +197,18 @@ static int config_props(AVFilterLink *link)
> >>     }
> >>
> >>     yadif->csp = av_pix_fmt_desc_get(link->format);
> >> +
> >> +    if (yadif->csp->nb_components > 1) {
> >> +        int w_chroma, h_chroma;
> >> +        h_chroma = AV_CEIL_RSHIFT(link->h, yadif->csp->log2_chroma_h);
> >> +        w_chroma = AV_CEIL_RSHIFT(link->w, yadif->csp->log2_chroma_w);
> >> +
> >> +        if (w_chroma < 3 || h_chroma < 4) {
> >> +            av_log(ctx, AV_LOG_ERROR, "Video with planes less than 3
> >> columns or 4 lines is not supported\n");
> >> +            return AVERROR(EINVAL);
> >> +        }
> >> +    }
> >> +
> >>
> >
> > Thanks for your quick patch.
> > Could you please make the size check for all components and remove the
> old
> > one to avoid having two size checks in a row?
> >
>
> Certainly, will send a v2 shortly.
>
> - Cosmin
>
>
Does this change also need to be replicated to bwdif_cuda?
Cosmin Stejerean Nov. 29, 2023, 9:59 p.m. UTC | #4 
> On Nov 29, 2023, at 11:36 AM, Dennis Mungai <dmngaie@gmail.com> wrote:
> 
> On Wed, 29 Nov 2023 at 22:26, Cosmin Stejerean via ffmpeg-devel <
> ffmpeg-devel@ffmpeg.org> wrote:
> 
>> 
>> 
>>> On Nov 28, 2023, at 5:30 AM, Thomas Mundt <tmundt75@gmail.com> wrote:
>>> 
>>> Hi Cosmin,
>>> 
>>> Cosmin Stejerean via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> schrieb am
>> Sa.,
>>> 25. Nov. 2023, 21:39:
>>> 
>>>> Fixes #10688
>>>> 
>>>> Signed-off-by: Cosmin Stejerean <cosmin@cosmin.at>
>>>> ---
>>>> libavfilter/vf_bwdif.c | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>> 
>>>> diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
>>>> index 137cd5ef13..bce11c39f7 100644
>>>> --- a/libavfilter/vf_bwdif.c
>>>> +++ b/libavfilter/vf_bwdif.c
>>>> @@ -197,6 +197,18 @@ static int config_props(AVFilterLink *link)
>>>>    }
>>>> 
>>>>    yadif->csp = av_pix_fmt_desc_get(link->format);
>>>> +
>>>> +    if (yadif->csp->nb_components > 1) {
>>>> +        int w_chroma, h_chroma;
>>>> +        h_chroma = AV_CEIL_RSHIFT(link->h, yadif->csp->log2_chroma_h);
>>>> +        w_chroma = AV_CEIL_RSHIFT(link->w, yadif->csp->log2_chroma_w);
>>>> +
>>>> +        if (w_chroma < 3 || h_chroma < 4) {
>>>> +            av_log(ctx, AV_LOG_ERROR, "Video with planes less than 3
>>>> columns or 4 lines is not supported\n");
>>>> +            return AVERROR(EINVAL);
>>>> +        }
>>>> +    }
>>>> +
>>>> 
>>> 
>>> Thanks for your quick patch.
>>> Could you please make the size check for all components and remove the
>> old
>>> one to avoid having two size checks in a row?
>>> 
>> 
>> Certainly, will send a v2 shortly.
>> 
>> - Cosmin
>> 
>> 
> Does this change also need to be replicated to bwdif_cuda?
> 


Good callout, it's like the that both bwdif_cuda and bwdif_vulkan need similar updates as they both have checks for width and height and would likely encounter the same problem on the chroma planes. I'll include both of those in v2.

On a side note the minimum width and height requirements are a bit different between the three filters in terms of whether 3x4, 3x3 or 4x4 are the minimum required dimensions. I'm assuming this is intentional based on the underlying filters but just wanted to double check whether it might be worth unifying all of these to say min 4x4 for consistency.

- Cosmin
diff mbox series

Patch

diff --git a/libavfilter/vf_bwdif.c b/libavfilter/vf_bwdif.c
index 137cd5ef13..bce11c39f7 100644
--- a/libavfilter/vf_bwdif.c
+++ b/libavfilter/vf_bwdif.c
@@ -197,6 +197,18 @@  static int config_props(AVFilterLink *link)
     }
 
     yadif->csp = av_pix_fmt_desc_get(link->format);
+
+    if (yadif->csp->nb_components > 1) {
+        int w_chroma, h_chroma;
+        h_chroma = AV_CEIL_RSHIFT(link->h, yadif->csp->log2_chroma_h);
+        w_chroma = AV_CEIL_RSHIFT(link->w, yadif->csp->log2_chroma_w);
+
+        if (w_chroma < 3 || h_chroma < 4) {
+            av_log(ctx, AV_LOG_ERROR, "Video with planes less than 3 columns or 4 lines is not supported\n");
+            return AVERROR(EINVAL);
+        }
+    }
+
     yadif->filter = filter;
     ff_bwdif_init_filter_line(&s->dsp, yadif->csp->comp[0].depth);