CmaMediaPipelineClient to watch media pipeline status

chromecast/cast/cast_resource.h

+// Copyright 2015 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 CAST_BASE_CAST_RESOURCE_H_

[gunsch, 2015/09/01 22:56:08]

nit: blank line above

[yucliu1, 2015/09/01 23:42:13]

Done.

+#define CAST_BASE_CAST_RESOURCE_H_

[halliwell, 2015/09/01 19:42:23]

nit: this doesn't match path.

[gunsch, 2015/09/01 22:56:08]

nit: move this back to chromecast/base/ :)

[yucliu1, 2015/09/01 23:42:13]

Done.

+
+namespace cast {

[halliwell, 2015/09/01 19:42:23]

nit: really should be an outer namespace chromecast here?

[yucliu1, 2015/09/01 20:15:47]

If this is supposed in cast/, we'd better keep consistency with internal/cast/.

[gunsch, 2015/09/01 22:56:08]

per offline, namespace is chromecast

[yucliu1, 2015/09/01 23:42:13]

Done.

+
+// Interface for resources needed to run application.
+class CastResource {

[halliwell, 2015/09/01 19:42:24]

nit: do we need it to be called CastResource if it's in cast?

+ public:
+  // Resources necessary to run cast apps.
+  enum Resource {

[gunsch, 2015/09/01 22:56:08]

optional naming q: if the class is Resource, should this be ResourceType?

[yucliu1, 2015/09/01 23:42:13]

I think it's better to use Resource since it's a union of different bits.

+    kResourceNone = 0,
+    // All resources necessary to render sounds, for example, audio pipeline,
+    // speaker, etc.
+    kResourceAudio = 1 << 0,

[halliwell, 2015/09/01 19:42:23]

what is audio for and where is it used?  I'm not convinced it actually maps to
anything.

[yucliu1, 2015/09/01 20:15:47]

internal code will use it.

[halliwell, 2015/09/01 20:24:39]

I don't believe it does anything useful with it, but we could discuss offline
since all the relevant code is internal.

+    // All resources necessary to render videos or images, for example, video
+    // pipeline, primary graphics plane, display, etc.

[halliwell, 2015/09/01 19:42:23]

see comment elsewhere, I think we need to separate images from video.

[yucliu1, 2015/09/01 20:15:47]

Single CastResource can hold multiple resources and multiple CastResource could
have the same resource.

[halliwell, 2015/09/01 20:24:39]

That seems ... useless.  The whole point of the resource management API is that
there are specific hardware resources that we have to release in order to give
access to other applications.  The primary display is one of those, it is
managed by the GL driver, and we're just lumping it in with the media pipeline
here.  We should separate them so that applications can be precise about what
hardware resources they need us to release.

[yucliu1, 2015/09/01 20:36:42]

Wait, should MediaPipeline use both audio and video?

+    kResourceScreenPrimary = 1 << 1,
+    // All resources necessary to render overlaid images, for example, secondary
+    // graphics plane, LCD, etc.
+    kResourceScreenSecondary = 1 << 2,
+    // Collection of resources used for display only combined with bitwise or.
+    kResourceDisplayOnly = (kResourceScreenPrimary | kResourceScreenSecondary),
+    // Collection of all resources combined with bitwise or.
+    kResourceAll =
+        (kResourceAudio | kResourceScreenPrimary | kResourceScreenSecondary),
+  };
+
+  class Client {
+   public:
+    virtual void OnResourceAcquired(CastResource* cast_resource) = 0;
+    virtual void OnResourceReleased(CastResource* cast_resource,
+                                    Resource resource) = 0;

[gunsch, 2015/09/01 23:04:15]

I just noticed in the internal CL that "resource" here is documented as
"remain". That seems backwards to me---the obvious thing to do here would be to
indicate which resource was released.

Can you clarify in docs how each of |Client|'s methods are intended to be used?

+
+   protected:
+    virtual ~Client() {}
+  };
+
+  // Called to release resources. Implementation should call
+  // Client::OnResourceReleased when resource is released on its side.
+  virtual void ReleaseResource(Resource resource) = 0;
+  virtual void SetCastResourceClient(Client* client) = 0;

[gunsch, 2015/09/01 22:56:08]

nit: can we put this first, to represent the general order that these calls
occur

+
+ protected:
+  virtual ~CastResource() {}

[gunsch, 2015/09/01 22:56:08]

why is this protected? Doesn't this limit CastResource ownership options?

[yucliu1, 2015/09/01 23:42:13]

With current implementation, CastResource is owned by creator. We don't want to
delete them as "CastResource*".

[gunsch, 2015/09/02 17:10:50]

Agreed, but I think this means that an owner can't have a
scoped_ptr<CastResource>?

[yucliu1, 2015/09/02 17:30:18]

The owner should know the type of CastResource. If they want to manage multiple
CastResources, these CastResource should share some common parts, which means
there's another base class.

+};

[gunsch, 2015/09/01 22:56:08]

DISALLOW_COPY_AND_ASSIGN(CastResource);

plus #include "base/macros.h"

+
+}  // namespace cast
+
+#endif