Abort MSP::OnPrefetchDone() if just after MSP::Release(). Let seek and config change survive.

media/base/android/media_source_player_unittest.cc

Index: media/base/android/media_source_player_unittest.cc
diff --git a/media/base/android/media_source_player_unittest.cc b/media/base/android/media_source_player_unittest.cc
index e329ac6eee6dc529fc3f4f6f46c40146036f6f21..e3f0064049ddb82824fbfc5efa0dc8a15ebd9810 100644
--- a/media/base/android/media_source_player_unittest.cc
+++ b/media/base/android/media_source_player_unittest.cc
@@ -11,6 +11,7 @@
 #include "media/base/android/media_drm_bridge.h"
 #include "media/base/android/media_player_manager.h"
 #include "media/base/android/media_source_player.h"
+#include "media/base/bind_to_loop.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/test_data_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -142,6 +143,7 @@ class MediaSourcePlayerTest : public testing::Test {
       : manager_(&message_loop_),
         demuxer_(new MockDemuxerAndroid(&message_loop_)),
         player_(0, &manager_, scoped_ptr<DemuxerAndroid>(demuxer_)),
+        decoder_callback_hook_executed_(false),
         surface_texture_a_is_next_(true) {}
   virtual ~MediaSourcePlayerTest() {}
 
@@ -167,6 +169,36 @@ class MediaSourcePlayerTest : public testing::Test {
     return player_.preroll_timestamp_;
   }
 
+  // Simulate player has reached starvation timeout.
+  void TriggerPlayerStarvation() {
+    player_.decoder_starvation_callback_.Cancel();
+    player_.OnDecoderStarved();
+  }
+
+  // Release() the player.
+  void ReleasePlayer() {
+    EXPECT_TRUE(player_.IsPlaying());
+    player_.Release();
+    EXPECT_FALSE(player_.IsPlaying());
+    EXPECT_FALSE(GetMediaDecoderJob(true));
+    EXPECT_FALSE(GetMediaDecoderJob(false));
+  }
+
+  void SetTestDecodeCallback(const base::Closure& test_decode_cb) {
+    player_.set_decode_callback_for_testing(test_decode_cb);

[acolwell GONE FROM CHROMIUM, 2013/10/31 17:54:00]

nit: This is only called in one location. Please just inline at the call site.

[wolenetz, 2013/11/01 20:45:28]

I did this to keep MSP::set_decode_callback_for_testing() private. The TEST_F
bodies do not have access to private MSP members. From our chat, I will keep
this test decode callback helper and move the binding setup into it from the
only test that uses it, below.
I'll also add warnings here and in MSP.h to help prevent usage creep of the
test-only MSP method.

+  }
+
+  // Asynch test callback posted upon decode completion to verify that a pending
+  // prefetch done event is cleared across |player_|'s Release(). This helps
+  // ensure the ReleaseWithOnPrefetchDoneAlreadyPosted test scenario is met.
+  void ReleaseWithPendingPrefetchDoneVerification() {
+    EXPECT_TRUE(player_.IsEventPending(player_.PREFETCH_DONE_EVENT_PENDING));
+    ReleasePlayer();
+    EXPECT_FALSE(player_.IsEventPending(player_.PREFETCH_DONE_EVENT_PENDING));
+    EXPECT_FALSE(decoder_callback_hook_executed_);
+    decoder_callback_hook_executed_ = true;
+  }
+
   DemuxerConfigs CreateAudioDemuxerConfigs() {
     DemuxerConfigs configs;
     configs.audio_codec = kCodecVorbis;
@@ -267,6 +299,30 @@ class MediaSourcePlayerTest : public testing::Test {
     return data;
   }
 
+  // Helper method for use at test start. It starts an audio decoder job and
+  // immediately feeds it some data to decode. Then, without letting the decoder
+  // job complete a decode cycle, it also starts player SeekTo(). Upon return,
+  // the player should not yet have sent the DemuxerSeek IPC request, though
+  // seek event should be pending. The audio decoder job will also still be
+  // decoding.
+  void StartAudioDecoderJobAndSeekToWhileDecoding(
+      const base::TimeDelta& seek_time) {
+    EXPECT_FALSE(GetMediaDecoderJob(true));
+    EXPECT_FALSE(player_.IsPlaying());
+    EXPECT_EQ(0, demuxer_->num_data_requests());
+    EXPECT_EQ(0.0, GetPrerollTimestamp().InMillisecondsF());
+    EXPECT_EQ(player_.GetCurrentTime(), GetPrerollTimestamp());
+    StartAudioDecoderJob();
+    EXPECT_TRUE(GetMediaDecoderJob(true));
+    EXPECT_FALSE(GetMediaDecoderJob(true)->is_decoding());
+    player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(0));
+    EXPECT_TRUE(GetMediaDecoderJob(true)->is_decoding());
+    player_.SeekTo(seek_time);
+    EXPECT_EQ(0.0, GetPrerollTimestamp().InMillisecondsF());
+    EXPECT_EQ(1, demuxer_->num_data_requests());
+    EXPECT_EQ(0, demuxer_->num_seek_requests());
+  }
+
   // Seek, including simulated receipt of |kAborted| read between SeekTo()
   // and OnDemuxerSeekDone(). Use this helper method only when the player
   // already has created the decoder job.
@@ -336,7 +392,7 @@ class MediaSourcePlayerTest : public testing::Test {
     EXPECT_EQ(expected_num_data_requests, demuxer_->num_data_requests());
 
     if (trigger_with_release_start) {
-      player_.Release();
+      ReleasePlayer();
 
       // Simulate demuxer's response to the video data request.
       player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForVideo());
@@ -468,12 +524,14 @@ class MediaSourcePlayerTest : public testing::Test {
         scheme_uuid, security_level, container, codecs);
   }
 
- protected:
   base::MessageLoop message_loop_;
   MockMediaPlayerManager manager_;
   MockDemuxerAndroid* demuxer_;  // Owned by |player_|.
   MediaSourcePlayer player_;
 
+  // Track whether a possibly asynch decoder callback test hook has run.
+  bool decoder_callback_hook_executed_;
+
   // We need to keep the surface texture while the decoder is actively decoding.
   // Otherwise, it may trigger unexpected crashes on some devices. To switch
   // surfaces, tests need to create a new surface texture without releasing
@@ -912,7 +970,7 @@ TEST_F(MediaSourcePlayerTest, DemuxerDataArrivesAfterRelease) {
   EXPECT_EQ(1, demuxer_->num_data_requests());
   EXPECT_TRUE(GetMediaDecoderJob(true));
 
-  player_.Release();
+  ReleasePlayer();
   player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(0));
 
   // The decoder job should have been released.
@@ -964,8 +1022,7 @@ TEST_F(MediaSourcePlayerTest, NoSeekForInitialReleaseAndStart) {
   EXPECT_EQ(1, demuxer_->num_data_requests());
   EXPECT_TRUE(GetMediaDecoderJob(false));
 
-  player_.Release();
-  EXPECT_FALSE(player_.IsPlaying());
+  ReleasePlayer();
 
   // Pass a new non-empty surface.
   CreateNextTextureAndSetVideoSurface();
@@ -986,7 +1043,6 @@ TEST_F(MediaSourcePlayerTest, BrowserSeek_MidStreamReleaseAndStart) {
   // Test that one browser seek is requested if player Release() + Start(), with
   // video data received between Release() and Start().
   BrowserSeekPlayer(true);
-  EXPECT_FALSE(GetMediaDecoderJob(false));
   EXPECT_EQ(1, demuxer_->num_data_requests());
 
   // Simulate browser seek is done and confirm player requests more data.
@@ -1140,7 +1196,7 @@ TEST_F(MediaSourcePlayerTest, PrerollContinuesAcrossReleaseAndStart) {
       // first request's data arrives before Start(). Though that data is not
       // seen by decoder, this assumption allows preroll continuation
       // verification and prevents multiple in-flight data requests.
-      player_.Release();
+      ReleasePlayer();
       player_.OnDemuxerDataAvailable(data);
       message_loop_.RunUntilIdle();
       EXPECT_FALSE(GetMediaDecoderJob(true));
@@ -1379,6 +1435,224 @@ TEST_F(MediaSourcePlayerTest, NewSurfaceWhileChangingConfigs) {
   EXPECT_EQ(0, demuxer_->num_seek_requests());
 }
 
+TEST_F(MediaSourcePlayerTest, ReleaseWithOnPrefetchDoneAlreadyPosted) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that if OnPrefetchDone() had already been posted before and is
+  // executed after Release(), player does not DCHECK.
+  StartAudioDecoderJob();
+
+  // Escape the original prefetch by decoding a single access unit.
+  player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(0));
+  message_loop_.Run();
+
+  // Prime the job with a few more access units, so that a later prefetch,
+  // triggered by starvation to simulate decoder underrun, can trivially
+  // post task to run OnPrefetchDone().
+  player_.OnDemuxerDataAvailable(
+      CreateReadFromDemuxerAckWithConfigChanged(true, 4));
+  EXPECT_TRUE(GetMediaDecoderJob(true) &&
+      GetMediaDecoderJob(true)->is_decoding());
+
+  // Simulate decoder underrun, so trivial prefetch starts while still decoding.
+  // The prefetch and posting of OnPrefetchDone() will not occur until next
+  // MediaDecoderCallBack() occurs.
+  TriggerPlayerStarvation();
+
+  // Upon the next successful decode callback, post a task to Release()

[qinmin, 2013/10/31 04:12:27]

nit: rephrase this. either "release |player_|" or "call Release() on the
|player_|".

[wolenetz, 2013/11/01 20:45:28]

Done.

[wolenetz, 2013/11/01 20:45:28]

Done.

+  // |player_|, such that the trivial OnPrefetchDone() task posting also occurs
+  // and should execute after the Release().
+  SetTestDecodeCallback(media::BindToLoop(

[acolwell GONE FROM CHROMIUM, 2013/10/31 17:54:00]

Do you really need this callback? Can't you just check for the pending prefetch
done in the loop below and trigger the release there? Since you are posting a
task with the callback, it seems like this should be possible. If not, then does
changing the number of AUs you make available above allow you to have better
control over the prefetch so you don't need the callback?

[wolenetz, 2013/11/01 20:45:28]

The problem is that RunUntilIdle() runs too much, and test needs to replicate
the exact scenario from bug 302867 where Release() is posted during the decode
cycle just after entering |PREFETCH_REQUEST_EVENT_PENDING| and just before the
resulting post-task of OnPrefetchDone(). If we tried to just
PostTask(MSP::Release()) immediately after TriggerPlayerStarvation(), there is
no guarantee that decoding has just completed and already posted a task to
MDJ::OnDecodeCompleted()->MSP::MediaDecoderCallback()->ProcessPendingEvents(do
the trivial prefetch+Posttask(OnPrefetchDone barrier). For clarity, I think this
bug is hit when:
1. Decoder job is decoding and has at least one more access unit beyond the one
currently being decoded.
2. Starvation callback is fired. Sets PREFETCH_REQUEST_EVENT_PENDING, but
ProcessPendingEvents() doesn't yet process it because job is still decoding.
3. MDJ::OnDecodeCompleted() task is posted to UI loop from decoder loop.
4. Before the posted task from #3 is run, some other task that calls
MSP::Release() is posted to UI loop.
5. Tasks from 3 and 4 execute on UI loop, resulting in following sequence:
MSP::MediaDecoderCallback(), then MSP::Release(), then MSP::OnPrefetchDone().

Per comment, above, I'll relocate the binding of the specific test callback to a
helper method and add comment that this binding is specifically for replicating
this fragile test scenario that is due to MDJ::Prefetch() sometimes post-tasking
the prefetch done barrier. Ideally, this will all get ripped out (including the
MSP test-only decode hook interface) once prefetch is reworked along with buffer
reworking in http://crbug.com/304234. If MSP::OnPrefetchDone() processing is
reentrant-safe, we could simplify in short-term by not PostTask'ing to the
prefetch done callback in MDJ::Prefetch(), and instead serially Run() it.

+      message_loop_.message_loop_proxy(),
+      base::Bind(
+          &MediaSourcePlayerTest_ReleaseWithOnPrefetchDoneAlreadyPosted_Test::
+              ReleaseWithPendingPrefetchDoneVerification,
+              base::Unretained(this))));
+
+  while (GetMediaDecoderJob(true))
+    message_loop_.RunUntilIdle();
+  EXPECT_TRUE(decoder_callback_hook_executed_);
+  EXPECT_EQ(2, demuxer_->num_data_requests());
+
+  // Player should have no decoder job until after Start().
+  StartAudioDecoderJob();
+  EXPECT_TRUE(GetMediaDecoderJob(true));
+}
+
+TEST_F(MediaSourcePlayerTest, SeekToThenReleaseThenDemuxerSeekAndDone) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that if Release() occurs after SeekTo(), but the DemuxerSeek IPC
+  // request has not yet been sent, that the seek request will be sent after
+  // Release(). Also, if OnDemuxerSeekDone() occurs prior to next Start(), then
+  // the player will resume correct post-seek preroll upon Start().
+  StartAudioDecoderJobAndSeekToWhileDecoding(
+      base::TimeDelta::FromMilliseconds(100));
+  ReleasePlayer();
+  EXPECT_EQ(1, demuxer_->num_seek_requests());
+
+  player_.OnDemuxerSeekDone(kNoTimestamp());
+  EXPECT_EQ(100.0, GetPrerollTimestamp().InMillisecondsF());
+  EXPECT_FALSE(GetMediaDecoderJob(true));
+  EXPECT_FALSE(player_.IsPlaying());
+
+  // Player should begin prefetch and resume preroll upon Start().
+  EXPECT_EQ(1, demuxer_->num_data_requests());
+  StartAudioDecoderJob();
+  EXPECT_TRUE(GetMediaDecoderJob(true));
+  EXPECT_TRUE(IsPrerolling(true));
+  EXPECT_EQ(100.0, GetPrerollTimestamp().InMillisecondsF());
+  EXPECT_EQ(2, demuxer_->num_data_requests());
+
+  // No further seek should have been requested since Release(), above.
+  EXPECT_EQ(1, demuxer_->num_seek_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, SeekToThenReleaseThenDemuxerSeekThenStart) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that if Release() occurs after SeekTo(), but the DemuxerSeek IPC
+  // request has not yet been sent, that the seek request will be sent after
+  // Release(). Also, if OnDemuxerSeekDone() does not occur until after the next
+  // Start(), then the player remains pending seek done and, after
+  // OnDemuxerSeekDone(), resumes correct post-seek preroll.
+  StartAudioDecoderJobAndSeekToWhileDecoding(
+      base::TimeDelta::FromMilliseconds(100));
+  ReleasePlayer();
+  EXPECT_EQ(1, demuxer_->num_seek_requests());
+
+  // Player should not prefetch upon Start() nor create the decoder job, due to
+  // awaiting DemuxerSeekDone.
+  StartAudioDecoderJob();
+  EXPECT_FALSE(GetMediaDecoderJob(true));
+  EXPECT_EQ(1, demuxer_->num_data_requests());
+
+  player_.OnDemuxerSeekDone(kNoTimestamp());
+  EXPECT_TRUE(GetMediaDecoderJob(true));
+  EXPECT_TRUE(IsPrerolling(true));
+  EXPECT_EQ(100.0, GetPrerollTimestamp().InMillisecondsF());
+  EXPECT_EQ(2, demuxer_->num_data_requests());
+
+  // No further seek should have been requested since Release(), above.
+  EXPECT_EQ(1, demuxer_->num_seek_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenConfigsAvailable) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that if Release() occurs after |kConfigChanged| detected and new
+  // configs requested of demuxer, and the requested configs arrive before
+  // the next Start(), then the player completes the pending config change
+  // processing on their receipt.
+  StartConfigChange(true, true, 0);
+  ReleasePlayer();
+
+  player_.OnDemuxerConfigsAvailable(CreateAudioDemuxerConfigs());
+  EXPECT_FALSE(GetMediaDecoderJob(true));
+  EXPECT_FALSE(player_.IsPlaying());
+  EXPECT_EQ(1, demuxer_->num_data_requests());
+
+  // Player should resume upon Start(), even without further configs supplied.
+  player_.Start();
+  EXPECT_TRUE(GetMediaDecoderJob(true));
+  EXPECT_TRUE(player_.IsPlaying());
+  EXPECT_EQ(2, demuxer_->num_data_requests());
+
+  // No further config request should have occurred since StartConfigChange().
+  EXPECT_EQ(1, demuxer_->num_config_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, ConfigChangedThenReleaseThenStart) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that if Release() occurs after |kConfigChanged| detected and new
+  // configs requested of demuxer, and the requested configs arrive after the
+  // next Start(), then the player pends job creation until the new configs
+  // arrive.
+  StartConfigChange(true, true, 0);
+  ReleasePlayer();
+
+  player_.Start();
+  EXPECT_TRUE(player_.IsPlaying());
+  EXPECT_FALSE(GetMediaDecoderJob(true));
+  EXPECT_EQ(1, demuxer_->num_data_requests());
+
+  player_.OnDemuxerConfigsAvailable(CreateAudioDemuxerConfigs());
+  EXPECT_TRUE(GetMediaDecoderJob(true));
+  EXPECT_EQ(2, demuxer_->num_data_requests());
+
+  // No further config request should have occurred since StartConfigChange().
+  EXPECT_EQ(1, demuxer_->num_config_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, BrowserSeek_ThenReleaseThenDemuxerSeekDone) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that Release() after a browser seek's DemuxerSeek IPC request has been
+  // sent behaves similar to a regular seek: if OnDemuxerSeekDone() arrives
+  // prior to the next Start()+SetVideoSurface(), then the player will resume
+  // correct post-seek preroll upon Start()+SetVideoSurface().
+  // Note, the SeekToThenReleaseThenDemuxerSeek* tests, above, cover the
+  // case where seek request has not yet been sent at time of Release(). Browser
+  // seeks use similar logic, so no extra test coverage is deemed necessary. On
+  // removal of browser seeks, consider retaining a regular seek version of this

[acolwell GONE FROM CHROMIUM, 2013/10/31 17:54:00]

This is confusing to me. Does this mean a new test needs to be created when
browser seek functionality has been removed? If the regular seek & browser seek
behavior should be the same, shouldn't we just test the regular seek version so
that we don't have to add non-browser seek test cases to maintain coverage when
we remove that functionality?

[wolenetz, 2013/11/01 20:45:28]

Oops! My 'Note' comments in these two new BrowserSeek tests were internally
inconsistent. I've removed the 'Notes' and added analogous regular seek cases,
too. BrowserSeek versions are also kept.

+  // test.
+  BrowserSeekPlayer(false);
+  base::TimeDelta expected_preroll_timestamp = player_.GetCurrentTime();
+  ReleasePlayer();
+
+  player_.OnDemuxerSeekDone(expected_preroll_timestamp);
+  EXPECT_FALSE(player_.IsPlaying());
+  EXPECT_FALSE(GetMediaDecoderJob(false));
+  EXPECT_EQ(expected_preroll_timestamp, GetPrerollTimestamp());
+
+  // Player should begin prefetch and resume preroll upon Start().
+  EXPECT_EQ(1, demuxer_->num_data_requests());
+  StartVideoDecoderJob();
+  CreateNextTextureAndSetVideoSurface();
+  EXPECT_TRUE(GetMediaDecoderJob(false));
+  EXPECT_TRUE(IsPrerolling(false));
+  EXPECT_EQ(expected_preroll_timestamp, GetPrerollTimestamp());
+  EXPECT_EQ(expected_preroll_timestamp, player_.GetCurrentTime());
+  EXPECT_EQ(2, demuxer_->num_data_requests());
+
+  // No further seek should have been requested since BrowserSeekPlayer().
+  EXPECT_EQ(1, demuxer_->num_seek_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, BrowserSeek_ThenReleaseThenStart) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that Release() after a browser seek's DemuxerSeek IPC request has been
+  // sent behaves similar to a regular seek: if OnDemuxerSeekDone() does not
+  // occur until after the next Start()+SetVideoSurface(), then the player
+  // remains pending seek done and, after OnDemuxerSeekDone(), resumes correct
+  // post-seek preroll.
+  // Note, the SeekToThenReleaseThenDemuxerSeek* tests, above, cover the
+  // case where seek request has not yet been sent at time of Release(). Browser
+  // seeks use similar logic, so no extra test coverage is deemed necessary. On
+  // removal of browser seeks, consider retaining a regular seek version of this
+  // test.

[acolwell GONE FROM CHROMIUM, 2013/10/31 17:54:00]

ditto

[wolenetz, 2013/11/01 20:45:28]

Done, per above. Thanks!

+  BrowserSeekPlayer(false);
+  base::TimeDelta expected_preroll_timestamp = player_.GetCurrentTime();
+  ReleasePlayer();
+
+  StartVideoDecoderJob();
+  CreateNextTextureAndSetVideoSurface();
+  EXPECT_FALSE(GetMediaDecoderJob(false));
+  EXPECT_EQ(1, demuxer_->num_data_requests());
+
+  player_.OnDemuxerSeekDone(expected_preroll_timestamp);
+  EXPECT_TRUE(GetMediaDecoderJob(false));
+  EXPECT_TRUE(IsPrerolling(false));
+  EXPECT_EQ(expected_preroll_timestamp, GetPrerollTimestamp());
+  EXPECT_EQ(expected_preroll_timestamp, player_.GetCurrentTime());
+  EXPECT_EQ(2, demuxer_->num_data_requests());
+
+  // No further seek should have been requested since BrowserSeekPlayer().
+  EXPECT_EQ(1, demuxer_->num_seek_requests());
+}
+
 // TODO(xhwang): Enable this test when the test devices are updated.
 TEST_F(MediaSourcePlayerTest, DISABLED_IsTypeSupported_Widevine) {
   if (!MediaCodecBridge::IsAvailable() || !MediaDrmBridge::IsAvailable()) {