Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1639273002:
H264: Thread-safe InitializeFFmpeg. Flag to control if InitializeFFmpeg should be called. (Closed)
|
Created:
4 years, 11 months ago by hbos Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionH264: 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
Committed: https://crrev.com/c5a39c25919191cec1b838a7e83ac028bb2e9d85
Cr-Commit-Position: refs/heads/master@{#11456}
Patch Set 1 #
Total comments: 11
Patch Set 2 : rtc_initialize_ffmpeg #
Messages
Total messages: 18 (9 generated)
Description was changed from ========== H264: Thread-safe InitializeFFmpeg. Flag to control if InitializeFFmpeg should be called. New flag: rtc_skip_ffmpeg_init, 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=5427 ========== to ========== H264: Thread-safe InitializeFFmpeg. Flag to control if InitializeFFmpeg should be called. New flag: rtc_skip_ffmpeg_init, 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 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5427 ==========
hbos@webrtc.org changed reviewers: + kjellander@webrtc.org, stefan@webrtc.org
Please take a look stefan, kjellander. https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:39: rtc::CriticalSection ffmpeg_init_lock; 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.
https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/B... File webrtc/modules/video_coding/BUILD.gn (right): https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/B... webrtc/modules/video_coding/BUILD.gn:143: if (!rtc_skip_ffmpeg_init) { Doesn't it make more sense to call this rtc_init_ffmpeg or similar? Now we get a double negation here... https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:39: rtc::CriticalSection ffmpeg_init_lock; On 2016/01/27 13:35:42, hbos wrote: > 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. 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? https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:40: bool ffmpeg_initialized = false; I guess an option could be to use atomics for this? Not sure if it really matters though, probably not? https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:186: // break other FFmpeg usage. See the |rtc_skip_ffmpeg_init| flag. How is it safe to do this in webrtc if Chrome can break by initializing ffmpeg twice? We will call InitDecode() multiple times.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
PTAL stefan. https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/B... File webrtc/modules/video_coding/BUILD.gn (right): https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/B... webrtc/modules/video_coding/BUILD.gn:143: if (!rtc_skip_ffmpeg_init) { On 2016/01/27 14:00:38, stefan-webrtc (holmer) wrote: > Doesn't it make more sense to call this rtc_init_ffmpeg or similar? Now we get a > double negation here... Yeah you're right, heh, I made it that way so I could default it to '<(build_with_chromium)', I think the only way to negate a value in GYP is to use the clunky 'conditions' or to evaluate a python expression (hardcore). I'll rename it rtc_initialize_ffmpeg to be similar to the WEBRTC_INITIALIZE_FFMPEG macro and use 'conditions' for its default value. https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:39: rtc::CriticalSection ffmpeg_init_lock; On 2016/01/27 14:00:39, stefan-webrtc (holmer) wrote: > On 2016/01/27 13:35:42, hbos wrote: > > 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. > > 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? 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. https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:40: bool ffmpeg_initialized = false; On 2016/01/27 14:00:38, stefan-webrtc (holmer) wrote: > I guess an option could be to use atomics for this? Not sure if it really > matters though, probably not? 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. https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:186: // break other FFmpeg usage. See the |rtc_skip_ffmpeg_init| flag. On 2016/01/27 14:00:39, stefan-webrtc (holmer) wrote: > How is it safe to do this in webrtc if Chrome can break by initializing ffmpeg > twice? We will call InitDecode() multiple times. 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."
Description was changed from ========== H264: Thread-safe InitializeFFmpeg. Flag to control if InitializeFFmpeg should be called. New flag: rtc_skip_ffmpeg_init, 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 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5427 ========== to ========== 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 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5427 ==========
lgtm for webrtc/modules https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:39: rtc::CriticalSection ffmpeg_init_lock; On 2016/01/28 10:31:09, hbos wrote: > On 2016/01/27 14:00:39, stefan-webrtc (holmer) wrote: > > On 2016/01/27 13:35:42, hbos wrote: > > > 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. > > > > 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? > > 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. I just wanted to make sure we considered it. Agree it doesn't add any value. https://codereview.webrtc.org/1639273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:40: bool ffmpeg_initialized = false; On 2016/01/28 10:31:09, hbos wrote: > On 2016/01/27 14:00:38, stefan-webrtc (holmer) wrote: > > I guess an option could be to use atomics for this? Not sure if it really > > matters though, probably not? > > 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. Leave as is.
Description was changed from ========== 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 BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5427 ========== to ========== 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 ==========
Thanks. Please take a look, kjellander.
*.gyp*, *.gn*: lgtm
The CQ bit was checked by hbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639273002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639273002/60001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/c5a39c25919191cec1b838a7e83ac028bb2e9d85 Cr-Commit-Position: refs/heads/master@{#11456} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c5a39c25919191cec1b838a7e83ac028bb2e9d85 Cr-Commit-Position: refs/heads/master@{#11456} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
