Fix status notifications for FFmpegDemuxerStream with a pending read

Fix status notifications for FFmpegDemuxerStream with a pending read

We shouldn't return from FFmpegDemuxerStream::SetEnabled prematurely if
there is a pending read, we still need to send the status notification.
Also expand test coverage for stream status changes to include status
changes with a pending read on the changed stream.

Review-Url: https://codereview.chromium.org/2825863002
Cr-Commit-Position: refs/heads/master@{#465884}
Committed: https://chromium.googlesource.com/chromium/src/+/81544b287da1e33806ac095b86a15d717ebfe27c

[servolk, 2017-04-18 20:15:58]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[servolk, 2017-04-18 20:16:09]

Description was changed from

==========
Fix status notifications for FFmpegDemuxerStream with a pending read

We shouldn't return from FFmpegDemuxerStream::SetEnabled prematurely if
there is a pending read, we still need to send the status notification.
Also expand test coverage for stream status changes to include status
changes with a pending read on the changed stream.
==========

to

==========
Fix status notifications for FFmpegDemuxerStream with a pending read

We shouldn't return from FFmpegDemuxerStream::SetEnabled prematurely if
there is a pending read, we still need to send the status notification.
Also expand test coverage for stream status changes to include status
changes with a pending read on the changed stream.
==========

[servolk, 2017-04-18 20:16:09]

servolk@chromium.org changed reviewers:
+ chcunningham@chromium.org

[commit-bot: I haz the power, 2017-04-18 20:16:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 00:03:29]

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

[commit-bot: I haz the power, 2017-04-19 00:03:30]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2017-04-19 01:01:34]

https://codereview.chromium.org/2825863002/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/2825863002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:1651: audio_stream->FlushBuffers();
What is the FlushBuffers needed for?

[servolk, 2017-04-19 01:05:33]

https://codereview.chromium.org/2825863002/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/2825863002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:1651: audio_stream->FlushBuffers();
On 2017/04/19 01:01:34, chcunningham wrote:
> What is the FlushBuffers needed for?

To ensure there's no prepared frames that are ready to be returned as soon as we
call stream->Read(). In this case we want the Read to not be satisfied
immediately, so that the stream status changes performed by the
CheckStreamStatusNotifications are done while there is a pending read (i.e. the
Stream::read_cb_ is non-null).

[chcunningham, 2017-04-20 01:00:25]

LGTM

https://codereview.chromium.org/2825863002/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/2825863002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:1651: audio_stream->FlushBuffers();
Makes good sense. Maybe a comment to explain that part.

[servolk, 2017-04-20 01:04:20]

https://codereview.chromium.org/2825863002/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/2825863002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:1651: audio_stream->FlushBuffers();
On 2017/04/20 01:00:25, chcunningham wrote:
> Makes good sense. Maybe a comment to explain that part. 

Done.

[servolk, 2017-04-20 01:04:23]

The CQ bit was checked by servolk@chromium.org

[servolk, 2017-04-20 01:04:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org
Link to the patchset: https://codereview.chromium.org/2825863002/#ps20001
(title: "Add a comment explaining FlushBuffers")

[commit-bot: I haz the power, 2017-04-20 01:05:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 03:19:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492650263098410,
"parent_rev": "448a53ebf59dd9d6faf0da8875ef34bc2a99c2a2", "commit_rev":
"81544b287da1e33806ac095b86a15d717ebfe27c"}

[commit-bot: I haz the power, 2017-04-20 03:19:38]

Description was changed from

==========
Fix status notifications for FFmpegDemuxerStream with a pending read

We shouldn't return from FFmpegDemuxerStream::SetEnabled prematurely if
there is a pending read, we still need to send the status notification.
Also expand test coverage for stream status changes to include status
changes with a pending read on the changed stream.
==========

to

==========
Fix status notifications for FFmpegDemuxerStream with a pending read

We shouldn't return from FFmpegDemuxerStream::SetEnabled prematurely if
there is a pending read, we still need to send the status notification.
Also expand test coverage for stream status changes to include status
changes with a pending read on the changed stream.

Review-Url: https://codereview.chromium.org/2825863002
Cr-Commit-Position: refs/heads/master@{#465884}
Committed:
https://chromium.googlesource.com/chromium/src/+/81544b287da1e33806ac095b86a1...
==========

[commit-bot: I haz the power, 2017-04-20 03:19:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/81544b287da1e33806ac095b86a1...