H.264 packetization mode 0 (try 2)

webrtc/common_types.h

Index: webrtc/common_types.h
diff --git a/webrtc/common_types.h b/webrtc/common_types.h
index c3e9a84c3cc729b5cb3e1b1287731cc102aa3ba0..53446203e332bda12d795cbd296af1b114487d90 100644
--- a/webrtc/common_types.h
+++ b/webrtc/common_types.h
@@ -542,10 +542,17 @@ struct VideoCodecVP9 {
 };
 
 // H264 specific.
+enum H264PacketizationMode {
+  kH264PacketizationModeNotSet,

[hbos, 2016/10/31 10:32:26]

Is this ever set? This enum is used in constructorless structs, if not set
properly it will have a garbage value, not this one. The GetDefaultH264Settings
uses kH264PacketizationMode1.

Oh but perhaps we memset the entire struct to 0? If that is how it ends up being
used I prefer to explicitly say "kH264PacketizationModeNotSet = 0," here to
imply that its value being 0 does have significance.

[hta-webrtc, 2016/10/31 15:03:04]

Good point about being explicit that it is 0. The struct VideoCodecH264 is
initialized using memset() all over the place - the last change I did (going
from DCHECK to LOG(WARNING) when it wasn't set) was because it's initialized
from Java, where I'm unable to figure out exactly where it is initialized.

+  kH264PacketizationMode0,  // Only single NALU allowed
+  kH264PacketizationMode1   // Non-interleaved - STAP-A, FU-A is allowed
+};
+
 struct VideoCodecH264 {
   VideoCodecProfile profile;
   bool frameDroppingOn;
   int keyFrameInterval;
+  H264PacketizationMode packetization_mode;

[magjed_webrtc, 2016/10/31 13:00:50]

Since the packetization mode information is included in cricket::VideoCodec, I
don't think we need to add it to the VideoCodecH264 struct as well. External
encoders can now include packetization mode information in the
cricket::VideoCodecs they return from 'std::vector<cricket::VideoCodec>&
WebRtcVideoEncoderFactory::supported_codecs()', and that information will be
passed back into 
'webrtc::VideoEncoder* WebRtcVideoEncoderFactory::CreateVideoEncoder(const
cricket::VideoCodec& codec)'. So the factory should be able to pass
packetization mode information (as well as profile information) to the
constructor of the webrtc::VideoEncoder, so it shouldn't be necessary to also
add it to webrtc::VideoCodec which is passed to 'int32_t
VideoEncoder::InitEncode(const webrtc::VideoCodec* codec_settings, int32_t
number_of_cores, size_t max_payload_size)'.

[hta-webrtc, 2016/10/31 20:54:52]

Now I'm somewhat confused..... what code path can we use to get the information
contained in a cricket::VideoCodec to the places where we need to have access to
that information?

The points we need the info are:
- video_coding/codecs/h264/h264_encoder_impl.cc (to set the encoder parameters)
- rtp_format_h264.cc (to check that we always take the single NALU path when
mode 0 is in effect)

Which structure that is visible from these 2 places (or the functions that
construct them) contains a cricket::VideoCodec?

[magjed_webrtc, 2016/11/01 19:23:38]

To clarify, I don't think we need to add packetization mode to
webrtc::VideoCodec (via VideoCodecUnion/VideoCodecH264), because it should be
redundant if we pass the cricket::VideoCodec to all encoder constructors.

To get cricket::VideoCodec to video_coding/codecs/h264/h264_encoder_impl.cc, we
need to pass the cricket::VideoCodec we have selected in
WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder all the way down
to H264EncoderImpl::H264EncoderImpl. I have been planning to do this change
anyway, so that we can send the profile information to H264EncoderImpl. More
concretely, the essential change is this:

diff --git a/webrtc/media/engine/webrtcvideoengine2.cc
b/webrtc/media/engine/webrtcvideoengine2.cc
 WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder(
     const VideoCodec& codec) {
     
-  if (type == webrtc::kVideoCodecVP8) {
-    return AllocatedEncoder(
-        webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kVp8), type, false);
-  } else if (type == webrtc::kVideoCodecVP9) {
-    return AllocatedEncoder(
-        webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kVp9), type, false);
-  } else if (type == webrtc::kVideoCodecH264) {
-    return AllocatedEncoder(
-        webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kH264), type,
false);
-  }
+  // Try creating internal encoder.
+  if (webrtc::VideoEncoder::IsSupportedSoftware(type))
+    return AllocatedEncoder(webrtc::VideoEncoder::Create(codec), codec, true);

diff --git a/webrtc/video/video_encoder.cc b/webrtc/video/video_encoder.cc
-VideoEncoder* VideoEncoder::Create(VideoEncoder::EncoderType codec_type) {
+VideoEncoder* VideoEncoder::Create(const cricket::VideoCodec& codec) {
+  const VideoCodecType codec_type = CodecTypeFromName(codec.name);
   RTC_DCHECK(IsSupportedSoftware(codec_type));
   switch (codec_type) {
-    case kH264:
-      return H264Encoder::Create();
-    case kVp8:
-      return VP8Encoder::Create();
-    case kVp9:
-      return VP9Encoder::Create();
-    case kUnsupportedCodec:
+    case kVideoCodecH264:
+      return H264Encoder::Create(codec);
+    case kVideoCodecVP8:
+      return VP8Encoder::Create(codec);
+    case kVideoCodecVP9:
+      return VP9Encoder::Create(codec);
+    default:
       RTC_NOTREACHED();
       return nullptr;
   }
-  RTC_NOTREACHED();
-  return nullptr;
 }
 
-  static VideoEncoder* Create(EncoderType codec_type);
+  static VideoEncoder* Create(const cricket::VideoCodec& codec);

diff --git a/webrtc/modules/video_coding/codecs/h264/h264.cc
b/webrtc/modules/video_coding/codecs/h264/h264.cc
-H264Encoder* H264Encoder::Create() {
+H264Encoder* H264Encoder::Create(const cricket::VideoCodec& codec) {
   RTC_DCHECK(H264Encoder::IsSupported());
 #if defined(WEBRTC_IOS) && defined(WEBRTC_VIDEO_TOOLBOX_SUPPORTED)
   if (IsH264CodecSupportedObjC()) {
@@ -71,7 +71,7 @@ H264Encoder* H264Encoder::Create() {
 #if defined(WEBRTC_USE_H264)
   RTC_CHECK(g_rtc_use_h264);
   LOG(LS_INFO) << "Creating H264EncoderImpl.";
-  return new H264EncoderImpl();
+  return new H264EncoderImpl(codec);
 #else
   RTC_NOTREACHED();
   return nullptr;

diff --git a/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc
b/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc
-H264EncoderImpl::H264EncoderImpl()
+H264EncoderImpl::H264EncoderImpl(const cricket::VideoCodec& codec)
     : openh264_encoder_(nullptr),
       number_of_cores_(0),
       encoded_image_callback_(nullptr),

   // These are NULL/0 if not externally negotiated.
   const uint8_t* spsData;
   size_t spsLen;