Allow muting/unmuting audio through media track API

media/renderers/renderer_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/renderers/renderer_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 187 matching lines @@
   if (old_rate == 0 && playback_rate > 0)
     video_renderer_->OnTimeStateChanged(true);
   else if (old_rate > 0 && playback_rate == 0)
     video_renderer_->OnTimeStateChanged(false);
 }
 
 void RendererImpl::SetVolume(float volume) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  if (audio_renderer_)
-    audio_renderer_->SetVolume(volume);
+  audio_volume_ = volume;
+  if (!audio_renderer_)
+    return;
+
+  if (audio_muted_) {
+    DVLOG(1) << "SetVolume ignored since audio is muted";

[wolenetz, 2016/04/14 20:43:39]

nit: s/muted/muted because audio stream is enabled/ and rename audio_muted_ to
audio_stream_currently_disabled_. IMHO that would make it more clear the reason
for the muted volume result.

[servolk, 2016/04/15 02:23:25]

Done.

+    return;
+  }
+
+  audio_renderer_->SetVolume(volume);
 }
 
 base::TimeDelta RendererImpl::GetMediaTime() {
   // No BelongsToCurrentThread() checking because this can be called from other
   // threads.
   return time_source_->CurrentMediaTime();
 }
 
 bool RendererImpl::HasAudio() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   return audio_renderer_ != NULL;
 }
 
 bool RendererImpl::HasVideo() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   return video_renderer_ != NULL;
 }
 
+void RendererImpl::OnEnabledAudioStreamsChanged(
+    const std::vector<const DemuxerStream*>& enabledAudioStreams) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DemuxerStream* audio_stream =
+      demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO);
+
+  bool audio_stream_enabled = false;
+  for (const DemuxerStream* demuxer_stream : enabledAudioStreams) {
+    DCHECK_EQ(demuxer_stream->type(), DemuxerStream::AUDIO);

[wolenetz, 2016/04/14 20:43:39]

aside: this DCHECK (and similar for video, below) satisfies my "perhaps" comment
portion on earlier patch set.

[servolk, 2016/04/15 02:23:25]

Acknowledged.

+    if (demuxer_stream == audio_stream)
+      audio_stream_enabled = true;

[wolenetz, 2016/04/14 20:43:39]

I'm not entirely clear how this could ever not be the case. Change to DCHECK +
set the flag?

[servolk, 2016/04/15 02:23:25]

Right, for now this check is always going to succeed, since we only support 1
audio stream max. I'll change to DCHECK + set for now. We can revisit this
later, when we add multi-track support.

[wolenetz, 2016/04/15 22:47:18]

Ack, though it's still not DCHECK + set. Please update.

[servolk, 2016/04/15 23:26:31]

Done.

+  }
+
+  // TODO(servolk): Need to find a way to completely disable audio (i.e. stop

[wolenetz, 2016/04/14 20:43:39]

nit: TODO+wolenetz

[servolk, 2016/04/15 02:23:25]

Done.

+  // audio decoder and switch video to wall clock) when audio stream is
+  // disabled, but for now just set volume to 0. crbug.com/599709
+  if (audio_renderer_) {
+    if (audio_stream_enabled) {

[wolenetz, 2016/04/14 20:43:39]

nit: can a track be enabled while it's already enabled? if so, we're setting the
volume twice in the renderer. Only SetVolume() if the intent is to really change
the volume.

[servolk, 2016/04/15 02:23:25]

It should never happen, since blink sends this notification only if stream state
has actually changed, I'll add a DCHECK(!audio_stream_currently_disabled_) to
ensure that stream state has actually changed.

[wolenetz, 2016/04/15 22:47:18]

Ack, though now (patch set 35) only one of the two cases is DCHECKed. The other
case (audio_stream_enabled && !audio_stream_currently_disabled_) could occur and
not hit a DCHECK (though it would at least now not trigger
audio_renderer_->SetVolume(...) here anymore.

[servolk, 2016/04/15 23:26:31]

Yes, I believe this is the best course of action for now, let me explain. I
actually tried to have DCHECK on both if branches (see patchset #34), but it
caused some test failures. The problem is that currently we initialize
audio_stream_currently_disabled_ to be false, but audio tracks are initially
created in disabled state and then the first audio track is explicitly enabled
by blink a bit later. So if we initially set audio_stream_currently_disabled_ to
false, then later blink enables the first audio track, we get here and
DCHECK(audio_stream_currently_disabled_) fails. Now we have two choices for how
to fix this:
1. We could initialize audio_stream_currently_disabled_ to be true and then
DCHECK and set it to false here. But I was worried that it could potentially
cause regressions, since we ignore SetVolume requests when
audio_stream_currently_disabled_ is true, and the timing of
SetVolume/OnEnabledAudioTracksChanged is not guaranteed when playback starts.
Plus unit tests don't take volume into account when computing audio hashes (see
my comments in the unit test files in this CL). So by initializing
audio_stream_currently_disabled_ to true it would be easy to accidentally
regress something and unit tests would not catch it.
2. Another solution is to have this code, where we only do SetVolume when going
from 'audio disabled' to 'audio enabled' state. By doing this we can still
initialize audio_stream_currently_disabled_ to false and thus guarantee that
SetVolume works exactly the same as before this CL, excluding the cases when
audio track has been explicitly enabled/disabled by the user. Then when playback
is being started we'll skip the unnecessary SetVolume when blink enables the
first audio track, and still allow it to happen when the audio track is
disabled/enabled.
Since changing audio_renderer volume is a temporary solution anyway, I've
decided to go with option #2 as it minimizes the chances of regressing
functionality, and we'll rework it soon enough anyway.

[wolenetz, 2016/04/15 23:52:17]

Acknowledged. That makes sense. Thanks for explaining.

+      DVLOG(3) << __FUNCTION__ << " enabled audio. SetVolume=" << audio_volume_;
+      audio_muted_ = false;
+      audio_renderer_->SetVolume(audio_volume_);
+    } else {
+      DVLOG(3) << __FUNCTION__ << " disabled audio. SetVolume=0";
+      audio_muted_ = true;
+      audio_renderer_->SetVolume(0);
+    }
+  }
+}
+
+void RendererImpl::OnSelectedVideoStreamChanged(
+    const DemuxerStream* selectedVideoStream) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DemuxerStream* video_stream =
+      demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO);
+  if (selectedVideoStream) {
+    DVLOG(3) << __FUNCTION__ << " selectedVideoStream=" << selectedVideoStream;
+    DCHECK_EQ(selectedVideoStream->type(), DemuxerStream::VIDEO);
+    CHECK_EQ(selectedVideoStream, video_stream);
+    // TODO(servolk): Need to find a way to stop video decoding/rendering when

[wolenetz, 2016/04/14 20:43:39]

nit: move this TODO to the else clause, and please include wolenetz in the ()
portion of the TODO.

[servolk, 2016/04/15 02:23:25]

Done.

+    // the video stream is deselected. crbug.com/599709
+  } else {
+    DVLOG(3) << __FUNCTION__ << " selectedVideoStream=none";
+  }
+}
+
 void RendererImpl::DisableUnderflowForTesting() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_UNINITIALIZED);
 
   underflow_disabled_for_testing_ = true;
 }
 
 void RendererImpl::EnableClocklessVideoPlaybackForTesting() {
   DVLOG(1) << __FUNCTION__;
@@ skipping 422 matching lines @@
   }
 
   // After OnError() returns, the pipeline may destroy |this|.
   base::ResetAndReturn(&error_cb_).Run(error);
 
   if (!flush_cb_.is_null())
     base::ResetAndReturn(&flush_cb_).Run();
 }
 
 }  // namespace media