DBus MediaAnalyticsClient and media_perception pb.

extensions/browser/api/media_perception_private/media_perception_api_manager.cc

+// 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 "extensions/browser/api/media_perception_private/media_perception_api_manager.h"
+
+#include "base/lazy_instance.h"
+#include "base/memory/ptr_util.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/media_analytics_client.h"
+#include "extensions/browser/event_router.h"
+#include "extensions/browser/extension_function.h"
+
+namespace mpp = extensions::api::media_perception_private;

[tbarzic, 2017/04/27 20:37:36]

can you use more meaningful name here?

[Luke Sorenson, 2017/05/03 23:56:07]

Done.

+
+namespace extensions {
+
+namespace {
+
+// p_result is owned by the caller.
+void PointProtoToIdl(const mri::Point& point, mpp::Point* p_result) {
+  if (point.has_x()) {
+    p_result->x.reset(new double(point.x()));
+  }
+  if (point.has_y()) {
+    p_result->y.reset(new double(point.y()));
+  }
+}
+
+// bbox_result is owned by the caller.
+void BoundingBoxProtoToIdl(const mri::BoundingBox& bounding_box,
+                           mpp::BoundingBox* bbox_result) {
+  if (bounding_box.has_normalized()) {
+    bbox_result->normalized.reset(new bool(bounding_box.normalized()));
+  }
+  if (bounding_box.has_top_left()) {
+    bbox_result->top_left.reset(new mpp::Point());

[tbarzic, 2017/04/27 20:37:36]

suiggestion:
use base::MakeUnique to create unique_ptrs

[Luke Sorenson, 2017/05/03 23:56:07]

In this case, the unique_ptrs already exist, because they are member variables
of the generated object (from idl) so I don't think base::MakeUnique is
applicable? 

Example of other code using a similar pattern:
https://cs.chromium.org/chromium/src/extensions/browser/api/webcam_private/we...

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

yes, this works, but I think

bbox_result->top_left = base::MakeUnique<mpp::Point>();

is easier to read.

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

Seems strange to waste CPU (even if it is fairly negligible) to re-construct the
entire std::unique_ptr object, when I can just replace the pointer with reset()
:) Is there a style guide which specifies not to use reset()?

[tbarzic, 2017/05/08 23:04:55]

I don't think there's a particular syle guide for this - I'm fine keeping it as
is.

[rkc1, 2017/05/08 23:34:09]

Look at the comment above base::MakeUnique. It specifically states that bare
calls to new should be treated with scrutiny.
Additionally, this does not take up any additional instructions - the compiler
is smart enough to optimize this.

In general when writing code, don't make assumptions on what will generate more
instructions at a language level. Compilers now are very smart and will be able
to see through things like this. When compilers do not do the right thing, we
should be fixing the compilers, not writing our code to not trigger a bug in a
particular compiler.

[Luke Sorenson, 2017/05/09 17:39:45]

Done. Changes made in https://codereview.chromium.org/2858353002/ (will appear
in new patch set)

+    PointProtoToIdl(bounding_box.top_left(), bbox_result->top_left.get());
+  }
+  if (bounding_box.has_bottom_right()) {
+    bbox_result->bottom_right.reset(new mpp::Point());
+    PointProtoToIdl(bounding_box.bottom_right(),
+                    bbox_result->bottom_right.get());
+  }
+}
+
+mpp::Entity EntityProtoToIdl(const mri::Entity& entity) {
+  mpp::Entity e_result;
+  if (entity.has_id()) {
+    e_result.id.reset(new int(entity.id()));
+  }
+  if (entity.has_type()) {
+    switch (entity.type()) {
+      case mri::Entity::FACE:
+        e_result.type = mpp::ENTITY_TYPE_FACE;
+        break;
+      case mri::Entity::PERSON:
+        e_result.type = mpp::ENTITY_TYPE_PERSON;
+        break;
+      default:
+        e_result.type = mpp::ENTITY_TYPE_UNSPECIFIED;
+    }
+  }
+  if (entity.has_confidence()) {
+    e_result.confidence.reset(new double(entity.confidence()));
+  }
+  if (entity.has_bounding_box()) {
+    e_result.bounding_box.reset(new mpp::BoundingBox());
+    BoundingBoxProtoToIdl(entity.bounding_box(), e_result.bounding_box.get());
+  }
+  return e_result;
+}
+
+mpp::FramePerception FramePerceptionProtoToIdl(
+    const mri::FramePerception& frame_perception) {
+  mpp::FramePerception fp_result;
+  if (frame_perception.has_frame_id()) {
+    fp_result.frame_id.reset(new int(frame_perception.frame_id()));
+  }
+  if (frame_perception.has_frame_width_in_px()) {
+    fp_result.frame_width_in_px.reset(
+        new int(frame_perception.frame_width_in_px()));
+  }
+  if (frame_perception.has_frame_height_in_px()) {
+    fp_result.frame_height_in_px.reset(
+        new int(frame_perception.frame_height_in_px()));
+  }
+  if (frame_perception.has_timestamp()) {
+    fp_result.timestamp.reset(new double(frame_perception.timestamp()));
+  }
+  if (frame_perception.entity_size() > 0) {
+    fp_result.entities.reset(new std::vector<mpp::Entity>());
+    for (const auto& entity : frame_perception.entity()) {
+      fp_result.entities->emplace_back(EntityProtoToIdl(entity));
+    }
+  }
+  return fp_result;
+}
+
+mpp::PerceptionSample PerceptionSampleProtoToIdl(
+    const mri::PerceptionSample& perception_sample) {
+  mpp::PerceptionSample ps_result;
+  if (perception_sample.has_frame_perception()) {
+    ps_result.frame_perception.reset(new mpp::FramePerception(
+        FramePerceptionProtoToIdl(perception_sample.frame_perception())));
+  }
+  // TODO(lasoren): Implement ImageFrameProtoToIdl.
+  return ps_result;
+}
+
+}  // namespace
+
+mpp::State StateProtoToIdl(const mri::State& state) {
+  mpp::State s_result;
+  if (state.has_status()) {
+    switch (state.status()) {
+      case mri::State::UNINITIALIZED:
+        s_result.status = mpp::STATUS_UNINITIALIZED;
+        break;
+      case mri::State::STARTED:
+        s_result.status = mpp::STATUS_STARTED;
+        break;
+      case mri::State::RUNNING:
+        s_result.status = mpp::STATUS_RUNNING;
+        break;
+      case mri::State::SUSPENDED:
+        s_result.status = mpp::STATUS_SUSPENDED;
+        break;
+      default:
+        // Status unset.
+        break;
+    }
+  }
+  if (state.has_device_context()) {
+    s_result.device_context.reset(new std::string(state.device_context()));
+  }
+  return s_result;
+}
+
+mri::State StateIdlToProto(const mpp::State& state) {
+  mri::State s_result;
+  switch (state.status) {
+    case mpp::STATUS_UNINITIALIZED:
+      s_result.set_status(mri::State::UNINITIALIZED);
+      break;
+    case mpp::STATUS_STARTED:
+      s_result.set_status(mri::State::STARTED);
+      break;
+    case mpp::STATUS_RUNNING:
+      s_result.set_status(mri::State::RUNNING);
+      break;
+    case mpp::STATUS_SUSPENDED:
+      s_result.set_status(mri::State::SUSPENDED);
+      break;
+    default:
+      // Status unset.
+      break;
+  }
+  if (state.device_context) {
+    s_result.set_device_context(*state.device_context);
+  }
+  return s_result;
+}
+
+mpp::MediaPerception MediaPerceptionProtoToIdl(
+    const mri::MediaPerception& media_perception) {
+  mpp::MediaPerception mpp_result;
+  if (media_perception.has_timestamp()) {
+    mpp_result.timestamp.reset(new double(media_perception.timestamp()));
+  }
+  if (media_perception.frame_perception_size() > 0) {
+    mpp_result.frame_perceptions.reset(new std::vector<mpp::FramePerception>());
+    for (const auto& frame_perception : media_perception.frame_perception()) {
+      mpp_result.frame_perceptions->emplace_back(
+          FramePerceptionProtoToIdl(frame_perception));
+    }
+  }
+  return mpp_result;
+}
+
+mpp::Diagnostics DiagnosticsProtoToIdl(const mri::Diagnostics& diagnostics) {
+  mpp::Diagnostics d_result;
+  if (diagnostics.perception_sample_size() > 0) {
+    d_result.perception_samples.reset(new std::vector<mpp::PerceptionSample>());
+    for (const auto& perception_sample : diagnostics.perception_sample()) {
+      d_result.perception_samples->emplace_back(
+          PerceptionSampleProtoToIdl(perception_sample));
+    }
+  }
+  return d_result;
+}
+
+// static
+MediaPerceptionAPIManager* MediaPerceptionAPIManager::Get(
+    content::BrowserContext* context) {
+  return GetFactoryInstance()->Get(context);
+}
+
+static base::LazyInstance<
+    BrowserContextKeyedAPIFactory<MediaPerceptionAPIManager>>::DestructorAtExit
+    g_factory = LAZY_INSTANCE_INITIALIZER;
+
+// static
+BrowserContextKeyedAPIFactory<MediaPerceptionAPIManager>*
+MediaPerceptionAPIManager::GetFactoryInstance() {
+  return g_factory.Pointer();
+}
+
+MediaPerceptionAPIManager::MediaPerceptionAPIManager(
+    content::BrowserContext* context)
+    : browser_context_(context), analytics_process_running_(false) {}
+
+MediaPerceptionAPIManager::~MediaPerceptionAPIManager() {}
+
+void MediaPerceptionAPIManager::GetState(const APIStateCallback& callback) {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  api_state_callback_ = callback;

[tbarzic, 2017/04/27 20:37:36]

This won't work if there is another request before the current one finishes.
You can bind callback as an argument to the callback passed to
dbus_client->State:

dbus_client->State(nullptr, 0,
base::Bind(&MediaPerceptionAPIManager::StateCallback,
                                          base::Unretained(this),
                                          callback));

Then have:

void MediaPerceptionAPIManager::StateCallback(const APIStateCallback& callback,
                                              bool succeeded,
                                              const uint8_t* bytes,
                                              size_t length) {
  // Do stuff
}

[Luke Sorenson, 2017/05/03 23:56:07]

Done.

+  dbus_client->State(nullptr, 0,
+                     base::Bind(&MediaPerceptionAPIManager::StateCallback,
+                                base::Unretained(this)));
+}
+
+void MediaPerceptionAPIManager::SetState(const mpp::State& state,
+                                         const APIStateCallback& callback) {
+  desired_state_ = StateIdlToProto(state);
+  api_state_callback_ = callback;
+
+  if (!analytics_process_running_) {
+    chromeos::MediaAnalyticsClient* dbus_client =
+        chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+    dbus_client->StartMediaAnalytics(base::Bind(

[tbarzic, 2017/04/27 20:37:36]

doing this if state is anything else than STARTED seems wrong.

[Luke Sorenson, 2017/05/03 23:56:07]

Done. Although in this case it would be for SetState RUNNING. For API, only
RUNNING and STARTED are allowed for SetState and I added that check.

+        &MediaPerceptionAPIManager::UpstartCallback, base::Unretained(this)));
+  } else {
+    SetStateInternal();
+  }
+}
+
+void MediaPerceptionAPIManager::SetStateInternal() {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  int size = desired_state_.ByteSize();
+  uint8_t buffer[size];
+  desired_state_.SerializeToArray(buffer, size);
+  dbus_client->State(buffer, size,
+                     base::Bind(&MediaPerceptionAPIManager::StateCallback,
+                                base::Unretained(this)));
+}
+
+void MediaPerceptionAPIManager::GetDiagnostics(
+    const APIGetDiagnosticsCallback& callback) {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  api_get_diagnostics_callback_ = callback;
+  dbus_client->GetDiagnostics(
+      base::Bind(&MediaPerceptionAPIManager::GetDiagnosticsCallback,
+                 base::Unretained(this)));
+}
+
+void MediaPerceptionAPIManager::UpstartCallback(bool succeeded) {
+  if (succeeded) {
+    analytics_process_running_ = true;
+    SetStateInternal();
+  } else {
+    analytics_process_running_ = false;
+    LOG(ERROR) << "Failed to start media analytics process via Upstart.";
+    // TODO(lasoren): Consider propagating error up to the frontend.

[tbarzic, 2017/04/27 20:37:36]

you should invoke the API callback so the reference to the API function is
released (and the app doesn't hang)

[Luke Sorenson, 2017/05/03 23:56:07]

Done.

+  }
+}
+
+void MediaPerceptionAPIManager::StateCallback(bool succeeded,
+                                              const uint8_t* bytes,
+                                              size_t length) {
+  mpp::State state;
+  if (!succeeded) {
+    state.status = mpp::STATUS_TIMEOUT;
+    api_state_callback_.Run(false, std::move(state));
+    return;
+  }
+  mri::State state_proto;
+  if (!state_proto.ParseFromArray(bytes, length)) {
+    LOG(ERROR) << "Failed to parse State message.";
+    api_state_callback_.Run(false, std::move(state));
+    return;
+  }
+  // If the media analytics process is started, register an event handler to get
+  // detections on the frontend.
+  if (state_proto.status() == mri::State::STARTED) {

[tbarzic, 2017/04/27 20:37:35]

This seems a little too magical :/

Does this mean that an app would have to poll GetState until it gets "STARTED"
state in order to receive media perception events?

Can this handler be added when MediaPerceptionAPIManager is created, or maybe
when an app adds OnMediaPerception listener?

[Luke Sorenson, 2017/05/03 23:56:07]

The app wouldn't need to poll until it gets STARTED, SetState also triggers the
StateCallback. STARTED is the immediate reply from the media analytics client
after you set the state to RUNNING.

I still think its less error prone to have the listener in the constructor so I
moved it there.

+    chromeos::MediaAnalyticsClient* dbus_client =
+        chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+    dbus_client->SetMediaPerceptionSignalHandler(
+        base::Bind(&MediaPerceptionAPIManager::MediaPerceptionSignalHandler,
+                   base::Unretained(this)));

[tbarzic, 2017/04/27 20:37:36]

Can we use weak ptr here instead of unretained.


Also, the handler should probably be unset when this is destructed.

[Luke Sorenson, 2017/05/03 23:56:07]

Handler unset when the class is destructed.

What would the syntax of weak ptr be in this case?

+  }
+  state = StateProtoToIdl(state_proto);
+  api_state_callback_.Run(true, std::move(state));
+}
+
+void MediaPerceptionAPIManager::GetDiagnosticsCallback(bool succeeded,
+                                                       const uint8_t* bytes,
+                                                       size_t length) {
+  mpp::Diagnostics diagnostics;
+  if (!succeeded) {
+    api_get_diagnostics_callback_.Run(false, std::move(diagnostics));

[tbarzic, 2017/04/27 20:37:35]

can we pass in the callback as GetDiagnosticsCallback arg and make
GetDisagnosticsCalblack standalone function?

[Luke Sorenson, 2017/05/03 23:56:07]

Done.

+    return;
+  }
+  mri::Diagnostics diagnostics_proto;
+  if (!diagnostics_proto.ParseFromArray(bytes, length)) {

[tbarzic, 2017/04/27 20:37:36]

I wonder if proto parsing should be done by dbus client (before invoking the
callback)?

[Luke Sorenson, 2017/05/03 23:56:07]

In my prior implementation the media_perception.proto was only accessible to the
API manager (due to violate checkdeps rules) and because the proto was located
in this directory. Now that I have moved the proto to the chromeos/dbus/
directory, I think I agree that the D-Bus client should do the proto parsing.
Making this change.

+    LOG(ERROR) << "Failed to parse State message.";
+    api_get_diagnostics_callback_.Run(false, std::move(diagnostics));
+    return;
+  }
+  diagnostics = DiagnosticsProtoToIdl(diagnostics_proto);
+  api_get_diagnostics_callback_.Run(true, std::move(diagnostics));
+}
+
+void MediaPerceptionAPIManager::MediaPerceptionSignalHandler(
+    const uint8_t* bytes,
+    size_t length) {
+  EventRouter* router = EventRouter::Get(browser_context_);
+  if (router && router->HasEventListener(mpp::OnMediaPerception::kEventName)) {

[tbarzic, 2017/04/27 20:37:36]

if (!router || !router->HasEventListener(...))
  return;

[Luke Sorenson, 2017/05/03 23:56:07]

Done.

+    mri::MediaPerception media_perception;
+    if (!media_perception.ParseFromArray(bytes, length)) {
+      LOG(ERROR) << "Failed to parse MediaPerception message.";
+      return;
+    }
+    mpp::MediaPerception mp_result =
+        MediaPerceptionProtoToIdl(media_perception);
+    std::unique_ptr<Event> event(
+        new Event(events::MEDIA_PERCEPTION_PRIVATE_ON_MEDIA_PERCEPTION,
+                  mpp::OnMediaPerception::kEventName,
+                  mpp::OnMediaPerception::Create(mp_result)));
+    router->BroadcastEvent(std::move(event));
+  }
+}
+
+}  // namespace extensions