Allow simultaneous audio and video config change

Allow simultaneous audio and video config change

When completing prefetch for multiple streams, if more than one stream has
|kConfigChanged| next, then the calls to DecodeMoreAudio() and
DecodeMoreVideo() need to allow possibility that
|CONFIG_CHANGE_EVENT_PENDING| is already set by the other stream.
Also includes a new unit test for this scenario.

R=qinmin@chromium.org,acolwell@chromium.org,xhwang@chromium.org
BUG=314170
TEST=All media_unittests pass on Android with MediaCodecBridge available.

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

[wolenetz, 2013-11-01 21:46:20]

PTAL @ PS1, mostly for approach:
qinmin@: OWNERS review.
xhwang@: Does the bug still repro with this applied?
acolwell@: General review.

I'll add a quick unit test in later PS. For now, is this approach looking good?
Would you prefer we add some bool arg like
"other_stream_config_change_already_pending" to DecodeMore*() routines, add to
top of each DecodeMore*() DCHECK_EQ(that bool,
IsEventPending(CONFIG_CHANGE_EVENT_PENDING)), and only set the arg to true for
DecodeMoreVideo() in OnPrefetchDone() if DecodeMoreAudio() detected config
change?

[xhwang, 2013-11-01 23:56:37]

I tried this patch and the test ( MediaSourceTest.ConfigChangeVideo) is passing.
Thanks for the quick fix!

[acolwell GONE FROM CHROMIUM, 2013-11-02 00:19:07]

I believe this approach will work as long as the code reading from the demuxer
never allows more than one kConfigChanged status in the DemuxerData it sends to
MSP/MDJ.

[wolenetz, 2013-11-02 01:16:38]

On 2013/11/02 00:19:07, acolwell wrote:
> I believe this approach will work as long as the code reading from the demuxer
> never allows more than one kConfigChanged status in the DemuxerData it sends
to
> MSP/MDJ.

MSD::OnBufferReady() ensures this: if demuxerstream's last read status is
|kConfigChanged|, then MSD::OnBufferReady() doesn't attempt to read and fill in
remaining access units in the DemuxerData(); instead it resizes it to what's
been read so far plus the kConfigChanged AU and sends the read ACK back to MSP.

[wolenetz, 2013-11-02 03:21:18]

PTAL @ PS2. All previous comments are addressed, including narrowed allowance
for exactly when a simultaneous config change is allowed (e.g. only
OnPrefetchDone()), and a new unit test is included.
Thanks!

[wolenetz, 2013-11-02 03:25:31]

On 2013/11/02 03:21:18, wolenetz wrote:
> PTAL @ PS2. All previous comments are addressed, including narrowed allowance
> for exactly when a simultaneous config change is allowed (e.g. only
> OnPrefetchDone()), and a new unit test is included.
> Thanks!

btw, PS1 passed xhwang@'s MSE content_browsertests even though it emitted
redundant demuxer config requests. The first response configs reset the pending
config change and re-created the jobs. The second one just updated the player's
cached configs, but the pending config change was already reset for both
streams, so was otherwise a no-op on receipt. PS2 eliminates this redundant
demuxer config request in the case of simultaneous A/V config change in
OnPrefetchDone().

[wolenetz, 2013-11-02 03:56:47]

On 2013/11/02 03:25:31, wolenetz wrote:
> On 2013/11/02 03:21:18, wolenetz wrote:
> > PTAL @ PS2. All previous comments are addressed, including narrowed
allowance
> > for exactly when a simultaneous config change is allowed (e.g. only
> > OnPrefetchDone()), and a new unit test is included.
> > Thanks!
> 
> btw, PS1 passed xhwang@'s MSE content_browsertests even though it emitted
> redundant demuxer config requests. The first response configs reset the
pending
> config change and re-created the jobs. The second one just updated the
player's
> cached configs, but the pending config change was already reset for both
> streams, so was otherwise a no-op on receipt. PS2 eliminates this redundant
> demuxer config request in the case of simultaneous A/V config change in
> OnPrefetchDone().

I have also confirmed MediaSourceTest.ConfigChangeVideo is passing with PS2,
even when "lucky enough" to hit simultaneous a/v config change in
OnPrefetchDone():
[VERBOSE1:media_source_player.cc(801)] OnDecoderStarved
[VERBOSE1:media_source_player.cc(895)] SetPendingEvent(PREFETCH_REQUEST)
[VERBOSE1:media_source_player.cc(461)] ProcessPendingEvents : 0x8
[VERBOSE1:media_source_player.cc(464)] ProcessPendingEvents : A video job is
still decoding.
[VERBOSE1:media_source_player.cc(535)] MediaDecoderCallback: 1, 0
[VERBOSE1:media_source_player.cc(461)] ProcessPendingEvents : 0x8
[VERBOSE1:media_source_player.cc(464)] ProcessPendingEvents : A video job is
still decoding.
[VERBOSE1:media_source_player.cc(535)] MediaDecoderCallback: 0, 0
[VERBOSE1:media_source_player.cc(461)] ProcessPendingEvents : 0x8
[VERBOSE1:media_source_player.cc(507)] ProcessPendingEvents : Handling
PREFETCH_REQUEST_EVENT.
[VERBOSE1:media_source_player.cc(895)] SetPendingEvent(PREFETCH_DONE)
[VERBOSE1:media_source_player.cc(903)] ClearPendingEvent(PREFETCH_REQUEST)

[VERBOSE1:media_source_player.cc(849)] OnPrefetchDone
[VERBOSE1:media_source_player.cc(903)] ClearPendingEvent(PREFETCH_DONE)
[VERBOSE1:media_source_player.cc(614)] DecodeMoreAudio
[VERBOSE1:media_source_player.cc(895)] SetPendingEvent(CONFIG_CHANGE)
[VERBOSE1:media_source_player.cc(461)] ProcessPendingEvents : 0x4
[VERBOSE1:media_source_player.cc(487)] ProcessPendingEvents : Handling
CONFIG_CHANGE_EVENT.
[VERBOSE1:media_source_player.cc(641)] DecodeMoreVideo
[27827:27867:1101/204627:424031382380:VERBOSE1:media_source_delegate.cc(668)]
OnMediaConfigRequest : 1
[27827:27867:1101/204627:424031382563:VERBOSE1:media_source_delegate.cc(713)]
NotifyDemuxerReady : 1
[VERBOSE1:media_source_player.cc(304)] OnDemuxerConfigsAvailable
[VERBOSE1:media_source_player.cc(720)] ConfigureAudioDecoderJob : creating new
audio decoder job
[VERBOSE1:media_decoder_job.cc(127)] BeginPrerolling(0.9)
[VERBOSE1:media_source_player.cc(769)] ConfigureVideoDecoderJob : creating new
video decoder job
[VERBOSE1:media_decoder_job.cc(127)] BeginPrerolling(0.9)
[VERBOSE1:media_source_player.cc(903)] ClearPendingEvent(CONFIG_CHANGE)

[VERBOSE1:media_source_player.cc(276)] StartInternal
[VERBOSE1:media_source_player.cc(895)] SetPendingEvent(PREFETCH_REQUEST)
[VERBOSE1:media_source_player.cc(461)] ProcessPendingEvents : 0x8
[VERBOSE1:media_source_player.cc(507)] ProcessPendingEvents : Handling
PREFETCH_REQUEST_EVENT.
[VERBOSE1:media_decoder_job.cc(217)] RequestData
[VERBOSE1:media_decoder_job.cc(217)] RequestData

[wolenetz, 2013-11-04 18:38:26]

ping :)
(PS2 is awaiting CR) Thanks!

[acolwell GONE FROM CHROMIUM, 2013-11-04 18:42:07]

lgtm % comment

https://codereview.chromium.org/50433007/diff/90001/media/base/android/media_...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/50433007/diff/90001/media/base/android/media_...
media/base/android/media_source_player.cc:630: if (video_change_already_pending)
{
I don't think the xxx_change_already_pending parameters are providing much value
here and below. It also makes the call sites confusing. I think you should just
use IsEventPending(CONFIG_CHANGE_EVENT_PENDING) as the condition here and just
keep a DCHECK(reconfig_xxx_decoder_) in the block before returning.

[wolenetz, 2013-11-04 19:30:24]

Thank you, acolwell@.
qinmin@ and xhwang@, please CR PS3.

https://codereview.chromium.org/50433007/diff/90001/media/base/android/media_...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/50433007/diff/90001/media/base/android/media_...
media/base/android/media_source_player.cc:630: if (video_change_already_pending)
{
On 2013/11/04 18:42:07, acolwell wrote:
> I don't think the xxx_change_already_pending parameters are providing much
value
> here and below. It also makes the call sites confusing. I think you should
just
> use IsEventPending(CONFIG_CHANGE_EVENT_PENDING) as the condition here and just
> keep a DCHECK(reconfig_xxx_decoder_) in the block before returning.

Done. Thank you for the rapid CR :)

[qinmin, 2013-11-04 19:53:56]

lgtm

[commit-bot: I haz the power, 2013-11-04 20:13:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/50433007/240001

[commit-bot: I haz the power, 2013-11-04 21:14:42]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-04 21:36:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/50433007/240001

[xhwang, 2013-11-04 21:55:47]

lgtm % one tiny nit

https://codereview.chromium.org/50433007/diff/240001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/50433007/diff/240001/media/base/android/media...
media/base/android/media_source_player.cc:664: DCHECK(reconfig_video_decoder_);
hmm, isn't this always true given line 659? Or are we trying to remind ourselves
on that? Anyway, let's land this CL first!

[wolenetz, 2013-11-04 22:00:55]

Thank you, xhwang. Please let me know if I'm misunderstanding your nit in my
response:

https://codereview.chromium.org/50433007/diff/240001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/50433007/diff/240001/media/base/android/media...
media/base/android/media_source_player.cc:664: DCHECK(reconfig_video_decoder_);
On 2013/11/04 21:55:48, xhwang wrote:
> hmm, isn't this always true given line 659? Or are we trying to remind
ourselves
> on that? Anyway, let's land this CL first!

This DCHECK is to ensure that the CONFIG_CHANGE_EVENT_PENDING was set already,
but for the *other* stream (the video stream). We already have line 658
protecting against attempted double-setting of this pending event for the same
stream. DecodeMoreVideo() has similar (with 'audio' and 'video' swapped).

[commit-bot: I haz the power, 2013-11-05 00:42:22]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-05 00:55:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/50433007/240001

[xhwang1, 2013-11-05 02:30:21]

https://codereview.chromium.org/50433007/diff/240001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/50433007/diff/240001/media/base/android/media...
media/base/android/media_source_player.cc:664: DCHECK(reconfig_video_decoder_);
On 2013/11/04 22:00:55, wolenetz wrote:
> On 2013/11/04 21:55:48, xhwang wrote:
> > hmm, isn't this always true given line 659? Or are we trying to remind
> ourselves
> > on that? Anyway, let's land this CL first!
> 
> This DCHECK is to ensure that the CONFIG_CHANGE_EVENT_PENDING was set already,
> but for the *other* stream (the video stream). We already have line 658
> protecting against attempted double-setting of this pending event for the same
> stream. DecodeMoreVideo() has similar (with 'audio' and 'video' swapped).

Oh, I didn't realize it's "video", not "audio". Sorry for the confusion.

[commit-bot: I haz the power, 2013-11-05 03:07:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/50433007/240001

[commit-bot: I haz the power, 2013-11-05 05:32:22]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-05 05:39:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/50433007/240001

[commit-bot: I haz the power, 2013-11-05 05:46:24]

Change committed as 232920