MediaPerceptionPrivate API impl and testing.

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 "chromeos/dbus/upstart_client.h"
+#include "extensions/browser/event_router.h"
+#include "extensions/browser/extension_function.h"
+
+namespace media_perception = extensions::api::media_perception_private;
+
+namespace extensions {
+
+namespace {
+
+// p_result is owned by the caller.
+void PointProtoToIdl(const mri::Point& point,

[tbarzic, 2017/05/09 01:28:36]

make this return unique_ptr<media_perception::Point>

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+                     media_perception::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,
+                           media_perception::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 media_perception::Point());
+    PointProtoToIdl(bounding_box.top_left(), bbox_result->top_left.get());
+  }
+  if (bounding_box.has_bottom_right()) {
+    bbox_result->bottom_right.reset(new media_perception::Point());
+    PointProtoToIdl(bounding_box.bottom_right(),
+                    bbox_result->bottom_right.get());
+  }
+}
+
+media_perception::Entity EntityProtoToIdl(const mri::Entity& entity) {
+  media_perception::Entity e_result;
+  if (entity.has_id()) {
+    e_result.id.reset(new int(entity.id()));
+  }
+  if (entity.has_type()) {
+    switch (entity.type()) {

[tbarzic, 2017/05/09 01:28:36]

consider moving this to a helper method
ConvertEntotyType or something along those lines

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+      case mri::Entity::FACE:
+        e_result.type = media_perception::ENTITY_TYPE_FACE;
+        break;
+      case mri::Entity::PERSON:
+        e_result.type = media_perception::ENTITY_TYPE_PERSON;
+        break;
+      default:

[tbarzic, 2017/05/09 01:28:36]

case mri::Entity::UNSPECIFIED:

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+        e_result.type = media_perception::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 media_perception::BoundingBox());
+    BoundingBoxProtoToIdl(entity.bounding_box(), e_result.bounding_box.get());

[tbarzic, 2017/05/09 01:28:36]

e_result.bounding_box = BoundingBoxProtoToIdl(entity.bounding_box());
// and have BoundingBoxProtoToIdl return a unique_ptr

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+  }
+  return e_result;
+}
+
+media_perception::FramePerception FramePerceptionProtoToIdl(
+    const mri::FramePerception& frame_perception) {
+  media_perception::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<media_perception::Entity>());
+    for (const auto& entity : frame_perception.entity()) {
+      fp_result.entities->emplace_back(EntityProtoToIdl(entity));
+    }
+  }
+  return fp_result;
+}
+
+media_perception::PerceptionSample PerceptionSampleProtoToIdl(
+    const mri::PerceptionSample& perception_sample) {
+  media_perception::PerceptionSample ps_result;
+  if (perception_sample.has_frame_perception()) {
+    ps_result.frame_perception.reset(new media_perception::FramePerception(
+        FramePerceptionProtoToIdl(perception_sample.frame_perception())));
+  }
+  // TODO(lasoren): Implement ImageFrameProtoToIdl.
+  return ps_result;
+}
+
+}  // namespace
+
+media_perception::State StateProtoToIdl(const mri::State& state) {
+  media_perception::State s_result;
+  if (state.has_status()) {
+    switch (state.status()) {
+      case mri::State::UNINITIALIZED:
+        s_result.status = media_perception::STATUS_UNINITIALIZED;
+        break;
+      case mri::State::STARTED:
+        s_result.status = media_perception::STATUS_STARTED;
+        break;
+      case mri::State::RUNNING:
+        s_result.status = media_perception::STATUS_RUNNING;
+        break;
+      case mri::State::SUSPENDED:
+        s_result.status = media_perception::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 media_perception::State& state) {
+  mri::State s_result;
+  switch (state.status) {
+    case media_perception::STATUS_UNINITIALIZED:
+      s_result.set_status(mri::State::UNINITIALIZED);
+      break;
+    case media_perception::STATUS_STARTED:
+      s_result.set_status(mri::State::STARTED);
+      break;
+    case media_perception::STATUS_RUNNING:
+      s_result.set_status(mri::State::RUNNING);
+      break;
+    case media_perception::STATUS_SUSPENDED:
+      s_result.set_status(mri::State::SUSPENDED);
+      break;
+    default:
+      // Status unset.

[tbarzic, 2017/05/09 01:28:36]

NOTREACHED() << "Status not set."

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+      break;
+  }
+  if (state.device_context) {
+    s_result.set_device_context(*state.device_context);
+  }
+  return s_result;
+}
+
+media_perception::MediaPerception MediaPerceptionProtoToIdl(
+    const mri::MediaPerception& media_perception) {
+  media_perception::MediaPerception mp_result;
+  if (media_perception.has_timestamp()) {
+    mp_result.timestamp.reset(new double(media_perception.timestamp()));
+  }
+  if (media_perception.frame_perception_size() > 0) {
+    mp_result.frame_perceptions.reset(
+        new std::vector<media_perception::FramePerception>());
+    for (const auto& frame_perception : media_perception.frame_perception()) {
+      mp_result.frame_perceptions->emplace_back(
+          FramePerceptionProtoToIdl(frame_perception));
+    }
+  }
+  return mp_result;
+}
+
+media_perception::Diagnostics DiagnosticsProtoToIdl(
+    const mri::Diagnostics& diagnostics) {
+  media_perception::Diagnostics d_result;
+  if (diagnostics.perception_sample_size() > 0) {
+    d_result.perception_samples.reset(
+        new std::vector<media_perception::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;

[tbarzic, 2017/05/09 01:28:36]

I think this should be leaky

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+
+// static
+BrowserContextKeyedAPIFactory<MediaPerceptionAPIManager>*
+MediaPerceptionAPIManager::GetFactoryInstance() {
+  return g_factory.Pointer();
+}
+
+MediaPerceptionAPIManager::MediaPerceptionAPIManager(
+    content::BrowserContext* context)
+    : browser_context_(context), analytics_process_running_(false) {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->SetMediaPerceptionSignalHandler(
+      base::Bind(&MediaPerceptionAPIManager::MediaPerceptionSignalHandler,
+                 base::Unretained(this)));
+}
+
+MediaPerceptionAPIManager::~MediaPerceptionAPIManager() {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->UnsetMediaPerceptionSignalHandler();
+  // TODO(lasoren): Verify this gets called at the right time.

[tbarzic, 2017/05/09 01:28:36]

What do you mean by this?

[Luke Sorenson, 2017/05/09 21:05:44]

I was curious when the destructor for this class is actually called and tested
this. Removing the TODO.

+  // Stop the separate media analytics process.
+  chromeos::UpstartClient* upstart_client =
+      chromeos::DBusThreadManager::Get()->GetUpstartClient();
+  upstart_client->StopMediaAnalytics();
+}
+
+void MediaPerceptionAPIManager::GetState(const APIStateCallback& callback) {
+  // Return uninitialized state if the media analytics process isn't running.
+  if (!analytics_process_running_) {
+    media_perception::State state_unitialized;
+    state_unitialized.status = media_perception::STATUS_UNINITIALIZED;
+    callback.Run(true, std::move(state_unitialized));
+    return;
+  }
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->State(mri::State(),  // Passing empty state changes nothing.
+                     base::Bind(&MediaPerceptionAPIManager::StateCallback,
+                                base::Unretained(this), callback));
+}
+
+void MediaPerceptionAPIManager::SetState(const media_perception::State& state,
+                                         const APIStateCallback& callback) {
+  mri::State state_proto = StateIdlToProto(state);
+  if (state_proto.status() != mri::State::RUNNING &&

[tbarzic, 2017/05/09 01:28:36]

This can probably be a DCHECK.

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+      state_proto.status() != mri::State::SUSPENDED) {
+    LOG(ERROR) << "Cannot set state to something other than RUNNING or "
+                  "SUSPENDED.";
+    media_perception::State empty_state;
+    callback.Run(false, std::move(empty_state));
+    return;
+  }
+  if (!analytics_process_running_) {

[tbarzic, 2017/05/09 01:28:36]

will we ever need to stop the process (from the app)?

Either way, isn't SUSPENDED state defiend as state where the naalytics process
is running, but not active? Wouldn't that mean StartMediaAnalytics has to be
called in that case as well?

[Luke Sorenson, 2017/05/09 21:05:45]

From a simplicity and security standpoint, we didn't want the app to be capable
of explicitly launching and killing the process, but rather that the process is
launched and killed at the appropriate time and the app doesn't need to care.

+    if (state_proto.status() == mri::State::RUNNING) {
+      chromeos::UpstartClient* dbus_client =
+          chromeos::DBusThreadManager::Get()->GetUpstartClient();
+      dbus_client->StartMediaAnalytics(
+          base::Bind(&MediaPerceptionAPIManager::UpstartCallback,
+                     base::Unretained(this), callback, state_proto));

[tbarzic, 2017/05/09 01:28:37]

Use weak ptr here (and elsewhere where you call into dbus_client)?

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+    } else {
+      media_perception::State empty_state;
+      callback.Run(false, std::move(empty_state));
+    }
+  } else {
+    SetStateInternal(callback, state_proto);
+  }
+}
+
+void MediaPerceptionAPIManager::SetStateInternal(
+    const APIStateCallback& callback,
+    const mri::State& state) {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->State(state,
+                     base::Bind(&MediaPerceptionAPIManager::StateCallback,
+                                base::Unretained(this), callback));
+}
+
+void MediaPerceptionAPIManager::GetDiagnostics(
+    const APIGetDiagnosticsCallback& callback) {
+  chromeos::MediaAnalyticsClient* dbus_client =
+      chromeos::DBusThreadManager::Get()->GetMediaAnalyticsClient();
+  dbus_client->GetDiagnostics(
+      base::Bind(&MediaPerceptionAPIManager::GetDiagnosticsCallback,
+                 base::Unretained(this), callback));
+}
+
+void MediaPerceptionAPIManager::UpstartCallback(
+    const APIStateCallback& callback,
+    const mri::State& state,
+    bool succeeded) {

[tbarzic, 2017/05/09 01:28:36]

if (!success) {
  callback.Run(false, media_perception::State);
  return;
}
analytics_process_running_ = true;
SetStateInternal(callback, state);

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+  if (succeeded) {
+    analytics_process_running_ = true;
+    SetStateInternal(callback, state);
+  } else {
+    analytics_process_running_ = false;
+    LOG(ERROR) << "Failed to start media analytics process via Upstart.";
+    media_perception::State empty_state;
+    callback.Run(false, std::move(empty_state));
+  }
+}
+
+void MediaPerceptionAPIManager::StateCallback(const APIStateCallback& callback,
+                                              bool succeeded,
+                                              const mri::State& state_proto) {
+  media_perception::State state;
+  if (!succeeded) {
+    state.status = media_perception::STATUS_TIMEOUT;
+    callback.Run(false, std::move(state));

[tbarzic, 2017/05/09 01:28:36]

just:
callback.Run(false, media_perception::State);

(the state.status is irelevant, given that the api function responds with an
error when success is false).

[Luke Sorenson, 2017/05/09 21:05:44]

Is there any precedent for / is it possible to respond with both and error and a
State object (so that I can provide things like TIMEOUT as status back to the
app)?

[tbarzic, 2017/05/09 21:19:14]

yes you can.
My preferred approach would be for this to return a status enum (instead of bool
succeeded), and then the api function to set an appropriate error.

Though, you could just have the function normally respond with the state object
set to error (instead of responding with an error)

[Luke Sorenson, 2017/05/10 18:50:16]

Acknowledged.

+    return;
+  }
+  state = StateProtoToIdl(state_proto);
+  callback.Run(true, std::move(state));

[tbarzic, 2017/05/09 01:28:36]

callback.Run(true, StateProtoToIdl(state_proto));

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+}
+
+void MediaPerceptionAPIManager::GetDiagnosticsCallback(
+    const APIGetDiagnosticsCallback& callback,
+    bool succeeded,
+    const mri::Diagnostics& diagnostics_proto) {
+  media_perception::Diagnostics diagnostics;
+  if (!succeeded) {
+    callback.Run(false, std::move(diagnostics));
+    return;

[tbarzic, 2017/05/09 01:28:36]

same as for StateCallback applies

[Luke Sorenson, 2017/05/09 21:05:44]

Done.

+  }
+  diagnostics = DiagnosticsProtoToIdl(diagnostics_proto);
+  callback.Run(true, std::move(diagnostics));
+}
+
+void MediaPerceptionAPIManager::MediaPerceptionSignalHandler(
+    const mri::MediaPerception& media_perception_proto) {
+  EventRouter* router = EventRouter::Get(browser_context_);
+  if (!router || !router->HasEventListener(

[tbarzic, 2017/05/09 01:28:36]

I think you'd be fine without HasEventListener check.

[Luke Sorenson, 2017/05/09 21:05:44]

Does it hurt to leave the check in? Can help with debugging?

[tbarzic, 2017/05/09 21:19:14]

it doesn't really hurt, but it's not that useful either :)

[Luke Sorenson, 2017/05/10 18:50:16]

Acknowledged.

+                     media_perception::OnMediaPerception::kEventName)) {
+    return;
+  }
+  media_perception::MediaPerception media_perception =
+      MediaPerceptionProtoToIdl(media_perception_proto);
+  std::unique_ptr<Event> event(
+      new Event(events::MEDIA_PERCEPTION_PRIVATE_ON_MEDIA_PERCEPTION,
+                media_perception::OnMediaPerception::kEventName,
+                media_perception::OnMediaPerception::Create(media_perception)));
+  router->BroadcastEvent(std::move(event));
+}
+
+}  // namespace extensions