Don't fallback if we've successfully opened a stream previously.

Don't fallback if we've successfully opened a stream previously.

Fixes an issue where we've opened streams through the AudioOutputDispatcher
successfully, but eventually fail to open a stream.  At this point the old
code would try to fallback to the high latency audio path and in the process
destroy the old AudioOutputDispatcher during Initialize()... leaving a bunch
of physical audio streams in the ether calling OnMoreDataResampler callbacks
which no longer have a way to stop the associated stream before being deleted.

This also explains the "double-Stop()" errors from issue 149815, since they
were not double-Stop() but rather Stop() on an AudioOutputProxy that the new
dispatcher didn't know about.

Rolls in the following fixes as well:
- Replaces lock with message_loop_ check.
- Fixes an issue where physical streams were incorrectly closed on Open failure.
- Rollback of previous double-Stop() and CHECK() changes.

BUG=150619
TEST=new media unittest.

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

[DaleCurtis, 2012-09-20 21:12:01]

I think this is it!

[DaleCurtis, 2012-09-20 21:45:01]

On 2012/09/20 21:12:01, DaleCurtis wrote:
> I think this is it!

To confirm, this is definitely it. I forced an eventual Open failure and got the
same stack trace as we're seeing in the wild.

[scherkus (not reviewing), 2012-09-21 01:24:18]

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_p...
File media/audio/audio_output_proxy.cc (right):

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_p...
media/audio/audio_output_proxy.cc:76: dispatcher_->CloseStream(this);
so to confirm... kStartError means Open() was successful so we still have to
Close()?

if so... comment?

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_p...
File media/audio/audio_output_proxy_unittest.cc (right):

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_p...
media/audio/audio_output_proxy_unittest.cc:881: EXPECT_CALL(stream1, Open())
can you sprinkle a one-liner comment before each stream block so we know what's
going on?

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:245: if (dispatcher_->OpenStream()) {
I'm not a fan of the implicit stateness in this class/function w.r.t. low
latency vs fallback vs streams are open vs we've failed

It'd be preferable to have the states more explicit w/ the implicit parts
DCHECK'd but I don't think it's possible w/ today's AOD/AOP classes.

For example, something like the following would be nice as it goes through the
same path from the top of the function every time, but I don't think it's quite
equivalent.

while (true) {
  switch (mode_) {
    case kLowLatency:
      DCHECK_EQ(output_params are LOW_LATENCY);
      if (dispatcher_->Open())
        return true;
      if (fallback not disabled) {
        mode_ = kHighLatency;
        output_params_ = SetupFallbackParams();
        Initialize();
        continue;
      }
      return false;

    case kHighLatency:
      DCHECK_EQ(output_params are PCM_LINEAR);
      DCHECK(fallback flag is enabled)
      return dispatcher_->Open();
  }
}


Any ideas?

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:319: void
AudioOutputResampler::CloseStream(AudioOutputProxy* stream_proxy) {
to confirm.. we never have to reset streams_opened_ to false?

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:337: DCHECK(HasOneRef()) << "Only the
AudioManager should hold a reference";
did this end up firing?

[DaleCurtis, 2012-09-21 02:57:27]

PTAL. Lets get this in tonight.

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_p...
File media/audio/audio_output_proxy.cc (right):

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_p...
media/audio/audio_output_proxy.cc:76: dispatcher_->CloseStream(this);
On 2012/09/21 01:24:18, scherkus wrote:
> so to confirm... kStartError means Open() was successful so we still have to
> Close()?
> 
> if so... comment?

Done.

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_p...
File media/audio/audio_output_proxy_unittest.cc (right):

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_p...
media/audio/audio_output_proxy_unittest.cc:881: EXPECT_CALL(stream1, Open())
On 2012/09/21 01:24:18, scherkus wrote:
> can you sprinkle a one-liner comment before each stream block so we know
what's
> going on?

Done.

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:245: if (dispatcher_->OpenStream()) {
On 2012/09/21 01:24:18, scherkus wrote:
> I'm not a fan of the implicit stateness in this class/function w.r.t. low
> latency vs fallback vs streams are open vs we've failed
> 
> It'd be preferable to have the states more explicit w/ the implicit parts
> DCHECK'd but I don't think it's possible w/ today's AOD/AOP classes.
> 
> For example, something like the following would be nice as it goes through the
> same path from the top of the function every time, but I don't think it's
quite
> equivalent.
> 
> while (true) {
>   switch (mode_) {
>     case kLowLatency:
>       DCHECK_EQ(output_params are LOW_LATENCY);
>       if (dispatcher_->Open())
>         return true;
>       if (fallback not disabled) {
>         mode_ = kHighLatency;
>         output_params_ = SetupFallbackParams();
>         Initialize();
>         continue;
>       }
>       return false;
> 
>     case kHighLatency:
>       DCHECK_EQ(output_params are PCM_LINEAR);
>       DCHECK(fallback flag is enabled)
>       return dispatcher_->Open();
>   }
> }
> 
> 
> Any ideas?

I'm not sure it's any more readable, but I've uploaded two patch sets, the first
(#3) using the old method and the second (#4) with a semi-state machine based
approach modeled after your example.  For ~40 lines of code, I don't think it
really matters which approach we use, but the new one will need extra scrubbing
if we want to land it since it's not as vetted.

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:319: void
AudioOutputResampler::CloseStream(AudioOutputProxy* stream_proxy) {
On 2012/09/21 01:24:18, scherkus wrote:
> to confirm.. we never have to reset streams_opened_ to false?

Nope, especially since AudioOutputDispatcherImpl may be holding on to streams
under the hood.  Once an open has succeeded we can't take the fallback path.

https://codereview.chromium.org/10958020/diff/4003/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:337: DCHECK(HasOneRef()) << "Only the
AudioManager should hold a reference";
On 2012/09/21 01:24:18, scherkus wrote:
> did this end up firing?

During testing when I messed up the AOP/AOC state machine, yes.  It's a check
which AODI used to enforce which it can't any more since AOR is the primary
interface and holds a single reference to AODI.

[DaleCurtis, 2012-09-21 03:21:39]

+henrika fyi.

[scherkus (not reviewing), 2012-09-21 03:27:33]

LGTM w/ old OpenStream

It should get cleaned up along with AOP/AODimpl
On Sep 20, 2012 8:21 PM, <dalecurtis@chromium.org> wrote:

> +henrika fyi.
>
>
http://codereview.chromium.**org/10958020/<http://codereview.chromium.org/109...
>

[commit-bot: I haz the power, 2012-09-21 03:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10958020/5012

[tommi (sloooow) - chröme, 2012-09-21 08:46:27]

Great work Dale!

https://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_d...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_d...
media/audio/audio_output_dispatcher_impl.cc:48: paused_proxies_--;
I'm curious - why not move the ++ below the if instead?

https://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_p...
File media/audio/audio_output_proxy_unittest.cc (right):

https://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_p...
media/audio/audio_output_proxy_unittest.cc:872: TEST_F(AudioOutputResamplerTest,
LowLatencyOpenEventuallyFails) {
nice!

https://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:338: DCHECK_EQ(MessageLoop::current(),
message_loop_);
thanks for adding these DCHECKs. Aside from the runtime checks they also make
the implementation much easier to understand.

[henrika (OOO until Aug 14), 2012-09-21 09:27:30]

Thanks Dale!

Glad to see that we are now making the changes on platform-generic level and
that you seem to have found the root cause. 

I also feel that your work also shows that we need to simplify this chain since
it makes debugging very difficult in its current state.

[henrika (OOO until Aug 14), 2012-09-21 09:27:40]

https://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_p...
File media/audio/audio_output_proxy_unittest.cc (right):

https://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_p...
media/audio/audio_output_proxy_unittest.cc:872: TEST_F(AudioOutputResamplerTest,
LowLatencyOpenEventuallyFails) {
+1

[DaleCurtis, 2012-09-21 17:07:59]

Sorry all this mess ended up being some bad code on my part guys.  I completely
agree with Henrik that we need to collapse this unholy chain of classes into
something more readable.

The headaches AudioOutputProxy -> AudioOutputResampler ->
AudioOutputDispatcherImpl -> AudioOutputDispatcher and their interactions with
both AudioOutputController and AudioManager*  are extremely hard to follow.

AudioOutputMixer is a good step in the right direction since it does away with
AudioOutputDispatcherImpl and could even do away with
AudioOutputProxy/AudioOutputDispatcher.

As we're implementing channel mixing in the coming weeks I'll keep an eye out
for ways we can simplify this beast.

http://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher_impl.cc (right):

http://codereview.chromium.org/10958020/diff/5012/media/audio/audio_output_di...
media/audio/audio_output_dispatcher_impl.cc:48: paused_proxies_--;
On 2012/09/21 08:46:27, tommi wrote:
> I'm curious - why not move the ++ below the if instead?

That would have made too much sense for this class. :) I'll tag it for cleanup.