Fix MediaSourcePlayer unittests and minor code cleanup.

Fix MediaSourcePlayer unittests and minor code cleanup.

Fixes a few tests were broken by minor changes in behavior 
introduced in http://crrev.com/220164.

BUG=281730
TEST=MediaSourcePlayerTest.StartTimeTicksResetAfterDecoderUnderruns
     and MediaSourcePlayerTest.ReadFromDemuxerAfterSeek pass now.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221781

[acolwell GONE FROM CHROMIUM, 2013-08-30 18:27:38]

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (left):

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:155: ClearDecodingData();
Not needed since the jobs are getting Release()'ed on the following lines.

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:402: if (is_clock_manager)
The starvation callback gets scheduled in the video-only case, but wasn't
getting cancelled. This just makes the code consistant about the stream that
schedules the callback below also cancels it.

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:603: timeout = std::max(timeout,
kMinStarvationTimeout);
This fixes a bug that was in the code where it would schedule 0ms starvation
callbacks at the beginning of playback because the current time position hadn't
moved forward yet. This would trigger unnecessary prefetching.

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player_unittest.cc:282:
player_->ReadFromDemuxerAck(data);
The new implementation always expects to hear back from the demuxer. The old
implementation didn't wait for pending reads to complete, which I believe could
have triggered buggy behavior if multiple seeks were in flight.

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player_unittest.cc:458:
player_->ReadFromDemuxerAck(CreateReadFromDemuxerAckForAudio(3));
The new implementation has to complete the pending decode before it is able to
prefetch. In the case of this test the decoder job is already waiting for a read
so technically the prefetch could be fulfilled by that pending read. For now I
have just updated the test to capture the current behavior. I plan to follow up
this change with one that tries to avoid the need for an extra read for the
prefetch.

[xhwang, 2013-08-30 19:50:32]

look good to me. But I'd like qinmin@ to take a look as he's more familiar with
these tests.

[qinmin, 2013-08-30 22:27:43]

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (left):

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:413: if (decode_status ==
MediaDecoderJob::DECODE_SUCCEEDED &&
why changing the order of these 2 if statement? 
First we have to update the clock, then determine whether media ends or not.
If we check the latter first, then the displayed time might be incorrect.

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player_unittest.cc:282:
player_->ReadFromDemuxerAck(data);
Previously, if a ReadFromDemuxerAck comes back while seek ack has not arrived,
we ignore the ReadFromDemuxerAck. Since read and seek are sent to the same
thread in render process, the ack should also come back in order, unless
MediaSourceDelegate won't cancel a read when a seek is requested.
If the new behavior requires that we always wait for ReadFromDemuxerAck before
we can send out a seek， it seems that we should remove the seek_request_id, if
all the seeks are acked

On 2013/08/30 18:27:38, acolwell wrote:
> The new implementation always expects to hear back from the demuxer. The old
> implementation didn't wait for pending reads to complete, which I believe
could
> have triggered buggy behavior if multiple seeks were in flight.

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player_unittest.cc:453: DCHECK(StartTimeTicks()
!= base::TimeTicks());
EXPECT_TRUE？ Same for all the DCHECKs below.

[acolwell GONE FROM CHROMIUM, 2013-09-06 00:34:55]

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (left):

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:413: if (decode_status ==
MediaDecoderJob::DECODE_SUCCEEDED &&
On 2013/08/30 22:27:43, qinmin wrote:
> why changing the order of these 2 if statement? 
> First we have to update the clock, then determine whether media ends or not.
> If we check the latter first, then the displayed time might be incorrect.

The blocks are mutually exclusive. decode status can't be DECODE_SUCCEEDED and
DECODE_OUTPUT_END_OF_STREAM at the same time. I figured it was better to get the
end of stream early return path earlier in the function.

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player_unittest.cc:282:
player_->ReadFromDemuxerAck(data);
On 2013/08/30 22:27:43, qinmin wrote:
> Previously, if a ReadFromDemuxerAck comes back while seek ack has not arrived,
> we ignore the ReadFromDemuxerAck. Since read and seek are sent to the same
> thread in render process, the ack should also come back in order, unless
> MediaSourceDelegate won't cancel a read when a seek is requested.
> If the new behavior requires that we always wait for ReadFromDemuxerAck before
> we can send out a seek， it seems that we should remove the seek_request_id, if
> all the seeks are acked
> 
> On 2013/08/30 18:27:38, acolwell wrote:
> > The new implementation always expects to hear back from the demuxer. The old
> > implementation didn't wait for pending reads to complete, which I believe
> could
> > have triggered buggy behavior if multiple seeks were in flight.

Unfortunately, I can't remove the ID yet. Multiple seeks can still be in flight.
This only handles the case where the seek occurs while the initial prefetch is
happening. If the prefetch is done, but a second seek is requested before a
previous one completes, we still need to use the ID to sort out which one is the
"last" one. I believe Matt is working on a CL that will make the multiple
pending seek situation go away, but until that happens we still need the id.

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player_unittest.cc:453: DCHECK(StartTimeTicks()
!= base::TimeTicks());
On 2013/08/30 22:27:43, qinmin wrote:
> EXPECT_TRUE？ Same for all the DCHECKs below.

Done.

[qinmin, 2013-09-06 00:56:33]

lgtm % comment

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
File media/base/android/media_source_player.cc (left):

https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
media/base/android/media_source_player.cc:413: if (decode_status ==
MediaDecoderJob::DECODE_SUCCEEDED &&
Looks like this is a bug. When decode status is DECODE_OUTPUT_END_OF_STREAM, we
should also update the timestamp. output EOS can come with valid audio data in
the same dequeueOutputBuffer() call.
We can leave this in a separate CL though.


On 2013/09/06 00:34:55, acolwell wrote:
> On 2013/08/30 22:27:43, qinmin wrote:
> > why changing the order of these 2 if statement? 
> > First we have to update the clock, then determine whether media ends or not.
> > If we check the latter first, then the displayed time might be incorrect.
> 
> The blocks are mutually exclusive. decode status can't be DECODE_SUCCEEDED and
> DECODE_OUTPUT_END_OF_STREAM at the same time. I figured it was better to get
the
> end of stream early return path earlier in the function.

[acolwell GONE FROM CHROMIUM, 2013-09-06 17:12:58]

On 2013/09/06 00:56:33, qinmin wrote:
> lgtm % comment
> 
>
https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
> File media/base/android/media_source_player.cc (left):
> 
>
https://codereview.chromium.org/23620012/diff/1/media/base/android/media_sour...
> media/base/android/media_source_player.cc:413: if (decode_status ==
> MediaDecoderJob::DECODE_SUCCEEDED &&
> Looks like this is a bug. When decode status is DECODE_OUTPUT_END_OF_STREAM,
we
> should also update the timestamp. output EOS can come with valid audio data in
> the same dequeueOutputBuffer() call.
> We can leave this in a separate CL though.

Ok. I'll fix this in a follow-up CL

[commit-bot: I haz the power, 2013-09-06 17:13:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/23620012/14001

[commit-bot: I haz the power, 2013-09-06 17:19:56]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-09-06 17:20:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/23620012/30001

[commit-bot: I haz the power, 2013-09-06 21:14:43]

Change committed as 221781