H.264 video codec support using OpenH264/FFmpeg

webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc

Index: webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc
diff --git a/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc b/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1ab15d301ef74da462ee48d9c380740ede33a3eb
--- /dev/null
+++ b/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc
@@ -0,0 +1,372 @@
+/*
+ *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ *
+ */
+
+#include "webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h"
+
+// OpenH264
+#include "codec_api.h"
+#include "codec_app_def.h"
+#include "codec_def.h"
+
+#include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/common_video/libyuv/include/webrtc_libyuv.h"
+
+namespace webrtc {
+
+namespace {
+const bool OPENH264_ENCODER_LOGGING = false;
+}  // anonymous namespace
+
+static VideoFrameType EVideoFrameType_to_VideoFrameType(
+    const EVideoFrameType& type) {

[stefan-webrtc, 2015/09/28 11:19:02]

You should be able to pass this by value since it's an enum?

[hbos, 2015/09/30 15:35:18]

Done.

+  switch (type) {
+    case videoFrameTypeInvalid:
+    case videoFrameTypeSkip:
+      return kSkipFrame;

[stefan-webrtc, 2015/09/28 11:19:02]

I think we can send videoFrameTypeSkip as a kDeltaFrame. kSkipFrame has been
used to notify the upper layers that the frame was dropped completely by the
encoder, which isn't the case for an h.264 skip frame, I think?

[hbos, 2015/09/30 15:35:18]

I don't think the upper layers are notified at all unless the encode was
successful? |encoded_image_callback_| is only used if we encoded something. I
changed videoFrameTypeSkip to kDeltaFrame, but I'm not sure this matters or if
it makes sense for a "skip" to be called "delta", sounds dubious.

[stefan-webrtc, 2015/10/01 08:19:30]

Well, kSkipFrame and an h.264 skip-encoded frame is a bit different. A
skip-encoded frame still means that some bits are to be sent (basically telling
the decoder that the frame is identical to the previous), while a kSkipFrame
simply notifies the upper layers that the frame in question wasn't encoded.

It may not matter in practice, but I'm not sure about that.

[hbos, 2015/10/01 12:19:45]

Acknowledged.

+    case videoFrameTypeIDR:

[noahric, 2015/09/25 00:11:31]

IDR == kKeyFrame, right?

Also SPS/PPS are treated as keyframes in the rest of webrtc. It's unclear if/how
those are handled here; does OpenH264 always put SPS/PPS/IDR in the same frame
and call that IDR?

[stefan-webrtc, 2015/09/28 11:19:02]

Agree. videoFrameTypeI should however probably be a kDeltaFrame since it doesn't
necessarily unset all reference buffers.

[hbos, 2015/09/28 11:33:20]

An I-frame is a key frame, and all IDR-frames are I-frames (but not all I-frames
are IDR-frames)? I'm not sure about the subtle differences, but I changed so
that IDR and I are both treated as kKeyFrame.

As for the SPS/PPS question: I don't know. Does any of the other reviewers know?

[hbos, 2015/09/30 15:35:18]

@stefan: Ok, kDeltaFrame for videoFrameTypeI it is. (And kKeyFrame for
videoFrameTypeIDR)

+    case videoFrameTypeI:
+    case videoFrameTypeP:
+    case videoFrameTypeIPMixed:
+      return kDeltaFrame;
+    default:
+      LOG(LS_WARNING) << "Unknown EVideoFrameType: " << type;
+      return kDeltaFrame;
+  }
+}
+
+// Copies the encoded bytes from |info| to |encoded_image| and updates the
+// fragmentation information of |frag_header|.
+//   After OpenH264 encoding, the encoded bytes are stored in |info| spread out
+// over a number of layers and "NAL units". Each NAL unit is a fragment starting
+// with the four-byte NAL header {0,0,0,1}. To save bytes, the NAL headers are

[noahric, 2015/09/25 00:11:31]

It's up to you, but it's probably not worth stripping the start code prefixes.
They should be insignificant in terms of relative size (other than SPS/PPS), and
it lets you turn things into basically a reserve size + memcpy.

[noahric, 2015/09/25 00:11:31]

For clarity, I would avoid calling those NAL headers. They are start codes or
start code prefixes. NAL headers generally refer to things like NAL
packetization headers.

[hbos, 2015/09/28 11:33:20]

   First comment:

There are assumptions about the inclusion/exclusion of these headers in other
code that I am not very familiar with. I had to do this for it work without
changing other H264 packetization code.

But it's an interesting point - do we want to do this? Because we do it I had to
add the RTPDefragmentization code which is used quiet hackily @
videoprocessor.cc:280. All other encoders produce data that can be directly fed
to the decoder, now with the H264 case we have to manipulate the byte stream
before feeding it to the decoder (something we have to do explicitly in local
unittests or that is done for us by the "depacketizer stuff" when receiving over
network/loopback).

I don't know how this works if communicating cross browsers. Would firefox also
strip the headers? If not, would we include double headers on the receive side?
I'm guessing there's a good reason for doing it.
stefan and mflodman might know more about why we do this and if we want to keep
doing it.

   Second comment:

OK, updated comment. Still calling them NAL units, is this correct?

[stefan-webrtc, 2015/09/28 11:53:55]

The RTP format for H264 specifies that start codes shouldn't be included in the
packets. I think Noah mainly suggested that it doesn't matter if you copy the
startcodes here, you can simply skip them when setting the fragmentation header
below.

[hbos, 2015/09/30 15:35:18]

Oh I see, I misunderstood. I didn't considered not stripping them for some
reason, but that is great. Then we don't need RtpDefragmentize anymore, encoded
data can be fed directly to the decoder! :)

+// excluded when copying to |encoded_image->_buffer|. However these headers must
+// be included for an H264Decoder to be able to decode the data.
+//   When fragments are sent over a network, the receiving end must re-insert
+// the NAL headers before each fragment.
+//   For image data encoded and decoded locally, RTPDefragmentize can be used
+// to convert (EncodedImage, RTPFragmentationHeader) with NAL headers excluded
+// to a decodable EncodedImage buffer with NAL headers included.
+static void RTPFragmentize(EncodedImage* encoded_image,
+                           const VideoFrame& frame,
+                           SFrameBSInfo* info,
+                           RTPFragmentationHeader* frag_header) {
+  // Calculate minimum buffer size required to hold encoded data.
+  size_t required_size = 0;
+  for (int iLayer = 0; iLayer < info->iLayerNum; ++iLayer) {
+    const SLayerBSInfo& layerInfo = info->sLayerInfo[iLayer];
+    for (int iNal = 0; iNal < layerInfo.iNalCount; ++iNal) {
+      required_size += (layerInfo.pNalLengthInByte[iNal] - 4);
+    }
+  }
+  if (encoded_image->_size < required_size) {
+    // Increase buffer size. Allocate enough to hold an unencoded image, this
+    // should be more than enough to hold any encoded data of future frames of
+    // the same size (avoiding possible future reallocation due to variations in
+    // required size).
+    encoded_image->_size = CalcBufferSize(
+        VideoType::kI420, frame.width(), frame.height());
+    if (encoded_image->_size < required_size) {
+      // Encoded data > unencoded data, wtf? Allocate required bytes.

[noahric, 2015/09/25 00:11:31]

Consider logging this. You're right, it's pretty WTF :)

[hbos, 2015/09/28 11:33:20]

Done.

+      encoded_image->_size = required_size;
+    }
+    if (encoded_image->_buffer != nullptr)
+      delete[] encoded_image->_buffer;

[noahric, 2015/09/25 00:11:31]

Consider storing the buffer separately and with scoped_ptr, for safety.

[hbos, 2015/09/28 11:33:20]

Done.

+    encoded_image->_buffer = new uint8_t[encoded_image->_size];
+  }
+
+  // Iterate layers and NAL units, copy encoded data to |encoded_image->_buffer|
+  // and note each NAL unit as a fragment, excluding its NAL header.
+  encoded_image->_length = 0;
+  std::vector<int> frags;
+  for (int iLayer = 0; iLayer < info->iLayerNum; ++iLayer) {
+    int iLayerLen = 0;
+    const SLayerBSInfo& layerInfo = info->sLayerInfo[iLayer];
+    // Copy the layer data to |encoded_image->_buffer|, excluding the 4-byte
+    // NAL headers.
+    for (int iNal = 0; iNal < layerInfo.iNalCount; ++iNal) {
+      // Expecting NAL header constant {0,0,0,1}.
+      DCHECK_EQ(layerInfo.pBsBuf[iLayerLen+0], static_cast<unsigned char>(0));
+      DCHECK_EQ(layerInfo.pBsBuf[iLayerLen+1], static_cast<unsigned char>(0));
+      DCHECK_EQ(layerInfo.pBsBuf[iLayerLen+2], static_cast<unsigned char>(0));
+      DCHECK_EQ(layerInfo.pBsBuf[iLayerLen+3], static_cast<unsigned char>(1));
+
+      memcpy(encoded_image->_buffer + encoded_image->_length,
+             layerInfo.pBsBuf + iLayerLen + 4,

[stefan-webrtc, 2015/09/28 11:19:02]

Do you know if these layers are stored in one chunk of memory? In that case you
could copy all at once and just set the fragmentation header to ignore the start
codes.

[hbos, 2015/09/30 15:35:18]

Done.

+             (layerInfo.pNalLengthInByte[iNal] - 4) * sizeof(unsigned char));
+      encoded_image->_length += (layerInfo.pNalLengthInByte[iNal] - 4);
+      frags.push_back(layerInfo.pNalLengthInByte[iNal] - 4);
+
+      iLayerLen += layerInfo.pNalLengthInByte[iNal];
+    }
+  }
+
+  frag_header->VerifyAndAllocateFragmentationHeader(frags.size());
+  for (size_t i = 0, off = 0; i < frags.size(); ++i) {
+    frag_header->fragmentationOffset[i] = off;
+    frag_header->fragmentationLength[i] = frags[i];
+    off += frags[i];

[stefan-webrtc, 2015/09/28 11:19:02]

frags -> fragments
off -> offset

[hbos, 2015/09/30 15:35:18]

Done.

+  }
+}
+
+void H264EncoderImpl::RTPDefragmentize(

[stefan-webrtc, 2015/09/28 11:19:02]

Is this a test function? In that case I would move it to the test code.

[hbos, 2015/09/30 15:35:18]

(Removed, no longer needed due to including start codes.)

+    const EncodedImage& encoded_image,
+    const RTPFragmentationHeader* frag_header,
+    uint8_t* enc_buffer_with_nal, size_t enc_buffer_with_nal_length) {
+  DCHECK_GE(enc_buffer_with_nal_length,
+            RTPDefragmentizeBufferLengthWithNAL(encoded_image, frag_header));
+  const unsigned char nal_header[] = { 0, 0, 0, 1 };
+  for (size_t i = 0; i < frag_header->fragmentationVectorSize; ++i) {
+    DCHECK_LE(frag_header->fragmentationOffset[i] +
+              frag_header->fragmentationLength[i],
+              encoded_image._length);
+
+    // Insert a NAL header constant {0,0,0,1}.
+    memcpy(enc_buffer_with_nal, nal_header, 4);
+    // Copy fragment data.
+    memcpy(enc_buffer_with_nal + 4,
+           encoded_image._buffer + frag_header->fragmentationOffset[i],
+           frag_header->fragmentationLength[i]);
+
+    enc_buffer_with_nal += (4 + frag_header->fragmentationLength[i]);
+  }
+}
+
+size_t H264EncoderImpl::RTPDefragmentizeBufferLengthWithNAL(

[stefan-webrtc, 2015/09/28 11:19:02]

Same with this.

[hbos, 2015/09/30 15:35:18]

(Removed, no longer needed due to including start codes.)

+    const EncodedImage& encoded_image,
+    const webrtc::RTPFragmentationHeader* frag_header) {
+  return encoded_image._length +
+      4 * static_cast<size_t>(frag_header->fragmentationVectorSize);
+}
+
+H264EncoderImpl::H264EncoderImpl()
+    : openh264_encoder_(nullptr),
+      encoded_image_callback_(nullptr) {
+}
+
+H264EncoderImpl::~H264EncoderImpl() {
+  Release();
+}
+
+int32_t H264EncoderImpl::InitEncode(const VideoCodec* codec_settings,
+                                    int32_t /*number_of_cores*/,
+                                    size_t /*max_payload_size*/) {
+  if (!codec_settings ||
+      codec_settings->codecType != VideoCodecType::kVideoCodecH264) {
+    return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+  }
+  if (codec_settings->maxFramerate == 0)
+    return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+  if (codec_settings->width < 1 || codec_settings->height < 1)
+    return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+
+  int release_ret = Release();
+  if (release_ret != WEBRTC_VIDEO_CODEC_OK)
+    return release_ret;
+  DCHECK(!openh264_encoder_);
+
+  // Create encoder.
+  if (WelsCreateSVCEncoder(&openh264_encoder_) != 0) {
+    // Failed to create encoder.
+    LOG(LS_ERROR) << "Failed to create OpenH264 encoder";
+    DCHECK(!openh264_encoder_);
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+  DCHECK(openh264_encoder_);
+  if (&codec_settings_ != codec_settings)

[noahric, 2015/09/25 00:11:31]

This won't ever be false, will it?

[hbos, 2015/09/28 11:33:20]

You're right. Fixed.

+    codec_settings_ = *codec_settings;
+
+  if (codec_settings_.targetBitrate == 0)
+    codec_settings_.targetBitrate = codec_settings_.startBitrate;
+
+  // Note: H264 codec specifics are ignored:
+  // - codec_settings->codecSpecific.H264.frameDroppingOn

[stefan-webrtc, 2015/09/28 11:19:02]

Have you taken a look to see if it's possible to configure something like this?
It's typically available in rate control settings.

See bEnableFrameSkip.

[hbos, 2015/09/30 15:35:18]

Done.

+  // - codec_settings->codecSpecific.H264.keyFrameInterval

[noahric, 2015/09/25 00:11:31]

Assuming it's set, you don't really want to skip it.

[stefan-webrtc, 2015/09/28 11:19:02]

Agree, we should use this.

[hbos, 2015/09/28 11:33:20]

I don't know what to do with these. I believe the iOS implementation use them.
I'll try to dig some more before a future patch set.

[hbos, 2015/09/30 15:35:18]

Using them both now to set SEncParamExt's bEnableFrameSkip and uiIntraPeriod.

+
+  // Initialization parameters.
+  //   There are two ways to initialize. There is SEncParamBase (cleared with
+  // memset(&p, 0, sizeof(SEncParamBase)) used in Initialize, and SEncParamExt
+  // which is a superset of SEncParamBase (cleared with GetDefaultParams) used
+  // in InitializeExt. We use SEncParamBase/Initialize.

[stefan-webrtc, 2015/09/28 11:19:02]

Is there a reason why we use those?

I think we should consider using SEncParamExt as it allows for some more
settings which could be useful, for instance:

iMultipleThreadIdc to enable multi-threading. May require slicing set to
SM_AUTO_SLICE, but probably makes sense to do for HD resolutions.

bEnableDenoise to enable denoising? (not sure we want this, but we do have it
for vp8 and vp9. It depends on its quality and complexity though, so I suggest
ignoring it for now)

uiMaxNalSize = 0


We can probably consider some of these and even more options down the line
though. But it could make sense to look at enabling multi-threading at least?

[hbos, 2015/09/30 15:35:18]

Using SEncParamExt now. iMultileThreadIdc set to auto and
sSpatialLayers[0].sSliceCfg.uiSliceMode to SM_AUTO_SLICE.
Setting uiMaxNalSize = 0 too even though it is the default. What does 0 mean?

+  SEncParamBase init_params;
+  memset(&init_params, 0, sizeof(SEncParamBase));
+  if (codec_settings_.mode == kRealtimeVideo) {
+    init_params.iUsageType = CAMERA_VIDEO_REAL_TIME;
+  } else if (codec_settings_.mode == kScreensharing) {
+    init_params.iUsageType = SCREEN_CONTENT_REAL_TIME;
+  } else {
+    return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+  }
+  init_params.iPicWidth = codec_settings_.width;
+  init_params.iPicHeight = codec_settings_.height;
+  // iTargetBitrate is in bit/s, targetBitrate is in kbit/s.
+  init_params.iTargetBitrate = codec_settings_.targetBitrate * 1000;
+  // Rate Control mode
+  init_params.iRCMode = RC_QUALITY_MODE;

[stefan-webrtc, 2015/09/28 11:19:02]

I suspect a better choice would be RC_BITRATE_MODE or RC_TIMESTAMP_MODE. I think
RC_TIMESTAMP_MODE would make most sense since then we can pass a timestamp in
milliseconds with each frame and the rate controller will try to use as many
bits as the interval to the previous frame allows for.

[hbos, 2015/09/30 15:35:18]

I've noticed that you get really laggy video if the timestamps are not correct
(e.g. forgetting to convert from 90kHz timestamps to ms). VideoProcessorImpl
uses incorrect timestamp values (it uses frame count as timestamp) so at least
one test fails if I use RC_TIMESTAMP_MODE.

Switched to RC_BITRATE_MODE and added a TODO to fix this in a later CL if we
want to have RC_TIMESTAMP_MODE. I think RC_BITRATE_MODE is less prone to errors
which might be good for the initial CL.

[stefan-webrtc, 2015/10/01 08:19:30]

I think RC_BITRATE_MODE makes more sense actually.

The  problem with RC_TIMESTAMP_MODE is that we will be delayed with one frame,
so we may run into problems if we have a long pause (few seconds) between frame
i-1 and frame i. The encoder may then think that it can spend a few seconds of
bit budget on frame i, which isn't true since that time has already passed... :)

[hbos, 2015/10/01 12:19:45]

Acknowledged. Removed TODO comment.

+  init_params.fMaxFrameRate = static_cast<float>(codec_settings_.maxFramerate);

[noahric, 2015/09/25 00:11:31]

Not sure how to set these, but there are few others I'd expect that need to be
set:
uiIntraPeriod - keyframe period. Should be far between, if periodic keyframes
aren't enabled.
iEntropyCodingModeFlag - the recent h264 qp parser doesn't understand cabac, so
make sure this is false
iNumRefFrame - 1; maybe inferred from camera_video_real_time?
uiProfileIdc - baseline, for fastest processing on decoders of various platforms
<something that disallows B frames> - maybe realtime implies this as well, or
maybe iNumRefFrame == 1 forces it

[stefan-webrtc, 2015/09/28 11:19:02]

CABAC must be false since we only support baseline profile, and not main.

[hbos, 2015/09/28 11:33:20]

Those parameters are available if SEncParamExt is used instead of SEncParamBase
and they are, amongst other parameters, set to default values with
GetDefaultParams. If the default values are used for the extra params not in
SEncParamBase I don't know whether or not there is a difference between using
SEncParamBase and SEncParamExt. Switching to SEncParamExt to make more
parameters available but not sure if I should explicitly set all parameters or
rely on GetDefaultParams.


Some FYI information about these parameters. They might say more to the
reviewers than me, much of it is like a different language to me.

These are comments from the source code about these properties from
codec_app_def.h/SEncParamExt and param_cvc.h/FillDefault.
- uiIntraPeriod: "period of Intra frame", "multiple of GOP size as desired"
- iEntropyCodingModeFlag: "0:CAVLC  1:CABAC"
- iNumRefFrame: "number of reference frame used"
- uiProfileIdc: "value of profile IDC (PRO_UNKNOWN for auto-detection)"

This is how the default values are set:
https://github.com/cisco/openh264/blob/master/codec/encoder/core/inc/param_sv...

Here's an example of existing chromium code setting these parameters:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...

iEntropyCodingModeFlag and uiProfileIdc are set appropriately by default. Do you
want me to explicitly set these anyway to make it explicit and in case the
openh264 default values change in the future or leave as-is after setting
defaults?
By default, iNumRefFrame = AUTO_REF_PIC_COUNT, which is = -1 with comment:
"encoder selects the number of reference frame automatically". Should I really
set this to 1 and not use the automatic setting?
As for uiIntraPeriod, I have no idea what to set this to. It defaults to 0.


Switched to SEncParamExt but relying on a lot of defaults atm. Had to set
sSpatialLayers[0] values for it to work.

[stefan-webrtc, 2015/09/28 11:53:55]

I think automatic is better. There is no reason that I know of where we don't
want to use multiple reference frames if possible. Or are you thinking of CPU
issues, Noah?
0 is no intra frame. That is good for the normal case, but if
codec_settings->codecSpecific.H264.keyFrameInterval is set we should use that to
set uiIntraPeriod, I believe.
Makes sense, which means you are configuring the base spatial layer (which is
the only layer we use).

[hbos, 2015/09/30 15:35:18]

Using keyFrameInterval to set uiIntraPeriod.

+
+  // Initialize.
+  if (openh264_encoder_->Initialize(&init_params) != 0) {
+    // Failed to initialize.

[stefan-webrtc, 2015/09/28 11:19:02]

No need for this comment.

[hbos, 2015/09/30 15:35:18]

Done.

+    LOG(LS_ERROR) << "Failed to initialize OpenH264 encoder";
+    Release();
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+  if (OPENH264_ENCODER_LOGGING) {
+    int trace_level = WELS_LOG_DETAIL;
+    openh264_encoder_->SetOption(ENCODER_OPTION_TRACE_LEVEL,
+                                 &trace_level);

[noahric, 2015/09/25 00:11:31]

What's the default? If nothing, then consider setting WELS_LOG_WARNING in an
else block.

[hbos, 2015/09/28 11:33:20]

It has a default value it uses, WELS_LOG_DEFAULT (= WELS_LOG_WARNING), no need
to set.
Because this is a constant I moved the SetOption to init.

+  }
+
+  // Initialize encoded image.
+  // Default buffer size: size of unencoded data (should be large enough).
+  encoded_image_._size = CalcBufferSize(
+      VideoType::kI420, codec_settings_.width, codec_settings_.height);
+  encoded_image_._buffer = new uint8_t[encoded_image_._size];
+  encoded_image_._completeFrame = true;
+  encoded_image_._encodedWidth = 0;
+  encoded_image_._encodedHeight = 0;
+  encoded_image_._length = 0;
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t H264EncoderImpl::Release() {
+  if (openh264_encoder_) {
+    int uninit_ret = openh264_encoder_->Uninitialize();
+    if (uninit_ret != 0) {
+      LOG(LS_WARNING) << "OpenH264 encoder's Uninitialize() returned "
+                      << "unsuccessful: " << uninit_ret;
+    }
+    WelsDestroySVCEncoder(openh264_encoder_);
+    openh264_encoder_ = nullptr;
+  }
+  if (encoded_image_._buffer != nullptr) {
+    delete[] encoded_image_._buffer;
+    encoded_image_._buffer = nullptr;
+  }
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t H264EncoderImpl::RegisterEncodeCompleteCallback(
+    EncodedImageCallback* callback) {
+  encoded_image_callback_ = callback;
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t H264EncoderImpl::SetRates(uint32_t bitrate, uint32_t framerate) {
+  if (bitrate <= 0 || framerate <= 0) {
+    return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+  }
+  codec_settings_.targetBitrate = bitrate;
+  codec_settings_.maxFramerate = framerate;
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t H264EncoderImpl::Encode(
+    const VideoFrame& frame, const CodecSpecificInfo* codec_specific_info,
+    const std::vector<VideoFrameType>* frame_types) {
+  if (!IsInitialized())
+    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
+  if (frame.IsZeroSize())
+    return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+  if (!encoded_image_callback_) {
+    LOG(LS_WARNING) << "InitEncode() has been called, but a callback function "
+                    << "has not been set with RegisterEncodeCompleteCallback()";
+    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
+  }
+
+  // Make |codec_settings_|'s size reflect the latest frame's size.
+  if (codec_settings_.width != frame.width() ||

[noahric, 2015/09/25 00:11:31]

That won't be right if simulcast is used, will it? Or does some other component
scale the frame before it arrives here?

That's hypothetical, of course, but it points at a more general point: I don't
think the encoder should try to match the input, it should match whatever it's
configured to produce.

(Also, it looks like this value isn't used outside of init, so it's kinda
confusing to have it modified here)

[hbos, 2015/09/28 11:33:20]

Sorry, I'm not sure I follow your simulcast comment. Won't be right if simulcast
is used? What do you mean? Whatever the path we just encode the frame, right?
I'm thinking scaling is someone else's problem?

I proceeded to encode with the new frame size and updated |codec_settings_| to
reflect the latest settings, even though |codec_settings_.width,height| was not
actually used (unlike |codec_settings_.targetBitrate,maxFramerate|).

But as you say perhaps we want to fail if encoding is attempted with the wrong
frame size. I'm not sure what is correct, it looks like VP8 adapts and VP9
crashes (DCHECK).
I changed the code to return WEBRTC_VIDEO_CODEC_ERR_SIZE if there is a size
mismatch.

I could remove |codec_settings_| completely now but it might be nice to have for
debugging?

+      codec_settings_.height != frame.height()) {
+    codec_settings_.width = frame.width();
+    codec_settings_.height = frame.height();
+  }
+
+  // Set encoder options.
+  int video_format = EVideoFormatType::videoFormatI420;
+  openh264_encoder_->SetOption(ENCODER_OPTION_DATAFORMAT,
+                               &video_format);
+  SBitrateInfo target_bitrate;
+  memset(&target_bitrate, 0, sizeof(SBitrateInfo));
+  target_bitrate.iLayer = SPATIAL_LAYER_ALL,
+  target_bitrate.iBitrate = codec_settings_.targetBitrate * 1000;
+  openh264_encoder_->SetOption(ENCODER_OPTION_BITRATE,
+                               &target_bitrate);

[noahric, 2015/09/25 00:11:31]

Why not just set these in SetRates?

[hbos, 2015/09/28 11:33:20]

Done.

+  float max_framerate = static_cast<float>(codec_settings_.maxFramerate);
+  openh264_encoder_->SetOption(ENCODER_OPTION_FRAME_RATE,
+                               &max_framerate);
+
+  // EncodeFrame input.
+  SSourcePicture picture;
+  memset(&picture, 0, sizeof(SSourcePicture));
+  picture.iPicWidth = frame.width();
+  picture.iPicHeight = frame.height();
+  picture.iColorFormat = video_format;
+  picture.uiTimeStamp = frame.timestamp();
+  picture.iStride[0] = frame.stride(kYPlane);
+  picture.iStride[1] = frame.stride(kUPlane);
+  picture.iStride[2] = frame.stride(kVPlane);
+  picture.pData[0] = const_cast<uint8_t*>(frame.buffer(kYPlane));
+  picture.pData[1] = const_cast<uint8_t*>(frame.buffer(kUPlane));
+  picture.pData[2] = const_cast<uint8_t*>(frame.buffer(kVPlane));
+
+  // EncodeFrame output.
+  SFrameBSInfo info;
+  memset(&info, 0, sizeof(SFrameBSInfo));
+
+  // Encode!
+  if (openh264_encoder_->EncodeFrame(&picture, &info) != 0) {

[noahric, 2015/09/25 00:11:30]

It's probably worth capturing the return and logging it.

[hbos, 2015/09/28 11:33:20]

Done.

+    LOG(LS_ERROR) << "OpenH264 frame encoding failed (EncodeFrame)";
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+
+  encoded_image_._encodedWidth = frame.width();
+  encoded_image_._encodedHeight = frame.height();
+  encoded_image_._timeStamp = frame.timestamp();
+  encoded_image_.capture_time_ms_ = frame.render_time_ms();
+  encoded_image_._frameType = EVideoFrameType_to_VideoFrameType(
+      info.eFrameType);
+
+  // Split encoded image up into fragments. This also updates |encoded_image_|.
+  RTPFragmentationHeader frag_header;
+  RTPFragmentize(&encoded_image_, frame, &info, &frag_header);
+
+  // Encoder can skip frames to save bandwidth in which case
+  // |encoded_image_._length| == 0.
+  if (encoded_image_._length > 0) {
+    // Deliver encoded image.
+    encoded_image_callback_->Encoded(encoded_image_, codec_specific_info,
+                                     &frag_header);
+  }
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+bool H264EncoderImpl::IsInitialized() {
+  return openh264_encoder_ != nullptr;
+}
+
+int32_t H264EncoderImpl::SetChannelParameters(
+    uint32_t packet_loss, int64_t rtt) {
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t H264EncoderImpl::SetPeriodicKeyFrames(bool enable) {
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t H264EncoderImpl::CodecConfigParameters(uint8_t* buffer, int32_t size) {
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+void H264EncoderImpl::OnDroppedFrame() {
+}
+
+}  // namespace webrtc