DBus MediaAnalyticsClient and media_perception pb.

chromeos/dbus/media_analytics_client.cc

Index: chromeos/dbus/media_analytics_client.cc
diff --git a/chromeos/dbus/media_analytics_client.cc b/chromeos/dbus/media_analytics_client.cc
new file mode 100644
index 0000000000000000000000000000000000000000..42839a7c91f700a8d7abe2f7041b40260a23839f
--- /dev/null
+++ b/chromeos/dbus/media_analytics_client.cc
@@ -0,0 +1,178 @@
+// Copyright 2017 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 "chromeos/dbus/media_analytics_client.h"
+
+#include <cstdint>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/weak_ptr.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_path.h"
+#include "dbus/object_proxy.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+// The MediaAnalyticsCleint implementation used in production.

[tbarzic, 2017/05/05 21:10:42]

nit:s/Cleint/Client

[Luke Sorenson, 2017/05/08 19:06:07]

Done.

+class MediaAnalyticsClientImpl : public MediaAnalyticsClient {
+ public:
+  MediaAnalyticsClientImpl() : dbus_proxy_(nullptr), weak_ptr_factory_(this) {}
+
+  ~MediaAnalyticsClientImpl() override {}
+
+  void SetMediaPerceptionSignalHandler(
+      const MediaPerceptionSignalHandler& handler) override {
+    media_perception_signal_handler_ = handler;

[tbarzic, 2017/05/05 21:10:42]

DCHECK that the handler was not set before this?

[Luke Sorenson, 2017/05/08 19:06:07]

Done.

+    // Connect to the MediaPerception proto signal.
+    dbus_proxy_->ConnectToSignal(

[tbarzic, 2017/05/05 21:10:42]

should this be done in init instead (or at least only when this is called the
first time)?

Doing something like:
SetMediaPerceptionHandler()
UnseMediaReceptionHandler();
SetMediaPerceptionHandler()

would register 2 signal handlers, and each received signal would invoke the
handler callback twice.

[Luke Sorenson, 2017/05/08 19:06:07]

Done.

+        media_perception::kMediaPerceptionInterface,
+        media_perception::kDetectionSignal,
+        base::Bind(&MediaAnalyticsClientImpl::OnDetectionSignalReceived,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&MediaAnalyticsClientImpl::OnSignalConnected,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void UnsetMediaPerceptionSignalHandler() override {
+    media_perception_signal_handler_.Reset();
+  }
+
+  void State(const mri::State& state, const StateCallback& callback) override {
+    dbus::MethodCall method_call(media_perception::kMediaPerceptionServiceName,
+                                 media_perception::kState);
+    if (state.has_status()) {  // Check that a new state is requested.

[tbarzic, 2017/05/05 21:10:42]

nit: put the comment above if

[Luke Sorenson, 2017/05/08 19:06:07]

Done.

+      int length = state.ByteSize();
+      uint8_t bytes[length];
+      state.SerializeToArray(bytes, length);
+      dbus::MessageWriter writer(&method_call);
+      writer.AppendArrayOfBytes(bytes, length);
+    }
+    dbus_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&MediaAnalyticsClientImpl::OnState,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
+  }
+
+  void GetDiagnostics(const DiagnosticsCallback& callback) override {
+    dbus::MethodCall method_call(media_perception::kMediaPerceptionServiceName,
+                                 media_perception::kGetDiagnostics);
+    // TODO(lasoren): Verify that this timeout setting is sufficient.
+    dbus_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&MediaAnalyticsClientImpl::OnGetDiagnostics,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
+  }
+
+ protected:
+  void Init(dbus::Bus* bus) override {
+    dbus_proxy_ = bus->GetObjectProxy(
+        media_perception::kMediaPerceptionServiceName,
+        dbus::ObjectPath(media_perception::kMediaPerceptionServicePath));
+  }
+
+ private:
+  void OnSignalConnected(const std::string& interface,
+                         const std::string& signal,
+                         bool succeeded) {
+    LOG_IF(ERROR, !succeeded)
+        << "Connect to " << interface << " " << signal << " failed.";
+  }
+
+  // Handler that is triggered when a MediaPerception proto is received from
+  // the media analytics process.
+  void OnDetectionSignalReceived(dbus::Signal* signal) {
+    const uint8_t* bytes = nullptr;
+    size_t length = 0;
+
+    dbus::MessageReader reader(signal);
+
+    if (!reader.PopArrayOfBytes(&bytes, &length)) {
+      LOG(ERROR) << "Invalid detection signal: " << signal->ToString();
+      return;
+    }
+
+    mri::MediaPerception media_perception;
+    if (!media_perception.ParseFromArray(bytes, length)) {
+      LOG(ERROR) << "Failed to parse MediaPerception message.";
+      return;
+    }
+
+    if (!media_perception_signal_handler_.is_null()) {

[tbarzic, 2017/05/05 21:10:42]

maybe do
if (media_perception_signal_handler_.is_null())
  return;
before you start extracting the perception message

[Luke Sorenson, 2017/05/08 19:06:07]

Done.

+      media_perception_signal_handler_.Run(media_perception);
+    }
+  }
+
+  void OnState(const StateCallback& callback, dbus::Response* response) {
+    mri::State state;
+    if (!response) {
+      LOG(ERROR) << "Call to State failed to get response.";
+      callback.Run(false, state);
+      return;
+    }
+
+    const uint8_t* bytes = nullptr;
+    size_t length = 0;
+
+    dbus::MessageReader reader(response);
+    if (!reader.PopArrayOfBytes(&bytes, &length)) {
+      LOG(ERROR) << "Invalid D-Bus response: " << response->ToString();
+      callback.Run(false, state);

[tbarzic, 2017/05/05 21:10:42]

return;

(here and other places you run error callback)

[Luke Sorenson, 2017/05/08 19:06:07]

Done.

+    }
+
+    if (!state.ParseFromArray(bytes, length)) {
+      LOG(ERROR) << "Failed to parse State message.";
+      callback.Run(false, state);
+    }
+
+    callback.Run(true, state);
+  }
+
+  void OnGetDiagnostics(const DiagnosticsCallback& callback,
+                        dbus::Response* response) {
+    mri::Diagnostics diagnostics;
+    if (!response) {
+      LOG(ERROR) << "Call to GetDiagnostics failed to get response.";
+      callback.Run(false, diagnostics);
+      return;
+    }
+
+    const uint8_t* bytes = nullptr;
+    size_t length = 0;
+
+    dbus::MessageReader reader(response);
+    if (!reader.PopArrayOfBytes(&bytes, &length)) {
+      LOG(ERROR) << "Invalid GetDiagnostics response: " << response->ToString();
+      callback.Run(false, diagnostics);
+    }
+
+    if (!diagnostics.ParseFromArray(bytes, length)) {
+      LOG(ERROR) << "Failed to parse Diagnostics message.";
+      callback.Run(false, diagnostics);
+    }
+
+    callback.Run(true, diagnostics);
+  }
+
+  dbus::ObjectProxy* dbus_proxy_;
+
+  // Stores a handler registered for receiving the media_perception.proto byte
+  // array.
+  MediaPerceptionSignalHandler media_perception_signal_handler_;
+
+  // For providing a pointer from an object of this class to bind to callbacks.

[tbarzic, 2017/05/05 21:10:42]

I don't think this comment is needed.

[Luke Sorenson, 2017/05/08 19:06:07]

Done.

+  base::WeakPtrFactory<MediaAnalyticsClientImpl> weak_ptr_factory_;

[tbarzic, 2017/05/05 21:10:42]

DISALLOW_COPY_AND_ASSIGN?

[Luke Sorenson, 2017/05/08 19:06:07]

Done.

+};
+
+MediaAnalyticsClient::~MediaAnalyticsClient() {}
+
+MediaAnalyticsClient* MediaAnalyticsClient::Create() {
+  return new MediaAnalyticsClientImpl;
+}
+
+MediaAnalyticsClient::MediaAnalyticsClient() {}
+
+}  // namespace chromeos