VideoToolbox encoder for cast senders.

media/cast/sender/h264_vt_encoder.cc

+// Copyright 2014 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 "media/cast/sender/h264_vt_encoder.h"
+
+#include "base/big_endian.h"
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
+#include "base/logging.h"
+
+#include <CoreMedia/CoreMedia.h>
+#include <VideoToolbox/VideoToolbox.h>
+
+namespace media {
+namespace cast {
+
+static const char* GetCVErrorString(CVReturn error) {

[miu, 2014/08/08 17:53:06]

This seems overkill for debug logging.  Suggest you remove it unless you feel
the DLOG's would be useful as VLOG's, and even then, the numerical code is
probably sufficient.

[jfroy, 2014/08/08 23:23:25]

This has all been removed in a following PS.

+  switch (error) {
+    case kCVReturnSuccess:
+      return "success";
+    case kCVReturnError:
+      return "error";
+    case kCVReturnInvalidArgument:
+      return "invalid argument";
+    case kCVReturnAllocationFailed:
+      return "allocation failed";
+    case kCVReturnInvalidDisplay:
+      return "invalid display";
+    case kCVReturnDisplayLinkAlreadyRunning:
+      return "display link already running";
+    case kCVReturnDisplayLinkNotRunning:
+      return "display link not running";
+    case kCVReturnDisplayLinkCallbacksNotSet:
+      return "display link callback not set";
+    case kCVReturnInvalidPixelFormat:
+      return "invalid pixel format";
+    case kCVReturnInvalidSize:
+      return "invalid size";
+    case kCVReturnInvalidPixelBufferAttributes:
+      return "invalid pixel buffer attributes";
+    case kCVReturnPixelBufferNotOpenGLCompatible:
+      return "pixel buffer not OpenGL compatible";
+    case kCVReturnWouldExceedAllocationThreshold:
+      return "would exceed allocation threshold";
+    case kCVReturnPoolAllocationFailed:
+      return "pool allocation failed";
+    case kCVReturnInvalidPoolAttributes:
+      return "invalid pool attributes";
+    default:
+      return "unknown error";
+  }
+}
+
+static const char* GetVTErrorString(OSStatus error) {

[miu, 2014/08/08 17:53:07]

ditto: re: overkill for debug logging

[jfroy, 2014/08/08 23:23:24]

This has all been removed in a following PS.

+  switch (error) {
+    case kVTPropertyNotSupportedErr:
+      return "property not supported";
+    case kVTPropertyReadOnlyErr:
+      return "read only property";
+    case kVTParameterErr:
+      return "invalid parameter";
+    case kVTInvalidSessionErr:
+      return "invalid session";
+    case kVTAllocationFailedErr:
+      return "allocation failed";
+    case kVTPixelTransferNotSupportedErr:
+      return "pixel transfer not supported";
+    case kVTCouldNotFindVideoDecoderErr:
+      return "could not find video decoder";
+    case kVTCouldNotCreateInstanceErr:
+      return "could not create instance";
+    case kVTCouldNotFindVideoEncoderErr:
+      return "could not find video encoder";
+    case kVTVideoDecoderBadDataErr:
+      return "video decoder bad data";
+    case kVTVideoDecoderUnsupportedDataFormatErr:
+      return "video decoder unsupported data format";
+    case kVTVideoDecoderMalfunctionErr:
+      return "video decoder malfunction";
+    case kVTVideoEncoderMalfunctionErr:
+      return "video encoder malfunction";
+    case kVTVideoDecoderNotAvailableNowErr:
+      return "video decoder not available";
+    case kVTImageRotationNotSupportedErr:
+      return "image rotation not supported";
+    case kVTVideoEncoderNotAvailableNowErr:
+      return "video encoder not available now";
+    case kVTFormatDescriptionChangeNotSupportedErr:
+      return "format description change not supported";
+    case kVTInsufficientSourceColorDataErr:
+      return "insufficient source color data";
+    case kVTCouldNotCreateColorCorrectionDataErr:
+      return "could not create color correction data";
+    case kVTColorSyncTransformConvertFailedErr:
+      return "ColorSync transform convert failed";
+    case kVTVideoDecoderAuthorizationErr:
+      return "video decoder authorization error";
+    case kVTVideoEncoderAuthorizationErr:
+      return "video encoder authorization error";
+    case kVTColorCorrectionPixelTransferFailedErr:
+      return "color correction pixel transfer failed";
+    case kVTMultiPassStorageIdentifierMismatchErr:
+      return "multi-pass storage identifier mismatch";
+    case kVTMultiPassStorageInvalidErr:
+      return "invalid multi-pass storage";
+    case kVTFrameSiloInvalidTimeStampErr:
+      return "invalid frame silo timestamp";
+    case kVTFrameSiloInvalidTimeRangeErr:
+      return "invalid frame silo time range";
+    case kVTCouldNotFindTemporalFilterErr:
+      return "could not find temporal filter";
+    case kVTPixelTransferNotPermittedErr:
+      return "pixel transfer not permitted";
+    default:
+      return "unknown error";
+  }
+}
+
+#pragma mark -
+
+// utility to log CFTypes
+
+std::ostream& operator<<(std::ostream& out, const CFStringRef& cfstring) {

[miu, 2014/08/08 17:53:07]

Rather than rolling your own, how about using the existing libraries?

https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/sys_s...

[jfroy, 2014/08/08 23:23:24]

I had no idea those existed.

This has all been removed in a following PS.

+  if (!cfstring) {
+    return out << "null";
+  }
+
+  const char* c_str;
+  c_str = CFStringGetCStringPtr(cfstring, kCFStringEncodingASCII);
+  if (c_str) {
+    return out << c_str;
+  }
+  c_str = CFStringGetCStringPtr(cfstring, kCFStringEncodingUTF8);
+  if (c_str) {
+    return out << c_str;
+  }
+
+  CFIndex length = CFStringGetLength(cfstring);
+  size_t size =
+      CFStringGetMaximumSizeForEncoding(length, kCFStringEncodingUTF8) + 1;
+  std::vector<char> c_str_buf;
+  c_str_buf.reserve(size);
+  CFStringGetCString(cfstring, c_str_buf.data(), size, kCFStringEncodingUTF8);
+  return out << c_str_buf.data();
+}
+
+std::ostream& operator<<(std::ostream& out, const CFNumberRef& cfnumber) {

[miu, 2014/08/08 17:53:08]

For this and the other ostream::operator<< definitions, perhaps the functions in
src/base/mac/foundation_util.h can be used instead?

If not, you should define these functions in foundation_util.h so they can be
used by everyone.

[jfroy, 2014/08/08 23:23:25]

This has all been removed in a following PS.

+  if (!cfnumber) {
+    return out << "null";
+  }
+
+  base::ScopedCFTypeRef<CFNumberFormatterRef> formatter(CFNumberFormatterCreate(
+      kCFAllocatorDefault, CFLocaleGetSystem(), kCFNumberFormatterNoStyle));
+  base::ScopedCFTypeRef<CFStringRef> as_str(
+      CFNumberFormatterCreateStringWithNumber(
+          kCFAllocatorDefault, formatter, cfnumber));
+  return out << as_str;
+}
+
+std::ostream& operator<<(std::ostream& out, const CFBooleanRef& cfboolean) {
+  if (!cfboolean) {
+    return out << "null";
+  }
+
+  base::ScopedCFTypeRef<CFStringRef> as_str(
+      CFBooleanGetValue(cfboolean) ? CFSTR("true") : CFSTR("false"));
+  return out << as_str;
+}
+
+struct CFTypeEmittable {
+  explicit CFTypeEmittable(CFTypeRef cfobject) : cfobject_(cfobject) {}
+  explicit operator bool() const { return cfobject_ != nullptr; }
+  operator CFTypeRef() const { return cfobject_; }
+  CFTypeRef get() const { return cfobject_; }
+  friend std::ostream& operator<<(std::ostream&, const CFTypeEmittable&);
+  CFTypeRef cfobject_;
+};
+
+std::ostream& operator<<(std::ostream& out, const CFTypeEmittable& emittable) {
+  if (!emittable) {
+    return out << "null";
+  }
+
+  if (CFGetTypeID(emittable) == CFStringGetTypeID()) {
+    return out << static_cast<CFStringRef>(emittable.get());
+  } else if (CFGetTypeID(emittable) == CFNumberGetTypeID()) {
+    return out << static_cast<CFNumberRef>(emittable.get());
+  } else if (CFGetTypeID(emittable) == CFBooleanGetTypeID()) {
+    return out << static_cast<CFBooleanRef>(emittable.get());
+  }
+  base::ScopedCFTypeRef<CFStringRef> as_str(CFCopyDescription(emittable));
+  return out << as_str;
+}
+
+// utility to log CM types
+
+std::ostream& operator<<(std::ostream& out, const CMTime& time) {
+  return out << "{value=" << time.value << ", timescale=" << time.timescale
+             << ", flags=" << time.flags << ", epoch=" << time.epoch << "}";
+}
+
+std::ostream& operator<<(std::ostream& out,
+                         const CMSampleTimingInfo& timing_info) {
+  return out << "{duration=" << timing_info.duration
+             << ", pts=" << timing_info.presentationTimeStamp
+             << ", dts=" << timing_info.decodeTimeStamp << "}";
+}
+
+#pragma mark -
+
+// utility to configure
+
+template <typename T>
+bool SetSessionProperty(VTSessionRef session,
+                        CFStringRef key,
+                        T value,
+                        CFTypeRef cfvalue) {
+  DVLOG(3) << __func__ << ": " << key << "=" << value;
+  OSStatus status = VTSessionSetProperty(session, key, cfvalue);
+  if (status != noErr) {
+    DLOG(ERROR) << __func__
+                << " VTSessionSetProperty failed: " << GetVTErrorString(status)
+                << " (" << status << ") " << key << "=" << value;
+  }
+  return status == noErr;
+}
+
+static bool SetSessionProperty(VTSessionRef session,
+                               CFStringRef key,
+                               uint32_t value) {
+  base::ScopedCFTypeRef<CFNumberRef> cfvalue(
+      CFNumberCreate(nullptr, kCFNumberSInt32Type, &value));
+  return SetSessionProperty(session, key, value, cfvalue);
+}
+
+static bool SetSessionProperty(VTSessionRef session,
+                               CFStringRef key,
+                               bool value) {
+  CFBooleanRef cfvalue = (value) ? kCFBooleanTrue : kCFBooleanFalse;
+  return SetSessionProperty(session, key, value, cfvalue);
+}
+
+static bool SetSessionProperty(VTSessionRef session,
+                               CFStringRef key,
+                               CFStringRef value) {
+  return SetSessionProperty(session, key, value, value);
+}
+
+static base::ScopedCFTypeRef<CFDictionaryRef> DictionaryWithKeyValue(
+    CFTypeRef key,
+    CFTypeRef value) {
+  CFTypeRef keys[1] = {key};
+  CFTypeRef values[1] = {value};
+  return base::ScopedCFTypeRef<CFDictionaryRef>(
+      CFDictionaryCreate(kCFAllocatorDefault,
+                         keys,
+                         values,
+                         1,
+                         &kCFTypeDictionaryKeyCallBacks,
+                         &kCFTypeDictionaryValueCallBacks));
+}
+
+#pragma mark -
+
+struct H264VideoToolboxEncoder::FrameContext {
+  base::TimeTicks capture_time;
+  FrameEncodedCallback frame_encoded_callback;
+};
+
+H264VideoToolboxEncoder::H264VideoToolboxEncoder(
+    scoped_refptr<CastEnvironment> cast_environment,
+    const VideoSenderConfig& video_config)
+    : cast_environment_(cast_environment),
+      cast_config_(video_config),
+      frame_id_(kStartFrameId),
+      last_keyframe_id_(kStartFrameId),
+      encode_next_frame_as_keyframe_(false) {
+  Initialize();
+}
+
+H264VideoToolboxEncoder::~H264VideoToolboxEncoder() {
+  Teardown();
+}
+
+CVPixelBufferPoolRef H264VideoToolboxEncoder::cv_pixel_buffer_pool() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(compression_session_);
+  return VTCompressionSessionGetPixelBufferPool(compression_session_);
+}
+
+void H264VideoToolboxEncoder::Initialize() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!compression_session_);
+
+  DVLOG(3) << __func__
+           << " width: " << cast_config_.width
+           << ", height: " << cast_config_.height
+           << ", start_bitrate: " << cast_config_.start_bitrate
+           << ", max_frame_rate:" << cast_config_.max_frame_rate;
+
+  // create the VT compression session
+
+  // Chrome manages input and output buffers and does not allow an encoder to
+  // provide buffers through the VideoEncodeAccelerator interface, so we pass
+  // null for |sourceImageBufferAttributes|, which otherwise causes VT to
+  // allocate a suitable pixel buffer pool.
+
+  // Note that the session context is given to the compression session as the
+  // callback context using a raw pointer. The C API does not allow us to use
+  // a smart pointer. However, this is still safe, because the output callback
+  // can only execute while this vea is alive (i.e. we fully own the
+  // compression session). This is enforced by a thread join inside
+  // VideoToolbox when invalidating the compression session in Destroy(). The
+  // only race left to worry about is when dispatching back to the vea thread.
+  // This is taken care of by using a smart pointer that will retain the
+  // session context for the duration of the task.
+
+  // On OS X, allow the hardware encoder. Don't require it, it does not support
+  // all configurations (some of which are used for testing).
+  DictionaryPtr encoder_spec(nullptr);
+#if !defined(OS_IOS)
+  encoder_spec = DictionaryWithKeyValue(
+      kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder,
+      kCFBooleanTrue);
+#endif
+
+  VTCompressionSessionRef session;
+  OSStatus status =
+      VTCompressionSessionCreate(kCFAllocatorDefault,
+                                 cast_config_.width,
+                                 cast_config_.height,
+                                 kCMVideoCodecType_H264,
+                                 encoder_spec,
+                                 nullptr /* sourceImageBufferAttributes */,
+                                 nullptr /* compressedDataAllocator */,
+                                 CompressionCallback,
+                                 reinterpret_cast<void*>(this),
+                                 &session);
+  if (status != noErr) {
+    DLOG(ERROR) << __func__ << " VTCompressionSessionCreate failed: "
+                << GetVTErrorString(status) << " (" << status << ")";
+    return;
+  }
+  compression_session_.reset(session);
+
+// query if we're using hardware
+#if defined(OS_IOS)
+  using_hardware_ = true;
+#else
+  CFBooleanRef using_hardware_cf = nullptr;
+  status = VTSessionCopyProperty(
+      session,
+      kVTCompressionPropertyKey_UsingHardwareAcceleratedVideoEncoder,
+      kCFAllocatorDefault,
+      &using_hardware_cf);
+  if (status == noErr) {
+    using_hardware_ = CFBooleanGetValue(using_hardware_cf);
+    CFRelease(using_hardware_cf);
+  }
+  DVLOG(3) << __func__ << " using hardware: " << using_hardware_;
+#endif
+
+  // configure the session
+  ConfigureSession();
+}
+
+static void SetConfigurationApplier(CFStringRef key,
+                                    CFTypeRef value,
+                                    VTCompressionSessionRef session) {
+  SetSessionProperty(session, key, CFTypeEmittable(value), value);

[miu, 2014/08/08 17:53:08]

Here and in ConfigureSession() below:

If you're going to ingore the return value, why not just call
VTSetSessionProperty directly?

Said another way: Don't ignore the return values.  If any calls are not OK,
something should fail (e.g., all calls to EncodeVideoFrame should return false).
 Given this is being invoked from the ctor, perhaps the application should crash
via CHECK_EQ(VTSetSessionProperty(...), OK)?

[jfroy, 2014/08/08 23:23:24]

The final SetSessionProperty used to log the error, but in a later PS (per
reviewer request) I removed almost all debug logging.

Failing on property errors is probably not a good idea. Some encoders will
reject certain properties or property values for whatever reason. It is also
hardware-dependent. Some properties are rejected at certain times but not others
(in-flight frames or not, state of the encoder, power state, battery state,
etc).

I consider the properties I am setting as best-efforts, meaning that failure is
tolerated. I'm gathering data on the implementation and perhaps certain
properties should indeed lead to encoding failure or initialization failure if
they can't be set.

+}
+
+void H264VideoToolboxEncoder::ConfigureSession() {
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_ProfileLevel,
+                     kVTProfileLevel_H264_Main_AutoLevel);
+
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_RealTime,
+                     true);
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_AllowFrameReordering,
+                     false);
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_MaxKeyFrameInterval,
+                     240u);
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration,
+                     240u);
+
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_AverageBitRate,
+                     static_cast<uint32_t>(cast_config_.start_bitrate));
+
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_ExpectedFrameRate,
+                     static_cast<uint32_t>(cast_config_.max_frame_rate));
+
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_ColorPrimaries,
+                     kCVImageBufferColorPrimaries_ITU_R_709_2);
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_TransferFunction,
+                     kCVImageBufferTransferFunction_ITU_R_709_2);
+  SetSessionProperty(compression_session_,
+                     kVTCompressionPropertyKey_YCbCrMatrix,
+                     kCVImageBufferYCbCrMatrix_ITU_R_709_2);
+
+  if (compression_properties_) {
+    CFDictionaryApplyFunction(
+        compression_properties_,
+        reinterpret_cast<CFDictionaryApplierFunction>(SetConfigurationApplier),
+        compression_session_.get());
+  }
+}
+
+void H264VideoToolboxEncoder::Teardown() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // If the compression session exists, invalidate it. This blocks until all

[miu, 2014/08/08 17:53:07]

re: "This blocks"

Never block the IO thread.  How about using base::WeakPtr<> to cancel all
outstanding callbacks?  You can find examples of this throughout src/media/cast
code.

[jfroy, 2014/08/08 23:23:25]

This will run in the video thread presumably. There is no way to avoid the
thread join done by VideoToolbox. Weak pointers are untenable because
VideoToolbox is a dumb C API; it only accepts a void* opaque context parameter.
The only race-free design is this: block until invalidation is complete and be
guaranteed that the output callback will not occur again.

If encoders are destroyed on the IO thread, this is more problematic, although
arguably at tear down there should not be any inbound or outbound IO left. In
that case I would probably wrap the VideoToolbox encoder in an object and use
weak pointers back to the main encoder object, much like the external encoder
implementation.

+  // pending output callbacks have returned and any internal threads have
+  // joined, ensuring no output callback ever sees a dangling encoder pointer.
+  if (compression_session_) {
+    VTCompressionSessionInvalidate(compression_session_);
+    compression_session_.reset();
+  }
+}
+
+#pragma mark -
+
+bool H264VideoToolboxEncoder::EncodeVideoFrame(
+      const scoped_refptr<media::VideoFrame>& video_frame,
+      const base::TimeTicks& capture_time,
+      const FrameEncodedCallback& frame_encoded_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (!compression_session_) {
+    DLOG(ERROR) << __func__ << " compression session is null";
+    return false;
+  }
+
+  // if the VideoFrame wraps a CVPixelBuffer, use that, otherwise wrap it
+  PixelBufferPtr pixel_buffer(video_frame->cv_pixel_buffer(),

[miu, 2014/08/08 17:53:07]

VideoFrame::cv_pixel_buffer() will return NULL no matter what.  Did you mean to
make a VideoFrame::SetCVPixelBuffer() call in WrapVideoFrame()?

[jfroy, 2014/08/08 23:23:25]

No. The idea here is that the frame producer will create VideoFrame objects
wrapping a CVPixelBuffer backed by the encoder's CVPixelBufferPool, which it
will then submit to the cast sender's video input object. We can discuss the
details in chat. For tab casting, it is true that the video frames will not wrap
a CVPixelBuffer (although that optimization could be made).

+                              base::scoped_policy::RETAIN);
+  if (!pixel_buffer) {
+    pixel_buffer = WrapVideoFrame(*video_frame);
+    if (!pixel_buffer) {
+      return false;
+    }
+  }
+
+  // convert the frame's timestamp to a CMTime
+  CMTime timestamp_cm;
+  if (capture_time.is_null()) {

[miu, 2014/08/08 17:53:09]

It's an illegal argument for capture_time to be null.  You could replace this
if-statement with:

  DCHECK(!capture_time.is_null());

...and put this at the top of this method (on line 438).

[jfroy, 2014/08/08 23:23:25]

Acknowledged.

+    timestamp_cm = kCMTimeInvalid;
+  } else {
+    timestamp_cm = CMTimeMake(capture_time.ToInternalValue(), USEC_PER_SEC);

[miu, 2014/08/08 17:53:06]

Never use ToInternalValue().  The timestamps are relative to some starting point
when being converted to intervals.  You could, conventionally, make the first
frame be t=0.  For example, the code in this else clause could become:

  if (first_frame_capture_time_.is_null())
    first_frame_capture_time_ = capture_time;
  timestamp_cm = CMTimeMake((capture_time -
first_frame_capture_time_).InMicroseconds(), USEC_PER_SEC);

[jfroy, 2014/08/08 23:23:26]

CMTime is also relative to a clock (which is not part of the data structure
either, you need to have implicit knowledge of the clock of a CMTime). This code
merely translates one clock-relative datatype to another (CMTime is effectively
a rational number with flags). Therefore I think using ToInternalValue is
exactly the right approach.

Also note that the EncodedFrame emitted for this input frame will use the
capture_time set in the frame context (see below).

[miu, 2014/08/25 19:21:17]

My point here is that ToInternalValue() is explicitly described as "do not use
in code."  It only has semantics internally to TimeTicks.  If the time origin
does not matter, how about:

  ... = CMTimeMake((capture_time - base::TimeTicks()).InMicroseconds(),
USEC_PER_SEC);

[jfroy, 2014/08/25 21:00:23]

Done.

+  }
+
+  // allocate frame compression context; will be deleted in the output callback
+  FrameContext* fc = new FrameContext();

[miu, 2014/08/08 17:53:07]

coding style: Don't abbreviate variable names.

[miu, 2014/08/08 17:53:08]

For safety, use scoped_ptr<> for this, and call scoped_ptr<>::release() when
transferring ownership to VTCompressionSessionEncodeFrame().

[jfroy, 2014/08/08 23:23:25]

Acknowledged.

[jfroy, 2014/08/08 23:23:25]

Acknowledged.

+  fc->capture_time = capture_time;
+  fc->frame_encoded_callback = frame_encoded_callback;
+
+  // encode the frame
+  DVLOG(3) << __func__ << " pts: " << timestamp_cm;
+
+  DictionaryPtr frame_props(nullptr);
+  if (encode_next_frame_as_keyframe_) {
+    frame_props = DictionaryWithKeyValue(kVTEncodeFrameOptionKey_ForceKeyFrame,
+                                         kCFBooleanTrue);
+    encode_next_frame_as_keyframe_ = false;
+  }
+
+  VTEncodeInfoFlags info;
+  OSStatus status = VTCompressionSessionEncodeFrame(
+    compression_session_,
+    pixel_buffer,
+    timestamp_cm,
+    kCMTimeInvalid,
+    frame_props,
+    reinterpret_cast<void*>(fc),
+    &info);
+  if (status != noErr) {
+    DLOG(ERROR) << __func__ << " VTCompressionSessionEncodeFrame failed: "
+                << GetVTErrorString(status) << " (" << status << ")";
+    return false;
+  }
+  if ((info & kVTEncodeInfo_FrameDropped)) {
+    DLOG(ERROR) << __func__ << " frame dropped";
+    return false;
+  }
+
+  return true;
+}
+
+void H264VideoToolboxEncoder::SetBitRate(int new_bit_rate) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  DVLOG(3) << __func__ << " bitrate: " << new_bit_rate;
+
+  if (!compression_session_) {

[miu, 2014/08/08 17:53:08]

Should this be DCHECK(compression_session_) instead?

[jfroy, 2014/08/08 23:23:25]

I used a log here because I was worried that since initialization is assumed to
succeed by VideoSender (it has no way of asking or checking if an encoder
actually initialized property and blindly reports success via the status
callback) that this may be called with an invalid session.

In the latest PS, this method is empty with a NOTE explaining that VideoToolbox
seems to ignore bitrate change requests once the encoder is active. I need to
research its behavior more (which is made more complicated by the facts that it
varies based on the underlying hardware and that iOS 8 is beta and has bugs).

+    DLOG(ERROR) << __func__ << " compression session is null";
+    return;
+  }
+
+//  if (new_bit_rate) {

[miu, 2014/08/08 17:53:07]

Why is this commented out?

[jfroy, 2014/08/08 23:23:25]

See my comment above.

+//    SetSessionProperty(compression_session_,
+//                       kVTCompressionPropertyKey_AverageBitRate,
+//                       static_cast<uint32_t>(new_bit_rate));
+//  }
+}
+
+void H264VideoToolboxEncoder::GenerateKeyFrame() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(compression_session_);
+
+  encode_next_frame_as_keyframe_ = true;
+}
+
+void H264VideoToolboxEncoder::LatestFrameIdToReference(uint32 /*frame_id*/) {}

[miu, 2014/08/08 17:53:08]

For documentation, can you make the body of this method the following:

  // Do nothing; not supported.

[jfroy, 2014/08/08 23:23:25]

Acknowledged.

+
+#pragma mark -
+
+static void VideoFramePixelBufferReleaseCallback(void* frame_ref,
+                                                 const void* data,
+                                                 size_t size,
+                                                 size_t num_planes,
+                                                 const void* planes[]) {
+  free(const_cast<void*>(data));
+  reinterpret_cast<media::VideoFrame*>(frame_ref)->Release();
+}
+
+H264VideoToolboxEncoder::PixelBufferPtr
+H264VideoToolboxEncoder::WrapVideoFrame(media::VideoFrame& frame) {

[miu, 2014/08/08 17:53:08]

nit: indent 4 spaces

[jfroy, 2014/08/08 23:23:24]

I've been formatting everything via clang-format / git cl format. The latest PS
is a bit different than what is here. What should be indented here?

+  static const size_t MAX_PLANES = 3;
+
+  media::VideoFrame::Format format = frame.format();
+  size_t num_planes = media::VideoFrame::NumPlanes(format);
+  gfx::Size coded_size = frame.coded_size();
+
+  // media::VideoFrame only supports YUV formats, so there is no way to
+  // leverage VideoToolbox's ability to convert RGBA formats automatically. In
+  // addition, most of the media::VideoFrame formats are YVU, which VT does not
+  // support. Finally, media::VideoFrame formats do not carry any information
+  // about the color space, transform or any other colorimetric information
+  // that is generally needed to fully specify the input data. So essentially
+  // require that the input be YCbCr 4:2:0 (either planar or biplanar) and
+  // assume the standard video dynamic range for samples (although most modern
+  // HDTVs support full-range video these days).
+  OSType pixel_format;
+  if (format == media::VideoFrame::Format::I420) {
+    pixel_format = kCVPixelFormatType_420YpCbCr8Planar;
+  } else if (format == media::VideoFrame::Format::NV12) {
+    pixel_format = kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange;
+  } else {
+    DLOG(ERROR) << __func__ << " unsupported frame format: " << format;
+    return PixelBufferPtr(nullptr);
+  }
+
+  // TODO(jfroy): support extended pixels (i.e. padding)
+  if (frame.coded_size() != frame.visible_rect().size()) {
+    DLOG(ERROR) << __func__ << " frame with extended pixels not supported: "
+                << " coded_size: " << coded_size.ToString()
+                << ", visible_rect: " << frame.visible_rect().ToString();
+    return PixelBufferPtr(nullptr);
+  }
+
+  DCHECK(media::VideoFrame::NumPlanes(format) <= MAX_PLANES);

[miu, 2014/08/08 17:53:07]

Consider moving this DCHECK up to line 542, so the error is caught sooner.

[jfroy, 2014/08/08 23:23:25]

Acknowledged.

+  void* plane_ptrs[MAX_PLANES];
+  size_t plane_widths[MAX_PLANES];
+  size_t plane_heights[MAX_PLANES];
+  size_t plane_bytes_per_row[MAX_PLANES];
+  for (size_t plane_i = 0; plane_i < num_planes; ++plane_i) {
+    plane_ptrs[plane_i] = frame.data(plane_i);
+    gfx::Size plane_size =
+        media::VideoFrame::PlaneSize(format, plane_i, coded_size);
+    plane_widths[plane_i] = plane_size.width();
+    plane_heights[plane_i] = plane_size.height();
+    plane_bytes_per_row[plane_i] = frame.stride(plane_i);
+  }
+
+  // CVPixelBufferCreateWithPlanarBytes needs a dummy plane descriptor or the
+  // release callback will not execute
+  void* descriptor =
+      calloc(1,

[miu, 2014/08/08 17:53:08]

nit: malloc(std::max(...));

[jfroy, 2014/08/08 23:23:25]

The memory needs to be zeroed. I thought calloc was the appropriate idiom to use
in that case. Should I memset after instead?

+             std::max(sizeof(CVPlanarPixelBufferInfo_YCbCrPlanar),
+                      sizeof(CVPlanarPixelBufferInfo_YCbCrBiPlanar)));
+
+  CVPixelBufferRef pixel_buffer;
+  CVReturn result =
+      CVPixelBufferCreateWithPlanarBytes(kCFAllocatorDefault,
+                                         coded_size.width(),
+                                         coded_size.height(),
+                                         format,
+                                         &descriptor,

[miu, 2014/08/08 17:53:07]

I think you meant descriptor and not &descriptor here.

[miu, 2014/08/08 17:53:08]

It's weird that you're passing an uninitialized structure here.  The
documentation for CVPixelBufferCreateWithPlanarBytes() says this can be NULL.

Are you absolutely sure the release callback won't be invoked if you provide
NULL here?  That would seem to point to a significant bug in the library.

[jfroy, 2014/08/08 23:23:24]

Good catch.

[jfroy, 2014/08/08 23:23:25]

Yes, I've extensively tested this behavior, decompiled CoreVideo and confirmed
it with other developers. CV completely ignores the plane pointers when
determining whether or not to call its release callback, but it does use the
plane pointers for data. I've filed a bug with Apple over this.

+                                         0,
+                                         num_planes,
+                                         plane_ptrs,
+                                         plane_widths,
+                                         plane_heights,
+                                         plane_bytes_per_row,
+                                         VideoFramePixelBufferReleaseCallback,
+                                         &frame,

[miu, 2014/08/08 17:53:09]

You meant frame.get(), not &frame.

[jfroy, 2014/08/08 23:23:24]

No, this is correct, since frame here is a media::VideoFrame&. But linting
pointed out that non-const references are not allowed, so I changed the function
to use a scoped_refptr and adjusted the code, all in a later PS.

+                                         nullptr,
+                                         &pixel_buffer);
+  if (result != kCVReturnSuccess) {
+    DLOG(ERROR) << __func__ << " CVPixelBufferCreateWithPlanarBytes failed: "
+                << GetCVErrorString(result) << " (" << result << ")";
+    return PixelBufferPtr(nullptr);
+  }
+
+  // The CVPixelBuffer now references the data of the frame, so increment its
+  // reference count manually. The release callback set on the pixel buffer will
+  // release the frame.
+  frame.AddRef();
+
+  return PixelBufferPtr(pixel_buffer);
+}
+
+#pragma mark -
+
+void H264VideoToolboxEncoder::CompressionCallback(
+    void* encoder_opaque,
+    void* frame_opaque,
+    OSStatus status,
+    VTEncodeInfoFlags info,
+    CMSampleBufferRef sbuf) {
+  H264VideoToolboxEncoder* encoder =
+      reinterpret_cast<H264VideoToolboxEncoder*>(encoder_opaque);
+  scoped_ptr<FrameContext> fc(reinterpret_cast<FrameContext*>(frame_opaque));

[miu, 2014/08/08 17:53:06]

style: Don't abbreviating variable names.

[jfroy, 2014/08/08 23:23:24]

Acknowledged.

+
+  // if encoding failed, report a platform error and bail
+  if (status != noErr) {
+    DLOG(ERROR) << __func__ << " encoding failed: " << GetVTErrorString(status)
+                << " (" << status << ")";
+    return;
+  }
+
+  // if the frame had to be dropped, bail
+  if ((info & kVTEncodeInfo_FrameDropped)) {
+    DVLOG(2) << __func__ << " frame dropped";
+    return;
+  }
+
+  // implementation only supports one frame (sample) per sample buffer
+  CMItemCount sample_count = CMSampleBufferGetNumSamples(sbuf);
+  if (sample_count > 1) {

[miu, 2014/08/08 17:53:07]

Is this expected?  Perhaps this should be DCHECK_EQ(sample_count, 1);

[jfroy, 2014/08/08 23:23:24]

For video, not likely (it certainly is for audio). I'm not sure what our best
option is here. Future hardware could behavior differently and I'd like to know
when that happens and on crash the client. A debug log or assertion won't tell
us what happens in the wild.

I think an assertion is better for now though.

+    DLOG(ERROR) << __func__
+                << " more than one sample in sample buffer: " << sample_count;
+    return;
+  }
+
+  // get the sample attachments, which will tell us if this is a keyframe
+  CFDictionaryRef sample_attachments =
+      static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex(
+          CMSampleBufferGetSampleAttachmentsArray(sbuf, true), 0));
+
+  // If the NotSync key is not present, it implies Sync, which indicates a
+  // keyframe (at least I think, VT documentation is, erm, sparse). Could
+  // alternatively use kCMSampleAttachmentKey_DependsOnOthers == false.
+  bool keyframe =
+      CFDictionaryContainsKey(sample_attachments,
+                              kCMSampleAttachmentKey_NotSync) == false;
+
+  // Generate a frame id and update the last keyframe id if needed.
+  // NOTE: VideoToolbox calls the output callback serially, so this is safe.
+  uint32 frame_id = ++encoder->frame_id_;
+  if (keyframe) {
+    encoder->last_keyframe_id_ = frame_id;
+  }
+
+  CMSampleTimingInfo timing_info;

[miu, 2014/08/08 17:53:09]

Should you be mapping timing_info back into a capture_time?  Could the codec do
temporal interpolation of frames?

If not, then please wrap these lines of code since they should only be executed
for debug logging:

  if (DVLOG_IS_ON(3)) {
    ...
  }

[jfroy, 2014/08/08 23:23:24]

Didn't know you could do that. The timing info was only for debug logging and
that code is removed in a later PS.

+  CMSampleBufferGetSampleTimingInfo(sbuf, 0, &timing_info);
+  DVLOG(3) << __func__
+           << ", timing info: " << timing_info
+           << ", keyframe: " << keyframe
+           << ", frame id: " << frame_id;
+
+  // create an EncodedFrame and fill in the basic information
+  scoped_ptr<EncodedFrame> encoded_frame(new EncodedFrame());
+  encoded_frame->frame_id = frame_id;
+  encoded_frame->reference_time = fc->capture_time;
+  encoded_frame->rtp_timestamp = GetVideoRtpTimestamp(fc->capture_time);
+  if (keyframe) {
+    encoded_frame->dependency = EncodedFrame::KEY;
+    encoded_frame->referenced_frame_id = frame_id;
+  } else {
+    encoded_frame->dependency = EncodedFrame::DEPENDENT;
+    // NOTE: Technically wrong, but without parsing the NALs our best guess is
+    // the last keyframe.
+    encoded_frame->referenced_frame_id = encoder->last_keyframe_id_;

[miu, 2014/08/08 17:53:07]

This needs to be true to the H264 codec.  If you don't know what the exact frame
dependencies are, you can always play it safe and do this (like in
external_video_encoder.cc):

  encoded_frame->referenced_frame_id = encoded_frame->frame_id - 1;

This means a cast sender or receiver is not allowed to drop any non-key frames
(i.e., all frames are dependent on their immediately prior frame, except for key
frames).  I believe that's exactly the case for the H264 codec.

[jfroy, 2014/08/08 23:23:25]

H.264 is allowed multiple frame references backward and forward. I do advise the
encoder not to use forward references (i.e. no frame reordering), but it could
totally decide to ignore that (unless the profile and forbids it, which it does
not as the code stands).

I think forcing the previous frame as a reference is too pessimistic.
VideoToolbox will emit keyframes are regular internals. Those absolutely cannot
be dropped because parameter sets are prefixed on those frames and required for
any subsequent frames until the next keyframe. But anything in the middle I
think is fair game to be dropped. That may mean some corrupt or dropped (at
display / decode time) frames on the receiver. But we'll save a lot of
unnecessary retransmissions.

+  }
+
+  // copy the frame data from the CM sample buffer to the encoded frame
+  CopySampleBufferToAnnexBBuffer(sbuf, encoded_frame->data, keyframe);
+
+  // post the frame's encoded callback
+  encoder->cast_environment_->PostTask(
+      CastEnvironment::MAIN,
+      FROM_HERE,
+      base::Bind(fc->frame_encoded_callback,
+                 base::Passed(&encoded_frame)));
+}
+
+template <typename NalSizeType>
+static void CopyNalsToAnnexB(
+    char* avcc_buffer,
+    const size_t avcc_size,
+    std::string& annexb_buffer) {

[miu, 2014/08/08 17:53:07]

Chromium style prohibits passing by reference.  Pass by pointer instead.

[jfroy, 2014/08/08 23:23:25]

Yep linting indicated that. Fixed in a later PS.

+  static_assert(sizeof(NalSizeType) == 1 ||

[miu, 2014/08/08 17:53:07]

Replace static_assert with COMPILE_ASSERT for Chromium code.

[jfroy, 2014/08/08 23:23:24]

Yep, other reviewer pointed it out. Fixed in a later PS.

+                sizeof(NalSizeType) == 2 ||
+                sizeof(NalSizeType) == 4,
+                "NAL size type has unsupported size");
+  static const char startcode_3[3] = {0, 0, 1};
+  size_t bytes_left = avcc_size;
+  while (bytes_left > 0) {
+    DCHECK(bytes_left > sizeof(NalSizeType));
+    NalSizeType nal_size;
+    base::ReadBigEndian(avcc_buffer, &nal_size);
+    bytes_left -= sizeof(NalSizeType);

[miu, 2014/08/08 17:53:08]

Consider using base::BigEndianReader instead.  It would simplify this code, in
that you wouldn't have to keep track of bytes_left and the avcc_buffer pointer.

[jfroy, 2014/08/08 23:23:25]

I can't because BigEndianReader does not have a templated read method. I'd have
to branch on the sizes. I made this a template function to get as much
performance as possible for the AVCC->Annex B conversion.

+    avcc_buffer += sizeof(NalSizeType);
+
+    DCHECK(bytes_left >= nal_size);
+    annexb_buffer.append(startcode_3, sizeof(startcode_3));
+    annexb_buffer.append(avcc_buffer, nal_size);
+    bytes_left -= nal_size;
+    avcc_buffer += nal_size;
+  }
+}
+
+void H264VideoToolboxEncoder::CopySampleBufferToAnnexBBuffer(
+    CMSampleBufferRef sbuf,
+    std::string& annexb_buffer,

[miu, 2014/08/08 17:53:08]

ditto: Pass by pointer, not by reference.

[jfroy, 2014/08/08 23:23:26]

Yep, fixed in a later PS.

+    bool keyframe) {
+  // Perform two pass, one to figure out the total output size, and another to
+  // copy the data after having performed a single output allocation. Note that
+  // we'll allocate a bit more because we'll count 4 bytes instead of 3 for
+  // video NALs.
+
+  // TODO(jfroy): There is a bug in
+  // CMVideoFormatDescriptionGetH264ParameterSetAtIndex, iterate until fail.
+  // rdar://17514276
+
+  OSStatus status;
+
+  // Get the sample buffer's block buffer and format description.
+  CMBlockBufferRef bb = CMSampleBufferGetDataBuffer(sbuf);
+  DCHECK(bb);
+  CMFormatDescriptionRef fdesc = CMSampleBufferGetFormatDescription(sbuf);
+  DCHECK(fdesc);
+
+  size_t bb_size = CMBlockBufferGetDataLength(bb);
+  size_t total_bytes = bb_size;
+
+  size_t pset_count;
+  int nal_size_field_bytes;
+  status = CMVideoFormatDescriptionGetH264ParameterSetAtIndex(
+      fdesc,
+      0,
+      nullptr,
+      nullptr,
+      &pset_count,
+      &nal_size_field_bytes);
+  if (status == kCMFormatDescriptionBridgeError_InvalidParameter) {
+    // rdar://17514276
+    DLOG(WARNING) << __func__
+        << " assuming 2 parameter sets and 4 bytes NAL length header "
+        << "(rdar://17514276)";
+    pset_count = 2;
+    nal_size_field_bytes = 4;
+  } else  if (status != noErr) {
+    DLOG(ERROR) << __func__
+        << " CMVideoFormatDescriptionGetH264ParameterSetAtIndex failed: "
+        << status;
+    return;
+  }
+
+  if (keyframe) {
+    const uint8_t* pset;
+    size_t pset_size;
+    for (size_t pset_i = 0; pset_i < pset_count; ++pset_i) {
+      status = CMVideoFormatDescriptionGetH264ParameterSetAtIndex(
+          fdesc,
+          pset_i,
+          &pset,
+          &pset_size,
+          nullptr,
+          nullptr);
+      if (status != noErr) {
+        DLOG(ERROR) << __func__
+            << " CMVideoFormatDescriptionGetH264ParameterSetAtIndex failed: "
+            << status;
+        return;
+      }
+      total_bytes += pset_size + nal_size_field_bytes;
+    }
+  }
+
+  annexb_buffer.reserve(total_bytes);
+
+  // Copy all parameter sets before keyframes.
+  if (keyframe) {
+    const uint8_t* pset;
+    size_t pset_size;
+    for (size_t pset_i = 0; pset_i < pset_count; ++pset_i) {
+      status = CMVideoFormatDescriptionGetH264ParameterSetAtIndex(
+          fdesc,
+          pset_i,
+          &pset,
+          &pset_size,
+          nullptr,
+          nullptr);
+      if (status != noErr) {
+        DLOG(ERROR) << __func__
+            << " CMVideoFormatDescriptionGetH264ParameterSetAtIndex failed: "
+            << status;
+        return;
+      }
+      static const char startcode_4[4] = {0, 0, 0, 1};
+      annexb_buffer.append(startcode_4, sizeof(startcode_4));
+      annexb_buffer.append(reinterpret_cast<const char*>(pset), pset_size);
+    }
+  }
+
+  // Block buffers can be composed of non-contiguous chunks. For the sake of
+  // keeping this code simple, flatten non-contiguous block buffers.
+  base::ScopedCFTypeRef<CMBlockBufferRef> contiguous_bb(
+      bb, base::scoped_policy::RETAIN);
+  if (!CMBlockBufferIsRangeContiguous(bb, 0, 0)) {
+    DVLOG(3) << __func__ << " copying block buffer to contiguous buffer";
+    contiguous_bb.reset();
+    status = CMBlockBufferCreateContiguous(kCFAllocatorDefault,
+                                           bb,
+                                           kCFAllocatorDefault,
+                                           nullptr,
+                                           0,
+                                           0,
+                                           0,
+                                           contiguous_bb.InitializeInto());
+    if (status != noErr) {
+      DLOG(ERROR) << __func__ << " CMBlockBufferCreateContiguous failed: "
+                  << status;
+      return;
+    }
+
+  }
+
+  // Copy all the NAL units. In the process convert them from AVCC format
+  // (length header) to AnnexB format (start code).
+  char* bb_data;
+  status = CMBlockBufferGetDataPointer(contiguous_bb,
+                                       0,
+                                       nullptr,
+                                       nullptr,
+                                       &bb_data);
+  if (status != noErr) {
+    DLOG(ERROR) << __func__ << " CMBlockBufferGetDataPointer failed: "
+                << status;
+    return;
+  }
+
+  if (nal_size_field_bytes == 1) {
+    CopyNalsToAnnexB<uint8_t>(bb_data, bb_size, annexb_buffer);
+  } else if (nal_size_field_bytes == 2) {
+    CopyNalsToAnnexB<uint16_t>(bb_data, bb_size, annexb_buffer);
+  } else if (nal_size_field_bytes == 4) {
+    CopyNalsToAnnexB<uint32_t>(bb_data, bb_size, annexb_buffer);
+  }
+}
+
+}  // namespace cast
+}  // namespace media