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

webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.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.
  *
  */
@@ skipping 16 matching lines @@
 
 namespace webrtc {
 
 namespace {
 
 const AVPixelFormat kPixelFormat = AV_PIX_FMT_YUV420P;
 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
 // |InitializeFFmpeg|.
 int LockManagerOperation(void** lock, AVLockOp op)
     EXCLUSIVE_LOCK_FUNCTION() UNLOCK_FUNCTION() {
   switch (op) {
     case AV_LOCK_CREATE:
       *lock = new rtc::CriticalSection();
       return 0;
     case AV_LOCK_OBTAIN:
       static_cast<rtc::CriticalSection*>(*lock)->Enter();
       return 0;
     case AV_LOCK_RELEASE:
       static_cast<rtc::CriticalSection*>(*lock)->Leave();
       return 0;
     case AV_LOCK_DESTROY:
       delete static_cast<rtc::CriticalSection*>(*lock);
       *lock = nullptr;
       return 0;
   }
   RTC_NOTREACHED() << "Unrecognized AVLockOp.";
   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.";
       return;
     }
     av_register_all();
     ffmpeg_initialized = true;
   }
 }
 
-#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) {
   VideoFrame* video_frame = static_cast<VideoFrame*>(opaque);
   delete video_frame;
 }
 
 // Called by FFmpeg when it needs a frame buffer to store decoded frames in.
 // The VideoFrames returned by FFmpeg at |Decode| originate from here. They are
 // reference counted and freed by FFmpeg using |AVFreeBuffer2|.
@@ skipping 83 matching lines @@
   Release();
 }
 
 int32_t H264DecoderImpl::InitDecode(const VideoCodec* codec_settings,
                                     int32_t number_of_cores) {
   if (codec_settings &&
       codec_settings->codecType != kVideoCodecH264) {
     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
 
   // Release necessary in case of re-initializing.
   int32_t ret = Release();
   if (ret != WEBRTC_VIDEO_CODEC_OK)
     return ret;
   RTC_DCHECK(!av_context_);
 
@@ skipping 155 matching lines @@
     return ret;
   }
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 bool H264DecoderImpl::IsInitialized() const {
   return av_context_ != nullptr;
 }
 
 }  // namespace webrtc