Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1657273002:
Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. (Closed)
|
Created:
4 years, 10 months ago by hbos Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAbility to disable the effects of |rtc_use_h264| with DisableRtcUseH264.
Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name.
The idea is to be able to turn off H264 from chromium with this function because...
1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled.
2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable.
3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not.
The idea is to call DisableRtcUseH264 in chromium's PeerConnectionDependencyFactory.
BUG=chromium:500605, chromium:468365
NOTRY=True
NOPRESUBMIT=True
Committed: https://crrev.com/9dc5928eb2b7653049b1c405485ca4351df28fbc
Cr-Commit-Position: refs/heads/master@{#11474}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Addressed comments (flag rename as TODO) #
Total comments: 1
Patch Set 3 : Gyp comment with #, not //, oops #
Messages
Total messages: 29 (12 generated)
hbos@webrtc.org changed reviewers: + kjellander@webrtc.org, stefan@webrtc.org, tommi@webrtc.org
Please take a look everyone. The idea is to be able to turn off H264 from chromium with this function so that... 1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled. 2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable. 3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not. The idea is to call SetRtcUseH264 in chromium's PeerConnectionDependencyFactory.
https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:16: #include "webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h" for what do we need the encoder header? https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:30: #if defined(WEBRTC_USE_H264) Is this flag for h264 in general or specific to openh264? Maybe we should rename it to WEBRTC_USE_OPENH264? https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:31: bool rtc_use_h264 = true; since this is a global variable, should have the g_ prefix https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:36: void SetRtcUseH264(bool enable) { 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. https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:38: rtc_use_h264 = enable; how do we guarantee thread safety? Wondering if we could trip off tsan tests? https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:66: if (IsH264CodecSupportedObjC()) { 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). https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:72: if (rtc_use_h264) { here, I would rather do: RTC_[D]CHECK(rtc_use_h264); return new H264EncoderImpl(); https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:94: if (rtc_use_h264) { RTC_CHECK(rtc_use_h264) https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/include/h264.h (right): https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/include/h264.h:33: // initialization and is not thread-safe. ah, not thread safe. Good comment. Feel free to ignore my tsan comment.
PTAL tommi https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:16: #include "webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h" On 2016/02/02 16:33:13, tommi-webrtc wrote: > for what do we need the encoder header? H264Encoder::Create() and H264Decoder::Create(). https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:30: #if defined(WEBRTC_USE_H264) On 2016/02/02 16:33:13, tommi-webrtc wrote: > Is this flag for h264 in general or specific to openh264? Maybe we should rename > it to WEBRTC_USE_OPENH264? 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. https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:31: bool rtc_use_h264 = true; On 2016/02/02 16:33:13, tommi-webrtc wrote: > since this is a global variable, should have the g_ prefix Done. https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:36: void SetRtcUseH264(bool enable) { On 2016/02/02 16:33:13, tommi-webrtc wrote: > 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. Done. (Calling it DisableRtcUseH264() after |rtc_use_h264|, could rename if/when flag is renamed) https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:38: rtc_use_h264 = enable; On 2016/02/02 16:33:13, tommi-webrtc wrote: > how do we guarantee thread safety? Wondering if we could trip off tsan tests? It's not, as per function comment in header. https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:66: if (IsH264CodecSupportedObjC()) { On 2016/02/02 16:33:13, tommi-webrtc wrote: > 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). (TODO for name changes.) https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:72: if (rtc_use_h264) { On 2016/02/02 16:33:13, tommi-webrtc wrote: > here, I would rather do: > > RTC_[D]CHECK(rtc_use_h264); > return new H264EncoderImpl(); Done. https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:94: if (rtc_use_h264) { On 2016/02/02 16:33:13, tommi-webrtc wrote: > RTC_CHECK(rtc_use_h264) Done. https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/include/h264.h (right): https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/include/h264.h:33: // initialization and is not thread-safe. On 2016/02/02 16:33:13, tommi-webrtc wrote: > ah, not thread safe. Good comment. Feel free to ignore my tsan comment. Acknowledged.
Description was changed from ========== Ability to disable the effects of |rtc_use_h264| with SetRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. BUG=chromium:500605, chromium:468365 ========== to ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. BUG=chromium:500605, chromium:468365 ==========
lgtm https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:16: #include "webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h" On 2016/02/02 17:08:19, hbos wrote: > On 2016/02/02 16:33:13, tommi-webrtc wrote: > > for what do we need the encoder header? > > H264Encoder::Create() and H264Decoder::Create(). ah sorry, I was thinking openh264 encode. never mind. https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:30: #if defined(WEBRTC_USE_H264) On 2016/02/02 17:08:19, hbos wrote: > On 2016/02/02 16:33:13, tommi-webrtc wrote: > > Is this flag for h264 in general or specific to openh264? Maybe we should > rename > > it to WEBRTC_USE_OPENH264? > > 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. Acknowledged. https://codereview.webrtc.org/1657273002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264.cc:36: void SetRtcUseH264(bool enable) { On 2016/02/02 17:08:19, hbos wrote: > On 2016/02/02 16:33:13, tommi-webrtc wrote: > > 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. > > Done. (Calling it DisableRtcUseH264() after |rtc_use_h264|, could rename if/when > flag is renamed) Acknowledged.
Please take a look stefan & kjellander. This is blocking some follow-up stuff required to get testing of H264 working in chromium/trybots.
lgtm
Please take a look stefan :)
Maybe you could add a comment in the cl description about what the purpose of this method is? Apart from that lgtm. https://codereview.webrtc.org/1657273002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264.gypi (right): https://codereview.webrtc.org/1657273002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264.gypi:28: // implementations. +1. Now the flag suggests that h264 is enabled/disabled entirely, when in fact it's only the third_party implementations. HW may still work.
Description was changed from ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. BUG=chromium:500605, chromium:468365 ========== to ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. The idea is to be able to turn off H264 from chromium with this function because... 1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled. 2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable. 3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not. The idea is to call DisableRtcUseH264 in chromium's PeerConnectionDependencyFactory. BUG=chromium:500605, chromium:468365 ==========
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/1657273002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657273002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7223)
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, kjellander@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1657273002/#ps40001 (title: "Gyp comment with #, not //, oops")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1657273002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657273002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
Description was changed from ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. The idea is to be able to turn off H264 from chromium with this function because... 1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled. 2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable. 3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not. The idea is to call DisableRtcUseH264 in chromium's PeerConnectionDependencyFactory. BUG=chromium:500605, chromium:468365 ========== to ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. The idea is to be able to turn off H264 from chromium with this function because... 1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled. 2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable. 3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not. The idea is to call DisableRtcUseH264 in chromium's PeerConnectionDependencyFactory. BUG=chromium:500605, chromium:468365 NOTRY=True NOPRESUBMIT=True ==========
On 2016/02/03 12:52:28, commit-bot: I haz the power wrote: > Exceeded global retry quota Huh? I'll just land with NOTRY=True NOPRESUBMIT=True
On 2016/02/03 12:52:28, commit-bot: I haz the power wrote: > Exceeded global retry quota Huh? I'll just land with NOTRY=True NOPRESUBMIT=True
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/1657273002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657273002/40001
Message was sent while issue was closed.
Description was changed from ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. The idea is to be able to turn off H264 from chromium with this function because... 1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled. 2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable. 3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not. The idea is to call DisableRtcUseH264 in chromium's PeerConnectionDependencyFactory. BUG=chromium:500605, chromium:468365 NOTRY=True NOPRESUBMIT=True ========== to ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. The idea is to be able to turn off H264 from chromium with this function because... 1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled. 2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable. 3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not. The idea is to call DisableRtcUseH264 in chromium's PeerConnectionDependencyFactory. BUG=chromium:500605, chromium:468365 NOTRY=True NOPRESUBMIT=True ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. The idea is to be able to turn off H264 from chromium with this function because... 1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled. 2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable. 3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not. The idea is to call DisableRtcUseH264 in chromium's PeerConnectionDependencyFactory. BUG=chromium:500605, chromium:468365 NOTRY=True NOPRESUBMIT=True ========== to ========== Ability to disable the effects of |rtc_use_h264| with DisableRtcUseH264. Renamed the WEBRTC_THIRD_PARTY_H264 macro to WEBRTC_USE_H264 to match flag name. The idea is to be able to turn off H264 from chromium with this function because... 1) The Chromium trybots will soon use this flag, we want to temporarily disable H264 from chromium even if flag is set in case something is broken. That way when we are ready to flip the switch the trybots will run our test code then and not after it is already enabled. 2) If feature is launched and we discover major problems we can easily disable H264 and merge with beta/stable. 3) Or, if feature is behind a *runtime* flag, this is how we would control if it is used or not. The idea is to call DisableRtcUseH264 in chromium's PeerConnectionDependencyFactory. BUG=chromium:500605, chromium:468365 NOTRY=True NOPRESUBMIT=True Committed: https://crrev.com/9dc5928eb2b7653049b1c405485ca4351df28fbc Cr-Commit-Position: refs/heads/master@{#11474} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9dc5928eb2b7653049b1c405485ca4351df28fbc Cr-Commit-Position: refs/heads/master@{#11474} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
