Linux Video Capture: Add V4L2VideoCaptureDelegate{Single,Multi}Plane.

media/video/capture/linux/v4l2_video_capture_delegate.h

Index: media/video/capture/linux/v4l2_video_capture_delegate.h
diff --git a/media/video/capture/linux/v4l2_video_capture_delegate.h b/media/video/capture/linux/v4l2_video_capture_delegate.h
new file mode 100644
index 0000000000000000000000000000000000000000..5a11fd622e0bfae4b82010d5d07dc47c0afb1455
--- /dev/null
+++ b/media/video/capture/linux/v4l2_video_capture_delegate.h
@@ -0,0 +1,139 @@
+// 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 MEDIA_VIDEO_CAPTURE_LINUX_V4L2_VIDEO_CAPTURE_DELEGATE_H_
+#define MEDIA_VIDEO_CAPTURE_LINUX_V4L2_VIDEO_CAPTURE_DELEGATE_H_
+
+#if defined(OS_OPENBSD)
+#include <sys/videoio.h>

[Pawel Osciak, 2015/03/06 10:43:55]

As mentioned in previous PS, this is does not appear to be full V4L2 and as far
as I can tell from what I can see of this file in Google search it does not
support MPLANE and other new API elements.

That said, I don't know if we support OpenBSD in general. You probably need to
figure out what the status of that is. We may have to make your MPLANE changes
only compile on non-BSD, but I don't know how we would verify that.

[mcasas, 2015/03/09 21:23:57]

There are no OpenBSD bots. 

My guess is that Chromium is patched before being packaged
for those distros.

I could preprocessor-filter out MPLANE parts. It'd
be dirty, so I'd rather do it if there's real need
for it. Any suggestions?

[Pawel Osciak, 2015/03/13 09:52:52]

This CL may breaking these platforms (may not even compile), so there is a real
need.

As for preprocessor-filtering, we are already using two separate classes,
hopefully all specifics can go in there. We'd have them in separate files and
not compile the mplane class' file if not supported. The only ifdefs in the code
would hopefully be in a factory method in the base class. That said, I don't
know if all splane features are supported in videoio.h. I don't know if it still
works as is right now. And I'm not sure how you'd compile-test this if you don't
have an OpenBSD environment.

Perhaps a good idea would be to ask the original author who added this and/or on
chromium-dev.

+#else
+#include <linux/videodev2.h>
+#endif
+
+#include "base/files/scoped_file.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_vector.h"
+#include "media/video/capture/video_capture_device.h"
+
+namespace media {
+
+// Class doing the actual Linux capture using V4L2 API. V4L2 SPLANE/MPLANE
+// capture specifics are implemented in derived classes. Created and destroyed
+// on the owner's thread, otherwise living and operating on |v4l2_task_runner_|.
+class V4L2VideoCaptureDelegate
+    : public base::RefCountedThreadSafe<V4L2VideoCaptureDelegate> {
+ public:
+  // Creates the appropiate VideoCaptureDelegate according to parameters.
+  static scoped_refptr<V4L2VideoCaptureDelegate> CreateV4L2VideoCaptureDelegate(
+      const VideoCaptureDevice::Name& device_name,
+      const VideoPixelFormat pixel_format,
+      const scoped_refptr<base::SingleThreadTaskRunner>& v4l2_task_runner,
+      int power_line_frequency);
+
+  // Returns the Cr pixel format for |v4l2_fourcc| or PIXEL_FORMAT_UNKNOWN.

[Pawel Osciak, 2015/03/06 10:43:55]

s/Cr/Chromium/

[mcasas, 2015/03/09 21:23:57]

Done.

+  static VideoPixelFormat V4l2FourCcToChromiumPixelFormat(uint32_t v4l2_fourcc);
+
+  // Composes a list of usable and supported pixel formats, in order of
+  // preference, with MJPEG prioritised depending on |prefer_mjpeg|.
+  static std::list<uint32_t> GetListOfUsableFourCss(bool prefer_mjpeg);

[emircan, 2015/03/04 02:47:47]

/s/FourCss/FourCcs/

[mcasas, 2015/03/09 21:23:57]

Done.

+
+  // Forward-to versions of VideoCaptureDevice virtual methods.
+  void AllocateAndStart(int width,
+                        int height,
+                        float frame_rate,
+                        scoped_ptr<VideoCaptureDevice::Client> client);
+  void StopAndDeAllocate();
+
+  void SetRotation(int rotation);
+
+ protected:
+  // Class keeping track of SPLANE/MPLANE V4L2 buffers, mmap()ed on construction
+  // and munmap()ed on destruction. Destruction is syntactically equalfor

[Pawel Osciak, 2015/03/06 10:43:55]

s/equalfor/equal for/

[mcasas, 2015/03/09 21:23:57]

Done.

+  // S/MPLANE but not construction, so this is implemented in derived classes.
+  // Internally it has a ScopedVector of planes, which for SPLANE will contain
+  // only one element.
+  class BufferTracker : public base::RefCounted<BufferTracker> {

[Pawel Osciak, 2015/03/06 10:43:55]

You should need to only declare this class here in .h and move impl to cc.

[mcasas, 2015/03/09 21:23:57]

Done.

+   public:
+    struct Plane {
+      void* start;
+      size_t length;
+    };
+
+    BufferTracker(int fd, const v4l2_buffer& buffer);
+
+    ScopedVector<Plane>& planes() { return planes_; }

[Pawel Osciak, 2015/03/06 10:43:55]

In what case would you like to use a non-const accessor? It would actually be a
bug, because clients cannot change the values, otherwise things would explode.

Also, I'd recommend having accessors like this:
uint8_t PlaneStart(size_t plane) const { return
static_cast<uint8_t*>(planes_[plane].start); }
size_t PlaneLength(size_t plane) const { return planes_[plane].length; }

You'd then instead of:
static_cast<uint8_t>(buffer_tracker->planes()[0]->start)
have:
buffer_tracker->PlaneStart(0)
etc.

[mcasas, 2015/03/09 21:23:57]

It's needed in the ctor of derived classes, f.i. for 
doing things like:

planes().push_back(plane.Pass());

since |planes_| cannot be accessed directly 
from derived classes.
Good suggestion, in that case I don't need the
const accessor.

[Pawel Osciak, 2015/03/13 09:52:52]

Right, the point is we don't want the users of this class to be able to modify
planes. So we need to make the accessor protected, if it can't be public const.
But we could just simply make planes_ protected instead and remove the accessor
altogether.

[mcasas, 2015/03/14 03:36:10]

I propose adding a protected AddMmapedPlane() method, 
that will offer the only non-const operation that should 
be available to children classes, i.e. adding a Plane. 
In this way, Struct Plane and |planes_| are private.
More details in last patchset's comments.

+    const ScopedVector<Plane>& planes() const { return planes_; }
+
+   protected:
+    friend class base::RefCounted<BufferTracker>;
+    virtual ~BufferTracker();
+
+   private:
+    ScopedVector<Plane> planes_;

[Pawel Osciak, 2015/03/06 10:43:55]

Plane does not have a specialized destructor and does not really have to be
created via operator new. You can just have std::vector<Plane> here.

[mcasas, 2015/03/09 21:23:56]

Done.

+  };
+
+  V4L2VideoCaptureDelegate(
+      const VideoCaptureDevice::Name& device_name,
+      const scoped_refptr<base::SingleThreadTaskRunner>& v4l2_task_runner,
+      int power_line_frequency);
+  virtual ~V4L2VideoCaptureDelegate();
+
+  // Creates the necessary, planarity-specific, internal tracking schemes.

[Pawel Osciak, 2015/03/06 10:43:55]

Please document arguments.

[mcasas, 2015/03/09 21:23:57]

Done.

+  virtual scoped_refptr<BufferTracker> CreateBufferTracker(
+      int fd,
+      const v4l2_buffer& buffer) = 0;
+
+  // Fill in |format| with the given parameters, in a planarity dependent way.
+  virtual void FillV4L2Format(v4l2_format* format,
+                              uint32_t width,
+                              uint32_t height,
+                              uint32_t pixelformat_fourcc) = 0;
+
+  // Finish filling |buffer| struct with planarity-dependent data.

[Pawel Osciak, 2015/03/06 10:43:55]

"Finish" doesn't really explain what this does and the function name already
says all this...

But you could have:

V4L2VideoCaptureDelegate::FillV4L2Buffer(v4l2_buffer* buffer, int i) {
  memset(&buffer, 0, sizeof(*buffer));
  buffer->memory = V4L2_MEMORY_MMAP;
  buffer->index = i;
  FinishFillingV4L2Buffer(buffer);
}

And have:

V4L2VideoCaptureDelegateSplane::FinishFillingV4L2Buffer(v4l2_buffer* buffer) {
  buffer->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
}

V4L2VideoCaptureDelegateMplane::FinishFillingV4L2Buffer(v4l2_buffer* buffer) {
  buffer->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
  buffer->planes = ...
  buffer->length = ...
}

Then Delegate would only have to call FillV4L2Buffer() and not worry about
anything else.


Also, please move QUERYBUF to CreateBuffer, it's much simpler this way:

// note capture_type_ is not needed
scoped_refptr<BufferTracker> CreateBufferTracker> CreateBufferTracker(int fd,
int index) {
  scoped_refptr<BufferTracker> buf_tracker = new BufferTracker...();
  v4l2_buffer buffer;
  FillV4L2Buffer(&buffer, i);
  if (!buf_tracker->Initialize(fd, &buffer))
    return nullptr;

  return buf_tracker;
}

BufferTrackerSPlane::Initialize(int fd, v4l2_buffer* buffer) {
  querybuf()...
  if (fail)
    return false;
  mmap()...
  if (fail)
   return false;

  return true;
}

For mplane, Initialize similar but also does what FinishFilling...() would.

[mcasas, 2015/03/09 21:23:57]

Done.
I just wanted to move as much common code as possible to 
the boilerplate in V4L2CaptureDelegate.

I propose something similar, hopefully covering your suggestions:

void AllocateAndStart(...) {
...
  // VIDIOC_REQBUFS
  for_each(buffer)
    if (!AllocateVideoBuffer(index))
      SetErrorState + return
}

bool AllocateVideoBuffer(...) {
  // QUERYBUF
  auto buffer_tracker = CreateBufferTracker()  // Pure factory.
  if (!buffer_tracker->Init(...))
    error_and_return
  // QBUF
}

BufferTrackerSPlane/BufferTrackerMPlane::CreateBufferTracker() {
  // return a BufferTracker{S,M}Plane
}

BufferTrackerSPlane/BufferTrackerMPlane::Init(
    int fd, const v4l2_buffer& buffer) {
  // do the mmap() here.
}



Which is similar to what you propose, with a few twists to
acommmodate the planarity/method derivation etc.

+  virtual void FinishFillingV4L2Buffer(v4l2_buffer* buffer) = 0;
+
+  // Sends the captured |buffer| to the |client|, synchronously.

[Pawel Osciak, 2015/03/06 10:43:55]

s/|client|/client_/

Also, please just take scoped_ptr to BufferTracker instead of v4l2_buffer. It's
the only thing you need in Send anyway, and you won't need the
buffer_tracker_pool() accessor.

[mcasas, 2015/03/09 21:23:57]

Done.

+  virtual void SendBuffer(const v4l2_buffer& buffer) = 0;
+
+  // A few accessors for SendBuffer()'s to access private member variables.
+  VideoCaptureFormat capture_format() const { return capture_format_; }
+  VideoCaptureDevice::Client* client() const { return client_.get(); }
+  int rotation() const { return rotation_; }
+  const std::vector<scoped_refptr<BufferTracker>>& buffer_tracker_pool() const {
+    return buffer_tracker_pool_;
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<V4L2VideoCaptureDelegate>;
+
+  // Request buffers from V4L2 and create BufferTrackers for them.
+  bool AllocateVideoBuffers();
+  void DoCapture();
+  void SetErrorState(const std::string& reason);
+
+  const v4l2_buf_type capture_type_;
+  const scoped_refptr<base::SingleThreadTaskRunner> v4l2_task_runner_;
+  const VideoCaptureDevice::Name device_name_;
+  const int power_line_frequency_;
+
+  // The following members are only known on AllocateAndStart().
+  VideoCaptureFormat capture_format_;
+  scoped_ptr<VideoCaptureDevice::Client> client_;
+  base::ScopedFD device_fd_;
+
+  // Vector of BufferTracker to keep track of mmap()ed pointers and their use.
+  std::vector<scoped_refptr<BufferTracker>> buffer_tracker_pool_;
+
+  bool is_capturing_;
+  int timeout_count_;
+
+  // Clockwise rotation in degrees. This value should be 0, 90, 180, or 270.
+  int rotation_;
+
+  DISALLOW_COPY_AND_ASSIGN(V4L2VideoCaptureDelegate);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_VIDEO_CAPTURE_LINUX_V4L2_VIDEO_CAPTURE_DELEGATE_H_