From c5a39c25919191cec1b838a7e83ac028bb2e9d85 Mon Sep 17 00:00:00 2001 From: hbos Date: Tue, 2 Feb 2016 02:26:05 -0800 Subject: [PATCH] H264: Thread-safe InitializeFFmpeg. Flag to control if InitializeFFmpeg should be called. New flag: rtc_initialize_ffmpeg, default value = !build_with_chromium. In WebRTC standalone we initialize FFmpeg by default, in Chromium we don't by default. Chromium is an external project that also use FFmpeg. If both projects do FFmpeg initialization code things will break. The flag makes it possible for other external projects than chromium to decide whether or not WebRTC should initialize FFmpeg. BUG=chromium:500605, chromium:468365, webrtc:5427 Review URL: https://codereview.webrtc.org/1639273002 Cr-Commit-Position: refs/heads/master@{#11456} --- webrtc/build/common.gypi | 12 +++++++++ webrtc/build/webrtc.gni | 8 ++++++ webrtc/modules/video_coding/BUILD.gn | 3 +++ .../video_coding/codecs/h264/h264.gypi | 7 ++++++ .../codecs/h264/h264_decoder_impl.cc | 25 ++++++++++--------- 5 files changed, 43 insertions(+), 12 deletions(-) diff --git a/webrtc/build/common.gypi b/webrtc/build/common.gypi index a77420c070..d5b79f9c13 100644 --- a/webrtc/build/common.gypi +++ b/webrtc/build/common.gypi @@ -142,11 +142,23 @@ # support, |ffmpeg_branding| has to separately be set to a value that # includes H.264, for example "Chrome". If FFmpeg is built without H.264, # compilation succeeds but |H264DecoderImpl| fails to initialize. + # See also: |rtc_initialize_ffmpeg|. # CHECK THE OPENH264, FFMPEG AND H.264 LICENSES/PATENTS BEFORE BUILDING. # http://www.openh264.org, https://www.ffmpeg.org/ 'rtc_use_h264%': 0, 'conditions': [ + # FFmpeg must be initialized for |H264DecoderImpl| to work. This can be + # done by WebRTC during |H264DecoderImpl::InitDecode| or externally. + # FFmpeg must only be initialized once. Projects that initialize FFmpeg + # externally, such as Chromium, must turn this flag off so that WebRTC + # does not also initialize. + ['build_with_chromium==0', { + 'rtc_initialize_ffmpeg%': 1, + }, { + 'rtc_initialize_ffmpeg%': 0, + }], + ['build_with_chromium==1', { # Exclude pulse audio on Chromium since its prerequisites don't require # pulse audio. diff --git a/webrtc/build/webrtc.gni b/webrtc/build/webrtc.gni index f784ab1830..886b899543 100644 --- a/webrtc/build/webrtc.gni +++ b/webrtc/build/webrtc.gni @@ -96,9 +96,17 @@ declare_args() { # support, |ffmpeg_branding| has to separately be set to a value that # includes H.264, for example "Chrome". If FFmpeg is built without H.264, # compilation succeeds but |H264DecoderImpl| fails to initialize. + # See also: |rtc_initialize_ffmpeg|. # CHECK THE OPENH264, FFMPEG AND H.264 LICENSES/PATENTS BEFORE BUILDING. # http://www.openh264.org, https://www.ffmpeg.org/ rtc_use_h264 = false + + # FFmpeg must be initialized for |H264DecoderImpl| to work. This can be done + # by WebRTC during |H264DecoderImpl::InitDecode| or externally. FFmpeg must + # only be initialized once. Projects that initialize FFmpeg externally, such + # as Chromium, must turn this flag off so that WebRTC does not also + # initialize. + rtc_initialize_ffmpeg = !build_with_chromium } # A second declare_args block, so that declarations within it can diff --git a/webrtc/modules/video_coding/BUILD.gn b/webrtc/modules/video_coding/BUILD.gn index 115ed3588b..2d9e08d347 100644 --- a/webrtc/modules/video_coding/BUILD.gn +++ b/webrtc/modules/video_coding/BUILD.gn @@ -140,6 +140,9 @@ source_set("webrtc_h264") { if (rtc_use_h264) { defines += [ "WEBRTC_THIRD_PARTY_H264" ] + if (rtc_initialize_ffmpeg) { + defines += [ "WEBRTC_INITIALIZE_FFMPEG" ] + } sources += [ "codecs/h264/h264_decoder_impl.cc", "codecs/h264/h264_decoder_impl.h", diff --git a/webrtc/modules/video_coding/codecs/h264/h264.gypi b/webrtc/modules/video_coding/codecs/h264/h264.gypi index 0e80561bdf..e82b0fd4cd 100644 --- a/webrtc/modules/video_coding/codecs/h264/h264.gypi +++ b/webrtc/modules/video_coding/codecs/h264/h264.gypi @@ -27,6 +27,13 @@ 'defines': [ 'WEBRTC_THIRD_PARTY_H264', ], + 'conditions': [ + ['rtc_initialize_ffmpeg==1', { + 'defines': [ + 'WEBRTC_INITIALIZE_FFMPEG', + ], + }], + ], 'dependencies': [ '<(DEPTH)/third_party/ffmpeg/ffmpeg.gyp:ffmpeg', '<(DEPTH)/third_party/openh264/openh264.gyp:openh264_encoder', 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 75d2bfa732..ea96f25a1e 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; bool ffmpeg_initialized = false; // 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,13 +178,15 @@ 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) - // Make sure FFmpeg has been initialized. + // 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_initialize_ffmpeg| flag. +#if defined(WEBRTC_INITIALIZE_FFMPEG) + // Make sure FFmpeg has been initialized. Subsequent |InitializeFFmpeg| calls + // do nothing. InitializeFFmpeg(); #endif