MJPEG acceleration for V4L2

content/common/gpu/media/v4l2_jpeg_decode_accelerator.h

Index: content/common/gpu/media/v4l2_jpeg_decode_accelerator.h
diff --git a/content/common/gpu/media/v4l2_jpeg_decode_accelerator.h b/content/common/gpu/media/v4l2_jpeg_decode_accelerator.h
new file mode 100644
index 0000000000000000000000000000000000000000..86d0e2d008bf2436db52c3c8b79fcd77fb570afa
--- /dev/null
+++ b/content/common/gpu/media/v4l2_jpeg_decode_accelerator.h
@@ -0,0 +1,172 @@
+// 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 CONTENT_COMMON_GPU_MEDIA_V4L2_JPEG_DECODE_ACCELERATOR_H_
+#define CONTENT_COMMON_GPU_MEDIA_V4L2_JPEG_DECODE_ACCELERATOR_H_
+
+#include <queue>
+#include <vector>
+
+#include "base/memory/linked_ptr.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread.h"
+#include "content/common/content_export.h"
+#include "content/common/gpu/media/v4l2_device.h"
+#include "media/base/bitstream_buffer.h"
+#include "media/base/video_frame.h"
+#include "media/video/jpeg_decode_accelerator.h"
+
+namespace content {
+
+class CONTENT_EXPORT V4L2JpegDecodeAccelerator
+    : public media::JpegDecodeAccelerator {
+ public:
+  V4L2JpegDecodeAccelerator(
+      const scoped_refptr<V4L2Device>& device,
+      const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner);
+  ~V4L2JpegDecodeAccelerator() override;
+
+  // Note: Initialize() are synchronous.

[wuchengli, 2015/06/12 11:07:37]

What do you mean it's synchronous? It posts a StartDevicePoll task.

[Pawel Osciak, 2015/06/16 07:13:22]

I believe this is because returning true from here means we have successfully
initialized, as opposed to having an IPC NotifyInitializeDone in the old VDA/VEA
model.

[henryhsu, 2015/06/16 09:37:25]

Modified comments.

+  bool Initialize(Client* client) override;

[Pawel Osciak, 2015/06/16 07:13:22]

Nit:
// media::JpegDecodeAccelerator implementation.

[henryhsu, 2015/06/16 09:37:24]

Done.

+
+  void Decode(const media::BitstreamBuffer& bitstream_buffer,
+              const scoped_refptr<media::VideoFrame>& video_frame) override;
+
+ private:
+  // Record for input buffers.
+  struct InputRecord {
+    InputRecord();
+    ~InputRecord();
+    void* address;          // mmap() address.
+    size_t length;          // mmap() length.
+    bool at_device;

[wuchengli, 2015/06/12 11:07:37]

Explain what this means.

[henryhsu, 2015/06/16 09:37:24]

Done.

+  };
+
+  // Record for output buffers.
+  struct OutputRecord {

[Pawel Osciak, 2015/06/16 07:13:22]

InputRecord and OutputRecord are identical. Could we use a common class instead?

[henryhsu, 2015/06/16 09:37:25]

Done.

+    OutputRecord();
+    ~OutputRecord();
+    void* address;          // mmap() address.
+    size_t length;          // mmap() length.
+    bool at_device;

[wuchengli, 2015/06/12 11:07:37]

Explain what this means.

[henryhsu, 2015/06/16 09:37:24]

Done.

+  };
+
+  // Job record. Jobs are processed in a FIFO order. This is separate from
+  // InputRecord, because an InputRecord may be returned before we dequeue
+  // the corresponding output buffer. It can't always be associated with
+  // an OutputRecord immediately either, because at the time of submission we
+  // may not have one available (and don't need one to submit input to the
+  // device).
+  struct JobRecord {
+    JobRecord(media::BitstreamBuffer bitstream_buffer,
+              scoped_refptr<media::VideoFrame> video_frame);
+    ~JobRecord();
+    media::BitstreamBuffer bitstream_buffer;
+    scoped_refptr<media::VideoFrame> frame;
+  };
+
+  enum {
+    // Arbitrarily tuned.
+    kInputBufferCount = 2,

[Pawel Osciak, 2015/06/16 07:13:22]

We always want to have inputcount == outputcount, so I'd just have one const
here.

[henryhsu, 2015/06/16 09:37:25]

Done.

+    kOutputBufferCount = 2,
+  };
+
+  enum {
+    kResetInputBuffer = 1 << 0,
+    kResetOutputBuffer = 1 << 1,
+  };
+
+  void Enqueue();
+  void Dequeue();
+  bool EnqueueInputRecord();
+  bool EnqueueOutputRecord();
+  bool CheckBufferAttributes();

[wuchengli, 2015/06/12 11:07:37]

Add documentation for this.

[henryhsu, 2015/06/16 09:37:24]

Done.

+  bool CreateInputBuffers();
+  bool CreateOutputBuffers();
+  void DestroyInputBuffers();
+  void DestroyOutputBuffers();
+  void ResetBuffers();

[wuchengli, 2015/06/12 11:07:37]

Add documentation for this.

[henryhsu, 2015/06/16 09:37:25]

Done.

+
+  void NotifyError(int32_t bitstream_buffer_id, Error error);
+  void NotifyErrorFromDecoderThread(int32_t bitstream_buffer_id, Error error);
+  void DestroyTask();

[wuchengli, 2015/06/12 11:07:37]

Add documentation for this.

I think DestroyTask shouldn't be grouped together with NotifyError and
NotifyErrorFromDecoderThread.

[henryhsu, 2015/06/16 09:37:25]

Done.

+
+  void DecodeTask(scoped_ptr<JobRecord> job_record);
+  void ServiceDeviceTask();
+
+  // Attempt to start/stop device_poll_thread_.

[wuchengli, 2015/06/12 11:07:37]

s/Attempt to//

[henryhsu, 2015/06/16 09:37:25]

Done.

+  void StartDevicePoll();
+  bool StopDevicePoll(bool keep_input_queue);
+
+  // Ran on device_poll_thread_ to wait for device events.
+  void DevicePollTask();
+
+  media::VideoFrame::Format output_format_;

[wuchengli, 2015/06/12 11:07:37]

Add a blank line if there's comment. Same for all other variables.

[henryhsu, 2015/06/16 09:37:24]

Done.

+  // Record current image size for checking image size is changed or not.

[wuchengli, 2015/06/12 11:07:37]

// Current image size used for checking if the size is changed.

[henryhsu, 2015/06/16 09:37:24]

Done.

+  gfx::Size image_coded_size_;
+  // Set to true when input or output buffer have to re-allocate.

[kcwu, 2015/06/12 06:59:42]

s/true/non-zero/

[henryhsu, 2015/06/16 09:37:25]

Done.

+  uint32_t reset_buffer_flag_;

[wuchengli, 2015/06/12 11:07:37]

Two booleans is simpler than a bit mask. Just use
|recreate_input_buffers_pending_| and |recreate_output_buffers_pending_|.
"reset" is not clear about whether the buffers are destroyed. "recreate" should
be more clear.

[Pawel Osciak, 2015/06/16 07:13:22]

+1 to this.

[henryhsu, 2015/06/16 09:37:25]

Done.

+
+  // ChildThread's task runner.
+  scoped_refptr<base::SingleThreadTaskRunner> child_task_runner_;
+
+  // GPU IO task runner.
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+
+  // The client of this class.
+  Client* client_;
+
+  // The V4L2Device this class is operating upon.
+  scoped_refptr<V4L2Device> device_;
+
+  // Thread to communicate with the device on.

[wuchengli, 2015/06/12 11:07:37]

I don't understand. Maybe you mean "Thread to communicate with the device"?

[Pawel Osciak, 2015/06/16 07:13:22]

This sentence is grammatically correct actually. :)

[henryhsu, 2015/06/16 09:37:24]

Yes.

+  base::Thread decoder_thread_;
+  // Decode task runner.
+  scoped_refptr<base::SingleThreadTaskRunner> decoder_task_runner_;
+  // Thread used to poll the V4L2 for events only.
+  base::Thread device_poll_thread_;
+  // Device poll task runner.
+  scoped_refptr<base::SingleThreadTaskRunner> device_poll_task_runner_;
+
+  // All the below members are to be accessed from decoder_thread_ only

[wuchengli, 2015/06/12 11:07:37]

s/to be//

[henryhsu, 2015/06/16 09:37:25]

Done.

+  // (if it's running).
+  std::queue<linked_ptr<JobRecord> > input_queue_;

[wuchengli, 2015/06/12 11:07:37]

The names of |input_queue_| and |running_jobs_| should be consistent.
s/input_queue_/input_jobs_/.

[Pawel Osciak, 2015/06/16 07:13:22]

How about job_input_queue_ as a compromise ? :)

[henryhsu, 2015/06/16 09:37:24]

Done.

+  std::queue<linked_ptr<JobRecord> > running_jobs_;

[wuchengli, 2015/06/15 07:55:28]

I don't think you need to use linked_ptr for |input_queue_| and |running_jobs_|.
It's cheap to copy BitstreamBuffer and media::VideoFrame is scoped_refptr.

[Pawel Osciak, 2015/06/16 07:13:22]

JobRecord is base::Bound, so we'd have to make it derive from RefCounted
otherwise. Personally I feel this is simpler and saves some copying as a bonus.

[henryhsu, 2015/06/16 09:37:25]

As discussed, I prefer to keep linked_ptr here because BitstreamBuffer size
depends on base::SharedMemoryHandle class.

+
+  // Input queue state.
+  bool input_streamon_;
+  // Number of input buffers enqueued to the device.
+  int input_buffer_queued_count_;
+  // Input buffers ready to use; LIFO since we don't care about ordering.
+  std::vector<int> free_input_buffers_;
+  // Mapping of int index to an input buffer record.

[wuchengli, 2015/06/12 11:07:37]

This is not a map. Update the comment. s/input_buffer_map_/input_buffers/

[Pawel Osciak, 2015/06/16 07:13:21]

We use it as a map though. The position in the vector maps v4l2_buffer.index ->
InputRecord, but we can use a vector instead of std::map, because we know they
are always 0..n. So we can do input_buffer_map[v4l2_buffer.index].

[henryhsu, 2015/06/16 09:37:24]

As Pawel said, I prefer to keep original name.

+  std::vector<InputRecord> input_buffer_map_;
+
+  // Output queue state.
+  bool output_streamon_;
+  // Number of output buffers enqueued to the device.
+  int output_buffer_queued_count_;
+  // Output buffers ready to use; LIFO since we don't care about ordering.

[wuchengli, 2015/06/12 11:07:37]

Explain what the int stands for.

[henryhsu, 2015/06/16 09:37:25]

Done.

+  std::vector<int> free_output_buffers_;
+  // Mapping of int index to an output buffer record.

[wuchengli, 2015/06/12 11:07:37]

This is not a map. s/output_buffer_map_/output_buffers_/. Update the comment.

[Pawel Osciak, 2015/06/16 07:13:22]

Same as input_buffer_map_.

[henryhsu, 2015/06/16 09:37:25]

same as input_buffer_map_.

+  std::vector<OutputRecord> output_buffer_map_;
+
+  // Weak factory for producing weak pointers on the decoder_thread_
+  base::WeakPtrFactory<V4L2JpegDecodeAccelerator> device_weak_factory_;

[wuchengli, 2015/06/12 11:07:37]

s/device_weak_factory_/weak_factory_/. This is the ptr factory for itself.
Calling it "device" is strange. It's common to call it |weak_facotry_|.

https://code.google.com/p/chromium/codesearch#search/&q=file:%5C.h%20WeakPtrF...

[Pawel Osciak, 2015/06/16 07:13:22]

The weak this factory must be the last member of the class.

[henryhsu, 2015/06/16 09:37:24]

Done.

[henryhsu, 2015/06/16 09:37:25]

Done.

+  // WeakPtr<> pointing to |this| for use in posting tasks from the decoder
+  // thread back to the ChildThread. Because the decoder thread is a member of
+  // this class, any task running on the decoder thread is guaranteed that this
+  // object is still alive. As a result, tasks posted from ChildThread to
+  // decoder thread should use base::Unretained(this), and tasks posted from
+  // the decoder thread to the ChildThread should use |device_weak_|.
+  base::WeakPtr<V4L2JpegDecodeAccelerator> device_weak_;

[kcwu, 2015/06/12 06:59:42]

Do we need to keep this? Just call device_weak_factory_.GetWeakPtr() instead.

[wuchengli, 2015/06/12 11:07:37]

s/device_weak_/weak_this_/

[henryhsu, 2015/06/16 09:37:24]

Done.

[henryhsu, 2015/06/16 09:37:24]

deleted.

+
+  DISALLOW_COPY_AND_ASSIGN(V4L2JpegDecodeAccelerator);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_COMMON_GPU_MEDIA_V4L2_JPEG_DECODE_ACCELERATOR_H_