Determine the audible state in MediaSourcePlayer

Determine the audible state in MediaSourcePlayer

We set the audible state every time after the pipeline
has successfully decoded the frame: to false if prerolling
and true otherwise, taking the volume into account.

Also, we set the state to false if the flow is going to be
 interrupted because of a coded error, abort or Pause.

As well, we set audible state to false when the starvation
happens and there is no data in the MediaDecoderJob queue,
we believe this indicates the demuxer freeze.

BUG=414810
Committed: https://crrev.com/2284586bd2d49cacd18c16bfe60fcb519c80b045
Cr-Commit-Position: refs/heads/master@{#322102}

[Tima Vaisburd, 2015-03-18 01:35:01]

timav@chromium.org changed reviewers:
+ qinmin@google.com, sievers@chromium.org

[Tima Vaisburd, 2015-03-18 01:35:02]

Please review

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

https://codereview.chromium.org/1008093002/diff/20001/media/base/android/medi...
media/base/android/media_source_player_unittest.cc:962:
message_loop_.PostDelayedTask(
I wanted to guarantee that this delayed task arrives after the delayed task in
the MediaSourcePlayer that actually changes the status. That's why I used
PostDelayedTask() instead of Sleep(). Please let me know if there is a better
way.

[no sievers, 2015-03-19 20:06:41]

Do we really need a timeout? Don't we know when playback pauses or stops?

On a related note: Overall, do we have something in place where we can detect if
the phone's volume level is 0 (rather then the player volume level)? Would be
nice if we could also override in that case and suspend timers (for both
MediaSource and MediaPlayer).

[Tima Vaisburd, 2015-03-19 22:05:40]

On 2015/03/19 20:06:41, sievers wrote:
> Do we really need a timeout? Don't we know when playback pauses or stops?

I think we do. I basically copied the idea from AudioStreamMonitor, with an
important difference
that I do not analyze the average power of the signal. That is, the active
processing of audio packets
with silence is considered audible, consistent with how this thing forks for
MediaPlayer (native player on Android)
and consistent with Min's comments (as far as I got he wanted this case to be
audible).
Other than that, monitoring audio packets as they go through the pipeline seems
the most reliable method
of detection what the pipeline is doing.

> On a related note: Overall, do we have something in place where we can detect
if
> the phone's volume level is 0 (rather then the player volume level)? Would be
> nice if we could also override in that case and suspend timers (for both
> MediaSource and MediaPlayer).

It feels for me like a too much coupling.
This change is supposed to be used in, at least, two places: the notification
that a background tab has a sound
(similar to what we have on the desktop) and to prevent webkit timers stopping.
I think that the former (notification)
should definitely be unrelated to the volume rocker position, since it is a
property of web page.
For the second case, to be honest, I'm not sure as well, but even if we do that,
I think it should be separate.
I'm not aware of the existing mechanism that changes Chrome behavior depending
on the device volume.

[no sievers, 2015-03-19 22:18:32]

On 2015/03/19 22:05:40, Tima Vaisburd wrote:
> On 2015/03/19 20:06:41, sievers wrote:
> > Do we really need a timeout? Don't we know when playback pauses or stops?
> 
> I think we do. I basically copied the idea from AudioStreamMonitor, with an
> important difference
> that I do not analyze the average power of the signal. That is, the active
> processing of audio packets
> with silence is considered audible, consistent with how this thing forks for
> MediaPlayer (native player on Android)
> and consistent with Min's comments (as far as I got he wanted this case to be
> audible).
> Other than that, monitoring audio packets as they go through the pipeline
seems
> the most reliable method
> of detection what the pipeline is doing.
> 
> > On a related note: Overall, do we have something in place where we can
detect
> if
> > the phone's volume level is 0 (rather then the player volume level)? Would
be
> > nice if we could also override in that case and suspend timers (for both
> > MediaSource and MediaPlayer).
> 
> It feels for me like a too much coupling.
> This change is supposed to be used in, at least, two places: the notification
> that a background tab has a sound
> (similar to what we have on the desktop) and to prevent webkit timers
stopping.
> I think that the former (notification)
> should definitely be unrelated to the volume rocker position, since it is a
> property of web page.
> For the second case, to be honest, I'm not sure as well, but even if we do
that,
> I think it should be separate.
> I'm not aware of the existing mechanism that changes Chrome behavior depending
> on the device volume.

Yes, sorry... this is totally separate and shouldn't be conflated with this
patch.
I just wanted to mention it so that we can keep track of it if it's feasible to
do.

[no sievers, 2015-03-19 22:19:27]

And I'll let Min comment on the approach then.

[qinmin, 2015-03-19 23:34:22]

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

[qinmin, 2015-03-19 23:34:23]

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

https://codereview.chromium.org/1008093002/diff/20001/media/base/android/medi...
media/base/android/media_source_player.cc:600: void
MediaSourcePlayer::UpdateAudibleStatus() {
I don't think we really need a timer for this. We can just wait for the decoder
callback and update the audible status. 
If we really want to be very accurate, we need to monitor the number of
remaining bytes in the AudioTrack object, rather than using a timer. 
For example, if the AudioDecoder is blocked on the AudioTrack while writing a
large chunk of data, the timer may expire and signal an idle state. However,
that is not really true since there are data in the AudioTrack

[Tima Vaisburd, 2015-03-20 00:56:14]

On 2015/03/19 23:34:23, qinmin wrote:
> I don't think we really need a timer for this. We can just wait for the
decoder
> callback and update the audible status. 

I do not understand how to detect the case when the media packets stop arriving
from
the demuxer without using a timer. This happens, for instance, when we stop
WebKit timers in the background.
I though the pipeline will just wait for the new data, no new callbacks will be
called. What am I missing?

> If we really want to be very accurate, we need to monitor the number of
> remaining bytes in the AudioTrack object, rather than using a timer. 

I made the delay deliberately large (1 second) to avoid frequent changes during
short-term starvations.
The desktop version has 2 seconds delay. I'd think we do not need very high
accuracy because of that,
but since the whole mechanism does not look right to you this maybe a moot
point.

[qinmin, 2015-03-20 02:28:27]

On 2015/03/20 00:56:14, Tima Vaisburd wrote:
> On 2015/03/19 23:34:23, qinmin wrote:
> > I don't think we really need a timer for this. We can just wait for the
> decoder
> > callback and update the audible status. 
> 
> I do not understand how to detect the case when the media packets stop
arriving
> from
> the demuxer without using a timer. This happens, for instance, when we stop
> WebKit timers in the background.
> I though the pipeline will just wait for the new data, no new callbacks will
be
> called. What am I missing?
> 

we can simply check whether EOS is reached or when the starvation timer fires. 

> > If we really want to be very accurate, we need to monitor the number of
> > remaining bytes in the AudioTrack object, rather than using a timer. 
> 
> I made the delay deliberately large (1 second) to avoid frequent changes
during
> short-term starvations.
> The desktop version has 2 seconds delay. I'd think we do not need very high
> accuracy because of that,
> but since the whole mechanism does not look right to you this maybe a moot
> point.

[Tima Vaisburd, 2015-03-24 18:49:34]

On 2015/03/20 02:28:27, qinmin wrote:
> we can simply check whether EOS is reached or when the starvation timer fires.


Done according to what we talked about, I updated the description to reflect
the new changes. Please take another look.

[Tima Vaisburd, 2015-03-24 18:50:36]

New version, this does not introduce a new timer and uses the existing
starvation timer to detect the renderer process freeze.

[qinmin, 2015-03-24 21:38:37]

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/audi...
File media/base/android/audio_decoder_job.h (right):

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/audi...
media/base/android/audio_decoder_job.h:37: double GetVolume() const;
just use inline functions for simple getter/setters

double volume() const { return volume_; }

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

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/medi...
media/base/android/media_source_player.cc:519: } else {
we need setAudible(false) here too

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/medi...
media/base/android/media_source_player.cc:675: if
(!audio_decoder_job_->HasData()) {
no {} needed

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/medi...
File media/base/android/media_source_player.h (right):

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/medi...
media/base/android/media_source_player.h:72: private:
we use one space indent for public and private keywords, please revert this

[Tima Vaisburd, 2015-03-24 22:24:51]

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/audi...
File media/base/android/audio_decoder_job.h (right):

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/audi...
media/base/android/audio_decoder_job.h:37: double GetVolume() const;
On 2015/03/24 21:38:37, qinmin wrote:
> just use inline functions for simple getter/setters
> 
> double volume() const { return volume_; }

Done.

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

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/medi...
media/base/android/media_source_player.cc:519: } else {
On 2015/03/24 21:38:37, qinmin wrote:
> we need setAudible(false) here too

Done.

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/medi...
media/base/android/media_source_player.cc:675: if
(!audio_decoder_job_->HasData()) {
On 2015/03/24 21:38:37, qinmin wrote:
> no {} needed

Done.

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/medi...
File media/base/android/media_source_player.h (right):

https://codereview.chromium.org/1008093002/diff/40001/media/base/android/medi...
media/base/android/media_source_player.h:72: private:
On 2015/03/24 21:38:37, qinmin wrote:
> we use one space indent for public and private keywords, please revert this

Oops, sorry

[qinmin, 2015-03-24 22:30:26]

lgtm

[Tima Vaisburd, 2015-03-24 23:26:14]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-03-24 23:27:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1008093002/60001

[commit-bot: I haz the power, 2015-03-25 00:17:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-25 00:18:40]

Patchset 4 (id:??) landed as
https://crrev.com/2284586bd2d49cacd18c16bfe60fcb519c80b045
Cr-Commit-Position: refs/heads/master@{#322102}