MJPEG acceleration for video capture using VAAPI

content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.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 CONTENT_BROWSER_RENDERER_HOST_GPU_JPEG_ACCELERATOR_ADAPTER_H_

[wuchengli, 2015/04/21 06:05:37]

I think the name GpuJpegDecoder is better. It's more consistent with the similar
roles for video decoders like GpuVideoDecoder and RtcVideoDecoder. 

[kcwu, 2015/04/22 14:43:33]

Done.

+#define CONTENT_BROWSER_RENDERER_HOST_GPU_JPEG_ACCELERATOR_ADAPTER_H_
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "content/common/content_export.h"
+#include "media/video/capture/video_capture_device.h"
+#include "media/video/jpeg_decode_accelerator.h"
+
+namespace base {
+class MessageLoopProxy;
+}
+
+namespace content {
+
+// Adapter to GpuJpegDecodeAccelerator for VideoCaptureDevice::Client. It takes
+// care of GpuJpegDecodeAccelerator creation, shared memory, and threading
+// issues. If decoding fails, it will fallback to software decode for current
+// frame and reject upcoming frames.
+//
+// This class is created, destroyed, and mainly accessed on device thread. The

[wuchengli, 2015/04/21 06:05:37]

Device thread only applies for Linux. s/thread/thread for Linux/. 

"accessed" is vague. We should explain what functions should be called on what
thread. Move "main accessed" to another sentence along with the rest of the
paragraph.

[kcwu, 2015/04/22 14:43:33]

I rewrote the comment and removed the confusing sentences.

+// decoding responses (JpegDecodeAccelerator::Client) are on IO thread. Since

[wuchengli, 2015/04/21 06:05:37]

s/Client/Client functions/. s/are on/are run on/

[kcwu, 2015/04/22 14:43:33]

Done.

+// it only decodes a frame at a time, the access is almost mutual exclusive:

[wuchengli, 2015/04/21 06:05:37]

The access of what?

almost? If it is "almost", it's not mutual exclusive. This mutual exclusive
thing and member access are hard to understand. |lock_| is enough for
synchronization. Right? Are you trying to say this is efficient? I'll discuss
with you in person. 

[kcwu, 2015/04/22 14:43:33]

I rewrote the comment and removed the confusing sentences.

+// device thread can only access members if IsDecoding() is false. IO thread
+// can only access if IsDecoding() is true.
+class CONTENT_EXPORT GpuJpegDecodeAcceleratorAdapter
+    : public media::JpegDecodeAccelerator::Client {
+ public:
+  // A lightweight check for caller to avoid IPC latency for known unsupported

[wuchengli, 2015/04/21 06:05:37]

Add "True if JPEG hardware decoding is supported on this device." at the
beginning. I think the original comments here should go to the implementation
(gpu_jpeg_decode_accelerator_adapter.cc). It should be a TODO to be moved to
GpuInfo.

[kcwu, 2015/04/22 14:43:34]

Done.

+  // platform. Initialize() can do the real platform supporting check but it
+  // requires an IPC.
+  static bool Supported();
+
+  // |device_client| is where we request VideoCaptureDevice::Client::Buffer as
+  // output buffer from and send decoded result to.
+  GpuJpegDecodeAcceleratorAdapter(
+      media::VideoCaptureDevice::Client* device_client);
+  ~GpuJpegDecodeAcceleratorAdapter();
+
+  // Create and intialize decoder in GPU side. Return false if failed.
+  bool Initialize();

[wuchengli, 2015/04/21 06:05:37]

add a blank line

[kcwu, 2015/04/22 14:43:34]

Done.

+  // Return true if in failed state. If so, the caller,
+  // VideoCaptureDevice::Client, should do software decoding by itself.

[wuchengli, 2015/04/21 06:05:37]

What the caller actually does shouldn't be here. You should explain the state of
GpuJpegDecodeAcceleratorAdapter and what should caller do with this object. For
example, can the caller decode another Jpeg image? Should the caller destroy
this object?

[kcwu, 2015/04/22 14:43:34]

Remove that sentence and explain DecodeCaptureData() behavior in its comment.

+  bool IsFailed();
+
+  // Decode JPEG stream. Parameters are modeled after

[wuchengli, 2015/04/21 06:05:37]

s/JPEG stream/a JPEG picture/. stream sounds like a series of images or a video
stream.

[kcwu, 2015/04/22 14:43:33]

Done.

+  // VideoCaptureDeviceClient::OnIncomingCapturedData.

[wuchengli, 2015/04/21 06:05:37]

Doesn't need to explain what this modeled from. Remove.

[kcwu, 2015/04/22 14:43:33]

Done.

+  void DecodeCapturedData(const uint8* data,
+                          int length,
+                          const media::VideoCaptureFormat& frame_format,
+                          int rotation,
+                          const base::TimeTicks& timestamp);
+
+  // JpegDecodeAccelerator::Client implementation.
+  // These will be called in IO thread.

[wuchengli, 2015/04/21 06:05:37]

s/in/on/

[kcwu, 2015/04/22 14:43:33]

Done.

+  virtual void VideoFrameReady(int32_t buffer_id) override;
+  virtual void NotifyError(int32_t buffer_id,
+                           media::JpegDecodeAccelerator::Error error) override;
+
+ private:
+  // Fail state indicates to use software decode. See comment in
+  // FallbackToSoftwareDecode for detail.
+  enum FailState { NOT_FAIL, FAILING, FAILED };
+
+  // For keeping parameters of DecodeCapturedData. In case we need to pass them
+  // to software decoder if hardware decoding failed.
+  struct CapturedData {
+    // If accessed inside DecodeCapturedData(), |data| is owned by capturer
+    // (i.e., v4l2 driver on linux). Otherwise, |data| is backed by
+    // |in_shared_memory_|.
+    uint8* data;
+    int length;
+    media::VideoCaptureFormat frame_format;
+    int rotation;
+    base::TimeTicks timestamp;
+  };
+
+  bool IsDecoding();
+  // Check |bitstream_buffer_id| from GPU process is expected.
+  bool IsExpectedDecodeResponse(int32 bitstream_buffer_id);
+  // Current frame decoding is done. Allow DecodeCapturedData() to accept next
+  // frame.
+  void DecodeDone();
+  // Fallback current frame to software decode. Set flag to reject future
+  // upcoming frames.
+  void FallbackToSoftwareDecode();

[wuchengli, 2015/04/21 06:05:37]

Fallback should be decided by the caller, not this class. Move this back to
video_capture_device_client.cc. This class should handle hardware decoder
related stuff only.

[kcwu, 2015/04/22 14:43:33]

How about modify the name to NotifyClientDecodeFailed, and replace
"device_client_->OnIncomingCapturedData" with |decode_failed_cb_|?
|decode_failed_cb_| is got from the ctor.

+
+  // The "customer" of GpuJpegDecodeAcceleratorAdapter.
+  media::VideoCaptureDevice::Client* device_client_;
+  // The main working thread. Used for DCHECK current thread.
+  scoped_refptr<base::MessageLoopProxy> device_thread_;

[wuchengli, 2015/04/21 06:05:37]

Change the name. device thread only applies for Linux. This class should be
generic.

[kcwu, 2015/04/22 14:43:34]

Done.

+  // All decoding requests are sent to |decoder_|.

[wuchengli, 2015/04/21 06:05:37]

Add "The underlying JPEG decode accelerator."

[kcwu, 2015/04/22 14:43:34]

Done.

+  scoped_ptr<media::JpegDecodeAccelerator> decoder_;
+
+  // Below fields are protected by |lock_|.
+  base::Lock lock_;
+
+  FailState fail_state_;
+
+  // Keep the captured data from camera. If hardware decode failed, pass this
+  // to software decoder as fallback.
+  CapturedData captured_data_;
+
+  // Next id for |in_buffer_|.
+  int32 next_bitstream_buffer_id_;
+
+  // Shared memory to store JPEG stream buffer. |in_buffer_| is backed by this.
+  scoped_ptr<base::SharedMemory> in_shared_memory_;
+  // JPEG stream buffer as input to JpegDecodeAccelerator.
+  media::BitstreamBuffer in_buffer_;
+
+  // Buffer to hold decoded output of JPEG decoder.
+  // |out_frame_| is backed by this.
+  scoped_refptr<media::VideoCaptureDevice::Client::Buffer> out_buffer_;
+  // VideoFrame to receive decoded output of JpegDecodeAccelerator.
+  scoped_refptr<media::VideoFrame> out_frame_;
+
+  DISALLOW_COPY_AND_ASSIGN(GpuJpegDecodeAcceleratorAdapter);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_RENDERER_HOST_GPU_JPEG_ACCELERATOR_ADAPTER_H_