CRAS: iodev_list - Ramp up/down for unmute/mute only in normal run

When device is not in normal run state, there is no need to start
ramping. This avoid the unwanted ramping down where device is
in no_stream state, and user changes mute state.

The scenario is:
1. Start playback
2. Stop playback
3. Device in no_stream state.
4. Press mute. Device starts ramping down.
5. Ramping does not proceed because there is no valid sample being
   played.
6. Start playback. Valid sample is being played. Ramping proceeds.
   cras_iodev_put_output_buffer does not mute samples because ramping is
   in progress, therefore, we have unexpected audio.

This CL avoids the unwanted ramping down request at 5 to avoid
unexpected audio.

BUG=chromium:669662
TEST=make check
TEST=Follow above scenario and check there is no unexpected audio.

Change-Id: Ia7c20d41f2e1def93ba0e966d8d482b6f703b038
Reviewed-on: https://chromium-review.googlesource.com/428731
Commit-Ready: Cheng-Yi Chiang <cychiang@chromium.org>
Tested-by: Cheng-Yi Chiang <cychiang@chromium.org>
Reviewed-by: Chinyue Chen <chinyue@chromium.org>

2 files changed, 67 insertions, 2 deletions

diff --git a/cras/src/server/cras_iodev_list.c b/cras/src/server/cras_iodev_list.c
index f9db422c..83597d2b 100644
--- a/cras/src/server/cras_iodev_list.c
+++ b/cras/src/server/cras_iodev_list.c
@@ -303,9 +303,11 @@ static void sys_mute_change(void *context, int muted, int user_muted,
 	int should_mute = muted || user_muted;
 
 	DL_FOREACH(devs[CRAS_STREAM_OUTPUT].iodevs, dev) {
-		if (cras_iodev_list_dev_is_enabled(dev) && dev->ramp) {
+		if (cras_iodev_list_dev_is_enabled(dev) && dev->ramp &&
+		    cras_iodev_state(dev) == CRAS_IODEV_STATE_NORMAL_RUN) {
 			/* Start ramping in audio thread and set mute/unmute
-			 * state on device.
+			 * state on device. This should only be done when
+			 * device is running with valid streams.
 			 *
 			 * 1. Mute -> Unmute: Set device unmute state after
 			 *                    ramping is started.
diff --git a/cras/src/tests/iodev_list_unittest.cc b/cras/src/tests/iodev_list_unittest.cc
index 140929b7..4169bbf8 100644
--- a/cras/src/tests/iodev_list_unittest.cc
+++ b/cras/src/tests/iodev_list_unittest.cc
@@ -5,6 +5,7 @@
 #include <algorithm>
 #include <stdio.h>
 #include <gtest/gtest.h>
+#include <map>
 
 extern "C" {
 #include "audio_thread.h"
@@ -77,6 +78,7 @@ static std::vector<struct cras_iodev*> set_mute_dev_vector;
 static struct cras_iodev *audio_thread_dev_start_ramp_dev;
 static int audio_thread_dev_start_ramp_called;
 static enum CRAS_IODEV_RAMP_REQUEST audio_thread_dev_start_ramp_req ;
+static std::map<const struct cras_iodev*, enum CRAS_IODEV_STATE> cras_iodev_state_ret;
 
 void dummy_update_active_node(struct cras_iodev *iodev,
                               unsigned node_idx,
@@ -733,6 +735,12 @@ TEST_F(IoDevTestSuite, OutputMuteChangedToMute) {
   cras_iodev_list_enable_dev(&d1_);
   cras_iodev_list_enable_dev(&d2_);
 
+  // Assume d1 and d2 devices are in normal run.
+  cras_iodev_state_ret[&d1_] = CRAS_IODEV_STATE_NORMAL_RUN;
+  cras_iodev_state_ret[&d2_] = CRAS_IODEV_STATE_NORMAL_RUN;
+  cras_iodev_state_ret[&d3_] = CRAS_IODEV_STATE_CLOSE;
+
+  // Execute the callback.
   observer_ops->output_mute_changed(NULL, 0, 1, 0);
 
   // d1_ should set mute state through audio_thread_dev_start_ramp.
@@ -747,6 +755,28 @@ TEST_F(IoDevTestSuite, OutputMuteChangedToMute) {
   EXPECT_EQ(2, set_mute_dev_vector.size());
   ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d2_));
   ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d3_));
+
+  // Assume d1_ is changed to no_stream run state
+  // It should not use ramp.
+  cras_iodev_state_ret[&d1_] = CRAS_IODEV_STATE_NO_STREAM_RUN;
+
+  // Clear stub data of interest.
+  audio_thread_dev_start_ramp_dev = NULL;
+  audio_thread_dev_start_ramp_called = 0;
+  set_mute_called = 0;
+  set_mute_dev_vector.clear();
+
+  // Execute the callback.
+  observer_ops->output_mute_changed(NULL, 0, 1, 0);
+
+  // Verify three devices all set mute state right away.
+  EXPECT_EQ(NULL, audio_thread_dev_start_ramp_dev);
+  EXPECT_EQ(0, audio_thread_dev_start_ramp_called);
+  EXPECT_EQ(3, set_mute_called);
+  EXPECT_EQ(3, set_mute_dev_vector.size());
+  ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d1_));
+  ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d2_));
+  ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d3_));
 }
 
 // Test output_mute_changed callback.
@@ -766,6 +796,12 @@ TEST_F(IoDevTestSuite, OutputMuteChangedToUnmute) {
   cras_iodev_list_enable_dev(&d1_);
   cras_iodev_list_enable_dev(&d2_);
 
+  // Assume d1 and d2 devices are in normal run.
+  cras_iodev_state_ret[&d1_] = CRAS_IODEV_STATE_NORMAL_RUN;
+  cras_iodev_state_ret[&d2_] = CRAS_IODEV_STATE_NORMAL_RUN;
+  cras_iodev_state_ret[&d3_] = CRAS_IODEV_STATE_CLOSE;
+
+  // Execute the callback.
   observer_ops->output_mute_changed(NULL, 0, 0, 0);
 
   // d1_ should set mute state through audio_thread_dev_start_ramp.
@@ -781,6 +817,28 @@ TEST_F(IoDevTestSuite, OutputMuteChangedToUnmute) {
   EXPECT_EQ(2, set_mute_dev_vector.size());
   ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d2_));
   ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d3_));
+
+  // Assume d1_ is changed to no_stream run state
+  // It should not use ramp.
+  cras_iodev_state_ret[&d1_] = CRAS_IODEV_STATE_NO_STREAM_RUN;
+
+  // Clear stub data of interest.
+  audio_thread_dev_start_ramp_dev = NULL;
+  audio_thread_dev_start_ramp_called = 0;
+  set_mute_called = 0;
+  set_mute_dev_vector.clear();
+
+  // Execute the callback.
+  observer_ops->output_mute_changed(NULL, 0, 1, 0);
+
+  // Verify three devices all set mute state right away.
+  EXPECT_EQ(NULL, audio_thread_dev_start_ramp_dev);
+  EXPECT_EQ(0, audio_thread_dev_start_ramp_called);
+  EXPECT_EQ(3, set_mute_called);
+  EXPECT_EQ(3, set_mute_dev_vector.size());
+  ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d1_));
+  ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d2_));
+  ASSERT_TRUE(device_in_vector(set_mute_dev_vector, &d3_));
 }
 
 // Test enable/disable an iodev.
@@ -1355,6 +1413,11 @@ int cras_iodev_set_mute(struct cras_iodev* iodev) {
   return 0;
 }
 
+enum CRAS_IODEV_STATE cras_iodev_state(const struct cras_iodev *iodev)
+{
+	return cras_iodev_state_ret[iodev];
+}
+
 struct stream_list *stream_list_create(stream_callback *add_cb,
                                        stream_callback *rm_cb,
                                        stream_create_func *create_cb,