[media.audio] Remove volume changed ACK
If this API
* has four clients that never read their events
* a user generates this event 16 times per day
* the system is up for 10 years
~1.3MB of kernel memory will be consumed.
Bug: 35429 #obsolete
TEST: Removed tests of this removed behavior
Change-Id: I090389282eb51cb45f1359a13b19df0d37295399
diff --git a/garnet/bin/setui/src/audio/stream_volume_control.rs b/garnet/bin/setui/src/audio/stream_volume_control.rs
index c346d7d..3e81b19 100644
--- a/garnet/bin/setui/src/audio/stream_volume_control.rs
+++ b/garnet/bin/setui/src/audio/stream_volume_control.rs
@@ -4,12 +4,12 @@
use {
crate::switchboard::base::{AudioStream, AudioStreamType},
- fidl::endpoints::create_proxy,
+ fidl::{self, endpoints::create_proxy},
fidl_fuchsia_media::{AudioRenderUsage, Usage},
- fidl_fuchsia_media_audio::{VolumeControlEvent, VolumeControlProxy},
+ fidl_fuchsia_media_audio::VolumeControlProxy,
fuchsia_async as fasync,
fuchsia_syslog::fx_log_err,
- futures::TryStreamExt,
+ futures::{FutureExt, TryFutureExt, TryStreamExt},
};
// Stores an AudioStream and a VolumeControl proxy bound to the AudioCore
@@ -77,18 +77,17 @@
// TODO(fxb/37777): Update |stored_stream| in StreamVolumeControl and send a notification
// when we receive an update.
let proxy_clone = vol_control_proxy.clone();
- fasync::spawn(async move {
- let mut stream = proxy_clone.take_event_stream();
- while let Some(evt) = stream.try_next().await.unwrap() {
- match evt {
- VolumeControlEvent::OnVolumeMuteChanged { new_volume: _, new_muted: _ } => {
- proxy_clone.notify_volume_mute_changed_handled().unwrap_or_else(move |e| {
- fx_log_err!("failed to notify volume mute change handling, {}", e);
- });
- }
- }
- }
- });
+ let consume_volume_events = async move {
+ let mut volume_events = proxy_clone.take_event_stream();
+ while let Some(_) = volume_events.try_next().await? {}
+
+ Ok(())
+ };
+ fasync::spawn(
+ consume_volume_events
+ .map_err(|e: fidl::Error| fx_log_err!("Volume event stream failed: {}", e))
+ .map(|_| ()),
+ );
Some(vol_control_proxy)
}
diff --git a/garnet/bin/setui/src/tests/audio_tests.rs b/garnet/bin/setui/src/tests/audio_tests.rs
index d1eb83c..039394e 100644
--- a/garnet/bin/setui/src/tests/audio_tests.rs
+++ b/garnet/bin/setui/src/tests/audio_tests.rs
@@ -145,8 +145,6 @@
(CHANGED_VOLUME_LEVEL, CHANGED_VOLUME_MUTED),
audio_core_service_handle.read().get_level_and_mute(AudioRenderUsage::Media).unwrap()
);
-
- assert_eq!(2, audio_core_service_handle.read().get_ack_count(AudioRenderUsage::Media).unwrap());
}
// Test to ensure mic input change events are received.
@@ -247,12 +245,6 @@
(CHANGED_VOLUME_LEVEL, CHANGED_VOLUME_MUTED),
audio_core_service_handle.read().get_level_and_mute(AudioRenderUsage::SystemAgent).unwrap()
);
-
- assert_eq!(2, audio_core_service_handle.read().get_ack_count(AudioRenderUsage::Media).unwrap());
- assert_eq!(
- 2,
- audio_core_service_handle.read().get_ack_count(AudioRenderUsage::SystemAgent).unwrap()
- );
}
// Test to ensure that when |pair_media_and_system_agent| is disabled, setting the media volume will
@@ -289,9 +281,6 @@
None,
audio_core_service_handle.read().get_level_and_mute(AudioRenderUsage::SystemAgent)
);
-
- assert_eq!(2, audio_core_service_handle.read().get_ack_count(AudioRenderUsage::Media).unwrap());
- assert_eq!(None, audio_core_service_handle.read().get_ack_count(AudioRenderUsage::SystemAgent));
}
// Test to ensure that when |pair_media_and_system_agent| is enabled, setting the media volume
@@ -339,12 +328,6 @@
(CHANGED_SYSTEM_LEVEL, CHANGED_SYSTEM_MUTED),
audio_core_service_handle.read().get_level_and_mute(AudioRenderUsage::SystemAgent).unwrap()
);
-
- assert_eq!(2, audio_core_service_handle.read().get_ack_count(AudioRenderUsage::Media).unwrap());
- assert_eq!(
- 1,
- audio_core_service_handle.read().get_ack_count(AudioRenderUsage::SystemAgent).unwrap()
- );
}
#[fuchsia_async::run_singlethreaded(test)]
diff --git a/garnet/bin/setui/src/tests/fakes/audio_core_service.rs b/garnet/bin/setui/src/tests/fakes/audio_core_service.rs
index c7caaea..d4520d8 100644
--- a/garnet/bin/setui/src/tests/fakes/audio_core_service.rs
+++ b/garnet/bin/setui/src/tests/fakes/audio_core_service.rs
@@ -17,27 +17,16 @@
/// usages.
pub struct AudioCoreService {
audio_streams: Arc<RwLock<HashMap<AudioRenderUsage, (f32, bool)>>>,
- ack_count: Arc<RwLock<HashMap<AudioRenderUsage, u32>>>,
}
impl AudioCoreService {
pub fn new() -> Self {
- Self {
- audio_streams: Arc::new(RwLock::new(HashMap::new())),
- ack_count: Arc::new(RwLock::new(HashMap::new())),
- }
+ Self { audio_streams: Arc::new(RwLock::new(HashMap::new())) }
}
pub fn get_level_and_mute(&self, usage: AudioRenderUsage) -> Option<(f32, bool)> {
get_level_and_mute(usage, &self.audio_streams)
}
-
- pub fn get_ack_count(&self, usage: AudioRenderUsage) -> Option<u32> {
- if let Some(count) = (*self.ack_count.read()).get(&usage) {
- return Some(*count);
- }
- None
- }
}
impl Service for AudioCoreService {
@@ -54,7 +43,6 @@
ServerEnd::<fidl_fuchsia_media::AudioCoreMarker>::new(channel).into_stream()?;
let streams_clone = self.audio_streams.clone();
- let ack_count_clone = self.ack_count.clone();
fasync::spawn(async move {
while let Some(req) = manager_stream.try_next().await.unwrap() {
#[allow(unreachable_patterns)]
@@ -69,7 +57,6 @@
volume_control,
render_usage,
streams_clone.clone(),
- ack_count_clone.clone(),
);
}
}
@@ -96,7 +83,6 @@
volume_control: ServerEnd<fidl_fuchsia_media_audio::VolumeControlMarker>,
render_usage: AudioRenderUsage,
streams: Arc<RwLock<HashMap<AudioRenderUsage, (f32, bool)>>>,
- ack_count: Arc<RwLock<HashMap<AudioRenderUsage, u32>>>,
) {
let mut stream = volume_control.into_stream().expect("volume control stream error");
fasync::spawn(async move {
@@ -114,7 +100,8 @@
} else {
(*streams.write()).insert(render_usage, (volume, DEFAULT_VOLUME_MUTED));
}
- control_handle.send_on_volume_mute_changed(volume, curr_mute)
+ control_handle
+ .send_on_volume_mute_changed(volume, curr_mute)
.expect("on volume mute changed");
}
fidl_fuchsia_media_audio::VolumeControlRequest::SetMute {
@@ -129,19 +116,10 @@
(*streams.write()).insert(render_usage, (DEFAULT_VOLUME_LEVEL, mute));
}
- control_handle.send_on_volume_mute_changed(curr_level, mute)
+ control_handle
+ .send_on_volume_mute_changed(curr_level, mute)
.expect("on volume mute changed");
}
- fidl_fuchsia_media_audio::VolumeControlRequest::NotifyVolumeMuteChangedHandled {
- control_handle: _
- } => {
- let mut ack_count_lock = ack_count.write();
- let mut new_count = 1;
- if let Some(count) = (*ack_count_lock).get(&render_usage) {
- new_count = *count + 1;
- }
- ack_count_lock.insert(render_usage, new_count);
- }
_ => {}
}
}
diff --git a/sdk/fidl/fuchsia.media.audio/fuchsia.media.audio.api b/sdk/fidl/fuchsia.media.audio/fuchsia.media.audio.api
index 38f0d8ab..be760cb 100644
--- a/sdk/fidl/fuchsia.media.audio/fuchsia.media.audio.api
+++ b/sdk/fidl/fuchsia.media.audio/fuchsia.media.audio.api
@@ -1,4 +1,4 @@
{
"fidl/fuchsia.media.audio/gain_control.fidl": "0882351775e1b05cfbe0aabcae2d1c47",
- "fidl/fuchsia.media.audio/volume_control.fidl": "ff622dae6eb90bbad52685446e2a4066"
+ "fidl/fuchsia.media.audio/volume_control.fidl": "f8f2c94e02a0ed42eca750d51ad89558"
}
\ No newline at end of file
diff --git a/sdk/fidl/fuchsia.media.audio/volume_control.fidl b/sdk/fidl/fuchsia.media.audio/volume_control.fidl
index 6e43a8d..6c4936b 100644
--- a/sdk/fidl/fuchsia.media.audio/volume_control.fidl
+++ b/sdk/fidl/fuchsia.media.audio/volume_control.fidl
@@ -24,8 +24,4 @@
/// Emitted when the volume or mute state of the audio element changes.
-> OnVolumeMuteChanged(float32 new_volume, bool new_muted);
- /// Acknowledges receipt of a volume or mute change event. Clients must
- /// acknowledge receipt to continue receiving events.
- // TODO(35429): Remove when FIDL gets event responses.
- NotifyVolumeMuteChangedHandled();
};
diff --git a/src/media/audio/audio_core/volume_control.cc b/src/media/audio/audio_core/volume_control.cc
index 06d7a93..2622268 100644
--- a/src/media/audio/audio_core/volume_control.cc
+++ b/src/media/audio/audio_core/volume_control.cc
@@ -10,11 +10,6 @@
namespace {
-// TODO(turnage): Move to FIDL
-constexpr int64_t kBacklogFullEpitaph = 88;
-
-// This layer of indirection on VolumeControl just holds a per-client ack count.
-//
// This exists because impls in a BindingSet cannot access their own binding to send events in a
// safe way.
class VolumeControlImpl : public fuchsia::media::audio::VolumeControl {
@@ -22,18 +17,12 @@
VolumeControlImpl(::media::audio::VolumeControl* volume_control)
: volume_control_(volume_control) {}
- // Counts an event sent to this client among the pending acks.
- // Returns the events already sent.
- uint64_t CountEvent() { return events_sent_without_ack_++; }
-
private:
// |fuchsia::media::audio::VolumeControl|
void SetVolume(float volume) override { volume_control_->SetVolume(volume); }
void SetMute(bool mute) override { volume_control_->SetMute(mute); }
- void NotifyVolumeMuteChangedHandled() override { events_sent_without_ack_ = 0; }
::media::audio::VolumeControl* volume_control_;
- uint64_t events_sent_without_ack_ = 0;
};
} // namespace
@@ -74,13 +63,7 @@
void VolumeControl::NotifyClientsOfState() {
for (auto& binding : bindings_.bindings()) {
- auto impl = static_cast<VolumeControlImpl*>(binding->impl().get());
- if (impl->CountEvent() < kMaxEventsSentWithoutAck) {
- binding->events().OnVolumeMuteChanged(current_volume_, muted_);
- } else {
- binding->Close(kBacklogFullEpitaph);
- AUD_LOG(WARNING) << "Disconnected volume control client because they did not ACK events";
- }
+ binding->events().OnVolumeMuteChanged(current_volume_, muted_);
}
}
diff --git a/src/media/audio/audio_core/volume_control_unittest.cc b/src/media/audio/audio_core/volume_control_unittest.cc
index b5e7070..1dfe2c7 100644
--- a/src/media/audio/audio_core/volume_control_unittest.cc
+++ b/src/media/audio/audio_core/volume_control_unittest.cc
@@ -12,9 +12,6 @@
namespace media::audio {
namespace {
-// TODO(turnage): Move to FIDL
-constexpr zx_status_t kBacklogFullEpitaph = 88;
-
class MockVolumeSetting : public VolumeSetting {
public:
void SetVolume(float volume) override { volume_ = volume; }
@@ -101,9 +98,7 @@
float volume;
bool muted;
auto client = BindVolumeControl();
- client.events().OnVolumeMuteChanged = [&client, &volume, &muted](float new_volume,
- bool new_muted) {
- client->NotifyVolumeMuteChangedHandled();
+ client.events().OnVolumeMuteChanged = [&volume, &muted](float new_volume, bool new_muted) {
volume = new_volume;
muted = new_muted;
};
@@ -134,9 +129,8 @@
bool muted;
size_t event_count = 0;
auto client = BindVolumeControl();
- client.events().OnVolumeMuteChanged = [&client, &event_count, &volume, &muted](float new_volume,
- bool new_muted) {
- client->NotifyVolumeMuteChangedHandled();
+ client.events().OnVolumeMuteChanged = [&event_count, &volume, &muted](float new_volume,
+ bool new_muted) {
volume = new_volume;
muted = new_muted;
++event_count;
@@ -161,9 +155,7 @@
float volume1;
bool muted1;
auto client1 = BindVolumeControl();
- client1.events().OnVolumeMuteChanged = [&client1, &volume1, &muted1](float new_volume,
- bool new_muted) {
- client1->NotifyVolumeMuteChangedHandled();
+ client1.events().OnVolumeMuteChanged = [&volume1, &muted1](float new_volume, bool new_muted) {
volume1 = new_volume;
muted1 = new_muted;
};
@@ -171,9 +163,7 @@
float volume2;
bool muted2;
auto client2 = BindVolumeControl();
- client2.events().OnVolumeMuteChanged = [&client2, &volume2, &muted2](float new_volume,
- bool new_muted) {
- client2->NotifyVolumeMuteChangedHandled();
+ client2.events().OnVolumeMuteChanged = [&volume2, &muted2](float new_volume, bool new_muted) {
volume2 = new_volume;
muted2 = new_muted;
};
@@ -186,27 +176,5 @@
EXPECT_FALSE(muted2);
}
-TEST_F(VolumeControlTest, ClientDisconnectsIfAckLimitExceeded) {
- size_t client1_event_count = 0;
- auto client1 = BindVolumeControl();
- client1.events().OnVolumeMuteChanged = [&client1, &client1_event_count](float _volume,
- bool _muted) {
- client1->NotifyVolumeMuteChangedHandled();
- client1_event_count += 1;
- };
-
- auto client2 = BindVolumeControl();
- zx_status_t client2_error = ZX_OK;
- client2.set_error_handler([&client2_error](zx_status_t error) { client2_error = error; });
-
- for (size_t i = 0; i < VolumeControl::kMaxEventsSentWithoutAck + 1; ++i) {
- client1->SetVolume(static_cast<float>(i) / 100.0);
- RunLoopUntilIdle();
- }
-
- EXPECT_EQ(client1_event_count, VolumeControl::kMaxEventsSentWithoutAck + 1);
- EXPECT_EQ(client2_error, kBacklogFullEpitaph);
-}
-
} // namespace
} // namespace media::audio
