diff mbox series

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

Message ID 1578716038-9645-1-git-send-email-mypopydev@gmail.com
State New
Headers show
Series [FFmpeg-devel,V1,01/11] lavfi/spp: add "quality" option in runtime change path
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Jun Zhao Jan. 11, 2020, 4:13 a.m. UTC
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(-)

Comments

Michael Niedermayer Jan. 11, 2020, 6:50 p.m. UTC | #1
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

[...]
mypopy@gmail.com Jan. 13, 2020, 1:19 a.m. UTC | #2
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
>
mypopy@gmail.com Jan. 13, 2020, 1:32 a.m. UTC | #3
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
Moritz Barsnick Jan. 13, 2020, 1:27 p.m. UTC | #4
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
mypopy@gmail.com Jan. 14, 2020, 1:31 a.m. UTC | #5
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)
Moritz Barsnick Jan. 14, 2020, 12:46 p.m. UTC | #6
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
mypopy@gmail.com Jan. 15, 2020, 3:41 a.m. UTC | #7
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.
diff mbox series

Patch

diff --git a/doc/filters.texi b/doc/filters.texi
index a2f862e..7459255 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -17271,6 +17271,15 @@  option may cause flicker since the B-Frames have often larger QP. Default is
 @code{0} (not enabled).
 @end table
 
+@subsection Commands
+
+This filter supports the following commands:
+@table @option
+@item level
+@item quality
+Same as quality option. And the command accepts the @code{max} same as the @code{6}.
+@end table
+
 @section sr
 
 Scale the input by applying one of the super-resolution methods based on
diff --git a/libavfilter/vf_spp.c b/libavfilter/vf_spp.c
index fe579ce..db381cd 100644
--- a/libavfilter/vf_spp.c
+++ b/libavfilter/vf_spp.c
@@ -444,7 +444,7 @@  static int process_command(AVFilterContext *ctx, const char *cmd, const char *ar
 {
     SPPContext *s = ctx->priv;
 
-    if (!strcmp(cmd, "level")) {
+    if (!strcmp(cmd, "level") || !strcmp(cmd, "quality")) {
         if (!strcmp(args, "max"))
             s->log2_count = MAX_LEVEL;
         else