Media Remoting: Add RemotingController.

media/remoting/remoting_controller.cc

+// Copyright 2016 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/remoting/remoting_controller.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "media/base/bind_to_current_loop.h"

[xhwang, 2016/10/04 06:30:29]

nit: not needed?

[xjz, 2016/10/04 19:21:29]

Done.

+
+namespace media {
+
+RemotingController::RemotingController(
+    mojom::RemotingSourceRequest source_request,
+    mojom::RemoterPtr remoter)
+    : is_fullscreen_(false),
+      is_sink_available_(false),
+      is_remoting_(false),
+      binding_(this, std::move(source_request)),
+      remoter_(std::move(remoter)),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_factory_(this) {}

[xhwang, 2016/10/04 06:30:29]

|has_audio| and |has_video| are not initialized. This could cause tricky issue
that's hard to debug. That's why I like the "Non-Static Class Member
Initializers" :)

https://chromium-cpp.appspot.com/

[xjz, 2016/10/04 19:21:29]

Done.

+
+RemotingController::~RemotingController() {}
+
+void RemotingController::OnSinkAvailable() {
+  DCHECK(task_runner_->BelongsToCurrentThread());

[xhwang, 2016/10/04 06:30:29]

Add include for base::SingleThreadTaskRunner

[xjz, 2016/10/04 19:21:29]

Done.

+
+  is_sink_available_ = true;
+  UpdateAndMaybeSwitch();
+}
+
+void RemotingController::OnSinkGone() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  is_sink_available_ = false;
+  UpdateAndMaybeSwitch();
+}
+
+void RemotingController::OnStarted() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  VLOG(1) << "Remoting started successively.";
+  if (is_remoting_)
+    switch_renderer_cb_.Run();
+  else
+    remoter_->Stop(mojom::RemotingStopReason::LOCAL_PLAYBACK);
+}
+
+void RemotingController::OnStartFailed(mojom::RemotingStartFailReason reason) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  VLOG(1) << "Failed to start remoting:" << reason;
+  is_remoting_ = false;
+}
+
+void RemotingController::OnMessageFromSink(
+    const std::vector<uint8_t>& message) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  // TODO(xjz): Merge with Eric's CL to handle the RPC messages here.
+  NOTIMPLEMENTED();
+}
+
+void RemotingController::OnStopped(mojom::RemotingStopReason reason) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  VLOG(1) << "Remoting stopped: " << reason;
+  is_remoting_ = false;
+}
+
+void RemotingController::OnEnteredFullscreen() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  is_fullscreen_ = true;
+  UpdateAndMaybeSwitch();
+}
+
+void RemotingController::OnExitedFullscreen() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  is_fullscreen_ = false;
+  UpdateAndMaybeSwitch();
+}
+
+void RemotingController::OnSetCdm(CdmContext* cdm_context) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  // TODO(xjz): Not implemented. Will add in up-coming change.
+  NOTIMPLEMENTED();
+}
+
+void RemotingController::SetSwitchRenderCallback(
+    const SwitchRendererCallback& cb) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(!cb.is_null());
+
+  switch_renderer_cb_ = cb;
+}
+
+void RemotingController::OnMetadata(const PipelineMetadata& metadata) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  has_video_ = metadata.has_video;
+  has_audio_ = metadata.has_audio;
+  DCHECK(has_video_ || has_audio_);
+
+  if (has_video_) {
+    DCHECK(metadata.video_decoder_config.IsValidConfig());
+    video_decoder_config_ = metadata.video_decoder_config;
+  }
+  if (has_audio_) {
+    DCHECK(metadata.audio_decoder_config.IsValidConfig());
+    audio_decoder_config_ = metadata.audio_decoder_config;
+  }
+  UpdateAndMaybeSwitch();
+}
+
+bool RemotingController::IsVideoConfigSupported() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  if (!has_video_)
+    return true;

[xhwang, 2016/10/04 06:30:29]

API-wise, it's really odd that IsVideoConfigSupported() returns true when
there's no video...

It'll probably be cleaner for the caller to check 

if (has_video_ && IsVideoConfigSupported())

Then you can DCHECK(has_video) here.

[xjz, 2016/10/04 19:21:29]

Done.

+
+  switch (video_decoder_config_.codec()) {
+    case VideoCodec::kCodecH264:
+    case VideoCodec::kCodecVP8:
+      return true;
+    default:
+      VLOG(2) << "Remoting does not support video codec: "
+              << video_decoder_config_.codec();
+      return false;
+  }
+}
+
+bool RemotingController::IsAudioConfigSupported() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  if (!has_audio_)
+    return true;

[xhwang, 2016/10/04 06:30:30]

ditto

[xjz, 2016/10/04 19:21:29]

Done.

+
+  switch (audio_decoder_config_.codec()) {
+    case AudioCodec::kCodecAAC:
+    case AudioCodec::kCodecMP3:
+    case AudioCodec::kCodecPCM:
+    case AudioCodec::kCodecVorbis:
+    case AudioCodec::kCodecFLAC:
+    case AudioCodec::kCodecAMR_NB:
+    case AudioCodec::kCodecAMR_WB:
+    case AudioCodec::kCodecPCM_MULAW:
+    case AudioCodec::kCodecGSM_MS:
+    case AudioCodec::kCodecPCM_S16BE:
+    case AudioCodec::kCodecPCM_S24BE:
+    case AudioCodec::kCodecOpus:
+    case AudioCodec::kCodecEAC3:
+    case AudioCodec::kCodecPCM_ALAW:
+    case AudioCodec::kCodecALAC:
+    case AudioCodec::kCodecAC3:
+      return true;
+    default:
+      VLOG(2) << "Remoting does not support audio codec: "
+              << audio_decoder_config_.codec();
+      return false;
+  }
+}
+
+void RemotingController::UpdateAndMaybeSwitch() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  // TODO(xjz): The switching logic for encrypted content will be added in a
+  // later CL.
+
+  // Demuxer is not initialized yet.
+  if (!has_audio_ && !has_video_)
+    return;
+
+  bool should_be_remoting =
+      is_sink_available_ && is_fullscreen_ && IsVideoConfigSupported() &&
+      IsAudioConfigSupported() && !video_decoder_config_.is_encrypted() &&

[xhwang, 2016/10/04 06:30:30]

What if the audio is encrypted?

[xjz, 2016/10/04 19:21:29]

Add check for encrypted audio too.

+      !switch_renderer_cb_.is_null();

[xhwang, 2016/10/04 06:30:29]

In what case can |switch_renderer_cb_| be null? Can we assume it'll always be
non-null and DCHECK?

[xhwang, 2016/10/04 06:30:30]

This line is a bit hard to reason about. Could you please add some comments
summarizing the condition that we could enter remoting mode?

Also, you could wrap this into a function like 

bool ShouldBeRemoting()

The nice thing about a helper function is that you can now return early, for
example:

bool ShouldBeRemoting() {
  // Only do remoting when the tab is being casted blabla
  if (!is_sink_available_)
    return false;

  // Add comments
  if (!is_fullscreen_)
    return false;

  ......
}

Then you can put comments for each condition and the code will be much easier to
read.

[xjz, 2016/10/04 19:21:29]

Done.

[xjz, 2016/10/04 19:21:29]

Done.

+  if (is_remoting_ == should_be_remoting)
+    return;
+
+  // Switch between local and remoting.
+  is_remoting_ = should_be_remoting;
+  if (is_remoting_) {
+    remoter_->Start();

[xhwang, 2016/10/04 06:30:29]

Please add a comment that switch_renderer_cb_.Run() will be called after Start()
finished (OnStarted()).

[xjz, 2016/10/04 19:21:29]

Done.

+  } else {
+    switch_renderer_cb_.Run();
+    remoter_->Stop(mojom::RemotingStopReason::LOCAL_PLAYBACK);
+  }
+}
+
+}  // namespace media