diff mbox

[FFmpeg-devel] ffplay: add startup volume option

Message ID 20161222182045.15269-1-gajjanag@yandex.com
State Accepted
Headers show

Commit Message

gajjanag@yandex.com Dec. 22, 2016, 6:20 p.m. UTC 
From: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>

Fixes Ticket 5389.

Signed-off-by: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
---
 doc/ffplay.texi |  4 ++++
 ffplay.c        | 10 +++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

Comments

Marton Balint Dec. 25, 2016, 1 a.m. UTC | #1 
On Thu, 22 Dec 2016, gajjanag@yandex.com wrote:

> From: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
>
> Fixes Ticket 5389.
>
> Signed-off-by: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
> ---
> doc/ffplay.texi |  4 ++++
> ffplay.c        | 10 +++++++++-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/doc/ffplay.texi b/doc/ffplay.texi
> index 073b457256..378a229d67 100644
> --- a/doc/ffplay.texi
> +++ b/doc/ffplay.texi
> @@ -62,6 +62,10 @@ see @ref{time duration syntax,,the Time duration section in the ffmpeg-utils(1)
> Seek by bytes.
> @item -nodisp
> Disable graphical display.
> +@item -volume
> +Set the startup volume. 0 means silence, 100 means no volume reduction or
> +amplification. Negative values are treated as 0, values above 100 are treated
> +as 100.
> @item -f @var{fmt}
> Force format.
> @item -window_title @var{title}
> diff --git a/ffplay.c b/ffplay.c
> index 911fd7f947..de603c0a27 100644
> --- a/ffplay.c
> +++ b/ffplay.c
> @@ -322,6 +322,7 @@ static int subtitle_disable;
> static const char* wanted_stream_spec[AVMEDIA_TYPE_NB] = {0};
> static int seek_by_bytes = -1;
> static int display_disable;
> +static int startup_volume = 100;
> static int show_status = 1;
> static int av_sync_type = AV_SYNC_AUDIO_MASTER;
> static int64_t start_time = AV_NOPTS_VALUE;
> @@ -3105,7 +3106,13 @@ static VideoState *stream_open(const char *filename, AVInputFormat *iformat)
>     init_clock(&is->audclk, &is->audioq.serial);
>     init_clock(&is->extclk, &is->extclk.serial);
>     is->audio_clock_serial = -1;
> -    is->audio_volume = SDL_MIX_MAXVOLUME;
> +    if (startup_volume < 0)
> +        av_log(NULL, AV_LOG_WARNING, "-volume=%d < 0, setting to 0\n", startup_volume);
> +    if (startup_volume > 100)
> +        av_log(NULL, AV_LOG_WARNING, "-volume=%d > 100, setting to 100\n", startup_volume);
> +    startup_volume = av_clip(startup_volume, 0, 100);
> +    startup_volume = av_clip(SDL_MIX_MAXVOLUME * startup_volume / 100.0, 0, SDL_MIX_MAXVOLUME);

Any reason you use 100.0 here instead of a simple integer (100)?

> +    is->audio_volume = startup_volume;
>     is->muted = 0;
>     is->av_sync_type = av_sync_type;
>     is->read_tid     = SDL_CreateThread(read_thread, "read_thread", is);
> @@ -3586,6 +3593,7 @@ static const OptionDef options[] = {
>     { "t", HAS_ARG, { .func_arg = opt_duration }, "play  \"duration\" seconds of audio/video", "duration" },
>     { "bytes", OPT_INT | HAS_ARG, { &seek_by_bytes }, "seek by bytes 0=off 1=on -1=auto", "val" },
>     { "nodisp", OPT_BOOL, { &display_disable }, "disable graphical display" },
> +    { "volume", OPT_INT | HAS_ARG, { &startup_volume}, "set startup volume 0=min 100=max", "volume" },
>     { "f", HAS_ARG, { .func_arg = opt_format }, "force format", "fmt" },
>     { "pix_fmt", HAS_ARG | OPT_EXPERT | OPT_VIDEO, { .func_arg = opt_frame_pix_fmt }, "set pixel format", "format" },
>     { "stats", OPT_BOOL | OPT_EXPERT, { &show_status }, "show status", "" },
> -- 
> 2.11.0

Otherwise LGTM, thanks.

Marton
Ganesh Ajjanagadde Dec. 25, 2016, 2:09 a.m. UTC | #2 
24.12.2016, 20:00, "Marton Balint" <cus@passwd.hu>:
> On Thu, 22 Dec 2016, gajjanag@yandex.com wrote:
>
>>  From: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
>>
>>  Fixes Ticket 5389.
>>
>>  Signed-off-by: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
>>  ---
>>  doc/ffplay.texi | 4 ++++
>>  ffplay.c | 10 +++++++++-
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>>  diff --git a/doc/ffplay.texi b/doc/ffplay.texi
>>  index 073b457256..378a229d67 100644
>>  --- a/doc/ffplay.texi
>>  +++ b/doc/ffplay.texi
>>  @@ -62,6 +62,10 @@ see @ref{time duration syntax,,the Time duration section in the ffmpeg-utils(1)
>>  Seek by bytes.
>>  @item -nodisp
>>  Disable graphical display.
>>  +@item -volume
>>  +Set the startup volume. 0 means silence, 100 means no volume reduction or
>>  +amplification. Negative values are treated as 0, values above 100 are treated
>>  +as 100.
>>  @item -f @var{fmt}
>>  Force format.
>>  @item -window_title @var{title}
>>  diff --git a/ffplay.c b/ffplay.c
>>  index 911fd7f947..de603c0a27 100644
>>  --- a/ffplay.c
>>  +++ b/ffplay.c
>>  @@ -322,6 +322,7 @@ static int subtitle_disable;
>>  static const char* wanted_stream_spec[AVMEDIA_TYPE_NB] = {0};
>>  static int seek_by_bytes = -1;
>>  static int display_disable;
>>  +static int startup_volume = 100;
>>  static int show_status = 1;
>>  static int av_sync_type = AV_SYNC_AUDIO_MASTER;
>>  static int64_t start_time = AV_NOPTS_VALUE;
>>  @@ -3105,7 +3106,13 @@ static VideoState *stream_open(const char *filename, AVInputFormat *iformat)
>>      init_clock(&is->audclk, &is->audioq.serial);
>>      init_clock(&is->extclk, &is->extclk.serial);
>>      is->audio_clock_serial = -1;
>>  - is->audio_volume = SDL_MIX_MAXVOLUME;
>>  + if (startup_volume < 0)
>>  + av_log(NULL, AV_LOG_WARNING, "-volume=%d < 0, setting to 0\n", startup_volume);
>>  + if (startup_volume > 100)
>>  + av_log(NULL, AV_LOG_WARNING, "-volume=%d > 100, setting to 100\n", startup_volume);
>>  + startup_volume = av_clip(startup_volume, 0, 100);
>>  + startup_volume = av_clip(SDL_MIX_MAXVOLUME * startup_volume / 100.0, 0, SDL_MIX_MAXVOLUME);
>
> Any reason you use 100.0 here instead of a simple integer (100)?

Nothing really, I assumed (incorrectly) that double to int cast does rounding instead of truncation,
which happens with integer division.
On the other hand, the difference here between rounding and truncation is < 1%.
Whatever you prefer, i.e can lrint it to get better rounding, or change to 100 and not bother.

>
>>  + is->audio_volume = startup_volume;
>>      is->muted = 0;
>>      is->av_sync_type = av_sync_type;
>>      is->read_tid = SDL_CreateThread(read_thread, "read_thread", is);
>>  @@ -3586,6 +3593,7 @@ static const OptionDef options[] = {
>>      { "t", HAS_ARG, { .func_arg = opt_duration }, "play \"duration\" seconds of audio/video", "duration" },
>>      { "bytes", OPT_INT | HAS_ARG, { &seek_by_bytes }, "seek by bytes 0=off 1=on -1=auto", "val" },
>>      { "nodisp", OPT_BOOL, { &display_disable }, "disable graphical display" },
>>  + { "volume", OPT_INT | HAS_ARG, { &startup_volume}, "set startup volume 0=min 100=max", "volume" },
>>      { "f", HAS_ARG, { .func_arg = opt_format }, "force format", "fmt" },
>>      { "pix_fmt", HAS_ARG | OPT_EXPERT | OPT_VIDEO, { .func_arg = opt_frame_pix_fmt }, "set pixel format", "format" },
>>      { "stats", OPT_BOOL | OPT_EXPERT, { &show_status }, "show status", "" },
>>  --
>>  2.11.0
>
> Otherwise LGTM, thanks.
>
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Marton Balint Dec. 25, 2016, 6:31 p.m. UTC | #3 
On Sat, 24 Dec 2016, Ganesh Ajjanagadde wrote:

>
>
> 24.12.2016, 20:00, "Marton Balint" <cus@passwd.hu>:
>> On Thu, 22 Dec 2016, gajjanag@yandex.com wrote:
>>
>>>  From: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
>>>
>>>  Fixes Ticket 5389.
>>>
>>>  Signed-off-by: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
>>>  ---
>>>  doc/ffplay.texi | 4 ++++
>>>  ffplay.c | 10 +++++++++-
>>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>>  diff --git a/doc/ffplay.texi b/doc/ffplay.texi
>>>  index 073b457256..378a229d67 100644
>>>  --- a/doc/ffplay.texi
>>>  +++ b/doc/ffplay.texi
>>>  @@ -62,6 +62,10 @@ see @ref{time duration syntax,,the Time duration section in the ffmpeg-utils(1)
>>>  Seek by bytes.
>>>  @item -nodisp
>>>  Disable graphical display.
>>>  +@item -volume
>>>  +Set the startup volume. 0 means silence, 100 means no volume reduction or
>>>  +amplification. Negative values are treated as 0, values above 100 are treated
>>>  +as 100.
>>>  @item -f @var{fmt}
>>>  Force format.
>>>  @item -window_title @var{title}
>>>  diff --git a/ffplay.c b/ffplay.c
>>>  index 911fd7f947..de603c0a27 100644
>>>  --- a/ffplay.c
>>>  +++ b/ffplay.c
>>>  @@ -322,6 +322,7 @@ static int subtitle_disable;
>>>  static const char* wanted_stream_spec[AVMEDIA_TYPE_NB] = {0};
>>>  static int seek_by_bytes = -1;
>>>  static int display_disable;
>>>  +static int startup_volume = 100;
>>>  static int show_status = 1;
>>>  static int av_sync_type = AV_SYNC_AUDIO_MASTER;
>>>  static int64_t start_time = AV_NOPTS_VALUE;
>>>  @@ -3105,7 +3106,13 @@ static VideoState *stream_open(const char *filename, AVInputFormat *iformat)
>>>      init_clock(&is->audclk, &is->audioq.serial);
>>>      init_clock(&is->extclk, &is->extclk.serial);
>>>      is->audio_clock_serial = -1;
>>>  - is->audio_volume = SDL_MIX_MAXVOLUME;
>>>  + if (startup_volume < 0)
>>>  + av_log(NULL, AV_LOG_WARNING, "-volume=%d < 0, setting to 0\n", startup_volume);
>>>  + if (startup_volume > 100)
>>>  + av_log(NULL, AV_LOG_WARNING, "-volume=%d > 100, setting to 100\n", startup_volume);
>>>  + startup_volume = av_clip(startup_volume, 0, 100);
>>>  + startup_volume = av_clip(SDL_MIX_MAXVOLUME * startup_volume / 100.0, 0, SDL_MIX_MAXVOLUME);
>>
>> Any reason you use 100.0 here instead of a simple integer (100)?
>
> Nothing really, I assumed (incorrectly) that double to int cast does rounding instead of truncation,
> which happens with integer division.
> On the other hand, the difference here between rounding and truncation is < 1%.
> Whatever you prefer, i.e can lrint it to get better rounding, or change to 100 and not bother.

I'd simply use 100 and not bother.

Thanks,
Marton
Ganesh Ajjanagadde Dec. 25, 2016, 9:36 p.m. UTC | #4 
25.12.2016, 13:31, "Marton Balint" <cus@passwd.hu>:
> On Sat, 24 Dec 2016, Ganesh Ajjanagadde wrote:
>
>>  24.12.2016, 20:00, "Marton Balint" <cus@passwd.hu>:
>>>  On Thu, 22 Dec 2016, gajjanag@yandex.com wrote:
>>>
>>>>   From: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
>>>>
>>>>   Fixes Ticket 5389.
>>>>
>>>>   Signed-off-by: Ganesh Ajjanagadde <gajjanag@alum.mit.edu>
>>>>   ---
>>>>   doc/ffplay.texi | 4 ++++
>>>>   ffplay.c | 10 +++++++++-
>>>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>>   diff --git a/doc/ffplay.texi b/doc/ffplay.texi
>>>>   index 073b457256..378a229d67 100644
>>>>   --- a/doc/ffplay.texi
>>>>   +++ b/doc/ffplay.texi
>>>>   @@ -62,6 +62,10 @@ see @ref{time duration syntax,,the Time duration section in the ffmpeg-utils(1)
>>>>   Seek by bytes.
>>>>   @item -nodisp
>>>>   Disable graphical display.
>>>>   +@item -volume
>>>>   +Set the startup volume. 0 means silence, 100 means no volume reduction or
>>>>   +amplification. Negative values are treated as 0, values above 100 are treated
>>>>   +as 100.
>>>>   @item -f @var{fmt}
>>>>   Force format.
>>>>   @item -window_title @var{title}
>>>>   diff --git a/ffplay.c b/ffplay.c
>>>>   index 911fd7f947..de603c0a27 100644
>>>>   --- a/ffplay.c
>>>>   +++ b/ffplay.c
>>>>   @@ -322,6 +322,7 @@ static int subtitle_disable;
>>>>   static const char* wanted_stream_spec[AVMEDIA_TYPE_NB] = {0};
>>>>   static int seek_by_bytes = -1;
>>>>   static int display_disable;
>>>>   +static int startup_volume = 100;
>>>>   static int show_status = 1;
>>>>   static int av_sync_type = AV_SYNC_AUDIO_MASTER;
>>>>   static int64_t start_time = AV_NOPTS_VALUE;
>>>>   @@ -3105,7 +3106,13 @@ static VideoState *stream_open(const char *filename, AVInputFormat *iformat)
>>>>       init_clock(&is->audclk, &is->audioq.serial);
>>>>       init_clock(&is->extclk, &is->extclk.serial);
>>>>       is->audio_clock_serial = -1;
>>>>   - is->audio_volume = SDL_MIX_MAXVOLUME;
>>>>   + if (startup_volume < 0)
>>>>   + av_log(NULL, AV_LOG_WARNING, "-volume=%d < 0, setting to 0\n", startup_volume);
>>>>   + if (startup_volume > 100)
>>>>   + av_log(NULL, AV_LOG_WARNING, "-volume=%d > 100, setting to 100\n", startup_volume);
>>>>   + startup_volume = av_clip(startup_volume, 0, 100);
>>>>   + startup_volume = av_clip(SDL_MIX_MAXVOLUME * startup_volume / 100.0, 0, SDL_MIX_MAXVOLUME);
>>>
>>>  Any reason you use 100.0 here instead of a simple integer (100)?
>>
>>  Nothing really, I assumed (incorrectly) that double to int cast does rounding instead of truncation,
>>  which happens with integer division.
>>  On the other hand, the difference here between rounding and truncation is < 1%.
>>  Whatever you prefer, i.e can lrint it to get better rounding, or change to 100 and not bother.
>
> I'd simply use 100 and not bother.

Pushed, thanks.

>
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/doc/ffplay.texi b/doc/ffplay.texi
index 073b457256..378a229d67 100644
--- a/doc/ffplay.texi
+++ b/doc/ffplay.texi
@@ -62,6 +62,10 @@  see @ref{time duration syntax,,the Time duration section in the ffmpeg-utils(1)
 Seek by bytes.
 @item -nodisp
 Disable graphical display.
+@item -volume
+Set the startup volume. 0 means silence, 100 means no volume reduction or
+amplification. Negative values are treated as 0, values above 100 are treated
+as 100.
 @item -f @var{fmt}
 Force format.
 @item -window_title @var{title}
diff --git a/ffplay.c b/ffplay.c
index 911fd7f947..de603c0a27 100644
--- a/ffplay.c
+++ b/ffplay.c
@@ -322,6 +322,7 @@  static int subtitle_disable;
 static const char* wanted_stream_spec[AVMEDIA_TYPE_NB] = {0};
 static int seek_by_bytes = -1;
 static int display_disable;
+static int startup_volume = 100;
 static int show_status = 1;
 static int av_sync_type = AV_SYNC_AUDIO_MASTER;
 static int64_t start_time = AV_NOPTS_VALUE;
@@ -3105,7 +3106,13 @@  static VideoState *stream_open(const char *filename, AVInputFormat *iformat)
     init_clock(&is->audclk, &is->audioq.serial);
     init_clock(&is->extclk, &is->extclk.serial);
     is->audio_clock_serial = -1;
-    is->audio_volume = SDL_MIX_MAXVOLUME;
+    if (startup_volume < 0)
+        av_log(NULL, AV_LOG_WARNING, "-volume=%d < 0, setting to 0\n", startup_volume);
+    if (startup_volume > 100)
+        av_log(NULL, AV_LOG_WARNING, "-volume=%d > 100, setting to 100\n", startup_volume);
+    startup_volume = av_clip(startup_volume, 0, 100);
+    startup_volume = av_clip(SDL_MIX_MAXVOLUME * startup_volume / 100.0, 0, SDL_MIX_MAXVOLUME);
+    is->audio_volume = startup_volume;
     is->muted = 0;
     is->av_sync_type = av_sync_type;
     is->read_tid     = SDL_CreateThread(read_thread, "read_thread", is);
@@ -3586,6 +3593,7 @@  static const OptionDef options[] = {
     { "t", HAS_ARG, { .func_arg = opt_duration }, "play  \"duration\" seconds of audio/video", "duration" },
     { "bytes", OPT_INT | HAS_ARG, { &seek_by_bytes }, "seek by bytes 0=off 1=on -1=auto", "val" },
     { "nodisp", OPT_BOOL, { &display_disable }, "disable graphical display" },
+    { "volume", OPT_INT | HAS_ARG, { &startup_volume}, "set startup volume 0=min 100=max", "volume" },
     { "f", HAS_ARG, { .func_arg = opt_format }, "force format", "fmt" },
     { "pix_fmt", HAS_ARG | OPT_EXPERT | OPT_VIDEO, { .func_arg = opt_frame_pix_fmt }, "set pixel format", "format" },
     { "stats", OPT_BOOL | OPT_EXPERT, { &show_status }, "show status", "" },