WIP - Add EME

media/remoting/remoting_controller.h

+// 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/media_observer.h"
+#include "media/mojo/interfaces/remoting.mojom.h"
+#include "mojo/public/cpp/bindings/binding.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+}
+
+namespace media {
+struct PipelineMetadata;
+}
+
+// This class does the following:

[miu, 2016/10/08 01:06:16]

This comment should be next to the class declaration, not the namespace
delcaration.

[xjz, 2016/10/20 21:25:28]

Done.

+// 1) Sends/Receives messages from/to Remoter;
+// 2) Monitors player events as a MediaObserver;
+// 3) May trigger the switch of the media renderer between local playback
+// and remoting.
+//
+namespace media {
+
+class RemotingController final : public MediaObserver,

[miu, 2016/10/08 01:06:16]

I think you should consider splitting this class up: The reason is that the
control logic for starting/ending a session around a CDM is significantly
different than that for a WMPI/Renderer, and it seems like we're mixing-up the
internal state and workflow a bit too much. I'll e-mail you some ideas I have
around this.

[xjz, 2016/10/20 21:25:28]

Done.

+                                 public mojom::RemotingSource {
+ public:
+  RemotingController(mojom::RemotingSourceRequest source_request,
+                     mojom::RemoterPtr remoter);
+  ~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 reason) override;
+
+  // MediaObserver implementations.
+  // This is called when the video element or its ancestor enters full screen.
+  // We currently use this as an indicator for immersive playback. May add other
+  // criteria (e.g. the actual display width/height of the video element) in
+  // future.
+  void OnEnteredFullscreen() override;
+  void OnExitedFullscreen() override;
+  void OnSetCdm(CdmContext* cdm_context) override;
+  void OnMetadataChanged(const PipelineMetadata& metadata) override;
+
+  using SwitchRendererCallback = base::Callback<void()>;
+  void SetSwitchRendererCallback(const SwitchRendererCallback& cb);
+
+  base::WeakPtr<RemotingController> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+  // The callback used to tell which CDM should be created.

[miu, 2016/10/08 01:06:16]

This comment isn't really necessary. But, the method needs a comment that's
similar to this. How about:

  // Query whether to create a Media Remoting CDM. |cb| could be run
synchronously or asynchronously, depending on whether the required information
is available now or later.

[xjz, 2016/10/20 21:25:28]

Done.

+  using CdmCheckCallback = base::Callback<void(bool is_remoting)>;
+  void ShouldCreateRemotingCdm(const CdmCheckCallback& cb);
+
+  bool is_remoting() const {
+    DCHECK(task_runner_->BelongsToCurrentThread());
+
+    if (cdm_remoting_controller_)
+      return cdm_remoting_controller_->is_remoting();
+    return is_remoting_;
+  }
+
+  bool is_remoting_failed() const {
+    DCHECK(task_runner_->BelongsToCurrentThread());
+
+    if (cdm_remoting_controller_)
+      return cdm_remoting_controller_->is_remoting_failed();
+
+    // Always false for non-encrypted contents.
+    return false;

[miu, 2016/10/08 01:06:16]

If this is the RemotingController owned by the CDM factory, I think you would
want it to return |is_remoting_failed_| here? If that's right, then the comment
is misleading and should probably be deleted.

[xjz, 2016/10/20 21:25:28]

Not applicable.

+  }
+
+ private:
+  bool IsVideoCodecSupported();
+  bool IsAudioCodecSupported();
+
+  // Helper to decide whether to enter or leave Remoting mode.
+  bool ShouldBeRemoting();
+
+  // Determines whether to enter or leave Remoting mode and switches if
+  // necessary.
+  void UpdateAndMaybeSwitch();
+
+  // Indicates if this media element or its ancestor enters full screen.
+  bool is_fullscreen_ = false;
+
+  // Indicates the remoting sink availablity.
+  bool is_sink_available_ = false;
+
+  // Indicates if remoting is started.
+  bool is_remoting_ = false;

[miu, 2016/10/08 01:06:16]

This is now "sort of" true because it will be false while a remoting session has
been started for the CDM. (This is one of the reasons I'm proposing we split-up
this class.)

[xjz, 2016/10/20 21:25:28]

Not applicable.

+
+  // Indicates whether audio or video is encrypted.
+  bool is_encrypted_ = false;
+
+  // Indicates if remoting cdm is created.
+  bool is_remoting_cdm_ = false;

[miu, 2016/10/08 01:06:16]

This isn't necessary, because this can always be determined from other data
members. You could make an inline helper method for it instead, such as:

  bool is_remoting() const {
    if (cdm_remoting_controller_)
      return cdm_remoting_controller_->is_remoting_;
    return is_remoting_;
  }

[xjz, 2016/10/20 21:25:28]

Not applicable.

+
+  // Indicates that remoting was stopped after CDM is created remotely. In this

[miu, 2016/10/08 01:06:16]

Suggested wording:

// Indicates that the video stack cannot continue operation. For example, if a
remoting session involving CDM content was stopped, there is no way to continue
playback because the CDM is required but is no longer available.

[xjz, 2016/10/20 21:25:28]

Add this comment to RemotingSessionState::SESSION_PERMANENTLY_STOPPED.

+  // case, local playing back can't continue without reloading the page.
+  bool is_remoting_failed_ = false;

[miu, 2016/10/08 01:06:16]

Actually, do you need this bool? Wouldn't it always be equal to:

  (audio_decoder_config_.is_encrypted() ||
   video_decoder_config_.is_encrypted()) &&
  !cdm_remoting_controller_

[miu, 2016/10/08 01:06:16]

naming: How about has_terminated_permanently_.

[xjz, 2016/10/20 21:25:28]

Not applicable.

[xjz, 2016/10/20 21:25:28]

Not applicable.

+
+  // Current audio/video config.
+  VideoDecoderConfig video_decoder_config_;
+  AudioDecoderConfig audio_decoder_config_;
+  bool has_audio_ = false;
+  bool has_video_ = false;
+
+  // The callback to switch the media renderer.
+  SwitchRendererCallback switch_renderer_cb_;
+
+  mojo::Binding<mojom::RemotingSource> binding_;
+  mojom::RemoterPtr remoter_;
+
+  // TODO(xjz): Add a media thread task runner for the received RPC messages for
+  // remoting media renderer in the up-coming change.
+  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  // The callback for checking whether remoting CDM should be created.
+  CdmCheckCallback cdm_check_cb_;

[miu, 2016/10/08 01:06:16]

It seems like this is a "run once" callback. Is that right? If so, you should
reset this callback (e.g., using base::ResetAndReturn()) each place where you
Run() it, since it would be catastrophic if it were called twice.

[xjz, 2016/10/20 21:25:28]

Done.

+
+  // This is the RemotingController that was created when remoting CDM was
+  // created. It was set when OnSetCdm() is called. After it is set, the
+  // |switch_renderer_cb_| is passed to it, and the current object is only used
+  // as a proxy to pass |metadata_| and the query of |is_remoting_| to it.
+  std::unique_ptr<RemotingController> cdm_remoting_controller_;

[miu, 2016/10/08 01:06:16]

It doesn't seem that you should be taking ownership of the other
RemotingController here. Perhaps let RemotingCDM always own it, and use a
WeakPtr here? (This is another reason I think we should split-up this class.)

[xjz, 2016/10/20 21:25:28]

Not applicable.

+
+  PipelineMetadata metadata_;
+
+  base::WeakPtrFactory<RemotingController> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemotingController);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_REMOTING_REMOTING_CONTROLLER_H_