MediaCodecPlayer implementation (stage 4 - preroll)

MediaCodecPlayer implementation (stage 4 - preroll)

We request the preroll to be done after seek or when decoder is
going to be created anew, i.e. for the first time or after release.

The preroll phase is finished when the frame pts becomes equal
or greater than the preroll timestamp. After that the decoder
suspend itself and reports the state to the player. The player
will resume decoders affer all of them are prerolled.

The first frame that should be rendered is detected during
the preroll phase. For audio we save it on the Java side and
pick it up and play when the next frame comes. For video we
render such frame during the preroll phase.

BUG=407577
Committed: https://crrev.com/17658f61356d6ce6d191f519b0cb13ea31759d0e
Cr-Commit-Position: refs/heads/master@{#344949}

[Tima Vaisburd, 2015-07-28 01:21:41]

timav@chromium.org changed reviewers:
+ qinmin@chromium.org

[Tima Vaisburd, 2015-07-28 01:21:41]

Min, this is the preroll. Please take a look. Unit tests are missing for now.

[qinmin, 2015-07-28 18:09:46]

https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:34: const int kEstimatedFramePeriod =
20;
why we need this variable? If we allow preroll for at pts 0, then we can ask
audio and video decoder to start decoding until they start generating frames.

https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:728: const bool do_render = (state ==
kRunning || state == kStopping);
I think it is ok to render if (state == kPrerolling && preroll_timestamp_ <=
pts)

[Tima Vaisburd, 2015-07-28 18:34:04]

https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:728: const bool do_render = (state ==
kRunning || state == kStopping);
On 2015/07/28 18:09:46, qinmin wrote:
> I think it is ok to render if (state == kPrerolling && preroll_timestamp_ <=
> pts)

I believe you mean "in addition to kRunning and kStopping" : after preroll is
done the state will switch to kRunning and we still need to render the frames.

All these hoops including the estimated frame period was intended to prevent the
situation where we render the first frame after preroll and then wait. According
to what I've seen this would be an audio frame in most cases. So, we will render
the first audio frame, then wait for video to preroll, then resume both. Do you
say that the first audio won't play anyway because audio needs to fill the audio
buffer and it would be ok?

[qinmin, 2015-07-28 18:55:06]

https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:728: const bool do_render = (state ==
kRunning || state == kStopping);
On 2015/07/28 18:34:04, Tima Vaisburd wrote:
> On 2015/07/28 18:09:46, qinmin wrote:
> > I think it is ok to render if (state == kPrerolling && preroll_timestamp_ <=
> > pts)
> 
> I believe you mean "in addition to kRunning and kStopping" : after preroll is
> done the state will switch to kRunning and we still need to render the frames.
> 
> All these hoops including the estimated frame period was intended to prevent
the
> situation where we render the first frame after preroll and then wait.
According
> to what I've seen this would be an audio frame in most cases. So, we will
render
> the first audio frame, then wait for video to preroll, then resume both. Do
you
> say that the first audio won't play anyway because audio needs to fill the
audio
> buffer and it would be ok? 

You can modify the MediaCodecBridge.playOutputBuffer() call to take a param to
buffer this frame instead of playing it, and when the next frame comes,
playoutputbuffer() will automatically start playing.

[Tima Vaisburd, 2015-07-28 23:37:14]

On 2015/07/28 18:55:06, qinmin wrote:
>
https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
> File media/base/android/media_codec_decoder.cc (right):
> 
>
https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
> media/base/android/media_codec_decoder.cc:728: const bool do_render = (state
==
> kRunning || state == kStopping);
> On 2015/07/28 18:34:04, Tima Vaisburd wrote:
> > On 2015/07/28 18:09:46, qinmin wrote:
> > > I think it is ok to render if (state == kPrerolling && preroll_timestamp_
<=
> > > pts)
> > 
> > I believe you mean "in addition to kRunning and kStopping" : after preroll
is
> > done the state will switch to kRunning and we still need to render the
frames.
> > 
> > All these hoops including the estimated frame period was intended to prevent
> the
> > situation where we render the first frame after preroll and then wait.
> According
> > to what I've seen this would be an audio frame in most cases. So, we will
> render
> > the first audio frame, then wait for video to preroll, then resume both. Do
> you
> > say that the first audio won't play anyway because audio needs to fill the
> audio
> > buffer and it would be ok? 
> 
> You can modify the MediaCodecBridge.playOutputBuffer() call to take a param to
> buffer this frame instead of playing it, and when the next frame comes,
> playoutputbuffer() will automatically start playing.

After thinking a while I got an impression that we should always assume that the
AudioTrack.write() might block forever if not playing and should always create a
buffer. We can copy for audio, but what to do with video? Maybe we should delay
the Render() until the preroll is done?

[qinmin, 2015-07-29 18:15:29]

On 2015/07/28 23:37:14, Tima Vaisburd wrote:
> On 2015/07/28 18:55:06, qinmin wrote:
> >
>
https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
> > File media/base/android/media_codec_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
> > media/base/android/media_codec_decoder.cc:728: const bool do_render = (state
> ==
> > kRunning || state == kStopping);
> > On 2015/07/28 18:34:04, Tima Vaisburd wrote:
> > > On 2015/07/28 18:09:46, qinmin wrote:
> > > > I think it is ok to render if (state == kPrerolling &&
preroll_timestamp_
> <=
> > > > pts)
> > > 
> > > I believe you mean "in addition to kRunning and kStopping" : after preroll
> is
> > > done the state will switch to kRunning and we still need to render the
> frames.
> > > 
> > > All these hoops including the estimated frame period was intended to
prevent
> > the
> > > situation where we render the first frame after preroll and then wait.
> > According
> > > to what I've seen this would be an audio frame in most cases. So, we will
> > render
> > > the first audio frame, then wait for video to preroll, then resume both.
Do
> > you
> > > say that the first audio won't play anyway because audio needs to fill the
> > audio
> > > buffer and it would be ok? 
> > 
> > You can modify the MediaCodecBridge.playOutputBuffer() call to take a param
to
> > buffer this frame instead of playing it, and when the next frame comes,
> > playoutputbuffer() will automatically start playing.
> 
> After thinking a while I got an impression that we should always assume that
the
> AudioTrack.write() might block forever if not playing and should always create
a
> buffer. We can copy for audio, but what to do with video? Maybe we should
delay
> the Render() until the preroll is done?

We should not hold the output buffer index. And displaying the frame won't have
any real impact on viewing experience.

[Tima Vaisburd, 2015-07-29 18:29:36]

On 2015/07/29 18:15:29, qinmin wrote:
> On 2015/07/28 23:37:14, Tima Vaisburd wrote:
> > On 2015/07/28 18:55:06, qinmin wrote:
> > >
> >
>
https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
> > > File media/base/android/media_codec_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
> > > media/base/android/media_codec_decoder.cc:728: const bool do_render =
(state
> > ==
> > > kRunning || state == kStopping);
> > > On 2015/07/28 18:34:04, Tima Vaisburd wrote:
> > > > On 2015/07/28 18:09:46, qinmin wrote:
> > > > > I think it is ok to render if (state == kPrerolling &&
> preroll_timestamp_
> > <=
> > > > > pts)
> > > > 
> > > > I believe you mean "in addition to kRunning and kStopping" : after
preroll
> > is
> > > > done the state will switch to kRunning and we still need to render the
> > frames.
> > > > 
> > > > All these hoops including the estimated frame period was intended to
> prevent
> > > the
> > > > situation where we render the first frame after preroll and then wait.
> > > According
> > > > to what I've seen this would be an audio frame in most cases. So, we
will
> > > render
> > > > the first audio frame, then wait for video to preroll, then resume both.
> Do
> > > you
> > > > say that the first audio won't play anyway because audio needs to fill
the
> > > audio
> > > > buffer and it would be ok? 
> > > 
> > > You can modify the MediaCodecBridge.playOutputBuffer() call to take a
param
> to
> > > buffer this frame instead of playing it, and when the next frame comes,
> > > playoutputbuffer() will automatically start playing.
> > 
> > After thinking a while I got an impression that we should always assume that
> the
> > AudioTrack.write() might block forever if not playing and should always
create
> a
> > buffer. We can copy for audio, but what to do with video? Maybe we should
> delay
> > the Render() until the preroll is done?
> 
> We should not hold the output buffer index. And displaying the frame won't
have
> any real impact on viewing experience.

Yes, I tried to implement pending Render and it proved to be not easy.

[Tima Vaisburd, 2015-07-30 18:16:39]

On 2015/07/29 18:29:36, Tima Vaisburd wrote:
> On 2015/07/29 18:15:29, qinmin wrote:
> > On 2015/07/28 23:37:14, Tima Vaisburd wrote:
> > > On 2015/07/28 18:55:06, qinmin wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
> > > > File media/base/android/media_codec_decoder.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1254293003/diff/20001/media/base/android/medi...
> > > > media/base/android/media_codec_decoder.cc:728: const bool do_render =
> (state
> > > ==
> > > > kRunning || state == kStopping);
> > > > On 2015/07/28 18:34:04, Tima Vaisburd wrote:
> > > > > On 2015/07/28 18:09:46, qinmin wrote:
> > > > > > I think it is ok to render if (state == kPrerolling &&
> > preroll_timestamp_
> > > <=
> > > > > > pts)
> > > > > 
> > > > > I believe you mean "in addition to kRunning and kStopping" : after
> preroll
> > > is
> > > > > done the state will switch to kRunning and we still need to render the
> > > frames.
> > > > > 
> > > > > All these hoops including the estimated frame period was intended to
> > prevent
> > > > the
> > > > > situation where we render the first frame after preroll and then wait.
> > > > According
> > > > > to what I've seen this would be an audio frame in most cases. So, we
> will
> > > > render
> > > > > the first audio frame, then wait for video to preroll, then resume
both.
> > Do
> > > > you
> > > > > say that the first audio won't play anyway because audio needs to fill
> the
> > > > audio
> > > > > buffer and it would be ok? 
> > > > 
> > > > You can modify the MediaCodecBridge.playOutputBuffer() call to take a
> param
> > to
> > > > buffer this frame instead of playing it, and when the next frame comes,
> > > > playoutputbuffer() will automatically start playing.
> > > 
> > > After thinking a while I got an impression that we should always assume
that
> > the
> > > AudioTrack.write() might block forever if not playing and should always
> create
> > a
> > > buffer. We can copy for audio, but what to do with video? Maybe we should
> > delay
> > > the Render() until the preroll is done?
> > 
> > We should not hold the output buffer index. And displaying the frame won't
> have
> > any real impact on viewing experience.
> 
> Yes, I tried to implement pending Render and it proved to be not easy.

I found that on Nexus 4 writing to AudioTrack after pause() sometimes truncates
the buffer
and we get a warning at
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android....
I believe this forces us to copy the buffer if we want to guarantee it is always
played.
Moreover, after writing a truncated buffer the reported had position sometimes
becomes larger than our total estimate (another semantics? bug?) and we crash at
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

[Tima Vaisburd, 2015-07-31 01:55:43]

Rewrote with saving the audio buffer and using it later. It seems to work much
better. Please take a look.

[qinmin, 2015-07-31 18:43:11]

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
media/base/android/media_codec_audio_decoder.cc:193:
CreatePendingBuffer(buffer_index, size, pts);
I prefer we just move the pending buffer to java side. Let mediaCodecBridge hold
the buffer instead. In that case, we just need to change the
MediaCodecBridge::PlayerOutputBuffer() to take a new param to buffer the data.

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:279: const bool needs_preroll =
(preroll_timestamp_ != base::TimeDelta());
why not preroll if timestamp is 0? it will help us sync audio and video by
removing the decoder initialization delay.

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:715: preroll_done = true;
why not just return false here.

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:717: } break;
move the break to a new line

[Tima Vaisburd, 2015-07-31 19:26:42]

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:279: const bool needs_preroll =
(preroll_timestamp_ != base::TimeDelta());
On 2015/07/31 18:43:10, qinmin wrote:
> why not preroll if timestamp is 0? it will help us sync audio and video by
> removing the decoder initialization delay.

I think decoder initialization delay has been already taken care of because
StartPlaybackDecoders()
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...)
at first configures both audio and video, and then starts both. If I'm not
missing anything, of course.
My thinking here was that with start_timestamp == 0 there could not be any prior
timestamp and so there is nothing to preroll.

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:715: preroll_done = true;
On 2015/07/31 18:43:10, qinmin wrote:
> why not just return false here.

Done.

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:717: } break;
On 2015/07/31 18:43:10, qinmin wrote:
> move the break to a new line

Done.

[Tima Vaisburd, 2015-08-01 04:34:43]

Min, I moved the pending buffer to the Java side and added a flag to start
prerolling at the very beginning. There are questions though. Please take a
look.

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/40001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:279: const bool needs_preroll =
(preroll_timestamp_ != base::TimeDelta());
On 2015/07/31 19:26:42, Tima Vaisburd wrote:
> On 2015/07/31 18:43:10, qinmin wrote:
> > why not preroll if timestamp is 0? it will help us sync audio and video by
> > removing the decoder initialization delay.
> 
> I think decoder initialization delay has been already taken care of because
> StartPlaybackDecoders()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...)
> at first configures both audio and video, and then starts both. If I'm not
> missing anything, of course.
> My thinking here was that with start_timestamp == 0 there could not be any
prior
> timestamp and so there is nothing to preroll.

Min explained that there is internal codec initialization that happens between
the moment we queue first input buffer till the moment we dequeue first output
buffer and for video codec it can be significant. Therefore after flush and
reconfiguration we need to do preroll event at pts == 0.
Done.

https://codereview.chromium.org/1254293003/diff/80001/media/base/android/medi...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/80001/media/base/android/medi...
media/base/android/media_codec_audio_decoder.cc:208: if (postpone)
In this patch Java side already has the list of pending buffers. I prefer to
keep the list there and remove these checks but I'd like to hear your opinion.

https://codereview.chromium.org/1254293003/diff/80001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1254293003/diff/80001/media/base/android/medi...
media/base/android/media_codec_player.cc:435:
audio_decoder_->SetNeedsPreroll(true);
Would it be better to put needs_preroll = true inside Flush() instead?

[Tima Vaisburd, 2015-08-03 19:44:34]

Added a unit test.

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
media/base/android/media_codec_decoder.cc:31: const int kOutputBufferTimeout =
10;
Upon more detailed examination of unit tests I found that 10 ms is better as it
eliminates the skipping of the frames. However in the tests the frame period is
20 ms (50 fps) which is more demanding than usual 24 or 30 fps.

[qinmin, 2015-08-03 21:25:15]

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
media/base/android/media_codec_audio_decoder.cc:203: // MediaCodecBrdge.java
only keeps one buffer postponed buffer, but we
I don't think this is true with you CL "keeps one buffer postponed buffer"

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
media/base/android/media_codec_decoder.cc:31: const int kOutputBufferTimeout =
10;
On 2015/08/03 19:44:34, Tima Vaisburd wrote:
> Upon more detailed examination of unit tests I found that 10 ms is better as
it
> eliminates the skipping of the frames. However in the tests the frame period
is
> 20 ms (50 fps) which is more demanding than usual 24 or 30 fps.

these values are too small, some devices will always timeout.

[Tima Vaisburd, 2015-08-03 22:49:05]

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
media/base/android/media_codec_audio_decoder.cc:203: // MediaCodecBrdge.java
only keeps one buffer postponed buffer, but we
On 2015/08/03 21:25:15, qinmin wrote:
> I don't think this is true with you CL "keeps one buffer postponed buffer"

I kept only one buffer on Java side instead of an array of buffers, put assert
there and removed DCHECKs here on C++ side.

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/100001/media/base/android/med...
media/base/android/media_codec_decoder.cc:31: const int kOutputBufferTimeout =
10;
On 2015/08/03 21:25:15, qinmin wrote:
> On 2015/08/03 19:44:34, Tima Vaisburd wrote:
> > Upon more detailed examination of unit tests I found that 10 ms is better as
> it
> > eliminates the skipping of the frames. However in the tests the frame period
> is
> > 20 ms (50 fps) which is more demanding than usual 24 or 30 fps.
> 
> these values are too small, some devices will always timeout.

Restored to 20 ms

[Tima Vaisburd, 2015-08-03 22:49:44]

timav@chromium.org changed reviewers:
+ wolenetz@chromium.org

[Tima Vaisburd, 2015-08-03 22:49:45]

Matt, please review this CL.

[wolenetz, 2015-08-04 21:34:21]

My apologies for slow review on this. I'll get to this soon (3rd in my CR queue
currently, after investigating a P1 at the moment.)

[qinmin, 2015-08-05 18:54:02]

https://codereview.chromium.org/1254293003/diff/140001/media/base/android/med...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/140001/media/base/android/med...
media/base/android/media_codec_audio_decoder.cc:204: if (frames_to_play < 0) {
simply DCHECK_GE(frames_to_play, 0) << "......"

[Tima Vaisburd, 2015-08-06 00:12:37]

In order to make preroll work across several stops and resumes during prerolling
phase I had to change the implementation. I moved the check whether we need
preroll to the player to StartPlaybackDecoders() and used barrier closure there.

https://codereview.chromium.org/1254293003/diff/140001/media/base/android/med...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/140001/media/base/android/med...
media/base/android/media_codec_audio_decoder.cc:204: if (frames_to_play < 0) {
On 2015/08/05 18:54:02, qinmin wrote:
> simply DCHECK_GE(frames_to_play, 0) << "......"

Done.

[qinmin, 2015-08-06 23:18:15]

lgtm

[wolenetz, 2015-08-17 19:52:50]

Thanks Tima for patience. I'll begin my CR iterations today on this.

[wolenetz, 2015-08-20 20:37:35]

wolenetz@chromium.org changed reviewers:
+ liberato@chromium.org

[wolenetz, 2015-08-20 20:37:36]

Tima, I have a few questions. And to rebalance the CR workload, I'm delegating
to liberato@ to do the remainder of videostack-eng CR of this.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:728: * 
@param buf Audio buffer to be rendered.
nit: document |postpone|

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:735:
Log.v(TAG, "Saving pending buffer");
nit: Is Log.v too spammy/verbose? I defer to qinmin@ on this (and same in the
other Log.v, below).

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:736: assert
mPendingAudioBuffer == null;
What protects (including any bridge java or .h method documentation) this assert
from firing? For instance, what if mAudioTrack is null the first time and
postpone was incorrectly set true by caller on the second time.. Then this
assert would fire.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:741: if
(mAudioTrack == null) {
Is there really a case where we should allow caching buf for postponed playback,
yet mAudioTrack is null? IOW, should this early-return check be moved prior to
the "if (postpone)", above?

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_audio_decoder.h (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_audio_decoder.h:8: #include <vector>
nit: insert blank line between <> and "" includes.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_bridge.h:238: // head position expressed in
frames.
nit: document postpone, index and size now that this is more complex.

[Tima Vaisburd, 2015-08-20 22:36:36]

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:728: * 
@param buf Audio buffer to be rendered.
On 2015/08/20 20:37:36, wolenetz wrote:
> nit: document |postpone|

Done.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:735:
Log.v(TAG, "Saving pending buffer");
On 2015/08/20 20:37:36, wolenetz wrote:
> nit: Is Log.v too spammy/verbose? I defer to qinmin@ on this (and same in the
> other Log.v, below).

I agree, deleted the two Log.v() lines.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:736: assert
mPendingAudioBuffer == null;
On 2015/08/20 20:37:36, wolenetz wrote:
> What protects (including any bridge java or .h method documentation) this
assert
> from firing? For instance, what if mAudioTrack is null the first time and
> postpone was incorrectly set true by caller on the second time.. Then this
> assert would fire.

I added the documentation. Only the convention that there should be no more than
one postponed buffer in a row. The assert is intended to check this convention.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:741: if
(mAudioTrack == null) {
On 2015/08/20 20:37:36, wolenetz wrote:
> Is there really a case where we should allow caching buf for postponed
playback,
> yet mAudioTrack is null? IOW, should this early-return check be moved prior to
> the "if (postpone)", above?

Done. I agree that mAudioTrack check is better be moved up to preserve the
behavior that we skip the buffers if audio is not configured.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_audio_decoder.h (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_audio_decoder.h:8: #include <vector>
On 2015/08/20 20:37:36, wolenetz wrote:
> nit: insert blank line between <> and "" includes.

Just removed this include, it is not needed.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_bridge.h:238: // head position expressed in
frames.
On 2015/08/20 20:37:36, wolenetz wrote:
> nit: document postpone, index and size now that this is more complex.

Done.

[liberato (no reviews please), 2015-08-20 22:54:09]

mostly nits.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_audio_decoder.cc:77: void
MediaCodecAudioDecoder::ReleaseMediaCodec() {
i don't think that this needs to be overridden.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_decoder.cc:77: ReleaseDecoderResources();
please consider not having virtual methods in the destructor.  if there is
cleanup that can't be neatly sorted out by object, then i think that it's still
better to document and DCHECK that things were done in the right order in the
superclasses.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_decoder.cc:95: ReleaseMediaCodec();
destruction is a bit tangled, with all the virtual functions in there. 
ReleaseMediaCodec is called more than once, for example, during video decoder
destruction via various implementations of ReleaseDecoderResources.

please consider sorting this out so that the cleanup order is clear.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_decoder.cc:183: return HasStream() &&
needs_preroll_ && !completed_;
either |needs_preroll_| or NeedsPreroll() can be better named -- i'd expect
NeedsPreroll() { return needs_preroll_; }

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_video_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_video_decoder.cc:235:
ReleaseOutputBuffer(buffer_index, pts, (size > 0), update_time,
can this set |start_pts_| and break?  if not, then please expand the comment.

[wolenetz, 2015-08-20 23:39:12]

I'll rubber-stamp qinmin@&&liberato@'s lgtm, without my full review of this.

A couple more nits, and a question:
In the CL description, I understand it to state that we request preroll after
decoder release. Does this make sense to do in all cases? Doing preroll right
after decoder release seems strange. Do I misunderstand?

https://codereview.chromium.org/1254293003/diff/220001/media/base/android/med...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/1254293003/diff/220001/media/base/android/med...
media/base/android/media_codec_bridge.h:242: // postpone == false, and if will
play two buffers: the postponed one and
nit: s/if/it/

https://codereview.chromium.org/1254293003/diff/220001/media/base/android/med...
File media/base/android/media_codec_decoder.h (right):

https://codereview.chromium.org/1254293003/diff/220001/media/base/android/med...
media/base/android/media_codec_decoder.h:277: // Returns true is we are in the
process of sync stop.
nit:s/is/if/

[Tima Vaisburd, 2015-08-21 00:27:40]

On 2015/08/20 23:39:12, wolenetz wrote:
> In the CL description, I understand it to state that we request preroll after
> decoder release. Does this make sense to do in all cases? Doing preroll right
> after decoder release seems strange. Do I misunderstand?

After some struggle I rewrite this phrase as
"We request the preroll to be done after seek or when decoder is
going to be created anew, i.e. for the first time or after release".

What I meant is as follows: if requested, preroll is always done
in the beginning of the playback, after prefetch. The request might not happen
at all though, in which case we won't have prefetching phase.
We need the prefetching when we (re)create the decoder, because its
configuration
and initialization (getting input, but not making output) can take time, and
after
the seek because there could be some data to be skipped and there is again
initialization after the flush.

In this respect releasing the codec makes the situation the same as before the
first
creation.

[Tima Vaisburd, 2015-08-21 00:31:22]

On 2015/08/21 00:27:40, Tima Vaisburd wrote:
> On 2015/08/20 23:39:12, wolenetz wrote:
> > In the CL description, I understand it to state that we request preroll
after
> > decoder release. Does this make sense to do in all cases? Doing preroll
right
> > after decoder release seems strange. Do I misunderstand?
> 
> After some struggle I rewrite this phrase as
> "We request the preroll to be done after seek or when decoder is
> going to be created anew, i.e. for the first time or after release".
> 
> What I meant is as follows: if requested, preroll is always done
> in the beginning of the playback, after prefetch. The request might not happen
> at all though, in which case we won't have prefetching phase.
> We need the prefetching when we (re)create the decoder, because its
> configuration
> and initialization (getting input, but not making output) can take time, and
> after
> the seek because there could be some data to be skipped and there is again
> initialization after the flush.
> 
> In this respect releasing the codec makes the situation the same as before the
> first
> creation.

Immediate correction:

What I meant is as follows: if requested, preroll is always done
in the beginning of the playback, after prefetch. The request might not happen
at all though, in which case we won't have prerolling phase.
We need the prerolling when we (re)create the decoder, because its configuration
and initialization (getting input, but not making output) can take time, and
after the seek because there could be some data to be skipped and there is again
initialization after the flush.

In this respect releasing the codec makes the situation the same as before the
first creation.

[wolenetz, 2015-08-21 01:28:52]

On 2015/08/21 00:31:22, Tima Vaisburd wrote:
> On 2015/08/21 00:27:40, Tima Vaisburd wrote:
> > On 2015/08/20 23:39:12, wolenetz wrote:
> > > In the CL description, I understand it to state that we request preroll
> after
> > > decoder release. Does this make sense to do in all cases? Doing preroll
> right
> > > after decoder release seems strange. Do I misunderstand?
> > 
> > After some struggle I rewrite this phrase as
> > "We request the preroll to be done after seek or when decoder is
> > going to be created anew, i.e. for the first time or after release".
> > 
> > What I meant is as follows: if requested, preroll is always done
> > in the beginning of the playback, after prefetch. The request might not
happen
> > at all though, in which case we won't have prefetching phase.
> > We need the prefetching when we (re)create the decoder, because its
> > configuration
> > and initialization (getting input, but not making output) can take time, and
> > after
> > the seek because there could be some data to be skipped and there is again
> > initialization after the flush.
> > 
> > In this respect releasing the codec makes the situation the same as before
the
> > first
> > creation.
> 
> Immediate correction:
> 
> What I meant is as follows: if requested, preroll is always done
> in the beginning of the playback, after prefetch. The request might not happen
> at all though, in which case we won't have prerolling phase.
> We need the prerolling when we (re)create the decoder, because its
configuration
> and initialization (getting input, but not making output) can take time, and
> after the seek because there could be some data to be skipped and there is
again
> initialization after the flush.
> 
> In this respect releasing the codec makes the situation the same as before the
> first creation.

ok that is more clear. Thank you!

[liberato (no reviews please), 2015-08-21 15:35:46]

On 2015/08/21 01:28:52, wolenetz wrote:
> On 2015/08/21 00:31:22, Tima Vaisburd wrote:
> > On 2015/08/21 00:27:40, Tima Vaisburd wrote:
> > > On 2015/08/20 23:39:12, wolenetz wrote:
> > > > In the CL description, I understand it to state that we request preroll
> > after
> > > > decoder release. Does this make sense to do in all cases? Doing preroll
> > right
> > > > after decoder release seems strange. Do I misunderstand?
> > > 
> > > After some struggle I rewrite this phrase as
> > > "We request the preroll to be done after seek or when decoder is
> > > going to be created anew, i.e. for the first time or after release".
> > > 
> > > What I meant is as follows: if requested, preroll is always done
> > > in the beginning of the playback, after prefetch. The request might not
> happen
> > > at all though, in which case we won't have prefetching phase.
> > > We need the prefetching when we (re)create the decoder, because its
> > > configuration
> > > and initialization (getting input, but not making output) can take time,
and
> > > after
> > > the seek because there could be some data to be skipped and there is again
> > > initialization after the flush.
> > > 
> > > In this respect releasing the codec makes the situation the same as before
> the
> > > first
> > > creation.
> > 
> > Immediate correction:
> > 
> > What I meant is as follows: if requested, preroll is always done
> > in the beginning of the playback, after prefetch. The request might not
happen
> > at all though, in which case we won't have prerolling phase.
> > We need the prerolling when we (re)create the decoder, because its
> configuration
> > and initialization (getting input, but not making output) can take time, and
> > after the seek because there could be some data to be skipped and there is
> again
> > initialization after the flush.
> > 
> > In this respect releasing the codec makes the situation the same as before
the
> > first creation.
> 
> ok that is more clear. Thank you!

great explanation!  i think that the long version is worth putting into the
code.

-fl

[liberato (no reviews please), 2015-08-21 15:35:47]

On 2015/08/21 01:28:52, wolenetz wrote:
> On 2015/08/21 00:31:22, Tima Vaisburd wrote:
> > On 2015/08/21 00:27:40, Tima Vaisburd wrote:
> > > On 2015/08/20 23:39:12, wolenetz wrote:
> > > > In the CL description, I understand it to state that we request preroll
> > after
> > > > decoder release. Does this make sense to do in all cases? Doing preroll
> > right
> > > > after decoder release seems strange. Do I misunderstand?
> > > 
> > > After some struggle I rewrite this phrase as
> > > "We request the preroll to be done after seek or when decoder is
> > > going to be created anew, i.e. for the first time or after release".
> > > 
> > > What I meant is as follows: if requested, preroll is always done
> > > in the beginning of the playback, after prefetch. The request might not
> happen
> > > at all though, in which case we won't have prefetching phase.
> > > We need the prefetching when we (re)create the decoder, because its
> > > configuration
> > > and initialization (getting input, but not making output) can take time,
and
> > > after
> > > the seek because there could be some data to be skipped and there is again
> > > initialization after the flush.
> > > 
> > > In this respect releasing the codec makes the situation the same as before
> the
> > > first
> > > creation.
> > 
> > Immediate correction:
> > 
> > What I meant is as follows: if requested, preroll is always done
> > in the beginning of the playback, after prefetch. The request might not
happen
> > at all though, in which case we won't have prerolling phase.
> > We need the prerolling when we (re)create the decoder, because its
> configuration
> > and initialization (getting input, but not making output) can take time, and
> > after the seek because there could be some data to be skipped and there is
> again
> > initialization after the flush.
> > 
> > In this respect releasing the codec makes the situation the same as before
the
> > first creation.
> 
> ok that is more clear. Thank you!

great explanation!  i think that the long version is worth putting into the
code.

-fl

[Tima Vaisburd, 2015-08-21 20:18:24]

> great explanation!  i think that the long version is worth putting into the
> code.

Added into MediaCodecPlayer::MaybePrerollDecoders()

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_audio_decoder.cc:77: void
MediaCodecAudioDecoder::ReleaseMediaCodec() {
On 2015/08/20 22:54:09, liberato wrote:
> i don't think that this needs to be overridden.

Done.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_decoder.cc:77: ReleaseDecoderResources();
On 2015/08/20 22:54:09, liberato wrote:
> please consider not having virtual methods in the destructor.  if there is
> cleanup that can't be neatly sorted out by object, then i think that it's
still
> better to document and DCHECK that things were done in the right order in the
> superclasses.

Please see the next comment.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_decoder.cc:95: ReleaseMediaCodec();
On 2015/08/20 22:54:09, liberato wrote:
> destruction is a bit tangled, with all the virtual functions in there. 
> ReleaseMediaCodec is called more than once, for example, during video decoder
> destruction via various implementations of ReleaseDecoderResources.
> 
> please consider sorting this out so that the cleanup order is clear.

I've seen your comment in the other CL where I call ReleaseDecoderResources()
from the player, but I wanted to keep the decoder class self-contained. I also
wanted to keep ReleaseMediaCodec virtual. I ended up making the subclasses do
the job, not superclasses as you mentioned. Please take a look, and suggestions
welcome!

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_decoder.cc:183: return HasStream() &&
needs_preroll_ && !completed_;
On 2015/08/20 22:54:09, liberato wrote:
> either |needs_preroll_| or NeedsPreroll() can be better named -- i'd expect
> NeedsPreroll() { return needs_preroll_; }

Renamed the method to NotCompletedAndNeedsPreroll()

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_video_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_video_decoder.cc:235:
ReleaseOutputBuffer(buffer_index, pts, (size > 0), update_time,
On 2015/08/20 22:54:09, liberato wrote:
> can this set |start_pts_| and break?  if not, then please expand the comment.

I'm not sure I completely understood your point. What I meant is that we cannot
calculate the delay (time_to_render) here even if we wanted to. I updated the
comment.

https://codereview.chromium.org/1254293003/diff/220001/media/base/android/med...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/1254293003/diff/220001/media/base/android/med...
media/base/android/media_codec_bridge.h:242: // postpone == false, and if will
play two buffers: the postponed one and
On 2015/08/20 23:39:12, wolenetz wrote:
> nit: s/if/it/

Done.

https://codereview.chromium.org/1254293003/diff/220001/media/base/android/med...
File media/base/android/media_codec_decoder.h (right):

https://codereview.chromium.org/1254293003/diff/220001/media/base/android/med...
media/base/android/media_codec_decoder.h:277: // Returns true is we are in the
process of sync stop.
On 2015/08/20 23:39:12, wolenetz wrote:
> nit:s/is/if/

Done.

[liberato (no reviews please), 2015-08-21 21:02:17]

lgtm.

thanks
-fl

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_decoder.cc:95: ReleaseMediaCodec();
On 2015/08/21 20:18:24, Tima Vaisburd wrote:
> On 2015/08/20 22:54:09, liberato wrote:
> > destruction is a bit tangled, with all the virtual functions in there. 
> > ReleaseMediaCodec is called more than once, for example, during video
decoder
> > destruction via various implementations of ReleaseDecoderResources.
> > 
> > please consider sorting this out so that the cleanup order is clear.
> 
> I've seen your comment in the other CL where I call ReleaseDecoderResources()
> from the player, but I wanted to keep the decoder class self-contained. I also
> wanted to keep ReleaseMediaCodec virtual. I ended up making the subclasses do
> the job, not superclasses as you mentioned. Please take a look, and
suggestions
> welcome! 

once one has virtual cleanup methods, making cleanup part of destruction is very
tricky without helper classes.

in general, having the cleanup in the most-derived class seems better to me than
it was before.  this assumes, of course, that we don't subclass it anymore.  :)

i have no better idea.

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
File media/base/android/media_codec_video_decoder.cc (right):

https://codereview.chromium.org/1254293003/diff/200001/media/base/android/med...
media/base/android/media_codec_video_decoder.cc:235:
ReleaseOutputBuffer(buffer_index, pts, (size > 0), update_time,
On 2015/08/21 20:18:24, Tima Vaisburd wrote:
> On 2015/08/20 22:54:09, liberato wrote:
> > can this set |start_pts_| and break?  if not, then please expand the
comment.
> 
> I'm not sure I completely understood your point. What I meant is that we
cannot
> calculate the delay (time_to_render) here even if we wanted to. I updated the
> comment.

i misread it.  sorry about that.

[Tima Vaisburd, 2015-08-21 21:28:13]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-08-21 21:28:13]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1254293003/#ps240001
(title: "Addressed Frank's comments")

[commit-bot: I haz the power, 2015-08-21 21:29:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254293003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254293003/240001

[commit-bot: I haz the power, 2015-08-21 22:22:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 22:22:33]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Tima Vaisburd, 2015-08-21 22:50:40]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-08-21 22:50:40]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
wolenetz@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1254293003/#ps260001
(title: "removed unused variable")

[commit-bot: I haz the power, 2015-08-21 22:52:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254293003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254293003/260001

[commit-bot: I haz the power, 2015-08-21 23:23:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 23:23:37]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[Tima Vaisburd, 2015-08-22 00:53:41]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-08-22 00:53:42]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
wolenetz@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1254293003/#ps280001
(title: "Rebased")

[commit-bot: I haz the power, 2015-08-22 00:53:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254293003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254293003/280001

[commit-bot: I haz the power, 2015-08-22 01:59:30]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-08-22 02:00:01]

Patchset 15 (id:??) landed as
https://crrev.com/17658f61356d6ce6d191f519b0cb13ea31759d0e
Cr-Commit-Position: refs/heads/master@{#344949}