[FFmpeg-devel] [PATCHv2 1/2] ffplay: add support for interactive volume control

[Marton Balint]

On Sat, 26 Sep 2015, Ganesh Ajjanagadde wrote:

> This is a feature heavily inspired by the mpv player. At the moment, methods
> for adjusting volume in ffplay are rather clumsy: either one needs to set it
> system-wide, or one needs to set it via the volume filter.
>
> This patch adds key bindings identical to the mpv defaults for muting/unmuting
> and increasing/decreasing the volume interactively without any introduction of
> external dependencies.
>
> TODO: doc update, possible mouse button bindings (mpv has this).
>
> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> ---
> ffplay.c | 34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/ffplay.c b/ffplay.c
> index d302793..eada863 100644
> --- a/ffplay.c
> +++ b/ffplay.c
> @@ -73,6 +73,9 @@ const int program_birth_year = 2003;
> /* Calculate actual buffer size keeping in mind not cause too frequent audio callbacks */
> #define SDL_AUDIO_MAX_CALLBACKS_PER_SEC 30
> 
> +/* Step size for volume control */
> +#define SDL_VOLUME_STEP 2

Maybe (SDL_MIX_MAXVOLUME/40) or something like that would be more 
readable/future proof.

> +
> /* no AV sync correction is done if below the minimum AV sync threshold */
> #define AV_SYNC_THRESHOLD_MIN 0.04
> /* AV sync correction is done if above the maximum AV sync threshold */
> @@ -246,6 +249,8 @@ typedef struct VideoState {
>     unsigned int audio_buf1_size;
>     int audio_buf_index; /* in bytes */
>     int audio_write_buf_size;
> +    int audio_volume;
> +    int muted;
>     struct AudioParams audio_src;
> #if CONFIG_AVFILTER
>     struct AudioParams audio_filter_src;
> @@ -1348,6 +1353,17 @@ static void toggle_pause(VideoState *is)
>     is->step = 0;
> }
> 
> +static void toggle_mute(VideoState *is)
> +{
> +    is->muted = !is->muted;
> +}
> +
> +static void update_volume(VideoState *is, int sign, int step)
> +{
> +    is->audio_volume += sign * step;
> +    is->audio_volume = av_clip(is->audio_volume, 0, SDL_MIX_MAXVOLUME);

Update audio_volume in one step to avoid the race condition reading the 
unclipped value from it in the audio thread.

> +}
> +
> static void step_to_next_frame(VideoState *is)
> {
>     /* if the stream is paused unpause it, then step */
> @@ -2447,7 +2463,10 @@ static void sdl_audio_callback(void *opaque, Uint8 *stream, int len)
>         len1 = is->audio_buf_size - is->audio_buf_index;
>         if (len1 > len)
>             len1 = len;
> -        memcpy(stream, (uint8_t *)is->audio_buf + is->audio_buf_index, len1);
> +        if (is->muted)
> +            memset(stream, 0, len1);
> +        else
> +            SDL_MixAudio(stream, (uint8_t *)is->audio_buf + is->audio_buf_index, len1, is->audio_volume);

I guess this only works because most SDL audio drivers reset the audio 
buffer before calling the callback. I am not sure this reliably works 
on all platforms, so probably it is better if you always initalize the 
buffer with zeroes before mixing into it. SDL2 explicitly needs this 
anyway.

Also you may want add a special case for max volume to eliminate the 
performance penalty of the extra memset/memcpy. (Yes, not really 
significant).

Something like:

if (!muted and maxvolume)
   memcpy
else {
   memset
   if (!muted)
     Mix
}

>         len -= len1;
>         stream += len1;
>         is->audio_buf_index += len1;
> @@ -2634,6 +2653,8 @@ static int stream_component_open(VideoState *is, int stream_index)
>         is->audio_src = is->audio_tgt;
>         is->audio_buf_size  = 0;
>         is->audio_buf_index = 0;
> +        is->audio_volume = SDL_MIX_MAXVOLUME;
> +        is->muted = 0;

Are you sure you want to reset the settings here? This get called when the 
user changes the audio stream as well. I think it is better if you only 
set these in stream_open.

>
>         /* init averaging filter */
>         is->audio_diff_avg_coef  = exp(log(0.01) / AUDIO_DIFF_AVG_NB);
> @@ -3311,6 +3332,17 @@ static void event_loop(VideoState *cur_stream)
>             case SDLK_SPACE:
>                 toggle_pause(cur_stream);
>                 break;
> +            case SDLK_m:
> +                toggle_mute(cur_stream);
> +                break;
> +            case SDLK_ASTERISK:

SDLK_KP_MULTIPLY is the numeric keypad key.

> +            case SDLK_0:
> +                update_volume(cur_stream, 1, SDL_VOLUME_STEP);
> +                break;
> +            case SDLK_SLASH:

SDLK_KP_DIVIDE

> +            case SDLK_9:
> +                update_volume(cur_stream, -1, SDL_VOLUME_STEP);
> +                break;
>             case SDLK_s: // S: Step to next frame
>                 step_to_next_frame(cur_stream);
>                 break;
> -- 
> 2.5.3

Regards,
Marton