Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264.

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

 /*
  *  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/include/h264.h"
 
-#if defined(WEBRTC_THIRD_PARTY_H264)
+#if defined(WEBRTC_USE_H264)
+#include "webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h"
 #include "webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h"

[tommi, 2016/02/02 16:33:13]

for what do we need the encoder header?

[hbos, 2016/02/02 17:08:19]

H264Encoder::Create() and H264Decoder::Create().

[tommi, 2016/02/02 17:11:19]

ah sorry, I was thinking openh264 encode. never mind.

-#include "webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h"
 #endif
 #if defined(WEBRTC_IOS)
 #include "webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.h"
 #include "webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.h"
 #endif
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 
 namespace webrtc {
 
+namespace {
+
+#if defined(WEBRTC_USE_H264)

[tommi, 2016/02/02 16:33:13]

Is this flag for h264 in general or specific to openh264? Maybe we should rename
it to WEBRTC_USE_OPENH264?

[hbos, 2016/02/02 17:08:19]

It's for OpenH264 encoding + FFmpeg decoding both. The flag is called
rtc_use_h264 so I named the macro after it. I'll add a TODO about flag/macro
name.

[tommi, 2016/02/02 17:11:19]

Acknowledged.

+bool rtc_use_h264 = true;

[tommi, 2016/02/02 16:33:13]

since this is a global variable, should have the g_ prefix

[hbos, 2016/02/02 17:08:19]

Done.

+#endif
+
+}  // namespace
+
+void SetRtcUseH264(bool enable) {

[tommi, 2016/02/02 16:33:13]

I'm assuming we do not want to allow flipping this value back and forth and that
this is specifically used to disable the feature (given that it's turned on by
default).  If so, then I think we should just call this DisableOpenH264() and it
takes no parameters.

[hbos, 2016/02/02 17:08:19]

Done. (Calling it DisableRtcUseH264() after |rtc_use_h264|, could rename if/when
flag is renamed)

[tommi, 2016/02/02 17:11:19]

Acknowledged.

+#if defined(WEBRTC_USE_H264)
+  rtc_use_h264 = enable;

[tommi, 2016/02/02 16:33:13]

how do we guarantee thread safety?  Wondering if we could trip off tsan tests?

[hbos, 2016/02/02 17:08:19]

It's not, as per function comment in header.

+#endif
+}
+
 // We need this file to be C++ only so it will compile properly for all
 // platforms. In order to write ObjC specific implementations we use private
 // externs. This function is defined in h264.mm.
 #if defined(WEBRTC_IOS)
 extern bool IsH264CodecSupportedObjC();
 #endif
 
 // If any H.264 codec is supported (iOS HW or OpenH264/FFmpeg).
 bool IsH264CodecSupported() {
 #if defined(WEBRTC_IOS)
   if (IsH264CodecSupportedObjC()) {
     return true;
   }
 #endif
-#if defined(WEBRTC_THIRD_PARTY_H264)
-  return true;
+#if defined(WEBRTC_USE_H264)
+  return rtc_use_h264;
 #else
   return false;
 #endif
 }
 
 H264Encoder* H264Encoder::Create() {
   RTC_DCHECK(H264Encoder::IsSupported());
 #if defined(WEBRTC_IOS) && defined(WEBRTC_VIDEO_TOOLBOX_SUPPORTED)
   if (IsH264CodecSupportedObjC()) {

[tommi, 2016/02/02 16:33:13]

since h264 is supported in this case, I think we should rename the
WEBRTC_USE_H264 as per the other comment.

Perhaps a better name for it would be WEBRTC_ENABLE_OPENH264 since the
'rtc_use_h264' variable can be changed independently of this macro being defined
(i.e. use vs enabled).

[hbos, 2016/02/02 17:08:19]

(TODO for name changes.)

     LOG(LS_INFO) << "Creating H264VideoToolboxEncoder.";
     return new H264VideoToolboxEncoder();
   }
 #endif
-#if defined(WEBRTC_THIRD_PARTY_H264)
-  LOG(LS_INFO) << "Creating H264EncoderImpl.";
-  return new H264EncoderImpl();
-#else
+#if defined(WEBRTC_USE_H264)
+  if (rtc_use_h264) {

[tommi, 2016/02/02 16:33:13]

here, I would rather do:

RTC_[D]CHECK(rtc_use_h264);
return new H264EncoderImpl();

[hbos, 2016/02/02 17:08:19]

Done.

+    LOG(LS_INFO) << "Creating H264EncoderImpl.";
+    return new H264EncoderImpl();
+  }
+#endif
   RTC_NOTREACHED();
   return nullptr;
-#endif
 }
 
 bool H264Encoder::IsSupported() {
   return IsH264CodecSupported();
 }
 
 H264Decoder* H264Decoder::Create() {
   RTC_DCHECK(H264Decoder::IsSupported());
 #if defined(WEBRTC_IOS) && defined(WEBRTC_VIDEO_TOOLBOX_SUPPORTED)
   if (IsH264CodecSupportedObjC()) {
     LOG(LS_INFO) << "Creating H264VideoToolboxDecoder.";
     return new H264VideoToolboxDecoder();
   }
 #endif
-#if defined(WEBRTC_THIRD_PARTY_H264)
-  LOG(LS_INFO) << "Creating H264DecoderImpl.";
-  return new H264DecoderImpl();
-#else
+#if defined(WEBRTC_USE_H264)
+  if (rtc_use_h264) {

[tommi, 2016/02/02 16:33:13]

RTC_CHECK(rtc_use_h264)

[hbos, 2016/02/02 17:08:19]

Done.

+    LOG(LS_INFO) << "Creating H264DecoderImpl.";
+    return new H264DecoderImpl();
+  }
+#endif
   RTC_NOTREACHED();
   return nullptr;
-#endif
 }
 
 bool H264Decoder::IsSupported() {
   return IsH264CodecSupported();
 }
 
 }  // namespace webrtc