Media Remoting: Add RemotingController.

media/remoting/remoting_controller.h

Index: media/remoting/remoting_controller.h
diff --git a/media/remoting/remoting_controller.h b/media/remoting/remoting_controller.h
new file mode 100644
index 0000000000000000000000000000000000000000..5120d3dc59caaa6b71d51504b976a8d450157865
--- /dev/null
+++ b/media/remoting/remoting_controller.h
@@ -0,0 +1,87 @@
+// 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.
+
+#ifndef MEDIA_REMOTING_REMOTING_CONTROLLER_H_
+#define MEDIA_REMOTING_REMOTING_CONTROLLER_H_
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "media/base/mediaplayer_observer.h"
+#include "media/mojo/interfaces/remoting.mojom.h"
+#include "mojo/public/cpp/bindings/binding.h"
+
+// This class does the following:
+// 1) Sends/Receives messages from/to Remoter;
+// 2) Monitors player events as a MediaPlayerObserver;
+// 3) May trigger the switch of the media renderer between local playback
+// and remoting.
+//
+namespace media {
+
+class MEDIA_EXPORT RemotingController final : public MediaPlayerObserver,

[xhwang, 2016/10/01 07:12:15]

You should NOT use MEDIA_EXPORT since this class doesn't belong to the "media"
target.

Actually since media/remoting is a source_set, I think you don't need *_EXPORT.
Could you please check whether you really need this? If you do, media_remoting
should have it's own EXPORT, e.g. MEDIA_REMOTING_EXPORT, similar to
MEDIA_BLINK_EXPORT.

[xjz, 2016/10/03 22:31:09]

Done.

+                                              public mojom::RemotingSource {
+ public:
+  // |remoter_factory| is just being used to create a Remoter and need not
+  // remain valid after the constructor returns.

[xhwang, 2016/10/01 07:12:15]

It's a bit odd that you pass in a factory and use it immediately in the ctor. In
that case, why don't you call remoter_factory->Create() first, then construct
this class, and pass in the RemotingSourceRequest and RemoterPtr? IMHO that
feels more natural and you don't need this comment any more.

[xjz, 2016/10/03 22:31:08]

Done.

+  explicit RemotingController(mojom::RemoterFactory* remoter_factory);
+  ~RemotingController() override;
+
+  // RemotingSource implementations.
+  void OnSinkAvailable() override;
+  void OnSinkGone() override;
+  void OnStarted() override;
+  void OnStartFailed(mojom::RemotingStartFailReason reason) override;
+  void OnMessageFromSink(const std::vector<uint8_t>& message) override;
+  void OnStopped(mojom::RemotingStopReason reson) override;

[xhwang, 2016/10/01 07:12:15]

s/reson/reason

[xjz, 2016/10/03 22:31:09]

Done.

+
+  // MediaPlayerObserver implementation.
+  void OnEnteredFullscreen() override;
+  void OnExitedFullscreen() override;
+  void OnSetCdm(CdmContext* cdm_context) override;
+  void OnDecoderConfigChanged(const AudioDecoderConfig& audio_config,
+                              const VideoDecoderConfig& video_config) override;

[xhwang, 2016/10/01 07:12:15]

nit: empty line here

[xjz, 2016/10/03 22:31:09]

Done.

+  using SwitchRendererCallback = base::Callback<void()>;
+  void SetSwitchRenderCallback(const SwitchRendererCallback& cb);
+
+  // Tells which renderer should be used.
+  bool is_remoting() const {
+    DCHECK(task_runner_->BelongsToCurrentThread());
+    return is_remoting_;
+  }
+
+ private:
+  bool isVideoConfigSupported();
+  bool isAudioConfigSupported();

[xhwang, 2016/10/01 07:12:15]

nit: s/is/Is

[xjz, 2016/10/03 22:31:08]

Done.

+
+  // Determines whether to enter or leave Remoting mode and switches if
+  // necessary.
+  void Update();

[xhwang, 2016/10/01 07:12:15]

nit: this function name seems too generic and doesn't reflect what it does...

[xjz, 2016/10/03 22:31:09]

Renamed as UpdateAndMaybeSwitch.

+
+  // Indicates if this media element or its ancestor enters full screen.
+  bool is_fullscreen_;

[xhwang, 2016/10/01 07:12:15]

nit: you can specify default value here, but different folder has different
rule, so it's totally up to you.

[xjz, 2016/10/03 22:31:09]

Leave it with the constructor. :)

+
+  // Indicates the remoting sink availablity.
+  bool is_sink_available_;
+
+  // Indicates if remoting is started.
+  bool is_remoting_;
+
+  // Current audio/video config.
+  VideoDecoderConfig video_decoder_config_;
+  AudioDecoderConfig audio_decoder_config_;
+
+  // The callback to switch the media renderer.
+  SwitchRendererCallback switch_renderer_cb_;
+
+  mojo::Binding<mojom::RemotingSource> binding_;
+  mojom::RemoterPtr remoter_;
+
+  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

[xhwang, 2016/10/01 07:12:15]

Looks like this class single threaded? If so, add a comment.

Also, it seems you only use |task_runner_| to make sure everything runs on the
same thread. In this case, you can use ThreadChecker:

https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?q=Thread...

[xjz, 2016/10/03 22:31:09]

In the up-coming changes, we may need to add media thread task runner to handle
the received RPC messages. So leave this as it is for now.

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

Please still add a comment about the threading model. You can update it in a
later CL.

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

Added a TODO comment.

+
+  DISALLOW_COPY_AND_ASSIGN(RemotingController);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_REMOTING_REMOTING_CONTROLLER_H_