diff mbox

[FFmpeg-devel] Ensure scaled video is divisible by n

Message ID 20190703153337.25116-1-lkiesow@uos.de
State Superseded
Headers show

Commit Message

Lars Kiesow July 3, 2019, 3:33 p.m. UTC
This patch adds a new option to the scale filter which ensures that the
output resolution is divisible by the given integer similar to using -n
in the `w` and `h` options. But this works even if the
`force_original_aspect_ratio` is used.

The use case for this is to set a fixed target resolution using `w` and
`h`, to use the `force_original_aspect_ratio` option to make sure that
the video always fits in the defined bounding box regardless of aspect
ratio, but to also make sure that the calculated output resolution is
divisible by n so in can be encoded with certain encoders/options if
that is required.

Signed-off-by: Lars Kiesow <lkiesow@uos.de>
---
 doc/filters.texi       | 5 +++++
 libavfilter/vf_scale.c | 9 ++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Paul B Mahol July 3, 2019, 3:40 p.m. UTC | #1
On 7/3/19, Lars Kiesow <lkiesow@uos.de> wrote:
> This patch adds a new option to the scale filter which ensures that the
> output resolution is divisible by the given integer similar to using -n
> in the `w` and `h` options. But this works even if the
> `force_original_aspect_ratio` is used.
>
> The use case for this is to set a fixed target resolution using `w` and
> `h`, to use the `force_original_aspect_ratio` option to make sure that
> the video always fits in the defined bounding box regardless of aspect
> ratio, but to also make sure that the calculated output resolution is
> divisible by n so in can be encoded with certain encoders/options if
> that is required.
>
> Signed-off-by: Lars Kiesow <lkiesow@uos.de>
> ---
>  doc/filters.texi       | 5 +++++
>  libavfilter/vf_scale.c | 9 ++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 700a76f239..1694fdda28 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -15215,6 +15215,11 @@ Please note that this is a different thing than
> specifying -1 for @option{w}
>  or @option{h}, you still need to specify the output resolution for this
> option
>  to work.
>
> +@item force_divisible_by
> +Ensures that the output resolution is divisible by the given integer
> similar
> +to using -n in the @option{w} and @option{h} options. But this works even
> if
> +the @option{force_original_aspect_ratio} is used.
> +
>  @end table
>
>  The values of the @option{w} and @option{h} options are expressions
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index f741419e7e..c02b82dd07 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -86,6 +86,7 @@ typedef struct ScaleContext {
>      int in_v_chr_pos;
>
>      int force_original_aspect_ratio;
> +    int force_divisible_by;
>
>      int nb_slices;
>
> @@ -237,10 +238,11 @@ static int config_props(AVFilterLink *outlink)
>          goto fail;
>
>      /* Note that force_original_aspect_ratio may overwrite the previous set
> -     * dimensions so that it is not divisible by the set factors anymore.
> */
> +     * dimensions so that it is not divisible by the set factors anymore
> +     * unless force_divisible_by is defined as well */
>      if (scale->force_original_aspect_ratio) {
> -        int tmp_w = av_rescale(h, inlink->w, inlink->h);
> -        int tmp_h = av_rescale(w, inlink->h, inlink->w);
> +        int tmp_w = av_rescale(h, inlink->w, inlink->h) /
> scale->force_divisible_by * scale->force_divisible_by;
> +        int tmp_h = av_rescale(w, inlink->h, inlink->w) /
> scale->force_divisible_by * scale->force_divisible_by;
>
>          if (scale->force_original_aspect_ratio == 1) {
>               w = FFMIN(tmp_w, w);
> @@ -589,6 +591,7 @@ static const AVOption scale_options[] = {
>      { "out_v_chr_pos",   "output vertical chroma position in luma grid/256"
>  , OFFSET(out_v_chr_pos), AV_OPT_TYPE_INT, { .i64 = -513}, -513, 512, FLAGS
> },
>      { "out_h_chr_pos",   "output horizontal chroma position in luma
> grid/256", OFFSET(out_h_chr_pos), AV_OPT_TYPE_INT, { .i64 = -513}, -513,
> 512, FLAGS },
>      { "force_original_aspect_ratio", "decrease or increase w/h if necessary
> to keep the original AR", OFFSET(force_original_aspect_ratio),
> AV_OPT_TYPE_INT, { .i64 = 0}, 0, 2, FLAGS, "force_oar" },
> +    { "force_divisible_by", "enforce that the output resolution is
> divisible by a defined integer", OFFSET(force_divisible_by),
> AV_OPT_TYPE_INT, { .i64 = 1}, 0, 256, FLAGS, "force_oar" },

Are you sure you need "force_oar" ?
If not, move it bellow 3 lines.

>      { "disable",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = 0 }, 0, 0, FLAGS,
> "force_oar" },
>      { "decrease", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = 1 }, 0, 0, FLAGS,
> "force_oar" },
>      { "increase", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = 2 }, 0, 0, FLAGS,
> "force_oar" },
> --
> 2.21.0
>
> _______________________________________________
> 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".
Lars Kiesow July 3, 2019, 3:57 p.m. UTC | #2
Hi Paul,

> >      { "force_original_aspect_ratio", "decrease or increase w/h if
> > necessary to keep the original AR",
> > OFFSET(force_original_aspect_ratio), AV_OPT_TYPE_INT, { .i64 = 0},
> > 0, 2, FLAGS, "force_oar" },
> > +    { "force_divisible_by", "enforce that the output resolution is
> > divisible by a defined integer", OFFSET(force_divisible_by),
> > AV_OPT_TYPE_INT, { .i64 = 1}, 0, 256, FLAGS, "force_oar" },  
> 
> Are you sure you need "force_oar" ?
> If not, move it bellow 3 lines.

I'm honestly not sure since I don't know what it's used for ;-)

From the AVOption Struct docs, I assumed that this should have the same
value as in force_original_aspect_ratio since it's used in the same
context. But it does work without and I can remove it if you want me to.
Should I?

Best regards,
Lars
Paul B Mahol July 3, 2019, 4:04 p.m. UTC | #3
On 7/3/19, Lars Kiesow <lkiesow@uos.de> wrote:
> Hi Paul,
>
>> >      { "force_original_aspect_ratio", "decrease or increase w/h if
>> > necessary to keep the original AR",
>> > OFFSET(force_original_aspect_ratio), AV_OPT_TYPE_INT, { .i64 = 0},
>> > 0, 2, FLAGS, "force_oar" },
>> > +    { "force_divisible_by", "enforce that the output resolution is
>> > divisible by a defined integer", OFFSET(force_divisible_by),
>> > AV_OPT_TYPE_INT, { .i64 = 1}, 0, 256, FLAGS, "force_oar" },
>>
>> Are you sure you need "force_oar" ?
>> If not, move it bellow 3 lines.
>
> I'm honestly not sure since I don't know what it's used for ;-)
>
> From the AVOption Struct docs, I assumed that this should have the same
> value as in force_original_aspect_ratio since it's used in the same
> context. But it does work without and I can remove it if you want me to.
> Should I?

Yes, Look at belllow 3 lines, they have "force_oar" at end, because
that gives special meaning to values.

Your option should not have these.

To confirm these, run "ffmpeg -h filter=scale" before and after you removed it.

>
> Best regards,
> Lars
>
Lars Kiesow July 3, 2019, 4:11 p.m. UTC | #4
On Wed, 3 Jul 2019 18:04:26 +0200
Paul B Mahol <onemda@gmail.com> wrote:

> On 7/3/19, Lars Kiesow <lkiesow@uos.de> wrote:
> > Hi Paul,
> >  
> >> >      { "force_original_aspect_ratio", "decrease or increase w/h
> >> > if necessary to keep the original AR",
> >> > OFFSET(force_original_aspect_ratio), AV_OPT_TYPE_INT, { .i64 =
> >> > 0}, 0, 2, FLAGS, "force_oar" },
> >> > +    { "force_divisible_by", "enforce that the output resolution
> >> > is divisible by a defined integer", OFFSET(force_divisible_by),
> >> > AV_OPT_TYPE_INT, { .i64 = 1}, 0, 256, FLAGS, "force_oar" },  
> >>
> >> Are you sure you need "force_oar" ?
> >> If not, move it bellow 3 lines.  
> >
> > I'm honestly not sure since I don't know what it's used for ;-)
> >
> > From the AVOption Struct docs, I assumed that this should have the
> > same value as in force_original_aspect_ratio since it's used in the
> > same context. But it does work without and I can remove it if you
> > want me to. Should I?  
> 
> Yes, Look at belllow 3 lines, they have "force_oar" at end, because
> that gives special meaning to values.
> 
> Your option should not have these.
> 
> To confirm these, run "ffmpeg -h filter=scale" before and after you
> removed it.

Oh, got it makes sense. Thanks.
Will update the patch.
diff mbox

Patch

diff --git a/doc/filters.texi b/doc/filters.texi
index 700a76f239..1694fdda28 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -15215,6 +15215,11 @@  Please note that this is a different thing than specifying -1 for @option{w}
 or @option{h}, you still need to specify the output resolution for this option
 to work.
 
+@item force_divisible_by
+Ensures that the output resolution is divisible by the given integer similar
+to using -n in the @option{w} and @option{h} options. But this works even if
+the @option{force_original_aspect_ratio} is used.
+
 @end table
 
 The values of the @option{w} and @option{h} options are expressions
diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index f741419e7e..c02b82dd07 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -86,6 +86,7 @@  typedef struct ScaleContext {
     int in_v_chr_pos;
 
     int force_original_aspect_ratio;
+    int force_divisible_by;
 
     int nb_slices;
 
@@ -237,10 +238,11 @@  static int config_props(AVFilterLink *outlink)
         goto fail;
 
     /* Note that force_original_aspect_ratio may overwrite the previous set
-     * dimensions so that it is not divisible by the set factors anymore. */
+     * dimensions so that it is not divisible by the set factors anymore
+     * unless force_divisible_by is defined as well */
     if (scale->force_original_aspect_ratio) {
-        int tmp_w = av_rescale(h, inlink->w, inlink->h);
-        int tmp_h = av_rescale(w, inlink->h, inlink->w);
+        int tmp_w = av_rescale(h, inlink->w, inlink->h) / scale->force_divisible_by * scale->force_divisible_by;
+        int tmp_h = av_rescale(w, inlink->h, inlink->w) / scale->force_divisible_by * scale->force_divisible_by;
 
         if (scale->force_original_aspect_ratio == 1) {
              w = FFMIN(tmp_w, w);
@@ -589,6 +591,7 @@  static const AVOption scale_options[] = {
     { "out_v_chr_pos",   "output vertical chroma position in luma grid/256"  , OFFSET(out_v_chr_pos), AV_OPT_TYPE_INT, { .i64 = -513}, -513, 512, FLAGS },
     { "out_h_chr_pos",   "output horizontal chroma position in luma grid/256", OFFSET(out_h_chr_pos), AV_OPT_TYPE_INT, { .i64 = -513}, -513, 512, FLAGS },
     { "force_original_aspect_ratio", "decrease or increase w/h if necessary to keep the original AR", OFFSET(force_original_aspect_ratio), AV_OPT_TYPE_INT, { .i64 = 0}, 0, 2, FLAGS, "force_oar" },
+    { "force_divisible_by", "enforce that the output resolution is divisible by a defined integer", OFFSET(force_divisible_by), AV_OPT_TYPE_INT, { .i64 = 1}, 0, 256, FLAGS, "force_oar" },
     { "disable",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = 0 }, 0, 0, FLAGS, "force_oar" },
     { "decrease", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = 1 }, 0, 0, FLAGS, "force_oar" },
     { "increase", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = 2 }, 0, 0, FLAGS, "force_oar" },