From 589673f1cbef93921900a835de52d05ad4f3a0e2 Mon Sep 17 00:00:00 2001 From: "andrew@webrtc.org" Date: Mon, 7 May 2012 21:42:49 +0000 Subject: [PATCH] Fix volume setting while not playing on PulseAudio. We now only set the volume when starting playout if the user has called SetSpeakerVolume while we weren't playing. This now also ensures it will actually get set to what the user requested rather than being overridden by a default value. Add tests to voe_auto_test. BUG=6140661 TEST=voe_auto_test Review URL: https://webrtc-codereview.appspot.com/566006 git-svn-id: http://webrtc.googlecode.com/svn/trunk@2188 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../source/linux/audio_device_pulse_linux.cc | 32 ++++++++++++----- .../source/linux/audio_device_pulse_linux.h | 1 + .../linux/audio_mixer_manager_pulse_linux.cc | 5 +-- .../fixtures/after_streaming_fixture.cc | 26 ++++++++------ .../fixtures/after_streaming_fixture.h | 9 +++-- .../test/auto_test/standard/volume_test.cc | 35 +++++++++++++++++++ 6 files changed, 82 insertions(+), 26 deletions(-) diff --git a/src/modules/audio_device/main/source/linux/audio_device_pulse_linux.cc b/src/modules/audio_device/main/source/linux/audio_device_pulse_linux.cc index 3eebce27cc..86edb0266c 100644 --- a/src/modules/audio_device/main/source/linux/audio_device_pulse_linux.cc +++ b/src/modules/audio_device/main/source/linux/audio_device_pulse_linux.cc @@ -93,6 +93,7 @@ AudioDeviceLinuxPulse::AudioDeviceLinuxPulse(const WebRtc_Word32 id) : _startPlay(false), _stopPlay(false), _AGC(false), + update_speaker_volume_at_startup_(false), _playBufDelayFixed(20), _sndCardPlayDelay(0), _sndCardRecDelay(0), @@ -127,7 +128,9 @@ AudioDeviceLinuxPulse::AudioDeviceLinuxPulse(const WebRtc_Word32 id) : _recStream(NULL), _playStream(NULL), _recStreamFlags(0), - _playStreamFlags(0) + _playStreamFlags(0), + _playBufferAttr(), + _recBufferAttr() { WEBRTC_TRACE(kTraceMemory, kTraceAudioDevice, id, "%s created", __FUNCTION__); @@ -539,7 +542,10 @@ WebRtc_Word32 AudioDeviceLinuxPulse::SpeakerVolumeIsAvailable(bool& available) WebRtc_Word32 AudioDeviceLinuxPulse::SetSpeakerVolume(WebRtc_UWord32 volume) { - + if (!_playing) { + // Only update the volume if it's been set while we weren't playing. + update_speaker_volume_at_startup_ = true; + } return (_mixerManager.SetSpeakerVolume(volume)); } @@ -2818,15 +2824,23 @@ bool AudioDeviceLinuxPulse::PlayThreadProcess() // Get the currently saved speaker volume WebRtc_UWord32 volume = 0; - _mixerManager.SpeakerVolume(volume); + if (update_speaker_volume_at_startup_) + _mixerManager.SpeakerVolume(volume); PaLock(); - // Set the same volume for all channels - pa_cvolume cVolumes; - const pa_sample_spec *spec = - LATE(pa_stream_get_sample_spec)(_playStream); - LATE(pa_cvolume_set)(&cVolumes, spec->channels, volume); + // NULL gives PA the choice of startup volume. + pa_cvolume* ptr_cvolume = NULL; + if (update_speaker_volume_at_startup_) { + pa_cvolume cVolumes; + ptr_cvolume = &cVolumes; + + // Set the same volume for all channels + const pa_sample_spec *spec = + LATE(pa_stream_get_sample_spec)(_playStream); + LATE(pa_cvolume_set)(&cVolumes, spec->channels, volume); + update_speaker_volume_at_startup_ = false; + } // Connect the stream to a sink if (LATE(pa_stream_connect_playback)( @@ -2834,7 +2848,7 @@ bool AudioDeviceLinuxPulse::PlayThreadProcess() _playDeviceName, &_playBufferAttr, (pa_stream_flags_t) _playStreamFlags, - &cVolumes, NULL) != PA_OK) + ptr_cvolume, NULL) != PA_OK) { WEBRTC_TRACE(kTraceError, kTraceAudioDevice, _id, " failed to connect play stream, err=%d", diff --git a/src/modules/audio_device/main/source/linux/audio_device_pulse_linux.h b/src/modules/audio_device/main/source/linux/audio_device_pulse_linux.h index b4a42eb718..1a71fe5704 100644 --- a/src/modules/audio_device/main/source/linux/audio_device_pulse_linux.h +++ b/src/modules/audio_device/main/source/linux/audio_device_pulse_linux.h @@ -330,6 +330,7 @@ private: bool _startPlay; bool _stopPlay; bool _AGC; + bool update_speaker_volume_at_startup_; private: WebRtc_UWord16 _playBufDelayFixed; // fixed playback delay diff --git a/src/modules/audio_device/main/source/linux/audio_mixer_manager_pulse_linux.cc b/src/modules/audio_device/main/source/linux/audio_mixer_manager_pulse_linux.cc index 7094d53c64..a0b6852355 100644 --- a/src/modules/audio_device/main/source/linux/audio_mixer_manager_pulse_linux.cc +++ b/src/modules/audio_device/main/source/linux/audio_mixer_manager_pulse_linux.cc @@ -39,7 +39,7 @@ AudioMixerManagerLinuxPulse::AudioMixerManagerLinuxPulse(const WebRtc_Word32 id) _paMute(0), _paVolSteps(0), _paSpeakerMute(false), - _paSpeakerVolume(0), + _paSpeakerVolume(PA_VOLUME_NORM), _paChannels(0), _paObjectsSet(false), _callbackValues(false) @@ -176,9 +176,6 @@ WebRtc_Word32 AudioMixerManagerLinuxPulse::OpenSpeaker( // output device to control _paOutputDeviceIndex = deviceIndex; - // Init the speaker volume to the normal volume - _paSpeakerVolume = PA_VOLUME_NORM; - WEBRTC_TRACE(kTraceInfo, kTraceAudioDevice, _id, " the output mixer device is now open"); diff --git a/src/voice_engine/main/test/auto_test/fixtures/after_streaming_fixture.cc b/src/voice_engine/main/test/auto_test/fixtures/after_streaming_fixture.cc index c12c806972..d1e6039e38 100644 --- a/src/voice_engine/main/test/auto_test/fixtures/after_streaming_fixture.cc +++ b/src/voice_engine/main/test/auto_test/fixtures/after_streaming_fixture.cc @@ -22,14 +22,13 @@ AfterStreamingFixture::AfterStreamingFixture() EXPECT_FALSE(fake_microphone_input_file_.empty()); SetUpLocalPlayback(); - StartPlaying(); + ResumePlaying(); + RestartFakeMicrophone(); } AfterStreamingFixture::~AfterStreamingFixture() { voe_file_->StopPlayingFileAsMicrophone(channel_); - voe_base_->StopSend(channel_); - voe_base_->StopPlayout(channel_); - voe_base_->StopReceive(channel_); + PausePlaying(); voe_base_->DeleteChannel(channel_); } @@ -47,6 +46,18 @@ void AfterStreamingFixture::RestartFakeMicrophone() { channel_, fake_microphone_input_file_.c_str(), true, true)); } +void AfterStreamingFixture::PausePlaying() { + EXPECT_EQ(0, voe_base_->StopSend(channel_)); + EXPECT_EQ(0, voe_base_->StopPlayout(channel_)); + EXPECT_EQ(0, voe_base_->StopReceive(channel_)); +} + +void AfterStreamingFixture::ResumePlaying() { + EXPECT_EQ(0, voe_base_->StartReceive(channel_)); + EXPECT_EQ(0, voe_base_->StartPlayout(channel_)); + EXPECT_EQ(0, voe_base_->StartSend(channel_)); +} + void AfterStreamingFixture::SetUpLocalPlayback() { EXPECT_EQ(0, voe_base_->SetSendDestination(channel_, 8000, kLoopbackIp)); EXPECT_EQ(0, voe_base_->SetLocalReceiver(0, 8000)); @@ -61,10 +72,3 @@ void AfterStreamingFixture::SetUpLocalPlayback() { voe_codec_->SetSendCodec(channel_, codec); } - -void AfterStreamingFixture::StartPlaying() { - EXPECT_EQ(0, voe_base_->StartReceive(channel_)); - EXPECT_EQ(0, voe_base_->StartPlayout(channel_)); - EXPECT_EQ(0, voe_base_->StartSend(channel_)); - RestartFakeMicrophone(); -} diff --git a/src/voice_engine/main/test/auto_test/fixtures/after_streaming_fixture.h b/src/voice_engine/main/test/auto_test/fixtures/after_streaming_fixture.h index c589a4c2a1..6b0a61f304 100644 --- a/src/voice_engine/main/test/auto_test/fixtures/after_streaming_fixture.h +++ b/src/voice_engine/main/test/auto_test/fixtures/after_streaming_fixture.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011 The WebRTC project authors. All Rights Reserved. + * Copyright (c) 2012 The WebRTC project authors. All Rights Reserved. * * Use of this source code is governed by a BSD-style license * that can be found in the LICENSE file in the root of the source @@ -34,9 +34,14 @@ class AfterStreamingFixture : public AfterInitializationFixture { // Restarts the fake microphone if it's been shut off earlier. void RestartFakeMicrophone(); + // Stops all sending and playout. + void PausePlaying(); + + // Resumes all sending and playout. + void ResumePlaying(); + private: void SetUpLocalPlayback(); - void StartPlaying(); }; diff --git a/src/voice_engine/main/test/auto_test/standard/volume_test.cc b/src/voice_engine/main/test/auto_test/standard/volume_test.cc index d714e42ecd..0060d63cdd 100644 --- a/src/voice_engine/main/test/auto_test/standard/volume_test.cc +++ b/src/voice_engine/main/test/auto_test/standard/volume_test.cc @@ -19,6 +19,34 @@ TEST_F(VolumeTest, DefaultSpeakerVolumeIsAtMost255) { EXPECT_LE(volume, 255u); } +TEST_F(VolumeTest, SetVolumeBeforePlayoutWorks) { + // This is a rather specialized test, intended to exercise some PulseAudio + // code. However, these conditions should be satisfied on any platform. + unsigned int original_volume = 0; + EXPECT_EQ(0, voe_volume_control_->GetSpeakerVolume(original_volume)); + Sleep(1000); + + EXPECT_EQ(0, voe_volume_control_->SetSpeakerVolume(200)); + unsigned int volume; + EXPECT_EQ(0, voe_volume_control_->GetSpeakerVolume(volume)); + EXPECT_EQ(200u, volume); + + PausePlaying(); + ResumePlaying(); + EXPECT_EQ(0, voe_volume_control_->GetSpeakerVolume(volume)); + // Ensure the volume has not changed after resuming playout. + EXPECT_EQ(200u, volume); + + PausePlaying(); + EXPECT_EQ(0, voe_volume_control_->SetSpeakerVolume(100)); + ResumePlaying(); + // Ensure the volume set while paused is retained. + EXPECT_EQ(0, voe_volume_control_->GetSpeakerVolume(volume)); + EXPECT_EQ(100u, volume); + + EXPECT_EQ(0, voe_volume_control_->SetSpeakerVolume(original_volume)); +} + TEST_F(VolumeTest, ManualSetVolumeWorks) { unsigned int original_volume = 0; EXPECT_EQ(0, voe_volume_control_->GetSpeakerVolume(original_volume)); @@ -26,15 +54,22 @@ TEST_F(VolumeTest, ManualSetVolumeWorks) { TEST_LOG("Setting speaker volume to 0 out of 255.\n"); EXPECT_EQ(0, voe_volume_control_->SetSpeakerVolume(0)); + unsigned int volume; + EXPECT_EQ(0, voe_volume_control_->GetSpeakerVolume(volume)); + EXPECT_EQ(0u, volume); Sleep(1000); TEST_LOG("Setting speaker volume to 100 out of 255.\n"); EXPECT_EQ(0, voe_volume_control_->SetSpeakerVolume(100)); + EXPECT_EQ(0, voe_volume_control_->GetSpeakerVolume(volume)); + EXPECT_EQ(100u, volume); Sleep(1000); // Set the volume to 255 very briefly so we don't blast the poor user // listening to this. This is just to test the call succeeds. EXPECT_EQ(0, voe_volume_control_->SetSpeakerVolume(255)); + EXPECT_EQ(0, voe_volume_control_->GetSpeakerVolume(volume)); + EXPECT_EQ(255u, volume); TEST_LOG("Setting speaker volume to the original %d out of 255.\n", original_volume);