[FFmpeg-devel,V1,01/11] lavfi/spp: add "quality" option in runtime change path

From: Jun Zhao <barryjzhao@tencent.com>

it's stranage to use option "level" in runtime change path but used
"quality" in option, add "quality" in runtime change path, it's more
intuitive and keep the "level" for compatibility.

Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
---
 doc/filters.texi     |    9 +++++++++
 libavfilter/vf_spp.c |    2 +-
 2 files changed, 10 insertions(+), 1 deletions(-)
  

On Sat, Jan 11, 2020 at 12:13:48PM +0800, Jun Zhao wrote:
> From: Jun Zhao <barryjzhao@tencent.com>
> 
> it's stranage to use option "level" in runtime change path but used
> "quality" in option, add "quality" in runtime change path, it's more
> intuitive and keep the "level" for compatibility.
> 
> Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> ---
>  doc/filters.texi     |    9 +++++++++
>  libavfilter/vf_spp.c |    2 +-
>  2 files changed, 10 insertions(+), 1 deletions(-)

not sure both should be documented

otherwise probably ok

thx

[...]
  

On Sun, Jan 12, 2020 at 2:50 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sat, Jan 11, 2020 at 12:13:48PM +0800, Jun Zhao wrote:
> > From: Jun Zhao <barryjzhao@tencent.com>
> >
> > it's stranage to use option "level" in runtime change path but used
> > "quality" in option, add "quality" in runtime change path, it's more
> > intuitive and keep the "level" for compatibility.
> >
> > Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> > ---
> >  doc/filters.texi     |    9 +++++++++
> >  libavfilter/vf_spp.c |    2 +-
> >  2 files changed, 10 insertions(+), 1 deletions(-)
>
> not sure both should be documented
I can't find a special reason to document one argr, so will keep both.
>
> otherwise probably ok
>
> thx
>
  

On Mon, Jan 13, 2020 at 9:19 AM mypopy@gmail.com <mypopy@gmail.com> wrote:
>
> On Sun, Jan 12, 2020 at 2:50 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Sat, Jan 11, 2020 at 12:13:48PM +0800, Jun Zhao wrote:
> > > From: Jun Zhao <barryjzhao@tencent.com>
> > >
> > > it's stranage to use option "level" in runtime change path but used
> > > "quality" in option, add "quality" in runtime change path, it's more
> > > intuitive and keep the "level" for compatibility.
> > >
> > > Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> > > ---
> > >  doc/filters.texi     |    9 +++++++++
> > >  libavfilter/vf_spp.c |    2 +-
> > >  2 files changed, 10 insertions(+), 1 deletions(-)
> >
> > not sure both should be documented
> I can't find a special reason to document one argr, so will keep both.
> >
> > otherwise probably ok
patchset pushed, thanks
  

On Sat, Jan 11, 2020 at 12:13:48 +0800, Jun Zhao wrote:
> +@item level
> +@item quality
> +Same as quality option. And the command accepts the @code{max} same as the @code{6}.
> +@end table

I'm sorry for coming in late, but this sentence doesn't make much sense
to me:
   And the command accepts the "max" same as the 6.
Are you trying to say that the allowed range for the command(s) is
(are) the same as for the option?

Moritz
  

On Mon, Jan 13, 2020 at 9:27 PM Moritz Barsnick <barsnick@gmx.net> wrote:
>
> On Sat, Jan 11, 2020 at 12:13:48 +0800, Jun Zhao wrote:
> > +@item level
> > +@item quality
> > +Same as quality option. And the command accepts the @code{max} same as the @code{6}.
> > +@end table
>
> I'm sorry for coming in late, but this sentence doesn't make much sense
> to me:
>    And the command accepts the "max" same as the 6.
> Are you trying to say that the allowed range for the command(s) is
> (are) the same as for the option?
>
No, from the code,

    if (!strcmp(args, "max"))
             s->log2_count = MAX_LEVEL;

this means, we can setting value "max" or "6" (MAX_LEVEL)
  

On Tue, Jan 14, 2020 at 09:31:56 +0800, mypopy@gmail.com wrote:
> > On Sat, Jan 11, 2020 at 12:13:48 +0800, Jun Zhao wrote:
> > > +Same as quality option. And the command accepts the @code{max} same as the @code{6}.

> > Are you trying to say that the allowed range for the command(s) is
> > (are) the same as for the option?

> No, from the code,
>
>     if (!strcmp(args, "max"))
>              s->log2_count = MAX_LEVEL;
>
> this means, we can setting value "max" or "6" (MAX_LEVEL)

Sorry, I didn't check the code, but was reading the docs from a user
point of view.

I suggest fixing this to:

Same as quality option. The command accepts the value @code{max} with
the same meaning as @code{6}.

Or something like this.

Thanks,
Moritz
  

On Tue, Jan 14, 2020 at 8:46 PM Moritz Barsnick <barsnick@gmx.net> wrote:
>
> On Tue, Jan 14, 2020 at 09:31:56 +0800, mypopy@gmail.com wrote:
> > > On Sat, Jan 11, 2020 at 12:13:48 +0800, Jun Zhao wrote:
> > > > +Same as quality option. And the command accepts the @code{max} same as the @code{6}.
>
> > > Are you trying to say that the allowed range for the command(s) is
> > > (are) the same as for the option?
>
> > No, from the code,
> >
> >     if (!strcmp(args, "max"))
> >              s->log2_count = MAX_LEVEL;
> >
> > this means, we can setting value "max" or "6" (MAX_LEVEL)
>
> Sorry, I didn't check the code, but was reading the docs from a user
> point of view.
>
> I suggest fixing this to:
>
> Same as quality option. The command accepts the value @code{max} with
> the same meaning as @code{6}.
>
Will update the doc part, thanks the suggestion.