Enable H.264 video WebRTC behind run-time flag and add WebRtcBrowserTest for H.264

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/82140fb2bbb534004ea49baa1f31aa8d9646775b
Cr-Commit-Position: refs/heads/master@{#376451}

[hbos_chromium, 2016-02-08 13:40:24]

Description was changed from

==========
Enable H.264 video in WebRTC, and add WebRtcBrowserTest test for H264.

BUG=
==========

to

==========
Enable H.264 video in WebRTC, and add WebRtcBrowserTest test for H264.

|rtc_use_h264| defaults to |proprietary_codecs| which is used on some Chromium
bots and in the official Chrome build.
  This CL removes the line "webrtc::DisableRtcUseH264();" meaning if this CL
lands, H.264 launches as the default in Chrome.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-08 13:41:16]

Description was changed from

==========
Enable H.264 video in WebRTC, and add WebRtcBrowserTest test for H264.

|rtc_use_h264| defaults to |proprietary_codecs| which is used on some Chromium
bots and in the official Chrome build.
  This CL removes the line "webrtc::DisableRtcUseH264();" meaning if this CL
lands, H.264 launches as the default in Chrome.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video in WebRTC, and add WebRtcBrowserTest
test for H264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-08 15:22:53]

hbos@chromium.org changed reviewers:
+ tommi@chromium.org

[hbos_chromium, 2016-02-08 15:22:54]

Please take a look, tommi. Will not land until we have green light to enable
this by default in Chrome (otherwise I'll have to add a runtime flag that can
disable it), but review process can start now.

https://codereview.chromium.org/1673183002/diff/1/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/1673183002/diff/1/chrome/browser/media/DEPS#n...
chrome/browser/media/DEPS:2: "+content/renderer/renderer_features.h",  # For
BUILDFLAG(RTC_USE_H264).
|rtc_use_h264| is a build flag from the webrtc repo. BUILDFLAG(RTC_USE_H264)
checks if the flag is set or not (renderer_features.h is auto-generated and
defines it). This would probably typically be placed where the flag is defined,
but because this does not exist in the webrtc repo I defined it closest to where
BUILDFLAG(RTC_USE_H264) is used, which was peer_connection_dependency_factory.cc
in content/renderer/...
  Now that we also use it in unittests, DEPS needed to be updated. This is not
to say that chrome/browser/media now depends on other content/renderer stuff.

[hbos_chromium, 2016-02-08 15:26:19]

hbos@chromium.org changed reviewers:
+ jochen@chromium.org

[hbos_chromium, 2016-02-08 15:26:19]

+jochen for the added DEPS content/renderer/renderer_features.h (remember
https://codereview.chromium.org/1641163002/?).

[hbos_chromium, 2016-02-08 15:29:03]

hbos@chromium.org changed reviewers:
+ phoglund@chromium.org

[hbos_chromium, 2016-02-08 15:29:03]

+phoglund for being closest OWNER of chrome_webrtc_browsertest.cc.

[phoglund_chromium, 2016-02-08 16:12:44]

lgtm for browser test

[tommi (sloooow) - chröme, 2016-02-08 18:45:56]

lgtm

[hbos_chromium, 2016-02-09 08:32:12]

hbos@chromium.org changed reviewers:
+ avi@chromium.org

[hbos_chromium, 2016-02-09 08:32:13]

+avi for DEPS added (jochen being OOO). Can you take a look?

[hbos_chromium, 2016-02-09 08:35:01]

Description was changed from

==========
Enable H.264 video in WebRTC, and add WebRtcBrowserTest
test for H264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video in WebRTC, and add WebRtcBrowserTest
test for H264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

Have three LGTMs in the Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/46qrH_wTx-0
Launch-related reviews in progress.

BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-09 08:37:43]

Description was changed from

==========
Enable H.264 video in WebRTC, and add WebRtcBrowserTest
test for H264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

Have three LGTMs in the Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/46qrH_wTx-0
Launch-related reviews in progress.

BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video in WebRTC in Chrome, and add
WebRtcBrowserTest test for H264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

Have three LGTMs in the Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/46qrH_wTx-0
Launch-related reviews in progress.

BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-09 08:57:51]

Description was changed from

==========
Enable H.264 video in WebRTC in Chrome, and add
WebRtcBrowserTest test for H264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

Have three LGTMs in the Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/46qrH_wTx-0
Launch-related reviews in progress.

BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video in WebRTC in Chrome and add
WebRtcBrowserTest test for H.264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

Have three LGTMs in the Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/46qrH_wTx-0
Launch-related reviews in progress.

BUG=chromium:500605, chromium:468365
==========

[Avi (use Gerrit), 2016-02-09 16:45:03]

If your build flags are not public content/ API, you need to rethink your build
flags.

https://codereview.chromium.org/1673183002/diff/1/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/1673183002/diff/1/chrome/browser/media/DEPS#n...
chrome/browser/media/DEPS:2: "+content/renderer/renderer_features.h",  # For
BUILDFLAG(RTC_USE_H264).
No.

content/public is the public API for content. You may not include any file
outside of it.

[hbos_chromium, 2016-02-10 09:48:08]

PTAL avi.

https://codereview.chromium.org/1673183002/diff/1/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/1673183002/diff/1/chrome/browser/media/DEPS#n...
chrome/browser/media/DEPS:2: "+content/renderer/renderer_features.h",  # For
BUILDFLAG(RTC_USE_H264).
On 2016/02/09 16:45:03, Avi wrote:
> No.
> 
> content/public is the public API for content. You may not include any file
> outside of it.

OK. I made renderer_features.h content/public/ instead. The only "feature" it
defines is the RTC_USE_H264 buildflag, no need for that not to be public.

[Avi (use Gerrit), 2016-02-10 17:50:21]

avi@chromium.org changed reviewers:
+ jam@chromium.org, nick@chromium.org

[Avi (use Gerrit), 2016-02-10 17:50:21]

+jam, nick

This is a bit beyond my knowledge of the build system. Right now there's a
generated renderer_features.h file that is used to configure media, but they
need it in chrome too. What are the rules for generating content api files?

[ncarter (slow), 2016-02-10 18:35:22]

lgtm

[jam, 2016-02-10 18:35:56]

On 2016/02/10 17:50:21, Avi wrote:
> +jam, nick
> 
> This is a bit beyond my knowledge of the build system. Right now there's a
> generated renderer_features.h file that is used to configure media, but they
> need it in chrome too. What are the rules for generating content api files?

why isn't this in third_party/webrtc/build/ build files?

it does seem a bit confusing that we have generated files that are included from
"content/public"

[hbos_chromium, 2016-02-10 20:39:38]

On 2016/02/10 18:35:56, jam wrote:
> On 2016/02/10 17:50:21, Avi wrote:
> > +jam, nick
> > 
> > This is a bit beyond my knowledge of the build system. Right now there's a
> > generated renderer_features.h file that is used to configure media, but they
> > need it in chrome too. What are the rules for generating content api files?
> 
> why isn't this in third_party/webrtc/build/ build files?
> 
> it does seem a bit confusing that we have generated files that are included
from
> "content/public"

The generated file defines a macro for if |rtc_use_h264|
is used or not. It would make the most sense to have this
file generated in the same place where the build flag is
defined, in third_party/webrtc/build/ as you pointed out.
The reason its not there is that the tool that generates
the header does not exist in the webrtc repo. Instead, I
chose the location where we do webrtc initialization
logic and where initially needed this macro (in
https://codereview.chromium.org/1641163002/, +CC brettw).

@phoglund, +CC kjellander:
  We could discuss making the tool available in webrtc
(the webrtc repo has symlinks to some chromium tools
already) and generating a file there.

Whether we do that or not I don't think it should block
this CL from landing. Moving the macro is a simple
follow-up CL or two that changes the include and some
build files.

[kjellander_chromium, 2016-02-10 20:43:52]

On 2016/02/10 20:39:38, hbos_chromium wrote:
> On 2016/02/10 18:35:56, jam wrote:
> > On 2016/02/10 17:50:21, Avi wrote:
> > > +jam, nick
> > > 
> > > This is a bit beyond my knowledge of the build system. Right now there's a
> > > generated renderer_features.h file that is used to configure media, but
they
> > > need it in chrome too. What are the rules for generating content api
files?
> > 
> > why isn't this in third_party/webrtc/build/ build files?
> > 
> > it does seem a bit confusing that we have generated files that are included
> from
> > "content/public"
> 
> The generated file defines a macro for if |rtc_use_h264|
> is used or not. It would make the most sense to have this
> file generated in the same place where the build flag is
> defined, in third_party/webrtc/build/ as you pointed out.
> The reason its not there is that the tool that generates
> the header does not exist in the webrtc repo. Instead, I
> chose the location where we do webrtc initialization
> logic and where initially needed this macro (in
> https://codereview.chromium.org/1641163002/, +CC brettw).
> 
> @phoglund, +CC kjellander:
>   We could discuss making the tool available in webrtc
> (the webrtc repo has symlinks to some chromium tools
> already) and generating a file there.

I prefer to not create more symlinks like that than absolutely necessary, as
it'll make it harder for us to move away from this non-supported way of reusing
the Chromium toolchain (and add more work to my old task for fixing this:
https://bugs.chromium.org/p/webrtc/issues/detail?id=5006).


> Whether we do that or not I don't think it should block
> this CL from landing. Moving the macro is a simple
> follow-up CL or two that changes the include and some
> build files.

[hbos_chromium, 2016-02-10 20:57:10]

On 2016/02/10 20:43:52, kjellander (chromium) wrote:
> On 2016/02/10 20:39:38, hbos_chromium wrote:
> > On 2016/02/10 18:35:56, jam wrote:
> > > On 2016/02/10 17:50:21, Avi wrote:
> > > > +jam, nick
> > > > 
> > > > This is a bit beyond my knowledge of the build system. Right now there's
a
> > > > generated renderer_features.h file that is used to configure media, but
> they
> > > > need it in chrome too. What are the rules for generating content api
> files?
> > > 
> > > why isn't this in third_party/webrtc/build/ build files?
> > > 
> > > it does seem a bit confusing that we have generated files that are
included
> > from
> > > "content/public"
> > 
> > The generated file defines a macro for if |rtc_use_h264|
> > is used or not. It would make the most sense to have this
> > file generated in the same place where the build flag is
> > defined, in third_party/webrtc/build/ as you pointed out.
> > The reason its not there is that the tool that generates
> > the header does not exist in the webrtc repo. Instead, I
> > chose the location where we do webrtc initialization
> > logic and where initially needed this macro (in
> > https://codereview.chromium.org/1641163002/, +CC brettw).
> > 
> > @phoglund, +CC kjellander:
> >   We could discuss making the tool available in webrtc
> > (the webrtc repo has symlinks to some chromium tools
> > already) and generating a file there.
> 
> I prefer to not create more symlinks like that than absolutely necessary, as
> it'll make it harder for us to move away from this non-supported way of
reusing
> the Chromium toolchain (and add more work to my old task for fixing this:
> https://bugs.chromium.org/p/webrtc/issues/detail?id=5006).
> 
> 
> > Whether we do that or not I don't think it should block
> > this CL from landing. Moving the macro is a simple
> > follow-up CL or two that changes the include and some
> > build files.

In that case, are you guys OK with me landing this (I think I have the LGTMs I
need?) or do you need me to move the generated file elsewhere?

[hbos_chromium, 2016-02-10 20:57:53]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-10 21:00:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673183002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673183002/20001

[commit-bot: I haz the power, 2016-02-10 21:19:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-10 21:19:15]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2016-02-11 12:19:31]

Description was changed from

==========
Enable H.264 video in WebRTC in Chrome and add
WebRtcBrowserTest test for H.264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

Have three LGTMs in the Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/46qrH_wTx-0
Launch-related reviews in progress.

BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video in WebRTC in Chrome and add
WebRtcBrowserTest test for H.264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

Have three LGTMs in the Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/46qrH_wTx-0
Launch-related reviews in progress.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-11 12:21:26]

Should put it behind a Finch flag before landing though. Will update CL.
Launch review stuff is till in progress.

[jam, 2016-02-11 21:39:57]

On 2016/02/10 20:57:10, hbos_chromium wrote:
> On 2016/02/10 20:43:52, kjellander (chromium) wrote:
> > On 2016/02/10 20:39:38, hbos_chromium wrote:
> > > On 2016/02/10 18:35:56, jam wrote:
> > > > On 2016/02/10 17:50:21, Avi wrote:
> > > > > +jam, nick
> > > > > 
> > > > > This is a bit beyond my knowledge of the build system. Right now
there's
> a
> > > > > generated renderer_features.h file that is used to configure media,
but
> > they
> > > > > need it in chrome too. What are the rules for generating content api
> > files?
> > > > 
> > > > why isn't this in third_party/webrtc/build/ build files?
> > > > 
> > > > it does seem a bit confusing that we have generated files that are
> included
> > > from
> > > > "content/public"
> > > 
> > > The generated file defines a macro for if |rtc_use_h264|
> > > is used or not. It would make the most sense to have this
> > > file generated in the same place where the build flag is
> > > defined, in third_party/webrtc/build/ as you pointed out.
> > > The reason its not there is that the tool that generates
> > > the header does not exist in the webrtc repo. Instead, I
> > > chose the location where we do webrtc initialization
> > > logic and where initially needed this macro (in
> > > https://codereview.chromium.org/1641163002/, +CC brettw).
> > > 
> > > @phoglund, +CC kjellander:
> > >   We could discuss making the tool available in webrtc
> > > (the webrtc repo has symlinks to some chromium tools
> > > already) and generating a file there.
> > 
> > I prefer to not create more symlinks like that than absolutely necessary, as
> > it'll make it harder for us to move away from this non-supported way of
> reusing
> > the Chromium toolchain (and add more work to my old task for fixing this:
> > https://bugs.chromium.org/p/webrtc/issues/detail?id=5006).
> > 
> > 
> > > Whether we do that or not I don't think it should block
> > > this CL from landing. Moving the macro is a simple
> > > follow-up CL or two that changes the include and some
> > > build files.
> 
> In that case, are you guys OK with me landing this (I think I have the LGTMs I
> need?) or do you need me to move the generated file elsewhere?

I don't feel strongly, so if the other reviewers are ok i'm fine with that

[hbos_chromium, 2016-02-12 14:19:34]

Patchset #4 (id:60001) has been deleted

[hbos_chromium, 2016-02-15 09:51:29]

Patchset #3 (id:40001) has been deleted

[hbos_chromium, 2016-02-15 09:55:32]

Patchset #3 (id:80001) has been deleted

[hbos_chromium, 2016-02-15 12:19:17]

Patchset #4 (id:120001) has been deleted

[hbos_chromium, 2016-02-15 12:24:44]

Description was changed from

==========
Enable H.264 video in WebRTC in Chrome and add
WebRtcBrowserTest test for H.264.

|rtc_use_h264| defaults to |proprietary_codecs| which is
used on some Chromium bots and in the official Chrome
build. This CL removes the line
"webrtc::DisableRtcUseH264()" meaning if this CL lands,
H.264 launches as the default in Chrome.

Have three LGTMs in the Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/46qrH_wTx-0
Launch-related reviews in progress.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-15 14:57:53]

Patchset #5 (id:160001) has been deleted

[hbos_chromium, 2016-02-15 15:32:47]

Patchset #4 (id:140001) has been deleted

[hbos_chromium, 2016-02-15 16:11:16]

Description was changed from

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

This CL:
- Adds Finch flag and a new build target for it. It is
placed in content/public/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- Based on the Finch flag, enabled or disables H.264 in
peer_connection_dependency_factory.
- If flag is on, runs a new H.264 browser test.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-15 16:11:46]

Description was changed from

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

This CL:
- Adds Finch flag and a new build target for it. It is
placed in content/public/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- Based on the Finch flag, enabled or disables H.264 in
peer_connection_dependency_factory.
- If flag is on, runs a new H.264 browser test.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

This CL:
- Adds Finch flag and a new build target for it. It is
placed in content/public/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- Based on the Finch flag, enables or disables H.264 in
peer_connection_dependency_factory.
- If flag is on, runs a new H.264 browser test.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-15 16:13:43]

Description was changed from

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

This CL:
- Adds Finch flag and a new build target for it. It is
placed in content/public/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- Based on the Finch flag, enables or disables H.264 in
peer_connection_dependency_factory.
- If flag is on, runs a new H.264 browser test.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

This CL:
- Adds Finch flag and a new build target for it. It is
placed in content/public/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- Based on the Finch flag, enables or disables H.264 in
peer_connection_dependency_factory.cc.
- If flag is on, runs a new H.264 browser test.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-15 16:14:10]

Description was changed from

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

This CL:
- Adds Finch flag and a new build target for it. It is
placed in content/public/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- Based on the Finch flag, enables or disables H.264 in
peer_connection_dependency_factory.cc.
- If flag is on, runs a new H.264 browser test.

COMMIT=False
BUG=chromium:500605, chromium:468365
==========

to

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

This CL:
- Adds Finch flag and a new build target for it. It is
placed in content/public/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- Based on the Finch 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
==========

[hbos_chromium, 2016-02-15 16:17:46]

CL updated to include Finch flag. Please take a look jochen, and please take
another look phoglund and tommi.

[tommi (sloooow) - chröme, 2016-02-15 16:24:29]

lgtm

[hbos_chromium, 2016-02-16 08:51:55]

PT(A)L jochen, phoglund.

[phoglund_chromium, 2016-02-17 09:31:17]

lgtm, one nit

https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/chrome_webrtc_browsertest.cc (right):

https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/c...
chrome/browser/media/chrome_webrtc_browsertest.cc:89: // Only run test if the
Finch feature corresponding to |rtc_use_h264| is on.
Nit: I prefer

if (finch not enabled) {
  log warning
  return
}

run test

[hbos_chromium, 2016-02-17 10:28:24]

Please take a look jochen.

https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/chrome_webrtc_browsertest.cc (right):

https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/c...
chrome/browser/media/chrome_webrtc_browsertest.cc:89: // Only run test if the
Finch feature corresponding to |rtc_use_h264| is on.
On 2016/02/17 09:31:17, phoglund_chrome wrote:
> Nit: I prefer
> 
> if (finch not enabled) {
>   log warning
>   return
> }
> 
> run test

Done.

[jochen (gone - plz use gerrit), 2016-02-17 10:55:41]

https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/D...
chrome/browser/media/DEPS:2: "+content/public/renderer/renderer_features.h",  #
For BUILDFLAG(RTC_USE_H264)
browser can't depend on renderer.

https://codereview.chromium.org/1673183002/diff/180001/content/public/rendere...
File content/public/renderer/media/webrtc/finch_h264_with_openh264_ffmpeg.cc
(right):

https://codereview.chromium.org/1673183002/diff/180001/content/public/rendere...
content/public/renderer/media/webrtc/finch_h264_with_openh264_ffmpeg.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
it's 2016

https://codereview.chromium.org/1673183002/diff/180001/content/renderer/media...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/1673183002/diff/180001/content/renderer/media...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:269:
LOG(WARNING) << "WebRTC H.264 with OpenH264/FFmpeg ENABLED.";
please use DVLOG()

[hbos_chromium, 2016-02-17 12:27:42]

Question for jochen / anyone who knows what is a more appropriate place to put
the flags.

https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/D...
chrome/browser/media/DEPS:2: "+content/public/renderer/renderer_features.h",  #
For BUILDFLAG(RTC_USE_H264)
On 2016/02/17 10:55:40, jochen (OOO) wrote:
> browser can't depend on renderer.

It's only depending on the RTC_USE_H264 buildflag and finch flag (nothing that
is otherwise "renderer").
- Do you suggest moving these? To where? Can I place it in content/public/?

(I defined the flags to be close to peer_connection_dependency_factory.cc which
uses them and initializes webrtc stuff, so it made sense to put the flags for if
they should be enabled there. They're not placed in the webrtc repo because that
repo does not have access to the deps the flag needs. See previous discussion
about the buildflag. Finch flag has not been discussed previously but I placed
it where I did for the same reasons.)

[jochen (gone - plz use gerrit), 2016-02-17 12:36:26]

On 2016/02/17 at 12:27:42, hbos wrote:
> Question for jochen / anyone who knows what is a more appropriate place to put
the flags.
> 
>
https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/DEPS
> File chrome/browser/media/DEPS (right):
> 
>
https://codereview.chromium.org/1673183002/diff/180001/chrome/browser/media/D...
> chrome/browser/media/DEPS:2: "+content/public/renderer/renderer_features.h", 
# For BUILDFLAG(RTC_USE_H264)
> On 2016/02/17 10:55:40, jochen (OOO) wrote:
> > browser can't depend on renderer.
> 
> It's only depending on the RTC_USE_H264 buildflag and finch flag (nothing that
is otherwise "renderer").
> - Do you suggest moving these? To where? Can I place it in content/public/?
> 
> (I defined the flags to be close to peer_connection_dependency_factory.cc
which uses them and initializes webrtc stuff, so it made sense to put the flags
for if they should be enabled there. They're not placed in the webrtc repo
because that repo does not have access to the deps the flag needs. See previous
discussion about the buildflag. Finch flag has not been discussed previously but
I placed it where I did for the same reasons.)

files that are included from both browser and renderer subdirectories should go
into common/

[hbos_chromium, 2016-02-18 10:58:15]

Patchset #6 (id:220001) has been deleted

[hbos_chromium, 2016-02-18 11:07:20]

PTAL jochen.
I moved the stuff from content/public/renderer/ to content/public/common/ (with
appropriate renames).

(Note: I attempted to make what the finch target
(finch_h264_with_openh264_ffmpeg) builds part of building content/public/common/
as to not have a separate build target for just two files but this lead to
various unexpected build problems in GN that I couldn't solve with half a day's
worth of work. So keeping it a separate target, which might be good anyway, not
having to depend on all of common for a single flag. Also, this target will go
away when the finch flag goes away so that means less risk to leave unnecessary
dependencies behind afterwards.)

[hbos_chromium, 2016-02-18 13:03:32]

hbos@chromium.org changed reviewers:
+ rkaplow@chromium.org

[hbos_chromium, 2016-02-18 13:03:33]

+rkaplow for finch_h264_with_openh264_ffmpeg.[h/cc].

[jochen (gone - plz use gerrit), 2016-02-18 15:18:09]

lgtm

https://codereview.chromium.org/1673183002/diff/260001/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/1673183002/diff/260001/chrome/browser/media/D...
chrome/browser/media/DEPS:2: "+content/public/common",
not needed (already allowed by chrome/DEPS)

[rkaplow, 2016-02-18 16:24:33]

lgtm

logic lg, but finch is a google-internal codename and ideally shouldn't be used
externally - would prefer if you rename the files and all part of the code
referencing finch.

[hbos_chromium, 2016-02-19 09:13:34]

Description was changed from

==========
Enable H.264 video WebRTC behind finch 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 Finch flag
|kWebRtcH264WithOpenH264FFmpeg| determines if the
|rtc_use_h264| encoder/decoder will be used at runtime.

The finch flag is off by default.

This CL:
- Adds Finch flag and a new build target for it. It is
placed in content/public/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- Based on the Finch 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
==========

to

==========
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/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- 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
==========

[hbos_chromium, 2016-02-19 09:15:43]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2016-02-19 09:15:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
phoglund@chromium.org, tommi@chromium.org, jochen@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1673183002/#ps280001
(title: "Renamed stuff, removing references to 'finch'")

[commit-bot: I haz the power, 2016-02-19 09:15:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673183002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673183002/280001

[hbos_chromium, 2016-02-19 09:16:18]

OK, rename before landing.

https://codereview.chromium.org/1673183002/diff/260001/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/1673183002/diff/260001/chrome/browser/media/D...
chrome/browser/media/DEPS:2: "+content/public/common",
On 2016/02/18 15:18:09, jochen wrote:
> not needed (already allowed by chrome/DEPS)

Done.

[commit-bot: I haz the power, 2016-02-19 09:40:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 09:40:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hbos_chromium, 2016-02-19 09:41:36]

The CQ bit was checked by hbos@chromium.org

[commit-bot: I haz the power, 2016-02-19 09:43:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673183002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673183002/280001

[commit-bot: I haz the power, 2016-02-19 13:06:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 13:07:01]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hbos_chromium, 2016-02-19 13:12:16]

The CQ bit was checked by hbos@chromium.org

[commit-bot: I haz the power, 2016-02-19 13:13:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673183002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673183002/280001

[commit-bot: I haz the power, 2016-02-19 16:19:41]

Description was changed from

==========
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/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- 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
==========

to

==========
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/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- 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
==========

[commit-bot: I haz the power, 2016-02-19 16:19:43]

Committed patchset #8 (id:280001)

[commit-bot: I haz the power, 2016-02-19 16:20:34]

Description was changed from

==========
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/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- 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
==========

to

==========
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/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- 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/82140fb2bbb534004ea49baa1f31aa8d9646775b
Cr-Commit-Position: refs/heads/master@{#376451}
==========

[commit-bot: I haz the power, 2016-02-19 16:20:36]

Patchset 8 (id:??) landed as
https://crrev.com/82140fb2bbb534004ea49baa1f31aa8d9646775b
Cr-Commit-Position: refs/heads/master@{#376451}

[hbos_chromium, 2016-02-19 16:33:44]

Description was changed from

==========
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/renderer/media/webrtc/ 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/renderer/
to not break include rules.
- 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/82140fb2bbb534004ea49baa1f31aa8d9646775b
Cr-Commit-Position: refs/heads/master@{#376451}
==========

to

==========
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/82140fb2bbb534004ea49baa1f31aa8d9646775b
Cr-Commit-Position: refs/heads/master@{#376451}
==========

[Guido Urdaneta, 2016-02-19 16:51:39]

A revert of this CL (patchset #8 id:280001) has been created in
https://codereview.chromium.org/1715753002/ by guidou@chromium.org.

The reason for reverting is: Caused compile error on WebRTC Win Builder Chromium
bot:

https://build.chromium.org/p/chromium.webrtc/builders/Win%20Builder/builds/34506

FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma/gomacc
"E:\b\depot_tools\win_toolchain\vs2013_files\4087e065abebdca6dbd0caca2910c6718d2ec67f\VC\bin\amd64_x86\cl.exe"
/nologo /showIncludes /FC
@obj\content\public\common\feature_h264_with_openh264_ffmpeg.feature_h264_with_openh264_ffmpeg.obj.rsp
/c ..\..\content\public\common\feature_h264_with_openh264_ffmpeg.cc
/Foobj\content\public\common\feature_h264_with_openh264_ffmpeg.feature_h264_with_openh264_ffmpeg.obj
/Fdobj\content\feature_h264_with_openh264_ffmpeg.cc.pdb 
e:\b\build\slave\win_builder\build\src\content\public\common\feature_h264_with_openh264_ffmpeg.h(9)
: fatal error C1083: Cannot open include file:
'content/public/common/common_features.h': No such file or directory
ninja: build stopped: subcommand failed.
.

[hbos_chromium, 2016-02-22 08:09:35]

Re-upload of this CL: https://codereview.chromium.org/1718883002/