On Sat, Sep 26, 2015 at 6:29 PM, Marton Balint <c...@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 <gajjanaga...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
