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

Ganesh Ajjanagadde gajjanag at mit.edu
Sun Sep 27 01:01:41 CEST 2015


On Sat, Sep 26, 2015 at 6:29 PM, Marton Balint <cus at passwd.hu> wrote:
>
> 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.

Changed to / 50.

>
>> +
>> /* 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.

Ok, updated.

>
>> +}
>> +
>> 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.

Good catch, I only looked at the old documentation.

>
> 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
> }

Changed.

>
>>         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.

I am still quite new to the organization of the code, and missed this
in my limited testing. Thanks a lot, changed.

>
>>
>>         /* 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

Ah, I was wondering why mpv had two sets of key bindings for
increase/decrease - it was referring to the numpad and not regular
keys. Furthermore, now i understand / and * - multiply for increase
and / for decrease :). Thanks, changed both. See updated patch.

>
>> +            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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list