Stop audio FFmpegDemuxerStreams if we get notified that audio rendering is disabled.

Stop audio FFmpegDemuxerStreams if we get notified that audio rendering is disabled.

This prevents reading the entire file until end of stream when attempting to read audio data while disabled.

Part of this bug was due to keeping two separate arrays of FFmpegDemuxerStreams and inspecting the wrong one. To further reinforce this change, the two arrays have been merged.

BUG=106735
TEST=disable audio service on Windows 7 then navigate to various HTML5 audio/video resource with a clean profile: it should take less time to load and the buffer bar should be only partially full instead of 100% full

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114699

[scherkus (not reviewing), 2011-12-13 03:52:56]

I tried a few testing approaches but didn't like them for various reasons.

I settled on a combination expecting the read to complete immediately _without_
running the message loop and file position as a sanity check.

Inspecting stopped_ required up-casting DemuxerStream to FFmpegDemuxerStream,
which none of the other tests do and that is also creeping in on implementation
details of FFmpegDemuxerStream.

Let me know what you think.

[acolwell GONE FROM CHROMIUM, 2011-12-13 16:45:43]

http://codereview.chromium.org/8890071/diff/3001/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8890071/diff/3001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:646: for (StreamVector::iterator iter =
streams_.begin();
I don't feel good about this iteration happening on every packet. If you want to
only have one vector own the stream, how about just changing packet_streams_ to
an index -> Type vector? Then you could just do something like
streams_[packet_stream_[packet->stream_index]]. When streams are disable you can
just update packet_stream_[packet->stream_index] to UNKNOWN and make sure
streams_[UNKNOWN] is always NULL.

http://codereview.chromium.org/8890071/diff/3001/media/filters/ffmpeg_demuxer...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/8890071/diff/3001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:517:
EXPECT_TRUE(demuxer_->GetPosition(&position));
This feels fragile & too indirect. What about just creating a testing helper
function on FFmpegDemuxer along the lines of bool IsStopped(Type type) or bool
AreAllStreamsStopped()?

[scherkus (not reviewing), 2011-12-13 17:24:56]

http://codereview.chromium.org/8890071/diff/3001/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8890071/diff/3001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:646: for (StreamVector::iterator iter =
streams_.begin();
On 2011/12/13 16:45:44, acolwell wrote:
> I don't feel good about this iteration happening on every packet. If you want
to
> only have one vector own the stream, how about just changing packet_streams_
to
> an index -> Type vector? Then you could just do something like
> streams_[packet_stream_[packet->stream_index]]. When streams are disable you
can
> just update packet_stream_[packet->stream_index] to UNKNOWN and make sure
> streams_[UNKNOWN] is always NULL.

This loop unrolls to checking streams_[AUDIO]->stream_index() and
streams_[VIDEO]->stream_index(). True we're doing one additional comparison but
I'm skeptical doing bounds checking on packet->stream_index + an indirect lookup
is any better -- but we're really splitting hairs here!

Is your concern that |streams_| could change to be large and we wouldn't notice
the performance hit? I could hand unroll the loop as described above -- it would
likely clear things up a bit and also prevent needless looping.

http://codereview.chromium.org/8890071/diff/3001/media/filters/ffmpeg_demuxer...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/8890071/diff/3001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:517:
EXPECT_TRUE(demuxer_->GetPosition(&position));
On 2011/12/13 16:45:44, acolwell wrote:
> This feels fragile & too indirect. What about just creating a testing helper
> function on FFmpegDemuxer along the lines of bool IsStopped(Type type) or bool
> AreAllStreamsStopped()?

I'll give it a shot.

[scherkus (not reviewing), 2011-12-15 02:37:58]

PTAL

I also added created some additional tests for edge cases that I ran into during
testing

[acolwell GONE FROM CHROMIUM, 2011-12-15 18:02:22]

LGTM

http://codereview.chromium.org/8890071/diff/13001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/8890071/diff/13001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:222: EXPECT_FALSE(stream);
nit: Do you need to assign to stream? Can't the GetStream() call be placed
inside the EXPECT_FALSE()?

http://codereview.chromium.org/8890071/diff/13001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:251: EXPECT_FALSE(stream);
nit: ditto. This check could probably be moved to InitializeDemuxer() so it
occurs on a wider range of tests.

[scherkus (not reviewing), 2011-12-15 19:24:37]

http://codereview.chromium.org/8890071/diff/13001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/8890071/diff/13001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:222: EXPECT_FALSE(stream);
On 2011/12/15 18:02:22, acolwell wrote:
> nit: Do you need to assign to stream? Can't the GetStream() call be placed
> inside the EXPECT_FALSE()?

Done.

Was copy+pasting :)

http://codereview.chromium.org/8890071/diff/13001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:251: EXPECT_FALSE(stream);
On 2011/12/15 18:02:22, acolwell wrote:
> nit: ditto. This check could probably be moved to InitializeDemuxer() so it
> occurs on a wider range of tests.

Done.