Only allow webrtc HW offload for H264 encode for extension process.

Only allow webrtc HW offload for H264 encode for extension process.

Add cmdline switch::kEnableWebRtcHWH264Encoding to control whether
to report WebRtc HW H264 encoding capability.

BUG=385941
R=xiyuan@chromium.org,kalman@chromium.org,posicak@chromium.org,jamesr@chromium.org
TBR=piman@chromium.org

Committed: https://crrev.com/44954d45ef230efdaa323a0e192caad55aac42a2
Cr-Commit-Position: refs/heads/master@{#292028}

[hshi1, 2014-08-14 18:34:06]

Note that this has dependency on another CL to land first:
https://codereview.chromium.org/457733002/

Need to check on the renderer process' command line for extension process flag.

+jamesr@ (OWNER content/renderer) for DEPS change
+posciak@ FYI

[jamesr, 2014-08-14 19:15:57]

https://codereview.chromium.org/453063002/diff/1/content/renderer/DEPS
File content/renderer/DEPS (right):

https://codereview.chromium.org/453063002/diff/1/content/renderer/DEPS#newcode4
content/renderer/DEPS:4: "+extensions/common/switches.h",
not lgtm. extensions/ depends on content/ so this would create a cycle

[hshi1, 2014-08-14 19:17:53]

On 2014/08/14 19:15:57, jamesr wrote:
> https://codereview.chromium.org/453063002/diff/1/content/renderer/DEPS
> File content/renderer/DEPS (right):
> 
>
https://codereview.chromium.org/453063002/diff/1/content/renderer/DEPS#newcode4
> content/renderer/DEPS:4: "+extensions/common/switches.h",
> not lgtm. extensions/ depends on content/ so this would create a cycle

I see, does that mean extensions code is allowed to access a singleton class
defined under content/renderer (or content/public)?

[jamesr, 2014-08-14 19:29:11]

extensions can only talk to the content public api

[hshi1, 2014-08-14 21:28:38]

Alright, PTAL patch set #2 which removes the cyclic dependency.

[jamesr, 2014-08-14 21:55:42]

+jam (content/ owner).  I don't think that it makes sense for content to know
about something being an extension or not.  If you just want to set a flag,
can't you do that more directly?  Or add a setting named after the thing you are
trying to enable?

[jam, 2014-08-15 17:37:00]

On 2014/08/14 21:55:42, jamesr wrote:
> +jam (content/ owner).  I don't think that it makes sense for content to know
> about something being an extension or not.  If you just want to set a flag,
> can't you do that more directly?  Or add a setting named after the thing you
are
> trying to enable?

right. 

see http://www.chromium.org/developers/content-module

[hshi1, 2014-08-18 23:32:26]

On 2014/08/15 17:37:00, jam wrote:
> On 2014/08/14 21:55:42, jamesr wrote:
> > +jam (content/ owner).  I don't think that it makes sense for content to
know
> > about something being an extension or not.  If you just want to set a flag,
> > can't you do that more directly?  Or add a setting named after the thing you
> are
> > trying to enable?
> 
> right. 
> 
> see http://www.chromium.org/developers/content-module

jam and jamesr: what's the recommended method to set a flag (instead of
appending a cmdline switch)? Thanks.

[hshi1, 2014-08-19 01:13:34]

PTAL patch set #3 - does this approach look ok to you?

[Pawel Osciak, 2014-08-19 01:30:27]

https://codereview.chromium.org/453063002/diff/40001/content/renderer/media/r...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/453063002/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_video_encoder_factory.cc:37: if
(cmd_line->HasSwitch(switches::kEnableWebRtcHWH264Encoding)) {
Could we please have this together with the kEnableWebRtcHWVp8Encoding flag,
i.e.:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

We could also move it to here out of there if that's preferable, but they should
be together.

[hshi1, 2014-08-19 17:34:49]

https://codereview.chromium.org/453063002/diff/40001/content/renderer/media/r...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/453063002/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_video_encoder_factory.cc:37: if
(cmd_line->HasSwitch(switches::kEnableWebRtcHWH264Encoding)) {
On 2014/08/19 01:30:27, Pawel Osciak wrote:
> Could we please have this together with the kEnableWebRtcHWVp8Encoding flag,
> i.e.:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> 
> We could also move it to here out of there if that's preferable, but they
should
> be together.

You mean also check for the VP8 flag above? Done.

[hshi1, 2014-08-19 17:39:02]

+kalman (OWNER extensions/renderer/dispatcher.cc)

Note that I had to append the switch for extension process.

If this is not idea, I can also move the append to browser process where we call
AppendExtraRendererCommandLineSwitches().

[not at google - send to devlin, 2014-08-19 17:45:57]

https://codereview.chromium.org/453063002/diff/80001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/453063002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:194:
command_line.AppendSwitch(::switches::kEnableWebRtcHWH264Encoding);
It's not clear that appending the command line switch here would work. What if
the Dispatcher gets created after some other component which attempts to read
the switch?

There's code somewhere in the browser which sets command line switches for the
extension process. You should add it there.

[hshi1, 2014-08-19 18:07:39]

https://codereview.chromium.org/453063002/diff/80001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/453063002/diff/80001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:194:
command_line.AppendSwitch(::switches::kEnableWebRtcHWH264Encoding);
Agreed, I'l move the AppendSwitch to browser.

[not at google - send to devlin, 2014-08-19 19:26:00]

lgtm

[Pawel Osciak, 2014-08-20 01:49:29]

https://codereview.chromium.org/453063002/diff/40001/content/renderer/media/r...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/453063002/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_video_encoder_factory.cc:37: if
(cmd_line->HasSwitch(switches::kEnableWebRtcHWH264Encoding)) {
On 2014/08/19 17:34:49, hshi1 wrote:
> On 2014/08/19 01:30:27, Pawel Osciak wrote:
> > Could we please have this together with the kEnableWebRtcHWVp8Encoding flag,
> > i.e.:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> > 
> > We could also move it to here out of there if that's preferable, but they
> should
> > be together.
> 
> You mean also check for the VP8 flag above? Done.

Not exactly, I meant we should not have checks for those flags separately, in
multiple places. We shouldn't have one check in GetSupportedProfiles and one
here. They have to be together in GetSupportedProfiles ideally, otherwise we'll
lose track of those things.

Also, I think that a method that is supposed to only convert codecs is maybe not
the right place to do this check. If anywhere, could we please have it at least
at the call site to GetSupportedProfiles or in the impl of GSP() itself.

[hshi1, 2014-08-22 19:04:27]

On 2014/08/20 01:49:29, Pawel Osciak wrote:
>
https://codereview.chromium.org/453063002/diff/40001/content/renderer/media/r...
> File content/renderer/media/rtc_video_encoder_factory.cc (right):
> 
>
https://codereview.chromium.org/453063002/diff/40001/content/renderer/media/r...
> content/renderer/media/rtc_video_encoder_factory.cc:37: if
> (cmd_line->HasSwitch(switches::kEnableWebRtcHWH264Encoding)) {
> On 2014/08/19 17:34:49, hshi1 wrote:
> > On 2014/08/19 01:30:27, Pawel Osciak wrote:
> > > Could we please have this together with the kEnableWebRtcHWVp8Encoding
flag,
> > > i.e.:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...
> > > 
> > > We could also move it to here out of there if that's preferable, but they
> > should
> > > be together.
> > 
> > You mean also check for the VP8 flag above? Done.
> 
> Not exactly, I meant we should not have checks for those flags separately, in
> multiple places. We shouldn't have one check in GetSupportedProfiles and one
> here. They have to be together in GetSupportedProfiles ideally, otherwise
we'll
> lose track of those things.
> 
> Also, I think that a method that is supposed to only convert codecs is maybe
not
> the right place to do this check. If anywhere, could we please have it at
least
> at the call site to GetSupportedProfiles or in the impl of GSP() itself.

Pawel: actually we do need to separate this specific H264-webRTC check from the
logic in GetSupportedProfiles.

This check is for a very specific business reason - that we only want to expose
H264-webRTC for extensions and not for the general web (I'll defer to PMs to
explain the reason behind this). On the other hand, GetSupportedProfiles() is
used by both webRTC and cast and it is intended to report the actual HW encode
capabilities, regardless of the process type.

[hshi1, 2014-08-22 22:33:19]

Updating list of reviewers:

xiyuan@ (chrome/browser/chromeos/*)
jam@ (content/browser/*)
posciak@ (content/renderer/media/*)

[xiyuan, 2014-08-22 22:36:22]

chrome/browser/chromeos/* LGTM

[Pawel Osciak, 2014-08-25 06:13:32]

rtc_video_encoder_factory.cc LGTM

I think we'll switch to a different flag to use for VDAs, something that is not
"*webrtc*".

[hshi1, 2014-08-25 17:24:58]

jamesr@: you still have a -2 on this CL. Can you review the latest patch set and
comment if it looks ok to you?

[jamesr, 2014-08-26 19:27:35]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-08-26 19:27:35]

lgtm

[hshi1, 2014-08-26 19:42:13]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-08-26 19:44:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hshi@chromium.org/453063002/160001

[commit-bot: I haz the power, 2014-08-26 20:29:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-26 20:34:32]

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

[commit-bot: I haz the power, 2014-08-26 20:34:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hshi1, 2014-08-26 23:13:22]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-08-26 23:14:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hshi@chromium.org/453063002/160001

[commit-bot: I haz the power, 2014-08-26 23:32:38]

Committed patchset #7 (160001) as 7d737bc4f415c4152ff0a227184faedf00b948a1

[commit-bot: I haz the power, 2014-09-10 02:47:17]

Patchset 7 (id:??) landed as
https://crrev.com/44954d45ef230efdaa323a0e192caad55aac42a2
Cr-Commit-Position: refs/heads/master@{#292028}