H264 HW encode using VideoToolbox

content/common/gpu/media/vt_video_encode_accelerator.cc

Index: content/common/gpu/media/vt_video_encode_accelerator.cc
diff --git a/content/common/gpu/media/vt_video_encode_accelerator.cc b/content/common/gpu/media/vt_video_encode_accelerator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..466c6c4179fee98885971ccf8877060bdc638bb3
--- /dev/null
+++ b/content/common/gpu/media/vt_video_encode_accelerator.cc
@@ -0,0 +1,328 @@
+// 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.
+
+#include "content/common/gpu/media/vt_video_encode_accelerator.h"
+
+#include "base/thread_task_runner_handle.h"
+#include "media/base/mac/coremedia_glue.h"
+#include "media/base/mac/corevideo_glue.h"
+#include "media/base/mac/video_frame_mac.h"
+
+namespace content {
+
+namespace {
+
+// Subjectively chosen.
+// TODO(emircan): Check if we can find the actual system capabilities via
+// creating VTCompressionSessions with varying requirements.
+// See crbug.com/584784.
+const size_t kNumInputBuffers = 4;
+const size_t kMaxFrameRateNumerator = 30;
+const size_t kMaxFrameRateDenominator = 1;
+const size_t kMaxResolutionWidth = 4096;
+const size_t kMaxResolutionHeight = 2160;
+// The ratio of |input_visible_size| area to the max expected output
+// BitstreamBuffer size in bytes. VideoToolbox returns variable sized encoded
+// data whereas media::VideoEncodeAccelerator provides a uniform BitstreamBuffer
+// size to fill this data into. This ratio is used to determine a size that
+// would ideally be big enough to fit all frames.
+const size_t kOutputBufferSizeRatio = 10;
+
+// Container for the associated data of a video frame being processed.
+struct InProgressFrameEncode {

[Pawel Osciak, 2016/02/08 04:33:42]

Could this be a private substruct of VTVEA?

[emircan, 2016/02/08 23:41:23]

Done.

+  const base::TimeDelta timestamp;
+  const base::TimeTicks reference_time;
+
+  InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks ref_time)
+      : timestamp(rtp_timestamp), reference_time(ref_time) {}
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(InProgressFrameEncode);
+};
+
+}  // namespace
+
+struct VTVideoEncodeAccelerator::BitstreamBufferRef {
+  BitstreamBufferRef(int32_t id,
+                     scoped_ptr<base::SharedMemory> shm,

[Pawel Osciak, 2016/02/08 04:33:42]

const& perhaps?

[emircan, 2016/02/08 23:41:23]

I cannot use std::move on const scoped_ptr&.

+                     size_t size)
+      : id(id), shm(std::move(shm)), size(size) {}
+  const int32_t id;
+  const scoped_ptr<base::SharedMemory> shm;
+  const size_t size;
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(BitstreamBufferRef);
+};
+
+VTVideoEncodeAccelerator::VTVideoEncodeAccelerator()
+    : videotoolbox_glue_(VideoToolboxGlue::Get()),

[Pawel Osciak, 2016/02/08 04:33:43]

We used to check this for != nullptr
(https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/...).
Perhaps it would be safer to still do this?

[emircan, 2016/02/08 23:41:23]

Hmm we dont have a similar IsSupported() call though. I will move it to
Initialize() and fail if it is nullptr.

+      client_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
+}
+
+VTVideoEncodeAccelerator::~VTVideoEncodeAccelerator() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+media::VideoEncodeAccelerator::SupportedProfiles
+VTVideoEncodeAccelerator::GetSupportedProfiles() {
+  DVLOG(3) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  SupportedProfiles profiles;
+  SupportedProfile profile;
+  profile.profile = media::H264PROFILE_BASELINE;
+  profile.max_framerate_numerator = kMaxFrameRateNumerator;
+  profile.max_framerate_denominator = kMaxFrameRateDenominator;
+  profile.max_resolution = gfx::Size(kMaxResolutionWidth, kMaxResolutionHeight);
+  profiles.push_back(profile);
+  return profiles;
+}
+
+bool VTVideoEncodeAccelerator::Initialize(
+    media::VideoPixelFormat format,
+    const gfx::Size& input_visible_size,
+    media::VideoCodecProfile output_profile,
+    uint32_t initial_bitrate,
+    Client* client) {
+  DVLOG(3) << __FUNCTION__
+           << ": input_format=" << media::VideoPixelFormatToString(format)
+           << ", input_visible_size=" << input_visible_size.ToString()
+           << ", output_profile=" << output_profile
+           << ", initial_bitrate=" << initial_bitrate;
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(client);
+  DCHECK_EQ(media::PIXEL_FORMAT_I420, format);

[Pawel Osciak, 2016/02/08 04:33:42]

I'd suggest if()s and careful check for argument values. VEA is a Pepper
interface too and there may be other clients that could try to create us with
various arguments and depend on us to fail Initialize().

[emircan, 2016/02/08 23:41:23]

Done.

+  DCHECK_EQ(media::H264PROFILE_BASELINE, output_profile);

[Pawel Osciak, 2016/02/08 04:33:42]

I'd prefer we if()'d profile here nevertheless. There is no requirement to check
supported profiles and some clients may not do this, relying on Initialize() to
fail.

[emircan, 2016/02/08 23:41:24]

Done.

+
+  bitrate_ = initial_bitrate;
+  input_visible_size_ = input_visible_size;
+
+  if (!ResetCompressionSession()) {
+    DLOG(ERROR) << "Failed creating compression session";
+    return false;
+  }
+
+  client_ptr_factory_.reset(new base::WeakPtrFactory<Client>(client));

[Pawel Osciak, 2016/02/08 04:33:42]

I'd suggest doing this before calling any methods, just in case (for later
changes in this class).

[emircan, 2016/02/08 23:41:23]

Ok, moving it right after the initial ifs.

+  client_ = client_ptr_factory_->GetWeakPtr();
+  client_->RequireBitstreamBuffers(
+      kNumInputBuffers, input_visible_size_,
+      input_visible_size_.GetArea() / kOutputBufferSizeRatio);
+  return true;
+}
+
+void VTVideoEncodeAccelerator::Encode(
+    const scoped_refptr<media::VideoFrame>& frame,
+    bool force_keyframe) {
+  DVLOG(3) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(compression_session_);
+  DCHECK(frame);
+
+  base::TimeTicks ref_time;
+  if (!frame->metadata()->GetTimeTicks(
+          media::VideoFrameMetadata::REFERENCE_TIME, &ref_time)) {
+    ref_time = base::TimeTicks::Now();
+  }
+  auto timestamp_cm = CoreMediaGlue::CMTimeMake(
+      frame->timestamp().InMicroseconds(), USEC_PER_SEC);
+  // Wrap information we'll need after the frame is encoded in a heap object.
+  // We'll get the pointer back from the VideoToolbox completion callback.
+  scoped_ptr<InProgressFrameEncode> request(new InProgressFrameEncode(
+      frame->timestamp(), ref_time));
+
+  // TODO(emircan): See if we can eliminate a copy here by using
+  // CVPixelBufferPool for the allocation of incoming VideoFrames.
+  base::ScopedCFTypeRef<CVPixelBufferRef> pixel_buffer =
+      media::WrapVideoFrameInCVPixelBuffer(*frame);
+  base::ScopedCFTypeRef<CFDictionaryRef> frame_props =
+      media::video_toolbox::DictionaryWithKeyValue(
+          videotoolbox_glue_->kVTEncodeFrameOptionKey_ForceKeyFrame(),
+          force_keyframe ? kCFBooleanTrue : kCFBooleanFalse);
+
+  OSStatus status = videotoolbox_glue_->VTCompressionSessionEncodeFrame(
+      compression_session_, pixel_buffer, timestamp_cm,
+      CoreMediaGlue::CMTime{0, 0, 0, 0}, frame_props,
+      reinterpret_cast<void*>(request.release()), nullptr);
+  DLOG_IF(ERROR, status != noErr)

[Pawel Osciak, 2016/02/08 04:33:42]

No need to NOTIFY_ERROR? Can we continue from next Encode() without
reinitialization after VTCompressionSessionEncodeFrame() fails?

[emircan, 2016/02/08 23:41:23]

Thanks for pointing out. I realize I haven't used enough NotifyErrors. Adding
them now to wherever appropriate.

+      << " VTCompressionSessionEncodeFrame failed: " << status;
+}
+
+void VTVideoEncodeAccelerator::UseOutputBitstreamBuffer(
+    const media::BitstreamBuffer& buffer) {
+  DVLOG(3) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_GE(buffer.size(), static_cast<size_t>(input_visible_size_.GetArea() /

[Pawel Osciak, 2016/02/08 04:33:42]

I think it'd be better to if() here please.

[emircan, 2016/02/08 23:41:23]

Done.

+                                               kOutputBufferSizeRatio));
+
+  scoped_ptr<base::SharedMemory> shm(
+      new base::SharedMemory(buffer.handle(), false));
+  if (!shm->Map(buffer.size())) {
+    DLOG(ERROR) << "Failed mapping shared memory.";

[Pawel Osciak, 2016/02/08 04:33:42]

NOTIFY_ERROR?

[emircan, 2016/02/08 23:41:23]

Done.

+    return;
+  }
+
+  scoped_ptr<BitstreamBufferRef> buffer_ref(
+      new BitstreamBufferRef(buffer.id(), std::move(shm), buffer.size()));
+  encoder_output_queue_.push_back(std::move(buffer_ref));

[Pawel Osciak, 2016/02/08 04:33:42]

Do we need to wake something up here? If we got Encode() first, but no output
buffers are ready at that time, we would stall?

[emircan, 2016/02/08 23:41:23]

Replied to the later comment.

+}
+
+void VTVideoEncodeAccelerator::RequestEncodingParametersChange(
+    uint32_t bitrate,
+    uint32_t framerate) {

[Pawel Osciak, 2016/02/08 04:33:42]

If this class cannot handle changing framerate, should we return an error if
such a change is requested?

[emircan, 2016/02/08 23:41:23]

Actually both changes aren't supported, but at least bitrate has an input param
for session creation. Restarting the session with the new bitrate param should
work, but that wouldn't be a cheap step. I will visit this issue in a later CL.
If we were to return error, how would the clients act accordingly?

+  DVLOG(3) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  bitrate_ = bitrate;

[Pawel Osciak, 2016/02/08 04:33:42]

We should preferably check input values here.

[emircan, 2016/02/08 23:41:24]

What kind of checks? I found (bitrate < 1) checks from CrOS VEAs. Would that be
enough?

+  if (!compression_session_)

[Pawel Osciak, 2016/02/08 04:33:42]

NOTIFY_ERROR?

[emircan, 2016/02/08 23:41:24]

Done.

+    return;
+
+  session_property_setter_->SetSessionProperty(

[Pawel Osciak, 2016/02/08 04:33:42]

SetSessionProperty() methods are bool, but we are not checking return values,
here and in other places...

[emircan, 2016/02/08 23:41:24]

I will go through them. 

+      videotoolbox_glue_->kVTCompressionPropertyKey_AverageBitRate(),
+      static_cast<int32_t>(bitrate_));
+}
+
+void VTVideoEncodeAccelerator::Destroy() {
+  DVLOG(3) << __FUNCTION__;
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  DestroyCompressionSession();
+  delete this;
+}
+
+// static
+void VTVideoEncodeAccelerator::CompressionCallback(void* encoder_opaque,

[Pawel Osciak, 2016/02/08 04:33:42]

Do we know what thread this is called on?

Should we DCHECK(encoder->thread_checker_.CalledOnValidThread()) ?

[emircan, 2016/02/08 23:41:23]

No. To quote the documentation: "This function may be called asynchronously, on
a different thread from the one that calls VTCompressionSessionEncodeFrame"

+                                                   void* request_opaque,
+                                                   OSStatus status,
+                                                   VTEncodeInfoFlags info,
+                                                   CMSampleBufferRef sbuf) {
+  DVLOG(3) << __FUNCTION__;
+
+  if (status != noErr)
+    DLOG(ERROR) << " encode failed: " << status;

[Pawel Osciak, 2016/02/08 04:33:43]

NOTIFY_ERROR?

[emircan, 2016/02/08 23:41:23]

Done.

+
+  if (info & VideoToolboxGlue::kVTEncodeInfo_FrameDropped) {
+    DVLOG(2) << " frame dropped";
+    return;
+  }
+
+  auto sample_attachments = static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex(
+      CoreMediaGlue::CMSampleBufferGetSampleAttachmentsArray(sbuf, true), 0));
+  const bool keyframe =
+      !CFDictionaryContainsKey(sample_attachments,
+                               CoreMediaGlue::kCMSampleAttachmentKey_NotSync());
+  auto encoder = reinterpret_cast<VTVideoEncodeAccelerator*>(encoder_opaque);
+  DCHECK(encoder);
+  if (encoder->encoder_output_queue_.empty()) {
+    DLOG(ERROR) << "No more bitstream buffer to encode into.";

[Pawel Osciak, 2016/02/08 04:33:42]

Please see my previous comment, but I think this could happen in normal cases if
Client called Encode() first and then UseOutputBitstreamBuffer(). This should be
an acceptable case I feel though and we should instead just not encode from
Encode(), but once an input:output buffer pair is available, either from
Encode() or UseOutputBitstreamBuffer() ?

Or, perhaps even better, if possible, always encode in Encode(), but defer this
copy here if encoder_output_queue_.empty(), putting sbuf on some kind of a queue
and copying after next UseOutputBitstreamBuffer()...

[emircan, 2016/02/08 23:41:23]

Thanks for pointing out. I didn't know Encode() and then
UseOutputBitstreamBuffer() is a valid case. I like the second option better, and
adding a new queue to retain those buffers.

+    encoder->client_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&Client::NotifyError, encoder->client_,
+                              kPlatformFailureError));
+    return;
+  }
+  scoped_ptr<VTVideoEncodeAccelerator::BitstreamBufferRef> buffer_ref =
+      std::move(encoder->encoder_output_queue_.front());
+  encoder->encoder_output_queue_.pop_front();
+
+  size_t used_buffer_size = 0;
+  const bool copy_rv = media::video_toolbox::CopySampleBufferToAnnexBBuffer(
+      sbuf, reinterpret_cast<uint8_t*>(buffer_ref->shm->memory()),
+      buffer_ref->size, keyframe, &used_buffer_size);
+  if (!copy_rv) {
+    DLOG(ERROR) << "Cannot copy output from SampleBuffer to AnnexBBuffer.";
+    encoder->encoder_output_queue_.push_back(std::move(buffer_ref));
+    return;
+  }
+
+  // This method is NOT called on |client_task_runner_|, so we still need to

[Pawel Osciak, 2016/02/08 04:33:42]

Uhh, if so, I don't think we can access encoder at all, including members like
encoder_output_queue_. Aren't we pushing to encoder_output_queue_ on
client_task_runner_ for example?

If that's the case, could we trampoline from this method at the beginning to
client_task_runner_ to actually handle things there?

[emircan, 2016/02/08 23:41:23]

I am posting a task as you suggest. 

But why wouldn't we access encoder at all? Accessing client_task_runner through
encoder is also done on cast code:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/...
. If your concern is regarding thread safety of std::deque, I checked it prior
and found mixed answers regarding it.

[Pawel Osciak, 2016/02/18 11:16:14]

Concurrent read-only calls to containers are ok, but we are not doing read-only
calls only. Also, even if each method of the container was atomic (like empty(),
push_back() etc.), the container wouldn't be locked across calls. So for example
checking !empty() here and then depending on it staying non-empty to be able to
safely call pop() here would not work without explicit locking if another thread
could be using it in parallel.

+  // post a task back to it to reach |client_|.
+  encoder->client_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&Client::BitstreamBufferReady, encoder->client_,
+                            buffer_ref->id, used_buffer_size, keyframe));
+}
+
+bool VTVideoEncodeAccelerator::ResetCompressionSession() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  DestroyCompressionSession();
+
+  base::ScopedCFTypeRef<CFDictionaryRef> encoder_spec =
+      media::video_toolbox::DictionaryWithKeyValue(videotoolbox_glue_
+        ->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder(),
+      kCFBooleanTrue);
+
+  // Keep these in-sync with those in ConfigureSession().

[Pawel Osciak, 2016/02/08 04:33:42]

Where is ConfigureSession() ?

[emircan, 2016/02/08 23:41:23]

Changed it to ConfigureCompressionSession().

+  CFTypeRef attributes_keys[] = {
+#if defined(OS_IOS)
+    kCVPixelBufferOpenGLESCompatibilityKey,
+#else
+    kCVPixelBufferOpenGLCompatibilityKey,
+#endif
+    kCVPixelBufferIOSurfacePropertiesKey,
+    kCVPixelBufferPixelFormatTypeKey
+  };
+  const int format[] = {

[Pawel Osciak, 2016/02/08 04:33:42]

Is this a fourcc? If so uint32_t please.

[emircan, 2016/02/08 23:41:24]

We need to create CFArrayRef<int> from it.

+      CoreVideoGlue::kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange};
+  CFTypeRef attributes_values[] = {
+      kCFBooleanTrue,
+      media::video_toolbox::DictionaryWithKeysAndValues(nullptr, nullptr, 0)
+          .release(),
+      media::video_toolbox::ArrayWithIntegers(format, arraysize(format))
+          .release()};
+  const base::ScopedCFTypeRef<CFDictionaryRef> attributes =
+      media::video_toolbox::DictionaryWithKeysAndValues(
+          attributes_keys, attributes_values, arraysize(attributes_keys));
+  for (auto& v : attributes_values)
+    CFRelease(v);
+
+  // Create the compression session.
+  OSStatus status = videotoolbox_glue_->VTCompressionSessionCreate(
+      kCFAllocatorDefault,
+      input_visible_size_.width(),
+      input_visible_size_.height(),
+      CoreMediaGlue::kCMVideoCodecType_H264,
+      encoder_spec,
+      attributes,
+      nullptr /* compressedDataAllocator */,
+      &VTVideoEncodeAccelerator::CompressionCallback,
+      reinterpret_cast<void*>(this),
+      compression_session_.InitializeInto());
+  if (status != noErr) {
+    DLOG(ERROR) << " VTCompressionSessionCreate failed: " << status;
+    return false;
+  }
+  ConfigureCompressionSession();
+  return true;
+}
+
+void VTVideoEncodeAccelerator::ConfigureCompressionSession() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(compression_session_);
+
+  session_property_setter_.reset(
+      new media::video_toolbox::SessionPropertySetter(compression_session_,
+                                                      videotoolbox_glue_));
+  session_property_setter_->SetSessionProperty(
+      videotoolbox_glue_->kVTCompressionPropertyKey_ProfileLevel(),
+      videotoolbox_glue_->kVTProfileLevel_H264_Baseline_AutoLevel());

[Pawel Osciak, 2016/02/08 04:33:42]

Is baseline preferred by us? Normally constrained baseline is more compatible
with various devices (unless this actually is constrained). Main may also be
better in general.

[emircan, 2016/02/08 23:41:24]

I discussed this with WebRTC people. I learned that Baseline is the only one
that openh264 supports -SW case- and compatible with Firefox as well. I am
planning to support only Baseline in this CL. I can add other profiles after
further discussion, and it wouldn't be hard.

[Pawel Osciak, 2016/02/18 11:16:14]

Are we sure that was Baseline, and not Constrained Baseline though? A good
number of HW decoders doesn't support Baseline, as it requires additional
features.

[emircan, 2016/03/03 19:18:19]

As far as I got answers from WebRTC folks, it is Baseline[0]. I can revisit this
to add new formats if needed.

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

+  session_property_setter_->SetSessionProperty(
+      videotoolbox_glue_->kVTCompressionPropertyKey_RealTime(), true);
+  session_property_setter_->SetSessionProperty(
+      videotoolbox_glue_->kVTCompressionPropertyKey_AverageBitRate(),
+      static_cast<int32_t>(bitrate_));
+  session_property_setter_->SetSessionProperty(
+      videotoolbox_glue_->kVTCompressionPropertyKey_AllowFrameReordering(),
+      false);
+}
+
+void VTVideoEncodeAccelerator::DestroyCompressionSession() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (compression_session_) {
+    videotoolbox_glue_->VTCompressionSessionInvalidate(compression_session_);
+    compression_session_.reset();
+  }
+}
+
+} // namespace content