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

media/base/android/media_source_player_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "media/base/android/media_codec_bridge.h"
 #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"
 #include "ui/gl/android/surface_texture.h"
 
 namespace media {
 
 // Helper macro to skip the test if MediaCodecBridge isn't available.
 #define SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE() \
   do { \
@@ skipping 111 matching lines @@
 
   DISALLOW_COPY_AND_ASSIGN(MockDemuxerAndroid);
 };
 
 class MediaSourcePlayerTest : public testing::Test {
  public:
   MediaSourcePlayerTest()
       : 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() {}
 
  protected:
   // Get the decoder job from the MediaSourcePlayer.
   MediaDecoderJob* GetMediaDecoderJob(bool is_audio) {
     if (is_audio) {
       return reinterpret_cast<MediaDecoderJob*>(
           player_.audio_decoder_job_.get());
     }
     return reinterpret_cast<MediaDecoderJob*>(
         player_.video_decoder_job_.get());
   }
 
   // Get the per-job prerolling status from the MediaSourcePlayer's job matching
   // |is_audio|. Caller must guard against NPE if the player's job is NULL.
   bool IsPrerolling(bool is_audio) {
     return GetMediaDecoderJob(is_audio)->prerolling();
   }
 
   // Get the preroll timestamp from the MediaSourcePlayer.
   base::TimeDelta GetPrerollTimestamp() {
     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;
     configs.audio_channels = 2;
     configs.audio_sampling_rate = 44100;
     configs.is_audio_encrypted = false;
     configs.duration_ms = kDefaultDurationInMs;
     scoped_refptr<DecoderBuffer> buffer = ReadTestDataFile("vorbis-extradata");
     configs.audio_extra_data = std::vector<uint8>(
         buffer->data(),
@@ skipping 80 matching lines @@
   }
 
   DemuxerData CreateAbortedAck(bool is_audio) {
     DemuxerData data;
     data.type = is_audio ? DemuxerStream::AUDIO : DemuxerStream::VIDEO;
     data.access_units.resize(1);
     data.access_units[0].status = DemuxerStream::kAborted;
     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.
   void SeekPlayer(bool is_audio, const base::TimeDelta& seek_time) {
     EXPECT_TRUE(GetMediaDecoderJob(is_audio));
 
     int original_num_seeks = demuxer_->num_seek_requests();
     int original_num_data_requests = demuxer_->num_data_requests();
 
     // Initiate a seek. Skip the round-trip of requesting seek from renderer.
@@ skipping 49 matching lines @@
         demuxer_->num_browser_seek_requests();
 
     EXPECT_FALSE(GetMediaDecoderJob(false));
     CreateNextTextureAndSetVideoSurface();
     StartVideoDecoderJob();
     EXPECT_TRUE(GetMediaDecoderJob(false));
     expected_num_data_requests++;
     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());
       EXPECT_FALSE(GetMediaDecoderJob(false));
       EXPECT_FALSE(player_.IsPlaying());
       EXPECT_EQ(expected_num_seek_requests, demuxer_->num_seek_requests());
       EXPECT_EQ(expected_num_data_requests, demuxer_->num_data_requests());
 
       CreateNextTextureAndSetVideoSurface();
       player_.Start();
@@ skipping 111 matching lines @@
   }
 
   bool IsTypeSupported(const std::vector<uint8>& scheme_uuid,
                        const std::string& security_level,
                        const std::string& container,
                        const std::vector<std::string>& codecs) {
     return MediaSourcePlayer::IsTypeSupported(
         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
   // their previous one. In CreateNextTextureAndSetVideoSurface(), we toggle
   // between two surface textures, only replacing the N-2 texture. Assumption is
   // that no more than N-1 texture is in use by decoder when
   // CreateNextTextureAndSetVideoSurface() is called.
   scoped_refptr<gfx::SurfaceTexture> surface_texture_a_;
   scoped_refptr<gfx::SurfaceTexture> surface_texture_b_;
   bool surface_texture_a_is_next_;
@@ skipping 418 matching lines @@
 
 TEST_F(MediaSourcePlayerTest, DemuxerDataArrivesAfterRelease) {
   SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
 
   // Test that the decoder should not crash if demuxer data arrives after
   // Release().
   StartAudioDecoderJob();
   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.
   EXPECT_FALSE(player_.IsPlaying());
   EXPECT_EQ(1, demuxer_->num_data_requests());
 
   // No seek requests should have occurred.
   EXPECT_EQ(0, demuxer_->num_seek_requests());
 }
 
@@ skipping 31 matching lines @@
 TEST_F(MediaSourcePlayerTest, NoSeekForInitialReleaseAndStart) {
   SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
 
   // Test that no seek is requested if player Release() + Start() occurs prior
   // to receiving any data.
   CreateNextTextureAndSetVideoSurface();
   StartVideoDecoderJob();
   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();
 
   player_.Start();
 
   // TODO(wolenetz/qinmin): Multiple in-flight data requests for same stream
   // should be prevented. See http://crbug.com/306314.
   EXPECT_EQ(2, demuxer_->num_data_requests());
 
   EXPECT_EQ(0, demuxer_->num_seek_requests());
   EXPECT_TRUE(GetMediaDecoderJob(false));
 }
 
 TEST_F(MediaSourcePlayerTest, BrowserSeek_MidStreamReleaseAndStart) {
   SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
 
   // 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.
   player_.OnDemuxerSeekDone(base::TimeDelta());
   EXPECT_TRUE(GetMediaDecoderJob(false));
   EXPECT_EQ(2, demuxer_->num_data_requests());
   EXPECT_EQ(1, demuxer_->num_seek_requests());
 }
 
 TEST_F(MediaSourcePlayerTest, PrerollAudioAfterSeek) {
@@ skipping 133 matching lines @@
     if (i == 1) {
       // While still prerolling, Release() and Start() the player.
       // TODO(qinmin): Simulation of multiple in-flight data requests (one from
       // before Release(), one from after Start()) is not included here, and
       // neither is any data enqueued for later decode if it arrives after
       // Release() and before Start(). See http://crbug.com/306314. Assumption
       // for this test, to prevent flakiness until the bug is fixed, is the
       // 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));
       StartAudioDecoderJob();
       EXPECT_TRUE(GetMediaDecoderJob(true));
     } else {
       player_.OnDemuxerDataAvailable(data);
       EXPECT_TRUE(GetMediaDecoderJob(true)->is_decoding());
       message_loop_.Run();
     }
@@ skipping 218 matching lines @@
   player_.OnDemuxerConfigsAvailable(CreateVideoDemuxerConfigs());
   MediaDecoderJob* second_job = GetMediaDecoderJob(false);
   EXPECT_NE(first_job, second_job);
   EXPECT_TRUE(second_job);
 
   EXPECT_EQ(3, demuxer_->num_data_requests());
   EXPECT_EQ(1, demuxer_->num_config_requests());
   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()) {
     LOG(INFO) << "Could not run test - not supported on device.";
     return;
   }
 
   uint8 kWidevineUUID[] = { 0xED, 0xEF, 0x8B, 0xA9, 0x79, 0xD6, 0x4A, 0xCE,
                             0xA3, 0xC8, 0x27, 0xDC, 0xD5, 0x1D, 0x21, 0xED };
 
@@ skipping 54 matching lines @@
 
   std::vector<std::string> codec_avc(1, "avc1");
   EXPECT_FALSE(IsTypeSupported(invalid_uuid, "L3", kVideoMp4, codec_avc));
   EXPECT_FALSE(IsTypeSupported(invalid_uuid, "L1", kVideoMp4, codec_avc));
 }
 
 // TODO(xhwang): Are these IsTypeSupported tests device specific?
 // TODO(xhwang): Add more IsTypeSupported tests.
 
 }  // namespace media