Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(94)

Issue 1718883002: Enable H.264 video WebRTC behind run-time flag and add WebRtcBrowserTest for H.264 (Closed)

Created:
4 years, 10 months ago by hbos_chromium
Modified:
4 years, 9 months ago
CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, phoglund+watch_chromium.org, posciak+watch_chromium.org, tnakamura+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Re-upload of https://codereview.chromium.org/1673183002/. Enable H.264 video WebRTC behind run-time flag and add WebRtcBrowserTest for H.264. The compile flag |rtc_use_h264| defaults to |proprietary_codecs| which is used on some Chromium bots and in the official Chrome build. The new run-time flag |kWebRtcH264WithOpenH264FFmpeg| determines if the |rtc_use_h264| encoder/decoder will be used at runtime. The run-time flag is off by default. This CL: - Adds run-time flag and a new build target for it. It is placed in content/public/common/ because it can't/shouldn't be placed in the webrtc repo and peer_connection_dependency_factory.cc lives in content/renderer/media/webrtc/. - Moves renderer_features and its generated header file target from content/renderer/ to content/public/common/ to not break include rules and renames it to common_features. - Based on the run-time flag, enables or disables H.264 in peer_connection_dependency_factory.cc. - If flag is on, runs a new H.264 browser test. BUG=chromium:500605, chromium:468365 Committed: https://crrev.com/b43b719e4e54f569eeefd2beb85517d75bf6cbfd Cr-Commit-Position: refs/heads/master@{#377556}

Patch Set 1 #

Patch Set 2 : browser_tests and content_renderer depending on common_features directly (prev indirect dep) #

Patch Set 3 : ('dependencies' instead of 'deps') #

Total comments: 2

Patch Set 4 : Renamed base::Feature name to match follow-up CL study name #

Patch Set 5 : content_common.gypi depending on common_features #

Total comments: 4

Patch Set 6 : Addressed brettw comments (remove colon and gen header from list) #

Total comments: 7

Patch Set 7 : Addressed brettw comments (adding to deps instead of removing) #

Patch Set 8 : Addressed brettw comments (removed LOG) #

Patch Set 9 : Rebase with master #

Patch Set 10 : common_features.h -> features.h + GN common_features -> features rename #

Patch Set 11 : (Updated comment) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -31 lines) Patch
M chrome/browser/media/chrome_webrtc_browsertest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +18 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -1 line 0 comments Download
M content/content.gyp View 1 2 3 4 5 6 7 8 9 10 5 chunks +18 lines, -5 lines 0 comments Download
M content/content_common.gypi View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M content/content_renderer.gypi View 1 2 chunks +2 lines, -1 line 0 comments Download
M content/public/common/BUILD.gn View 1 2 3 4 5 6 7 8 9 2 chunks +18 lines, -0 lines 0 comments Download
A content/public/common/feature_h264_with_openh264_ffmpeg.h View 1 2 3 4 5 6 7 8 9 1 chunk +22 lines, -0 lines 0 comments Download
A content/public/common/feature_h264_with_openh264_ffmpeg.cc View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download
M content/public/renderer/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -1 line 0 comments Download
M content/renderer/BUILD.gn View 1 2 3 4 5 6 7 8 9 4 chunks +2 lines, -9 lines 0 comments Download
M content/renderer/media/webrtc/peer_connection_dependency_factory.cc View 1 2 3 4 5 6 7 8 9 3 chunks +11 lines, -14 lines 0 comments Download

Messages

Total messages: 40 (12 generated)
hbos_chromium
Please take a look, kjellander
4 years, 10 months ago (2016-02-22 11:56:06 UTC) #4
jochen (gone - plz use gerrit)
please only use TBR for one of the documented purposes: https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Reviewed-TBR-
4 years, 10 months ago (2016-02-22 14:16:34 UTC) #5
kjellander_chromium
Oh dear, this is complicated. As Jochen said; remove the TBR= line from the description. ...
4 years, 10 months ago (2016-02-22 14:45:54 UTC) #6
hbos_chromium
On 2016/02/22 14:45:54, kjellander (chromium) wrote: > Oh dear, this is complicated. As Jochen said; ...
4 years, 10 months ago (2016-02-22 15:18:29 UTC) #9
hbos_chromium
PTAL ncarter, jochen, rkaplow, tommi. This is a re-upload of https://codereview.chromium.org/1673183002/ with some minor changes ...
4 years, 10 months ago (2016-02-22 15:43:40 UTC) #10
kjellander_chromium
On 2016/02/22 15:18:29, hbos_chromium wrote: > On 2016/02/22 14:45:54, kjellander (chromium) wrote: > > Oh ...
4 years, 10 months ago (2016-02-22 15:45:34 UTC) #11
hbos_chromium
Oops forgot to +brettw. Please take a look.
4 years, 10 months ago (2016-02-22 15:47:23 UTC) #13
brettw
https://codereview.chromium.org/1718883002/diff/80001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/1718883002/diff/80001/content/content.gyp#newcode54 content/content.gyp:54: ':common_features', Delete the colon here. I don't actually know ...
4 years, 10 months ago (2016-02-22 18:04:03 UTC) #14
hbos_chromium
PTAL everyone! https://codereview.chromium.org/1718883002/diff/80001/content/content.gyp File content/content.gyp (right): https://codereview.chromium.org/1718883002/diff/80001/content/content.gyp#newcode54 content/content.gyp:54: ':common_features', On 2016/02/22 18:04:03, brettw wrote: > ...
4 years, 10 months ago (2016-02-22 20:32:40 UTC) #15
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1718883002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718883002/100001
4 years, 10 months ago (2016-02-22 21:38:09 UTC) #17
brettw
lgtm https://codereview.chromium.org/1718883002/diff/100001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1718883002/diff/100001/chrome/test/BUILD.gn#newcode1100 chrome/test/BUILD.gn:1100: "//content/public/common:common_features", I don't think there's any downside to ...
4 years, 10 months ago (2016-02-22 22:50:21 UTC) #18
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-22 23:30:13 UTC) #20
brettw
Wait, not lgtm. There is already a chrome common features target just called "features" in ...
4 years, 10 months ago (2016-02-23 00:28:31 UTC) #21
hbos_chromium
On 2016/02/23 00:28:31, brettw wrote: > Wait, not lgtm. > > There is already a ...
4 years, 10 months ago (2016-02-23 08:12:50 UTC) #22
hbos_chromium
PTAL everyone (brettw, jochen, ncarter, kjellander, tommi, rkaplow). Want this in before branch cut. https://codereview.chromium.org/1718883002/diff/100001/chrome/test/BUILD.gn ...
4 years, 10 months ago (2016-02-23 08:21:04 UTC) #23
hbos_chromium
:) https://codereview.chromium.org/1718883002/diff/100001/content/renderer/media/webrtc/peer_connection_dependency_factory.cc File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1718883002/diff/100001/content/renderer/media/webrtc/peer_connection_dependency_factory.cc#newcode269 content/renderer/media/webrtc/peer_connection_dependency_factory.cc:269: LOG(WARNING) << "WebRTC H.264 with OpenH264/FFmpeg ENABLED."; On ...
4 years, 10 months ago (2016-02-23 10:40:56 UTC) #24
kjellander_chromium
lgtm
4 years, 10 months ago (2016-02-23 10:47:54 UTC) #25
hbos_chromium
brettw, did I understand correctly that your comment #21 does not apply because of a ...
4 years, 10 months ago (2016-02-24 08:24:41 UTC) #26
hbos_chromium
PTAL jochen, brettw and tommi. (ncarter/rkaplow are OOO) Note the flaky compile problem described in ...
4 years, 10 months ago (2016-02-24 09:30:20 UTC) #27
rkaplow
lgtm feature logic lgtm
4 years, 10 months ago (2016-02-24 16:12:23 UTC) #28
brettw
On 2016/02/24 08:24:41, hbos_chromium wrote: > brettw, did I understand correctly that your comment #21 ...
4 years, 10 months ago (2016-02-24 18:16:27 UTC) #29
hbos_chromium
PTAL jochen, tommi. The fact that all trybots still compiled on the first attempt after ...
4 years, 10 months ago (2016-02-25 08:12:40 UTC) #30
jochen (gone - plz use gerrit)
lgtm
4 years, 10 months ago (2016-02-25 09:51:25 UTC) #31
tommi (sloooow) - chröme
lgtm
4 years, 10 months ago (2016-02-25 10:00:41 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1718883002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718883002/200001
4 years, 10 months ago (2016-02-25 10:12:19 UTC) #35
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 9 months ago (2016-02-25 11:29:23 UTC) #37
commit-bot: I haz the power
Patchset 11 (id:??) landed as https://crrev.com/b43b719e4e54f569eeefd2beb85517d75bf6cbfd Cr-Commit-Position: refs/heads/master@{#377556}
4 years, 9 months ago (2016-02-25 11:31:01 UTC) #39
hbos_chromium
4 years, 9 months ago (2016-02-25 15:03:03 UTC) #40
Message was sent while issue was closed.
On 2016/02/25 11:31:01, commit-bot: I haz the power wrote:
> Patchset 11 (id:??) landed as
> https://crrev.com/b43b719e4e54f569eeefd2beb85517d75bf6cbfd
> Cr-Commit-Position: refs/heads/master@{#377556}

Looks like it landed cleanly, waterfall remained green! :)

Though I just noticed there is a content/public/common/content_features.[cc/h]
(for base::Features, not generated BUILDFLAG "features"), so I can see if I can
move kWebRtcH264WithOpenH264FFmpeg (of feature_h264_with_openh264_ffmpeg.[cc/h])
into it in a follow-up CL.

Powered by Google App Engine
This is Rietveld 408576698