Mojo Video Capture service in media/capture

media/capture/service/stream_impl.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_CAPTURE_SERVICE_STREAM_IMPL_H_
+#define MEDIA_CAPTURE_SERVICE_STREAM_IMPL_H_
+
+#include <map>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "media/capture/interfaces/video_capture.mojom.h"
+#include "media/capture/service/video_capture_device_client_impl.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/interface_request.h"
+#include "mojo/public/cpp/system/buffer.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+class TimeTicks;
+}
+
+namespace media {
+class VideoFrame;
+}
+
+namespace media {
+
+class MojoVideoFrame;
+
+// This class bridges a VideoCaptureDeviceClientImpl with the mojo pipe where we
+// push captured VideoFrames.
+class StreamImpl : public mojom::VideoCaptureStream,
+                   public VideoCaptureDeviceClientImpl::Delegate {
+ public:
+  // |start_callback| is called when this object is started by its client.
+  // |close_callback| is called when this object is stopped by its client.
+  // |error_callback| is called upon internal error or NotifyError().
+  StreamImpl(mojo::InterfaceRequest<mojom::VideoCaptureStream> request,
+             const base::Closure& start_callback,
+             const base::Closure& close_callback,
+             const base::Closure& error_callback);
+
+  ~StreamImpl() override;
+
+  // mojom::VideoCaptureStream
+  void Start(mojom::VideoCaptureStreamClientPtr client) override;
+  void Stop() override;
+
+  // VideoCaptureDeviceClientImpl::Delegate
+  void OnFrame(const scoped_refptr<MojoVideoFrame>& frame,
+               const base::TimeTicks& timestamp) override;
+  void OnError(const std::string& reason) override;
+
+ private:
+  friend class StreamImplTest;
+
+  // The following Notify* methods mimic those in StreamClient and act as a
+  // bridge to Mojo. They all must not be called until the stream has been
+  // Start()ed.
+
+  // Pushes a |frame| to the client.
+  void NotifyFrameAvailable(const scoped_refptr<MojoVideoFrame>& frame,
+                            const base::TimeTicks& timestamp);
+  void NotifyError(const mojo::String& error);
+
+  // Bound to the client's FrameAvailable message to let it notify us that it's
+  // finished with the VideoFrame identified by |buffer_id|.
+  void OnFrameConsumed(int32_t buffer_id);
+
+  mojo::Binding<mojom::VideoCaptureStream> binding_;
+  const base::Closure start_callback_;
+  const base::Closure close_callback_;
+  const base::Closure error_callback_;
+
+  mojom::VideoCaptureStreamClientPtr client_;
+
+  bool paused_ = false;

[perkj_chrome, 2016/03/04 15:34:02]

not used ?

[mcasas, 2016/03/04 19:59:45]

Done.

+
+  // Map of VideoFrames in use by a client. Each VF gets an id assigned by which
+  // the client knows it. The buffer ID is just a simple running counter.
+  int running_buffer_id_ = 0;
+  std::map<int32_t, const scoped_refptr<MojoVideoFrame>&> in_use_video_frames_;

[magjed_chromium, 2016/03/04 15:41:42]

nit: I would prefer the variable name |video_frames_in_use_| instead.

[magjed_chromium, 2016/03/04 15:41:42]

The value type in this std::map is a const-ref, which looks super dangerous.
Shouldn't it be a scoped_refptr<MojoVideoFrame>, i.e. a non-ref? Also, I don't
see the frames being used anywhere, so why not use a set instead of a map? Is it
a map to keep the frames alive?

[mcasas, 2016/03/04 19:59:45]

Done.
We search for the |buffer_id| in OnFrameConsumed().
  const auto video_frame = video_frames_in_use_.find(buffer_id);

[mcasas, 2016/03/04 19:59:46]

Then we'll change it of course.

[magjed_chromium, 2016/03/07 15:36:24]

Yes, but you never access the mapped frame in the |video_frame| iterator which
is of type std::pair<const key_type,mapped_type>, where key_type is int32_t and
mapped_type is a MojoVideoFrame. Why have a mapped object if you don't access
it? You can use the exact same code video_frames_in_use_.find(buffer_id); where
|video_frames_in_use_| is a set<int32_t>.

[mcasas, 2016/03/07 20:57:49]

Maybe we're not understanding how I thought this
was supposed to work :)
VCDCImpl has a pool of Mojo...VideoFrames. Each
time a frame of this type is created, an associated
MojoSharedBuffer is created and mapped, and remains
so during the lifetime of the said VideoFrame. A VF
can only be inside the Pool, hence "available" or not,
and then it's in use. When a VF is taken out of the pool,
StreamImpl receives it and assigns a number to it; this 
number is bound to the release callback. 

Only the constituents of the VideoFrame are sent across 
the Mojo channel, where each StreamClient(s) will 
reconstruct a peer VideoFrame. The original VideoFrame 
must stay alive and "in use" until the StreamClient is finished.

When the client is finished with the VideoFrame, it will ping 
us back via OnFrameConsumed() callback where the 
|buffer_id| is bound. Then we find the VideoFrame by
id and remove it from the list. We need to keep the
VideoFrame alive on this side of the Mojo boundary,
hence I chose to keep them all lined up in a map.

So basically a VF is either in the VCDCImpl's Pool or
is kept here until the Mojo clients are finished. Much
simpler than the current design in renderer_host.

(Ok, is slightly more complex because the VFs being 
taken out of the Pool are wrapped into another VF.)

So I'm not sure what you don't grok. Is it that
you'd like to remove the |in_use_video_frames_| 
and bind the VideoFrame themselves to the release 
callback...? 

[magjed_chromium, 2016/03/09 14:11:18]

I didn't grok the purpose of this map since you don't use the mapped frames. The
only purpose I could think of was to keep the frames alive, but you captured the
scoped_refptr by-ref so that would not help the frames stay alive. Can you add a
comment that the purpose of the map is to keep the frames alive? It's also a bit
worrying that the tests didn't catch that the frames were not kept alive. I
guess the frames were returned immediately to the pool previously so that you
would never allocate more than one frame. Can you add a test that checks that a
new frame is received if the previous frame is not returned?

Actually, your suggestion to bind the scoped_refptrs directly to the release
callback seems cleaner if you don't plan to use |in_use_video_frames_| for
anything else.

+
+  // The TaskRunner where this class lives, needed for OnFrame() and OnError()
+  // to PostTask() to.
+  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  base::WeakPtrFactory<StreamImpl> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(StreamImpl);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_CAPTURE_SERVICE_STREAM_IMPL_H_