CRAS: audio_thread - Fill one min_cb_level of zeros before start

Currently, we fill samples to device starting at buffer level 0. This
is prone to underrun if the sample of next cycle comes late. To avoid
this we should fill 1 min_cb_level of zeros.
Add the diagram of state transition of an output device.
Also, move the dev_running check out of cras_iodev_no_stream_playback so
the state transition can be more readable.

Do cras_alsa_attemp_resume in dev_running of alsa_io. This is to make
sure dev_running does not return 0 while device is later resumed by
other cras_alsa_attemp_resume call in cras_alsa_helpers.

BUG=chromium:519942,chromium:623514
TEST=switch headphone/speaker and see there is no underrun.
TEST=make check

Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
Change-Id: I0319be98a03cc7aa10bfddcd6e679ba3d8d15424
Reviewed-on: https://chromium-review.googlesource.com/356875
Reviewed-by: Dylan Reid <dgreid@chromium.org>

6 files changed, 90 insertions, 26 deletions

diff --git a/cras/src/server/audio_thread.c b/cras/src/server/audio_thread.c
index 58c4041d..c8255a7d 100644
--- a/cras/src/server/audio_thread.c
+++ b/cras/src/server/audio_thread.c
@@ -1116,6 +1116,42 @@ static void update_estimated_rate(struct audio_thread *thread,
 	}
 }
 
+/* Output device state transition diagram:
+ *
+ *                          ----------------
+ *                          | S0  Closed   |<-------------.
+ *                          ----------------              |
+ *      iodev_list enables     |                          |
+ *      device and adds to     |                          |
+ *      audio_thread           V                          | iodev_list removes
+ *                          ----------------              | device from
+ *                          | S1  Open     |              | audio_thread and
+ *                          ----------------              | closes device
+ *                             |                          |
+ *          Sample is ready    |                          |
+ *                             V                          |
+ *                          ----------------              |
+ *                          | S2  Normal   |              |
+ *                          ----------------              |
+ *                             |     ^                    |
+ *        There is no stream   |     | Sample is ready    |
+ *                             V     |                    |
+ *                          ----------------              |
+ *                          | S3 No Stream |--------------
+ *                          ----------------
+ *                            ^  |     |  ^
+ *   (a) There is no stream   |  |     |  | (b) There is stream but sample is
+ *                            |  |     |  |     not ready yet
+ *                             --       --
+ *
+ *  Device in open_devs can be in one of S1, S2, S3.
+ *
+ *            dev_running         no_stream_state
+ *      S1        0                      0
+ *      S2        1                      0
+ *      S3        1                      1
+ */
+
 /* Returns 0 on success negative error on device failure. */
 static int write_output_samples(struct audio_thread *thread,
 				struct open_dev *adev)
@@ -1130,21 +1166,32 @@ static int write_output_samples(struct audio_thread *thread,
 	struct cras_audio_area *area = NULL;
 	int is_running;
 
-	/* Let device handle playback when there is no stream. */
-	if (!odev->streams)
+	is_running = odev->dev_running(odev);
+
+	/* S2 => S3 and S3 => S3 (a):
+	 * Let device handle playback when there is no stream. */
+	if (is_running && !odev->streams)
 		return cras_iodev_no_stream_playback(odev, 1);
 
 	if (odev->no_stream_state) {
-		/* If there is sample ready in dev_stream, let device resume
+		/* S3 => S2:
+		 * If there is sample ready in dev_stream, let device resume
 		 * normal playback from no stream playback state if needed. */
 		if (dev_playback_frames(thread, adev)) {
 			cras_iodev_no_stream_playback(odev, 0);
 		} else {
-			/* If device is in no stream state, and the sample of
+			/* S3 => S3 (b):
+			 * If device is in no stream state, and the sample of
 			 * the newly attached stream is not ready yet,
 			 * keep device in no stream playback. */
 			return cras_iodev_no_stream_playback(odev, 1);
 		}
+	} else if (!is_running && dev_playback_frames(thread, adev)) {
+		/* S1 => S2:
+		 * If device is not started yet, and there is sample ready from
+		 * stream, fill 1 min_cb_level of zeros first and fill sample
+		 * from stream later. */
+		cras_iodev_fill_odev_zeros(odev, odev->min_cb_level);
 	}
 
 	rc = cras_iodev_frames_queued(odev);
@@ -1195,8 +1242,6 @@ static int write_output_samples(struct audio_thread *thread,
 		total_written += written;
 	}
 
-	is_running = odev->dev_running(odev);
-
 	/* In the case where we have written samples or there are samples in
 	 * device, start it if needed. */
 	if (total_written || hw_level) {
diff --git a/cras/src/server/cras_alsa_io.c b/cras/src/server/cras_alsa_io.c
index df8e9c12..b75b44b3 100644
--- a/cras/src/server/cras_alsa_io.c
+++ b/cras/src/server/cras_alsa_io.c
@@ -306,6 +306,17 @@ static int dev_running(const struct cras_iodev *iodev)
 	struct alsa_io *aio = (struct alsa_io *)iodev;
 	snd_pcm_t *handle = aio->handle;
 
+	/* If device is suspended, resume the device to its previous state.
+	 * Otherwise, we might get 0 from dev_running but the device is
+	 * resumed later by other cras_alsa_attempt_resume call in
+	 * cras_alsa_helpers. */
+	if (snd_pcm_state(handle) == SND_PCM_STATE_SUSPENDED) {
+		/* If cras_alsa_attempt_resume really fails, let it be handled
+		 * in later cras_alsa_attempt_resume calls because it is not
+		 * clean to let the user of dev_running to handle it. */
+		cras_alsa_attempt_resume(handle);
+	}
+
 	return snd_pcm_state(handle) == SND_PCM_STATE_RUNNING;
 }
 
diff --git a/cras/src/server/cras_iodev.c b/cras/src/server/cras_iodev.c
index 6d504ec9..44fb850b 100644
--- a/cras/src/server/cras_iodev.c
+++ b/cras/src/server/cras_iodev.c
@@ -847,9 +847,6 @@ int cras_iodev_no_stream_playback(struct cras_iodev *odev, int enable)
 	if (odev->direction != CRAS_STREAM_OUTPUT)
 		return -EINVAL;
 
-	if (!odev->dev_running(odev))
-		return 0;
-
 	rc = odev->no_stream(odev, enable);
 	if (rc < 0)
 		return rc;
diff --git a/cras/src/tests/alsa_io_unittest.cc b/cras/src/tests/alsa_io_unittest.cc
index 11d4bfc9..f93d9292 100644
--- a/cras/src/tests/alsa_io_unittest.cc
+++ b/cras/src/tests/alsa_io_unittest.cc
@@ -732,6 +732,26 @@ TEST(AlsaIoInit, ResumeDevice) {
   alsa_iodev_destroy(iodev);
 }
 
+TEST(AlsaIoInit, ResumeDeviceInDevRunning) {
+  struct cras_iodev *iodev;
+  int rc;
+
+  ResetStubData();
+  iodev = alsa_iodev_create(0, test_card_name, 0, test_dev_name,
+                            NULL, ALSA_CARD_TYPE_INTERNAL, 0,
+                            NULL, NULL, fake_hctl,
+                            CRAS_STREAM_OUTPUT, 0, 0);
+  ASSERT_EQ(0, alsa_iodev_legacy_complete_init(iodev));
+
+  // Attempt to resume if the device is suspended.
+  snd_pcm_state_ret = SND_PCM_STATE_SUSPENDED;
+  rc = iodev->dev_running(iodev);
+  EXPECT_EQ(0, rc);
+  EXPECT_EQ(1, cras_alsa_attempt_resume_called);
+
+  alsa_iodev_destroy(iodev);
+}
+
 TEST(AlsaIoInit, DspNameDefault) {
   struct alsa_io *aio;
   struct cras_alsa_mixer * const fake_mixer = (struct cras_alsa_mixer*)2;
diff --git a/cras/src/tests/audio_thread_unittest.cc b/cras/src/tests/audio_thread_unittest.cc
index 4552dd17..954d13f7 100644
--- a/cras/src/tests/audio_thread_unittest.cc
+++ b/cras/src/tests/audio_thread_unittest.cc
@@ -418,6 +418,9 @@ TEST_F(StreamDeviceSuite, WriteOutputSamplesNoStream) {
   thread_add_open_dev(thread_, &iodev);
   adev = thread_->open_devs[CRAS_STREAM_OUTPUT];
 
+  // Assume device is started.
+  dev_running_ret = 1;
+
   // cras_iodev should handle no stream playback.
   write_output_samples(thread_, adev);
   EXPECT_EQ(1, cras_iodev_no_stream_playback_called);
@@ -490,10 +493,14 @@ TEST_F(StreamDeviceSuite, WriteOutputSamplesWithStream) {
   write_output_samples(thread_, adev);
   EXPECT_EQ(0, cras_iodev_start_called);
 
-  // Assume there are 100 samples written in this cycle.
+  // Assume sample from streams are ready.
+  dev_stream_playback_frames_ret = 100;
+  // Assume there are 100 samples from streams written in this cycle.
   cras_iodev_all_streams_written_ret = 100;
   write_output_samples(thread_, adev);
   EXPECT_EQ(1, cras_iodev_start_called);
+  // Fill min_cb_level of zeros before samples from streams.
+  EXPECT_EQ(iodev.min_cb_level, cras_iodev_fill_odev_zeros_frames);
   EXPECT_EQ(cras_iodev_all_streams_written_ret,
             cras_iodev_put_output_buffer_nframes);
 
diff --git a/cras/src/tests/iodev_unittest.cc b/cras/src/tests/iodev_unittest.cc
index e39e6ba8..8bcbbbd1 100644
--- a/cras/src/tests/iodev_unittest.cc
+++ b/cras/src/tests/iodev_unittest.cc
@@ -990,22 +990,6 @@ TEST(IoDev, FillZeros) {
   EXPECT_EQ(0, rc);
 }
 
-TEST(IoDev, NoStreamPlaybackNotRunning) {
-  struct cras_iodev iodev;
-  int rc;
-
-  memset(&iodev, 0, sizeof(iodev));
-
-  ResetStubData();
-
-  iodev.dev_running = dev_running;
-
-  // Device is not running. No need to fill zeros.
-  rc = cras_iodev_no_stream_playback(&iodev, 1);
-  EXPECT_EQ(0, rc);
-  EXPECT_EQ(0, put_buffer_nframes);
-}
-
 TEST(IoDev, NoStreamPlaybackRunning) {
   struct cras_iodev iodev;
   struct cras_audio_format fmt;