Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1601813005:
rtc_use_h264 flag for building OpenH264/FFmpeg (Closed)
|
Created:
4 years, 11 months ago by hbos Modified:
4 years, 11 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, 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. |
Descriptionrtc_use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg, false by default but can be overridden in supplement.gypi and build_overrides/webrtc.gni.
BUG=468365
NOTRY=True
Committed: https://crrev.com/902c03e724f2f732e67a631f8bd5ba5cac3276a6
Cr-Commit-Position: refs/heads/master@{#11333}
Patch Set 1 : use_h264=true on all supported platforms (!ios), will default to false before landing #
Total comments: 4
Patch Set 2 : Rebase with master #
Total comments: 4
Patch Set 3 : Addressed comments (rtc_use_h264, supplement.gypi) #Patch Set 4 : rtc_use_h264 = false (not overriding defaults) #
Messages
Total messages: 25 (14 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== use_h264 flag Attempting to build third_party/openh264 and third_party/ffmpeg in WebRTC. BUG=468365 ========== to ========== use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg Attempting to build third_party/openh264 and third_party/ffmpeg in WebRTC. BUG=468365 ==========
hbos@webrtc.org changed reviewers: + kjellander@chromium.org
Description was changed from ========== use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg Attempting to build third_party/openh264 and third_party/ffmpeg in WebRTC. BUG=468365 ========== to ========== use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg. Attempting to build third_party/openh264 and third_party/ffmpeg (in WebRTC) by switching default to true, will be back to false before landing. BUG=468365 ==========
Please take a look kjellander. (Can't land until https://codereview.chromium.org/1602443005 has rolled in fixing the win_[x64_]clang_rel errors but that won't change this CL.) We talked about reusing use_openh264 for the sake of having less flags and I sort of agreed that we could do that, but I don't think so anymore. See if we use use_openh264 in webrtc, in GN this variable is only defined if something depends on openh264's GN file which pulls openh264_args.gn, but we should only depend on it if use_openh264 is true. If we remedy this by including openh264_args.gn ourselves it works if use_openh264 is false but if its true and we depend on openh264 it ends up being declared twice (error). I suggest that use_openh264 is only used in chromium as a condition for including it to "All" targets (currently the case). In webrtc, we only use our own flag, use_h264, and don't care about the value of use_openh264. That means that the trybots only need to override 2 flags: use_h264 = true (use both openh264 and ffmpeg) ffmpeg_branding = "Chrome" (or other branding supporting H.264) https://codereview.webrtc.org/1601813005/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1601813005/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:142: # compilation succeeds but |H264DecoderImpl| fails to initialize. (Not asserting that ffmpeg_branding has to specifically be "Chrome", might be other valid configurations) https://codereview.webrtc.org/1601813005/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:152: }], Know where to do "override defaults" similar to build_overrides/webrtc.gni? There's gyp_webrtc of course but I wasn't sure about the "if OS != ios" then and I didn't find something similar to build_overrides/ so I placed the condition here (temporarily, to be removed before landing)
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
https://codereview.webrtc.org/1601813005/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1601813005/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:152: }], On 2016/01/19 11:01:27, hbos wrote: > Know where to do "override defaults" similar to build_overrides/webrtc.gni? > There's gyp_webrtc of course but I wasn't sure about the "if OS != ios" then and > I didn't find something similar to build_overrides/ so I placed the condition > here (temporarily, to be removed before landing) I believe I've told you before, it's https://chromium.googlesource.com/external/webrtc/+/master/webrtc/supplement.... https://codereview.webrtc.org/1601813005/diff/40001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1601813005/diff/40001/webrtc/build/common.gypi#... webrtc/build/common.gypi:147: # 'use_h264%': 0, # TODO(hbos): enc/dec in follow up CL(s). You don't need to define this here if you define it in both the if and the else clause below. Or is that why you've commented it? Either way this file needs to be updated. https://codereview.webrtc.org/1601813005/diff/40001/webrtc/build/webrtc.gni File webrtc/build/webrtc.gni (right): https://codereview.webrtc.org/1601813005/diff/40001/webrtc/build/webrtc.gni#n... webrtc/build/webrtc.gni:101: use_h264 = false # TODO(hbos): enc/dec in follow up CL(s). Variables that are not used in Chromium shall be prefixed with rtc_
Patchset #3 (id:60001) has been deleted
PTAL kjellander (Will remove overriding defaults before landing) https://codereview.webrtc.org/1601813005/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1601813005/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:152: }], On 2016/01/20 19:28:45, kjellander (webrtc) wrote: > On 2016/01/19 11:01:27, hbos wrote: > > Know where to do "override defaults" similar to build_overrides/webrtc.gni? > > There's gyp_webrtc of course but I wasn't sure about the "if OS != ios" then > and > > I didn't find something similar to build_overrides/ so I placed the condition > > here (temporarily, to be removed before landing) > > I believe I've told you before, it's > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/supplement.... Thanks. Yeah I believe you did, but I couldn't find it afterwards. https://codereview.webrtc.org/1601813005/diff/40001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1601813005/diff/40001/webrtc/build/common.gypi#... webrtc/build/common.gypi:147: # 'use_h264%': 0, # TODO(hbos): enc/dec in follow up CL(s). On 2016/01/20 19:28:45, kjellander (webrtc) wrote: > You don't need to define this here if you define it in both the if and the else > clause below. > Or is that why you've commented it? Either way this file needs to be updated. (Since I couldn't remember supplement.gypi I did the if-else here, the commented out line being how it should have been without the if-else) https://codereview.webrtc.org/1601813005/diff/40001/webrtc/build/webrtc.gni File webrtc/build/webrtc.gni (right): https://codereview.webrtc.org/1601813005/diff/40001/webrtc/build/webrtc.gni#n... webrtc/build/webrtc.gni:101: use_h264 = false # TODO(hbos): enc/dec in follow up CL(s). On 2016/01/20 19:28:45, kjellander (webrtc) wrote: > Variables that are not used in Chromium shall be prefixed with rtc_ Done.
lgtm assuming you remove the mentioned defaults in the final PS.
Description was changed from ========== use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg. Attempting to build third_party/openh264 and third_party/ffmpeg (in WebRTC) by switching default to true, will be back to false before landing. BUG=468365 ========== to ========== rtc_use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg, false by default but can be overridden in supplement.gypi and build_overrides/webrtc.gni. BUG=468365 NOTRY=True ==========
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1601813005/#ps100001 (title: "rtc_use_h264 = false (not overriding defaults)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1601813005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601813005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2986)
hbos@webrtc.org changed reviewers: + stefan@webrtc.org - kjellander@chromium.org
Please take a look stefan, OWNER needed for h264.gypi
On 2016/01/21 10:46:07, hbos wrote: > Please take a look stefan, OWNER needed for h264.gypi 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/1601813005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601813005/100001
Message was sent while issue was closed.
Description was changed from ========== rtc_use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg, false by default but can be overridden in supplement.gypi and build_overrides/webrtc.gni. BUG=468365 NOTRY=True ========== to ========== rtc_use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg, false by default but can be overridden in supplement.gypi and build_overrides/webrtc.gni. BUG=468365 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== rtc_use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg, false by default but can be overridden in supplement.gypi and build_overrides/webrtc.gni. BUG=468365 NOTRY=True ========== to ========== rtc_use_h264 flag (replacing use_third_party_h264 flag) for building OpenH264/FFmpeg, false by default but can be overridden in supplement.gypi and build_overrides/webrtc.gni. BUG=468365 NOTRY=True Committed: https://crrev.com/902c03e724f2f732e67a631f8bd5ba5cac3276a6 Cr-Commit-Position: refs/heads/master@{#11333} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/902c03e724f2f732e67a631f8bd5ba5cac3276a6 Cr-Commit-Position: refs/heads/master@{#11333} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
