H264: Thread-safe InitializeFFmpeg. Flag to control if InitializeFFmpeg should be called.

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

Index: webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc
diff --git a/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc b/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc
index 75d2bfa73245a050aa9688e53b0f0fe1538884b0..54d84dc58b4a2479e2e937f33955ffb4e7a694c4 100644
--- a/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc
+++ b/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc
@@ -34,8 +34,9 @@ const size_t kYPlaneIndex = 0;
 const size_t kUPlaneIndex = 1;
 const size_t kVPlaneIndex = 2;
 
-#if !defined(WEBRTC_CHROMIUM_BUILD)
+#if defined(WEBRTC_INITIALIZE_FFMPEG)
 
+rtc::CriticalSection ffmpeg_init_lock;

[hbos, 2016/01/27 13:35:42]

Is it OK to have a global variable that is a class object? I know it is
generally discouraged, but I need a lock for InitializeFFmpeg, which is a global
function.

[stefan-webrtc, 2016/01/27 14:00:39]

Hm, I'm not sure, but I think it should be OK. We could get into trouble with
increasing the number of static initializers in Chrome, but that shouldn't be a
problem since we don't include this in the Chrome build.

Would it make any sense to make the initialization function and this lock static
in H264DecoderImpl?

[hbos, 2016/01/28 10:31:09]

If I expose it in the header then I have to make sure the
WEBRTC_INITIALIZE_FFMPEG macro is defined in any target that might include the
header (if the flag is on). It should be possible to force defines on any
targets that depend on the h264 target in both GYP and GN, I don't know off the
top of my head but I could look into that. Is there a good reason to make it
static though? Do you want InitializeFFmpeg to be public and callable from
outside this file? The current lazy init approach makes sense I think and
doesn't require it to be publicly/staticly visible.

[stefan-webrtc, 2016/01/28 10:39:46]

I just wanted to make sure we considered it. Agree it doesn't add any value.

 bool ffmpeg_initialized = false;

[stefan-webrtc, 2016/01/27 14:00:38]

I guess an option could be to use atomics for this? Not sure if it really
matters though, probably not?

[hbos, 2016/01/28 10:31:09]

Don't think it matters since I have to use the lock anyway, or, at least during
init. You mean std::atomic_flag or similar to avoid a lock in the case of it
already having been initialized? Meh, probably not worth it and it would mean
another global object.

[stefan-webrtc, 2016/01/28 10:39:46]

Leave as is.

 
 // Called by FFmpeg to do mutex operations if initialized using
@@ -61,10 +62,8 @@ int LockManagerOperation(void** lock, AVLockOp op)
   return -1;
 }
 
-// TODO(hbos): Assumed to be called on a single thread. Should DCHECK that
-// InitializeFFmpeg is only called on one thread or make it thread safe.
-// See https://bugs.chromium.org/p/webrtc/issues/detail?id=5427.
 void InitializeFFmpeg() {
+  rtc::CritScope cs(&ffmpeg_init_lock);
   if (!ffmpeg_initialized) {
     if (av_lockmgr_register(LockManagerOperation) < 0) {
       RTC_NOTREACHED() << "av_lockmgr_register failed.";
@@ -75,7 +74,7 @@ void InitializeFFmpeg() {
   }
 }
 
-#endif  // !defined(WEBRTC_CHROMIUM_BUILD)
+#endif  // defined(WEBRTC_INITIALIZE_FFMPEG)
 
 // Called by FFmpeg when it is done with a frame buffer, see AVGetBuffer2.
 void AVFreeBuffer2(void* opaque, uint8_t* data) {
@@ -179,12 +178,13 @@ int32_t H264DecoderImpl::InitDecode(const VideoCodec* codec_settings,
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
   }
 
-  // In Chromium FFmpeg will be initialized outside of WebRTC and we should not
-  // attempt to do so ourselves or it will be initialized twice.
-  // TODO(hbos): Put behind a different flag in case non-chromium project wants
-  // to initialize externally.
-  // See https://bugs.chromium.org/p/webrtc/issues/detail?id=5427.
-#if !defined(WEBRTC_CHROMIUM_BUILD)
+  // FFmpeg must have been initialized (with |av_lockmgr_register| and
+  // |av_register_all|) before we proceed. |InitializeFFmpeg| does this, which
+  // makes sense for WebRTC standalone. In other cases, such as Chromium, FFmpeg
+  // is initialized externally and calling |InitializeFFmpeg| would be
+  // thread-unsafe and result in FFmpeg being initialized twice, which could
+  // break other FFmpeg usage. See the |rtc_skip_ffmpeg_init| flag.

[stefan-webrtc, 2016/01/27 14:00:39]

How is it safe to do this in webrtc if Chrome can break by initializing ffmpeg
twice? We will call InitDecode() multiple times.

[hbos, 2016/01/28 10:31:09]

Calling InitializeFFmpeg multiple times won't init multiple times due to our
|ffmpeg_initialized| flag. The problem is Chrome's ffmpeg init doesn't know
about our init flag and we don't know about Chrome's init flag, so we can't have
two methods for initializing. But either init method can be called multiple
times because it ensures not to init again if it already has. (Unfortunately
FFmpeg doesn't tell us whether or not it has been initialized in the sense we
care about.)

Since we can't check "Has FFmpeg been initialized?" independent of a flag of our
own we either always do it ourselves or we leave the responsibility completely
to the external project (Chrome) which turned off WEBRTC_INITIALIZE_FFMPEG. (If
Chrome didn't init FFmpeg, InitDecode will return an error code because it won't
find the H264 decoder).

Added clarifying comment: "Subsequent |InitializeFFmpeg| calls do nothing."

+#if defined(WEBRTC_INITIALIZE_FFMPEG)
   // Make sure FFmpeg has been initialized.
   InitializeFFmpeg();
 #endif