GpuArcVideoService

Implements GpuArcVideoService and its host

In the GPU process, GpuArcVideoService (arc::VideoHost) is registered
via content's gpu client hook. It also manages additional IPC channel
per accelerator and dispatches calls to ArcVideoAccelerator.

In the browser process, GpuArcVideoServiceHost listens events of
ArcBridgeService and pass to GpuArcVideoService.

BUG=b/25057601
TEST=video from arc playbacks

Committed: https://crrev.com/e5654af3fec812e02719216a0014698b235cd52f
Cr-Commit-Position: refs/heads/master@{#387799}

[kcwu, 2016-02-01 12:41:06]

kcwu@chromium.org changed reviewers:
+ owenlin@chromium.org, posciak@chromium.org

[kcwu, 2016-02-03 15:00:51]

Description was changed from

==========
GpuArcVideoService

BUG=b/25057601
==========

to

==========
Implements GpuArcVideoService and its host
    
In the GPU process, GpuArcVideoService (arc::VideoHost) is registered
via content's gpu client hook. It also manages additional IPC channel
per accelerator and dispatches calls to ArcVideoAccelerator.
    
In the browser process, GpuArcVideoServiceHost listens events of
ArcBridgeService and pass to GpuArcVideoService.
    
BUG=b/25057601
TEST=video from arc playbacks
==========

[kcwu, 2016-03-08 07:39:36]

PTAL

[wuchengli, 2016-03-14 06:50:56]

wuchengli@chromium.org changed reviewers:
+ lhchavez@chromium.org, wuchengli@chromium.org

[wuchengli, 2016-03-14 06:50:57]

Luis. Please take a look. Thanks.

[Owen Lin, 2016-03-14 08:45:52]

https://codereview.chromium.org/1641353003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1641353003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.h:23: // This class
passes requests from arc::VideoInstance to GpuArcVideoService to
s/passes/takes/

How aobut:
This class takes requests for creating channels of video accelerators from
arc::VideoInstance and forwards these requests to GpuArcVideoServce. It also
returns the created channels back to the arc::VideoInstance.

https://codereview.chromium.org/1641353003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.h:30: //
GpuArcVideoService, in the GPU process.
I don't think this comment helps. I would suggest only keep the comments about
relation of this class and GpuArcVideoService.

For example,

This class is the proxy end of GpuArcVideoService and runs in the browser
process. The corresponding end "GpuArcVideoService" runs in the GPU process.

https://codereview.chromium.org/1641353003/diff/100001/chrome/gpu/chrome_cont...
File chrome/gpu/chrome_content_gpu_client.cc (right):

https://codereview.chromium.org/1641353003/diff/100001/chrome/gpu/chrome_cont...
chrome/gpu/chrome_content_gpu_client.cc:18: // MojoProxyResolverFactoryImpl is
strongly bound to the Mojo message pipe it
What's a MojoProxyResolverFactory? 
This one?
https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/mojo_pro...

Are you talking about the message pipe in the request?

https://codereview.chromium.org/1641353003/diff/100001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/100001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:238:
mojo::InterfaceRequest<::arc::VideoHost> req)
s/req/request/ ?

https://codereview.chromium.org/1641353003/diff/100001/components/arc/common/...
File components/arc/common/video.mojom (right):

https://codereview.chromium.org/1641353003/diff/100001/components/arc/common/...
components/arc/common/video.mojom:8: // Notifies Chrome a request for ipc
channel of video acceleration.
... for an ipc channel ... ?

But why the name is OnXXX. Usually XXX will be some kind of event.

Can we just use requestArcVideoAcceleratorChannel?

The comment could be:
Requests an ipc channel from Chrome to be used in the video accelerator. Will
this call fail (due to resource limitation)? If yet, you should document what
happens.


In addition, why a pid is required here? Do we only allow one
ArcVideoAccelerator per process? Please also explain what's is a token? How is
it being used?

[kcwu, 2016-03-14 12:55:49]

https://codereview.chromium.org/1641353003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1641353003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.h:23: // This class
passes requests from arc::VideoInstance to GpuArcVideoService to
On 2016/03/14 08:45:52, Owen Lin wrote:
> s/passes/takes/
> 
> How aobut:
> This class takes requests for creating channels of video accelerators from
> arc::VideoInstance and forwards these requests to GpuArcVideoServce. It also
> returns the created channels back to the arc::VideoInstance.

Done.

https://codereview.chromium.org/1641353003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.h:30: //
GpuArcVideoService, in the GPU process.
On 2016/03/14 08:45:52, Owen Lin wrote:
> I don't think this comment helps. I would suggest only keep the comments about
> relation of this class and GpuArcVideoService.
> 
> For example,
> 
> This class is the proxy end of GpuArcVideoService and runs in the browser
> process. The corresponding end "GpuArcVideoService" runs in the GPU process.

Done.

https://codereview.chromium.org/1641353003/diff/100001/chrome/gpu/chrome_cont...
File chrome/gpu/chrome_content_gpu_client.cc (right):

https://codereview.chromium.org/1641353003/diff/100001/chrome/gpu/chrome_cont...
chrome/gpu/chrome_content_gpu_client.cc:18: // MojoProxyResolverFactoryImpl is
strongly bound to the Mojo message pipe it
On 2016/03/14 08:45:52, Owen Lin wrote:
> What's a MojoProxyResolverFactory? 
> This one?
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/mojo_pro...
> 
> Are you talking about the message pipe in the request?

Ah, sorry. I copied and pasted the comment without modification. I mean
GpuArcVideoService.

https://codereview.chromium.org/1641353003/diff/100001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/100001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:238:
mojo::InterfaceRequest<::arc::VideoHost> req)
On 2016/03/14 08:45:52, Owen Lin wrote:
> s/req/request/ ?

Done.

https://codereview.chromium.org/1641353003/diff/100001/components/arc/common/...
File components/arc/common/video.mojom (right):

https://codereview.chromium.org/1641353003/diff/100001/components/arc/common/...
components/arc/common/video.mojom:8: // Notifies Chrome a request for ipc
channel of video acceleration.
On 2016/03/14 08:45:52, Owen Lin wrote:
> ... for an ipc channel ... ?
> 
> But why the name is OnXXX. Usually XXX will be some kind of event.
> 
> Can we just use requestArcVideoAcceleratorChannel?

The naming "On..." follows other arc interfaces (*.mojom in this folder). On*
for Host side and no "On" for Instance side.


> The comment could be:
> Requests an ipc channel from Chrome to be used in the video accelerator. Will
> this call fail (due to resource limitation)? If yet, you should document what
> happens.
Done.

> In addition, why a pid is required here? Do we only allow one
> ArcVideoAccelerator per process? Please also explain what's is a token? How is
> it being used?
They are required to establish mojo channel and create message pipes. 

There are two steps to create message pipe using the new mojo edk.
(It's no longer a simple unix pipe as before)
1. exchange a pipe handle for mojo internal use. ChildProcessLaunched(pid) and
SetParentPipeHandle(handle) are called on parent and child side, respectively.
2. exchange a (random) token to create pipe for mojo interfaces. Then
CreateParentMessagePipe(token) and CreateChildMessagePipe(token) are called on
parent and child side, respectively.

I added comments for them. However, I don't want to add too much since it is
kind of mojo low level things.

[wuchengli, 2016-03-16 06:51:38]

Luis. Please review *.mojom and chrome/gpu/gpu_arc_video_service.cc. Thanks.

[Owen Lin, 2016-03-16 08:41:29]

lgtm

[kcwu, 2016-03-16 09:31:47]

kcwu@chromium.org changed reviewers:
+ cpu@chromium.org, dcheng@chromium.org

[kcwu, 2016-03-16 09:31:48]

Owner review, please
cpu: chrome/app, chrome/gpu
lhchavez: chrome/browser/chromeos/arc, components/arc, and  Connect() in
gpu_arc_video_service.cc
dcheng: *.mojom and ipc security in general

[dcheng, 2016-03-16 17:44:51]

https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc:61: auto* ptr_info =
new mojo::InterfacePtrInfo<arc::VideoHost>();
How about using PostTaskAndReplyWithResult rather than doing this?

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/chrome_cont...
File chrome/gpu/chrome_content_gpu_client.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/chrome_cont...
chrome/gpu/chrome_content_gpu_client.cc:35:
registry->AddService<arc::VideoHost>(base::Bind(&CreateGpuArcVideoService));
Shouldn't this be able to deduce the template parameter?

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:27: // by kPidBase and don't conflict with
legitimate process.
How come this isn't a problem for other mojo services?

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:111: bool Connect(std::string token) {
const std::string&

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:207:
accelerator_->BindDmabuf(static_cast<PortType>(port), index, fd);
Am I missing another CL? Where is this defined? (Similar question for
chromeos::arc::VideoFormat and other types mentioned above in the type
converters)

[Luis Héctor Chávez, 2016-03-16 19:07:55]

Overall approach in this side lg.

https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc:47:
arc_bridge_service()->video_instance()->Init(
I'd just add DCHECKs every time you use arc_bridge_service()->video_instance().

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:27: // by kPidBase and don't conflict with
legitimate process.
On 2016/03/16 17:44:50, dcheng wrote:
> How come this isn't a problem for other mojo services?

Other mojo services are running in the same pid namespace. Also, after the
recent switch to Ports EDK, the pid is not even used at all in Linux.

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:32: uint32_t kPidBase = 0x3F000000;  //
arbitrary chosen.
arbitrarily.

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:124: // base::Unretained is safe because we
owned |client_|
nit: we own. Same below.

https://codereview.chromium.org/1641353003/diff/120001/components/arc/common/...
File components/arc/common/video.mojom (right):

https://codereview.chromium.org/1641353003/diff/120001/components/arc/common/...
components/arc/common/video.mojom:10: // Replys |channel_handle| of mojo initial
message pipe and |token| for
nit: s/Replys/Replies with/

[cpu_(ooo_6.6-7.5), 2016-03-16 19:25:09]

chrome/app looks fine. I am not a good reviewer for /gpu/

[kcwu, 2016-03-17 08:07:44]

add sievers@chromium.org for chrome/gpu owner review
(owner added in crrev.com/1549473002)
PTAL, thanks.

https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc:47:
arc_bridge_service()->video_instance()->Init(
On 2016/03/16 19:07:54, Luis Héctor Chávez wrote:
> I'd just add DCHECKs every time you use
arc_bridge_service()->video_instance().

Done.

https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc:61: auto* ptr_info =
new mojo::InterfacePtrInfo<arc::VideoHost>();
On 2016/03/16 17:44:50, dcheng wrote:
> How about using PostTaskAndReplyWithResult rather than doing this?

Done.

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/chrome_cont...
File chrome/gpu/chrome_content_gpu_client.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/chrome_cont...
chrome/gpu/chrome_content_gpu_client.cc:35:
registry->AddService<arc::VideoHost>(base::Bind(&CreateGpuArcVideoService));
On 2016/03/16 17:44:50, dcheng wrote:
> Shouldn't this be able to deduce the template parameter?

Done.

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:27: // by kPidBase and don't conflict with
legitimate process.
On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> On 2016/03/16 17:44:50, dcheng wrote:
> > How come this isn't a problem for other mojo services?
> 
> Other mojo services are running in the same pid namespace. Also, after the
> recent switch to Ports EDK, the pid is not even used at all in Linux.

Do you know that the pid is just not used yet or will be deprecated in the
future?

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:32: uint32_t kPidBase = 0x3F000000;  //
arbitrary chosen.
On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> arbitrarily.

Done.

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:111: bool Connect(std::string token) {
On 2016/03/16 17:44:50, dcheng wrote:
> const std::string&

Done.

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:124: // base::Unretained is safe because we
owned |client_|
On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> nit: we own. Same below.

Done.

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:207:
accelerator_->BindDmabuf(static_cast<PortType>(port), index, fd);
On 2016/03/16 17:44:50, dcheng wrote:
> Am I missing another CL? Where is this defined? (Similar question for
> chromeos::arc::VideoFormat and other types mentioned above in the type
> converters)

They are in this pending CL
https://codereview.chromium.org/1549473002/

https://codereview.chromium.org/1641353003/diff/120001/components/arc/common/...
File components/arc/common/video.mojom (right):

https://codereview.chromium.org/1641353003/diff/120001/components/arc/common/...
components/arc/common/video.mojom:10: // Replys |channel_handle| of mojo initial
message pipe and |token| for
On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> nit: s/Replys/Replies with/

Done.

[kcwu, 2016-03-18 13:03:48]

kcwu@chromium.org changed reviewers:
+ sievers@chromium.org

[kcwu, 2016-03-18 13:03:49]

Ah, said adding @sievers but actually forgot. Sorry.

[no sievers, 2016-03-18 18:22:10]

lgtm

[wuchengli, 2016-03-21 06:53:14]

Luis. PTAL. Thanks.

[kcwu, 2016-03-21 10:04:50]

@cpu (for chrome/app) and @dcheng, PTAL. Thanks.

[jam, 2016-03-21 14:53:56]

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

[jam, 2016-03-21 14:53:57]

I got pointed at this cl from https://codereview.chromium.org/1808743002/.

I'm trying to figure out why GpuPreferences are exposed in content/public/gpu.
It looks like that this cl uses it to create ArcGpuVideoDecodeAccelerator, but
that class isn't in this cl and is also not in the codebase.

IMO CLs shouldn't be sent for review until they have green trybots (i.e. one
condition of that is that all the code is there)

[wuchengli, 2016-03-24 06:50:24]

On 2016/03/16 19:25:09, cpu wrote:
> chrome/app looks fine. I am not a good reviewer for /gpu/
cpu@ can you give LGT-M for chorme/app? Thanks.
dcheng@ Please take a look again. Thanks.

[kcwu, 2016-03-24 13:14:13]

On 2016/03/21 14:53:57, jam wrote:
> I got pointed at this cl from https://codereview.chromium.org/1808743002/.
I already moved the code changing content hook into this CL. Please take a look.

> I'm trying to figure out why GpuPreferences are exposed in content/public/gpu.
> It looks like that this cl uses it to create ArcGpuVideoDecodeAccelerator, but
> that class isn't in this cl and is also not in the codebase.
> 
> IMO CLs shouldn't be sent for review until they have green trybots (i.e. one
> condition of that is that all the code is there)

Actually all needed CLs are already there, working, and under review parallelly.
They are
http://crrev.com/1745903002 VDA factory. Need GpuPreferences.
http://crrev.com/1822983002 VDA support external buffer import
http://crrev.com/1816203003 VDA::Flush return buffers
http://crrev.com/1549473002 ArcGVDA. Call VDA factory and call VDA methods.
http://crrev.com/1641353003 GpuArcVideoService. Use content hook, IPC, and call
ArcGVDA (this CL)
Sometimes they failed to build because some of them are revised and/or rebased
but not all refreshed at the same time.

[Luis Héctor Chávez, 2016-03-24 17:24:10]

On 2016/03/17 08:07:44, kcwu wrote:
> add mailto:sievers@chromium.org for chrome/gpu owner review
> (owner added in crrev.com/1549473002)
> PTAL, thanks.
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
> File chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc (right):
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
> chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc:47:
> arc_bridge_service()->video_instance()->Init(
> On 2016/03/16 19:07:54, Luis Héctor Chávez wrote:
> > I'd just add DCHECKs every time you use
> arc_bridge_service()->video_instance().
> 
> Done.
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/browser/chromeo...
> chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc:61: auto* ptr_info =
> new mojo::InterfacePtrInfo<arc::VideoHost>();
> On 2016/03/16 17:44:50, dcheng wrote:
> > How about using PostTaskAndReplyWithResult rather than doing this?
> 
> Done.
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/chrome_cont...
> File chrome/gpu/chrome_content_gpu_client.cc (right):
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/chrome_cont...
> chrome/gpu/chrome_content_gpu_client.cc:35:
> registry->AddService<arc::VideoHost>(base::Bind(&CreateGpuArcVideoService));
> On 2016/03/16 17:44:50, dcheng wrote:
> > Shouldn't this be able to deduce the template parameter?
> 
> Done.
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
> File chrome/gpu/gpu_arc_video_service.cc (right):
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
> chrome/gpu/gpu_arc_video_service.cc:27: // by kPidBase and don't conflict with
> legitimate process.
> On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> > On 2016/03/16 17:44:50, dcheng wrote:
> > > How come this isn't a problem for other mojo services?
> > 
> > Other mojo services are running in the same pid namespace. Also, after the
> > recent switch to Ports EDK, the pid is not even used at all in Linux.
> 
> Do you know that the pid is just not used yet or will be deprecated in the
> future?

It was used in the past, but nowadays it's not used in Linux. It's still better
to pass it in for consistency: https://codereview.chromium.org/1740093002

> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
> chrome/gpu/gpu_arc_video_service.cc:32: uint32_t kPidBase = 0x3F000000;  //
> arbitrary chosen.
> On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> > arbitrarily.
> 
> Done.
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
> chrome/gpu/gpu_arc_video_service.cc:111: bool Connect(std::string token) {
> On 2016/03/16 17:44:50, dcheng wrote:
> > const std::string&
> 
> Done.
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
> chrome/gpu/gpu_arc_video_service.cc:124: // base::Unretained is safe because
we
> owned |client_|
> On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> > nit: we own. Same below.
> 
> Done.
> 
>
https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
> chrome/gpu/gpu_arc_video_service.cc:207:
> accelerator_->BindDmabuf(static_cast<PortType>(port), index, fd);
> On 2016/03/16 17:44:50, dcheng wrote:
> > Am I missing another CL? Where is this defined? (Similar question for
> > chromeos::arc::VideoFormat and other types mentioned above in the type
> > converters)
> 
> They are in this pending CL
> https://codereview.chromium.org/1549473002/
> 
>
https://codereview.chromium.org/1641353003/diff/120001/components/arc/common/...
> File components/arc/common/video.mojom (right):
> 
>
https://codereview.chromium.org/1641353003/diff/120001/components/arc/common/...
> components/arc/common/video.mojom:10: // Replys |channel_handle| of mojo
initial
> message pipe and |token| for
> On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> > nit: s/Replys/Replies with/
> 
> Done.

[Luis Héctor Chávez, 2016-03-24 17:24:20]

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:31: base::ProcessHandle
RemapChildPid(uint32_t pid) {
Can you move this somewhere in components/arc and make both this and
arc_bridge_bootstrap.cc consume it?

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:270: accelerator_stubs_[stub.get()] =
std::move(stub);
nit: accelerator_stubs_.insert(std::make_pair(stub.get(), std::move(stub)));

https://codereview.chromium.org/1641353003/diff/200001/components/arc/common/...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1641353003/diff/200001/components/arc/common/...
components/arc/common/video_accelerator.mojom:9: enum HalPixelFormatExtension {
Do you think all these enums will need modification in the future? If that is
the case, please add a [Extensible] tag on each one.

https://codereview.chromium.org/1641353003/diff/200001/components/arc/common/...
components/arc/common/video_accelerator.mojom:55:
Initialize(ArcVideoAcceleratorConfig config) => (bool result);
Can you add explicit ordinals?

[dcheng, 2016-03-24 20:29:15]

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/120001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:27: // by kPidBase and don't conflict with
legitimate process.
On 2016/03/17 at 08:07:44, kcwu wrote:
> On 2016/03/16 19:07:55, Luis Héctor Chávez wrote:
> > On 2016/03/16 17:44:50, dcheng wrote:
> > > How come this isn't a problem for other mojo services?
> > 
> > Other mojo services are running in the same pid namespace. Also, after the
> > recent switch to Ports EDK, the pid is not even used at all in Linux.
> 
> Do you know that the pid is just not used yet or will be deprecated in the
future?

If the pid isn't used at all, then why do we need to do this remapping?

https://codereview.chromium.org/1641353003/diff/200001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:992: return NULL;
nullptr

https://codereview.chromium.org/1641353003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc:20:
registry->ConnectToRemoteService<arc::VideoHost>(mojo::GetProxy(&host_ptr));
Shouldn't this autodeduce?

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:194: ashmem_handle.release().value(),
&scoped_platform_handle);
Mojo folks: can we make this API a little more straightforward to use, so the
first parameter can also just be a ScopedHandle? Then you could use it like

PassWrappedPlatformhandle(std::move(ashmem_handle), &scoped_platform_handle)

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:199: static_cast<size_t>(offset),
These static casts make me nervous, is there some guarantee that arc++ and
chromium will build with the same word size?

[Luis Héctor Chávez, 2016-03-24 23:16:53]

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:199: static_cast<size_t>(offset),
On 2016/03/24 20:29:14, dcheng wrote:
> These static casts make me nervous, is there some guarantee that arc++ and
> chromium will build with the same word size?

No, and in fact for x86_64 devices it is guaranteed that they will be built with
different word sizes (ARC is built with 32-bits).

[kcwu, 2016-03-28 13:17:35]

https://codereview.chromium.org/1641353003/diff/200001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:992: return NULL;
On 2016/03/24 20:29:14, dcheng wrote:
> nullptr

Done.

https://codereview.chromium.org/1641353003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/gpu_arc_video_service_host.cc:20:
registry->ConnectToRemoteService<arc::VideoHost>(mojo::GetProxy(&host_ptr));
On 2016/03/24 20:29:14, dcheng wrote:
> Shouldn't this autodeduce?

Done.

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:31: base::ProcessHandle
RemapChildPid(uint32_t pid) {
On 2016/03/24 17:24:20, Luis Héctor Chávez wrote:
> Can you move this somewhere in components/arc and make both this and
> arc_bridge_bootstrap.cc consume it?

Done.

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:270: accelerator_stubs_[stub.get()] =
std::move(stub);
On 2016/03/24 17:24:20, Luis Héctor Chávez wrote:
> nit: accelerator_stubs_.insert(std::make_pair(stub.get(), std::move(stub)));

Done.

https://codereview.chromium.org/1641353003/diff/200001/components/arc/common/...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1641353003/diff/200001/components/arc/common/...
components/arc/common/video_accelerator.mojom:9: enum HalPixelFormatExtension {
On 2016/03/24 17:24:20, Luis Héctor Chávez wrote:
> Do you think all these enums will need modification in the future? If that is
> the case, please add a [Extensible] tag on each one.

Done.

https://codereview.chromium.org/1641353003/diff/200001/components/arc/common/...
components/arc/common/video_accelerator.mojom:55:
Initialize(ArcVideoAcceleratorConfig config) => (bool result);
On 2016/03/24 17:24:20, Luis Héctor Chávez wrote:
> Can you add explicit ordinals?

Done.

[Luis Héctor Chávez, 2016-03-28 16:01:28]

components/arc lgtm with nits.

https://codereview.chromium.org/1641353003/diff/220001/components/arc/arc_bri...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/1641353003/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_bootstrap.cc:37: base::ProcessHandle
RemapChildPid(uint32_t pid) {
nit: s/uint32_t/base::ProcessHandle/ in all this.

https://codereview.chromium.org/1641353003/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_bootstrap.cc:265: const pid_t kArcPid =
RemapChildPid(0);
nit: const base::ProcessHandle

[kcwu, 2016-03-28 18:16:50]

https://codereview.chromium.org/1641353003/diff/220001/components/arc/arc_bri...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/1641353003/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_bootstrap.cc:37: base::ProcessHandle
RemapChildPid(uint32_t pid) {
On 2016/03/28 16:01:27, Luis Héctor Chávez wrote:
> nit: s/uint32_t/base::ProcessHandle/ in all this.

Done.

https://codereview.chromium.org/1641353003/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_bootstrap.cc:265: const pid_t kArcPid =
RemapChildPid(0);
On 2016/03/28 16:01:27, Luis Héctor Chávez wrote:
> nit: const base::ProcessHandle

Done.

[jam, 2016-03-30 16:24:26]

On 2016/03/24 13:14:13, kcwu wrote:
> On 2016/03/21 14:53:57, jam wrote:
> > I got pointed at this cl from https://codereview.chromium.org/1808743002/.
> I already moved the code changing content hook into this CL. Please take a
look.
> 
> > I'm trying to figure out why GpuPreferences are exposed in
content/public/gpu.
> > It looks like that this cl uses it to create ArcGpuVideoDecodeAccelerator,
but
> > that class isn't in this cl and is also not in the codebase.
> > 
> > IMO CLs shouldn't be sent for review until they have green trybots (i.e. one
> > condition of that is that all the code is there)
> 
> Actually all needed CLs are already there, working, and under review
parallelly.
> They are
> http://crrev.com/1745903002 VDA factory. Need GpuPreferences.
> http://crrev.com/1822983002 VDA support external buffer import
> http://crrev.com/1816203003 VDA::Flush return buffers
> http://crrev.com/1549473002 ArcGVDA. Call VDA factory and call VDA methods.
> http://crrev.com/1641353003 GpuArcVideoService. Use content hook, IPC, and
call
> ArcGVDA (this CL)
> Sometimes they failed to build because some of them are revised and/or rebased
> but not all refreshed at the same time.

this takes a lot of effort since I had to look through a few changes to figure
out how this new parameter is used. per my previous comment, it would be nice to
review changes that are self contained and are compiling.

after following the above cls, the gpu preferences is sent from content to
chrome, then back to content again in the
content::GpuVideoDecodeAcceleratorFactory::CreateVDA call. there's no point
sending that parameter from content-> chrome and then back.
GpuVideoDecodeAcceleratorFactory::CreateVDA in content/gpu can have access to
that that struct internally.

[dcheng, 2016-03-30 16:52:36]

My question about RemapChildPid has still not been answered. If the child pid is
not significant and not used by Mojo, why do we need it at all?

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:199: static_cast<size_t>(offset),
On 2016/03/24 at 23:16:53, Luis Héctor Chávez wrote:
> On 2016/03/24 20:29:14, dcheng wrote:
> > These static casts make me nervous, is there some guarantee that arc++ and
> > chromium will build with the same word size?
> 
> No, and in fact for x86_64 devices it is guaranteed that they will be built
with different word sizes (ARC is built with 32-bits).

If that's the case, why not just make the arguments uint32_t?

https://codereview.chromium.org/1641353003/diff/260001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/260001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:158: // ::arc::VideoAcceleratorService
impementation:
Nit: implementation.

[Luis Héctor Chávez, 2016-03-30 16:56:09]

On 2016/03/30 16:52:36, dcheng wrote:
> My question about RemapChildPid has still not been answered. If the child pid
is
> not significant and not used by Mojo, why do we need it at all?

We don't _need_ it, but I prefer to provide reasonable values instead of
garbage/hardcoded data and comply with the interface in case the behavior
changes in the future.

> 
>
https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
> File chrome/gpu/gpu_arc_video_service.cc (right):
> 
>
https://codereview.chromium.org/1641353003/diff/200001/chrome/gpu/gpu_arc_vid...
> chrome/gpu/gpu_arc_video_service.cc:199: static_cast<size_t>(offset),
> On 2016/03/24 at 23:16:53, Luis Héctor Chávez wrote:
> > On 2016/03/24 20:29:14, dcheng wrote:
> > > These static casts make me nervous, is there some guarantee that arc++ and
> > > chromium will build with the same word size?
> > 
> > No, and in fact for x86_64 devices it is guaranteed that they will be built
> with different word sizes (ARC is built with 32-bits).
> 
> If that's the case, why not just make the arguments uint32_t?
> 
>
https://codereview.chromium.org/1641353003/diff/260001/chrome/gpu/gpu_arc_vid...
> File chrome/gpu/gpu_arc_video_service.cc (right):
> 
>
https://codereview.chromium.org/1641353003/diff/260001/chrome/gpu/gpu_arc_vid...
> chrome/gpu/gpu_arc_video_service.cc:158: // ::arc::VideoAcceleratorService
> impementation:
> Nit: implementation.

[dcheng, 2016-03-30 16:58:33]

On 2016/03/30 at 16:56:09, lhchavez wrote:
> On 2016/03/30 16:52:36, dcheng wrote:
> > My question about RemapChildPid has still not been answered. If the child
pid is
> > not significant and not used by Mojo, why do we need it at all?
> 
> We don't _need_ it, but I prefer to provide reasonable values instead of
garbage/hardcoded data and comply with the interface in case the behavior
changes in the future.
> 

Sure, but I don't understand what's wrong with the namespaced pid? If Mojo
starts using it, then things will explode and we can figure out the right story
from there (since it won't necessarily be a problem just for arc). And if it
doesn't, then it doesn't matter, and RemapChildPid just looks bizarre.

[kcwu, 2016-03-30 17:03:44]

On 2016/03/30 16:24:26, jam wrote:
> after following the above cls, the gpu preferences is sent from content to
> chrome, then back to content again in the
> content::GpuVideoDecodeAcceleratorFactory::CreateVDA call. there's no point
> sending that parameter from content-> chrome and then back.
> GpuVideoDecodeAcceleratorFactory::CreateVDA in content/gpu can have access to
> that that struct internally.

GpuPreferences is keep by GpuChildThread. How can CreateVDA access to it? Is
there a static method to get GpuChildThread object?
Can we make GpuPreferences a global variable internally in content?

[no sievers, 2016-03-30 17:21:46]

On 2016/03/30 17:03:44, kcwu wrote:
> On 2016/03/30 16:24:26, jam wrote:
> > after following the above cls, the gpu preferences is sent from content to
> > chrome, then back to content again in the
> > content::GpuVideoDecodeAcceleratorFactory::CreateVDA call. there's no point
> > sending that parameter from content-> chrome and then back.
> > GpuVideoDecodeAcceleratorFactory::CreateVDA in content/gpu can have access
to
> > that that struct internally.
> 
> GpuPreferences is keep by GpuChildThread. How can CreateVDA access to it? Is
> there a static method to get GpuChildThread object?
> Can we make GpuPreferences a global variable internally in content?

Notice that
https://codereview.chromium.org/1753383003/diff/180001/content/public/browser...
also exposed this private structure in the public api :/

[jam, 2016-03-30 17:44:17]

On 2016/03/30 17:03:44, kcwu wrote:
> On 2016/03/30 16:24:26, jam wrote:
> > after following the above cls, the gpu preferences is sent from content to
> > chrome, then back to content again in the
> > content::GpuVideoDecodeAcceleratorFactory::CreateVDA call. there's no point
> > sending that parameter from content-> chrome and then back.
> > GpuVideoDecodeAcceleratorFactory::CreateVDA in content/gpu can have access
to
> > that that struct internally.
> 
> GpuPreferences is keep by GpuChildThread. How can CreateVDA access to it? Is
> there a static method to get GpuChildThread object?

sure add a getter in GpuChildThread and you can get to it from the factory via
static_cast<GpuChildThread*>(ChildThread::Get())

> Can we make GpuPreferences a global variable internally in content?

[kcwu, 2016-03-31 14:30:00]

On 2016/03/30 17:44:17, jam wrote:
> On 2016/03/30 17:03:44, kcwu wrote:
> > GpuPreferences is keep by GpuChildThread. How can CreateVDA access to it? Is
> > there a static method to get GpuChildThread object?
> 
> sure add a getter in GpuChildThread and you can get to it from the factory via
> static_cast<GpuChildThread*>(ChildThread::Get())

The compiler complains static_cast
  cannot convert from pointer to base class 'content::ChildThread' to pointer to
derived class 'content::GpuChildThread' because the base is virtual

and dynamic_cast is not permitted. Any suggestions?

[jam, 2016-04-01 16:23:14]

On 2016/03/31 14:30:00, kcwu wrote:
> On 2016/03/30 17:44:17, jam wrote:
> > On 2016/03/30 17:03:44, kcwu wrote:
> > > GpuPreferences is keep by GpuChildThread. How can CreateVDA access to it?
Is
> > > there a static method to get GpuChildThread object?
> > 
> > sure add a getter in GpuChildThread and you can get to it from the factory
via
> > static_cast<GpuChildThread*>(ChildThread::Get())
> 
> The compiler complains static_cast
>   cannot convert from pointer to base class 'content::ChildThread' to pointer
to
> derived class 'content::GpuChildThread' because the base is virtual
> 
> and dynamic_cast is not permitted. Any suggestions?

ah, in that case add a tls pointer like for renderthreadimpl:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

[Luis Héctor Chávez, 2016-04-01 16:51:21]

On 2016/03/30 16:58:33, dcheng wrote:
> On 2016/03/30 at 16:56:09, lhchavez wrote:
> > On 2016/03/30 16:52:36, dcheng wrote:
> > > My question about RemapChildPid has still not been answered. If the child
> pid is
> > > not significant and not used by Mojo, why do we need it at all?
> > 
> > We don't _need_ it, but I prefer to provide reasonable values instead of
> garbage/hardcoded data and comply with the interface in case the behavior
> changes in the future.
> > 
> 
> Sure, but I don't understand what's wrong with the namespaced pid? If Mojo
> starts using it, then things will explode and we can figure out the right
story
> from there (since it won't necessarily be a problem just for arc). And if it
> doesn't, then it doesn't matter, and RemapChildPid just looks bizarre.

Let's use the same pid for all ARC processes then.

[kcwu, 2016-04-06 12:42:25]

John, Daniel, Luis, PTAL.

https://codereview.chromium.org/1641353003/diff/260001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/260001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:158: // ::arc::VideoAcceleratorService
impementation:
On 2016/03/30 16:52:36, dcheng wrote:
> Nit: implementation.

Done.

[Luis Héctor Chávez, 2016-04-06 15:23:13]

https://codereview.chromium.org/1641353003/diff/280001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/280001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:232: mojo::edk::ChildProcessLaunched(0);
nit: Use a constant.

constexpr base::ProcessHandle kUnusedChildProcessHandle = 0;

https://codereview.chromium.org/1641353003/diff/280001/components/arc/arc_bri...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/1641353003/diff/280001/components/arc/arc_bri...
components/arc/arc_bridge_bootstrap.cc:255: mojo::edk::ChildProcessLaunched(0);
Same here.

[kcwu, 2016-04-07 01:02:27]

https://codereview.chromium.org/1641353003/diff/280001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/280001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:232: mojo::edk::ChildProcessLaunched(0);
On 2016/04/06 15:23:13, Luis Héctor Chávez wrote:
> nit: Use a constant.
> 
> constexpr base::ProcessHandle kUnusedChildProcessHandle = 0; 

Done.

https://codereview.chromium.org/1641353003/diff/280001/components/arc/arc_bri...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/1641353003/diff/280001/components/arc/arc_bri...
components/arc/arc_bridge_bootstrap.cc:255: mojo::edk::ChildProcessLaunched(0);
On 2016/04/06 15:23:13, Luis Héctor Chávez wrote:
> Same here.

Done.

[dcheng, 2016-04-07 02:17:57]

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:181: static_cast<size_t>(length));
My earlier question still stands: is the arc process ever going to be > 32-bit.
Will it ever make sense for the arc process to bind >
std::numeric_limits<uint32_t>::max() bytes of shared memory?

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:193: int fd =
scoped_platform_handle.release().handle;
Pass this as ScopedFD instead of just passing around a raw fd.

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:231: constexpr base::ProcessHandle
kUnusedChildProcessHandle = 0;
constexpr is still on the banned list. I think we're pretty close to saying that
we're comfortable depending on VS2015 features, but let's be sure.

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.h (right):

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.h:47: std::map<AcceleratorStub*,
scoped_ptr<AcceleratorStub>> accelerator_stubs_;
Use unique_ptr. Also, don't forget to #include <memory>

[kcwu, 2016-04-07 08:17:58]

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:181: static_cast<size_t>(length));
On 2016/04/07 02:17:57, dcheng wrote:
> My earlier question still stands: is the arc process ever going to be >
32-bit.
> Will it ever make sense for the arc process to bind >
> std::numeric_limits<uint32_t>::max() bytes of shared memory?

No, the length is about few times frame size. I added check for "length >
std::numeric_limits<size_t>::max()".

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:193: int fd =
scoped_platform_handle.release().handle;
On 2016/04/07 02:17:57, dcheng wrote:
> Pass this as ScopedFD instead of just passing around a raw fd.

Done.

This modified arc_video_accelerator.h, cc @owenlin

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:231: constexpr base::ProcessHandle
kUnusedChildProcessHandle = 0;
On 2016/04/07 02:17:57, dcheng wrote:
> constexpr is still on the banned list. I think we're pretty close to saying
that
> we're comfortable depending on VS2015 features, but let's be sure.

Done.

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.h (right):

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.h:47: std::map<AcceleratorStub*,
scoped_ptr<AcceleratorStub>> accelerator_stubs_;
On 2016/04/07 02:17:57, dcheng wrote:
> Use unique_ptr. Also, don't forget to #include <memory>

Done.

[dcheng, 2016-04-07 08:19:20]

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:181: static_cast<size_t>(length));
On 2016/04/07 at 08:17:57, kcwu wrote:
> On 2016/04/07 02:17:57, dcheng wrote:
> > My earlier question still stands: is the arc process ever going to be >
32-bit.
> > Will it ever make sense for the arc process to bind >
> > std::numeric_limits<uint32_t>::max() bytes of shared memory?
> 
> No, the length is about few times frame size. I added check for "length >
std::numeric_limits<size_t>::max()".

So... why not just pass it as uint32_t in the IDL? =)

[kcwu, 2016-04-07 08:32:33]

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1641353003/diff/300001/chrome/gpu/gpu_arc_vid...
chrome/gpu/gpu_arc_video_service.cc:181: static_cast<size_t>(length));
On 2016/04/07 08:19:20, dcheng wrote:
> On 2016/04/07 at 08:17:57, kcwu wrote:
> > On 2016/04/07 02:17:57, dcheng wrote:
> > > My earlier question still stands: is the arc process ever going to be >
> 32-bit.
> > > Will it ever make sense for the arc process to bind >
> > > std::numeric_limits<uint32_t>::max() bytes of shared memory?
> > 
> > No, the length is about few times frame size. I added check for "length >
> std::numeric_limits<size_t>::max()".
> 
> So... why not just pass it as uint32_t in the IDL? =)

Done. Similar cases are changed to uint32 as well.

[jam, 2016-04-08 16:19:32]

lgtm

[dcheng, 2016-04-10 05:01:23]

https://codereview.chromium.org/1641353003/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_service_launcher.cc (right):

https://codereview.chromium.org/1641353003/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_service_launcher.cc:34:
arc_service_manager_->AddService(make_scoped_ptr(
BTW chromeos directory uses base::WrapUnique and std::unique_ptr now.

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
components/arc/common/video_accelerator.mojom:18: [Extensible]
Extensible means that mojo won't do any verifications check on the value right?
Any particular reason that all these enums are extensible?

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
components/arc/common/video_accelerator.mojom:55:
Initialize@0(ArcVideoAcceleratorConfig config) => (bool result);
Please add comments to the interface for what the various methods are for / what
the arguments are (especially things like index)

[edzyaani9093, 2016-04-10 07:31:48]

edzyaani9093@gmail.com changed reviewers:
+ edzyaani9093@gmail.com

[edzyaani9093, 2016-04-10 08:21:41]

lgtm

[kcwu, 2016-04-11 04:07:32]

https://codereview.chromium.org/1641353003/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_service_launcher.cc (right):

https://codereview.chromium.org/1641353003/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_service_launcher.cc:34:
arc_service_manager_->AddService(make_scoped_ptr(
On 2016/04/10 05:01:23, dcheng wrote:
> BTW chromeos directory uses base::WrapUnique and std::unique_ptr now.

Thanks for the information, I didn't know WrapUnique. I'd prefer a separate CL
to change ArcServiceManager.

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
components/arc/common/video_accelerator.mojom:18: [Extensible]
On 2016/04/10 05:01:23, dcheng wrote:
> Extensible means that mojo won't do any verifications check on the value
right?
> Any particular reason that all these enums are extensible?

I could remove Extensible from Error. Other enums are indeed extensible.
HalPixelFormatExtension may expand with new formats. BufferFlag are bitmask used
in BufferMetadata.flags.

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
components/arc/common/video_accelerator.mojom:55:
Initialize@0(ArcVideoAcceleratorConfig config) => (bool result);
On 2016/04/10 05:01:23, dcheng wrote:
> Please add comments to the interface for what the various methods are for /
what
> the arguments are (especially things like index)

This mojom file is the clone of ArcVideoAccelerator interface. Since
ArcVideoAccelerator is the canonical definition, I added file comments that
refers to ArcVideoAccelerator for detail. Is this okay?

[dcheng, 2016-04-11 04:26:32]

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
components/arc/common/video_accelerator.mojom:18: [Extensible]
On 2016/04/11 04:07:32, kcwu wrote:
> On 2016/04/10 05:01:23, dcheng wrote:
> > Extensible means that mojo won't do any verifications check on the value
> right?
> > Any particular reason that all these enums are extensible?
> 
> I could remove Extensible from Error. Other enums are indeed extensible.
> HalPixelFormatExtension may expand with new formats. BufferFlag are bitmask
used
> in BufferMetadata.flags.
> 

Can you help me understand why HalPixelFormatExtension needs to be extensible?
If we need to handle new values, wouldn't we just add them to the enum
definition? It doesn't look like a bitmask to me.

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
components/arc/common/video_accelerator.mojom:55:
Initialize@0(ArcVideoAcceleratorConfig config) => (bool result);
On 2016/04/11 04:07:32, kcwu wrote:
> On 2016/04/10 05:01:23, dcheng wrote:
> > Please add comments to the interface for what the various methods are for /
> what
> > the arguments are (especially things like index)
> 
> This mojom file is the clone of ArcVideoAccelerator interface. Since
> ArcVideoAccelerator is the canonical definition, I added file comments that
> refers to ArcVideoAccelerator for detail. Is this okay?

Sounds good.

[kcwu, 2016-04-11 04:44:33]

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
components/arc/common/video_accelerator.mojom:18: [Extensible]
On 2016/04/11 04:26:32, dcheng wrote:
> On 2016/04/11 04:07:32, kcwu wrote:
> > On 2016/04/10 05:01:23, dcheng wrote:
> > > Extensible means that mojo won't do any verifications check on the value
> > right?
> > > Any particular reason that all these enums are extensible?
> > 
> > I could remove Extensible from Error. Other enums are indeed extensible.
> > HalPixelFormatExtension may expand with new formats. BufferFlag are bitmask
> used
> > in BufferMetadata.flags.
> > 
> 
> Can you help me understand why HalPixelFormatExtension needs to be extensible?
> If we need to handle new values, wouldn't we just add them to the enum
> definition? It doesn't look like a bitmask to me.

I don't fully understand "Extensible". Luis suggested adding "Extensible" if
enums will need modification. I thought "modification" includes "adding new
values". Isn't it?
https://codereview.chromium.org/1641353003/diff/200001/components/arc/common/...

[dcheng, 2016-04-11 04:56:54]

lgtm

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1641353003/diff/340001/components/arc/common/...
components/arc/common/video_accelerator.mojom:18: [Extensible]
On 2016/04/11 04:44:33, kcwu wrote:
> On 2016/04/11 04:26:32, dcheng wrote:
> > On 2016/04/11 04:07:32, kcwu wrote:
> > > On 2016/04/10 05:01:23, dcheng wrote:
> > > > Extensible means that mojo won't do any verifications check on the value
> > > right?
> > > > Any particular reason that all these enums are extensible?
> > > 
> > > I could remove Extensible from Error. Other enums are indeed extensible.
> > > HalPixelFormatExtension may expand with new formats. BufferFlag are
bitmask
> > used
> > > in BufferMetadata.flags.
> > > 
> > 
> > Can you help me understand why HalPixelFormatExtension needs to be
extensible?
> > If we need to handle new values, wouldn't we just add them to the enum
> > definition? It doesn't look like a bitmask to me.
> 
> I don't fully understand "Extensible". Luis suggested adding "Extensible" if
> enums will need modification. I thought "modification" includes "adding new
> values". Isn't it?
>
https://codereview.chromium.org/1641353003/diff/200001/components/arc/common/...

Hmm, this is surprising to me. In the interest of unblocking this CL, I'll start
a separate thread to resolve this (sammc@ pointed me at
https://codereview.chromium.org/1618963006, which certainly seems to indicate
this is the case)

[Luis Héctor Chávez, 2016-04-14 21:42:14]

Btw, why hasn't this side of the change landed yet?

https://codereview.chromium.org/1641353003/diff/360001/components/arc/common/...
File components/arc/common/video.mojom (left):

https://codereview.chromium.org/1641353003/diff/360001/components/arc/common/...
components/arc/common/video.mojom:5: module arc;
module arc.mojom;

https://codereview.chromium.org/1885683005/

https://codereview.chromium.org/1641353003/diff/360001/components/arc/common/...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1641353003/diff/360001/components/arc/common/...
components/arc/common/video_accelerator.mojom:8: module arc;
module arc.mojom;

https://codereview.chromium.org/1885683005/

[kcwu, 2016-04-15 04:22:00]

On 2016/04/14 21:42:14, Luis Héctor Chávez wrote:
> Btw, why hasn't this side of the change landed yet?

This CL depends on http://crrev.com/1549473002/. And 1549473002 depends on
http://crrev.com/1816203003 and http://crrev.com/1822983002.

[kcwu, 2016-04-15 04:42:24]

Hi @jam,

In order to cut down dependency of this CL (this CL depends on
http://crrev.com/1549473002/. And 1549473002 depends on
http://crrev.com/1816203003 and http://crrev.com/1822983002.)
We decided to move chrome/gpu/arc_video_accelerator.h from 1549473002 (which is
already approved) to this CL. So this CL could be merged independently.

Could you take a take a look? What I did are
- rebase to ToT
- move arc_video_accelerator.h
- move chrome/gpu directoy stubs (BUILD.gn, DEPS, OWNERS, chrome_gpu.gypi)
No other code changes.

[kcwu, 2016-04-15 13:57:32]

Patchset #21 (id:400001) has been deleted

[kcwu, 2016-04-15 13:57:49]

Patchset #21 (id:420001) has been deleted

[kcwu, 2016-04-15 14:46:04]

Patchset #22 (id:460001) has been deleted

[kcwu, 2016-04-15 14:46:24]

Patchset #21 (id:440001) has been deleted

[jam, 2016-04-15 16:14:00]

On 2016/04/15 04:42:24, kcwu wrote:
> Hi @jam,
> 
> In order to cut down dependency of this CL (this CL depends on
> http://crrev.com/1549473002/. And 1549473002 depends on
> http://crrev.com/1816203003 and http://crrev.com/1822983002.)
> We decided to move chrome/gpu/arc_video_accelerator.h from 1549473002 (which
is
> already approved) to this CL. So this CL could be merged independently.
> 
> Could you take a take a look? What I did are
> - rebase to ToT
> - move arc_video_accelerator.h
> - move chrome/gpu directoy stubs (BUILD.gn, DEPS, OWNERS, chrome_gpu.gypi)
> No other code changes.

still lgtm

[kcwu, 2016-04-15 17:35:15]

The CQ bit was checked by kcwu@chromium.org

[kcwu, 2016-04-15 17:35:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
owenlin@chromium.org, sievers@chromium.org, lhchavez@chromium.org,
edzyaani9093@gmail.com, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1641353003/#ps480001
(title: "fix build")

[commit-bot: I haz the power, 2016-04-15 17:35:45]

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

[kcwu, 2016-04-15 17:35:59]

The CQ bit was unchecked by kcwu@chromium.org

[kcwu, 2016-04-16 02:35:49]

The CQ bit was checked by kcwu@chromium.org

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

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

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

Description was changed from

==========
Implements GpuArcVideoService and its host
    
In the GPU process, GpuArcVideoService (arc::VideoHost) is registered
via content's gpu client hook. It also manages additional IPC channel
per accelerator and dispatches calls to ArcVideoAccelerator.
    
In the browser process, GpuArcVideoServiceHost listens events of
ArcBridgeService and pass to GpuArcVideoService.
    
BUG=b/25057601
TEST=video from arc playbacks
==========

to

==========
Implements GpuArcVideoService and its host
    
In the GPU process, GpuArcVideoService (arc::VideoHost) is registered
via content's gpu client hook. It also manages additional IPC channel
per accelerator and dispatches calls to ArcVideoAccelerator.
    
In the browser process, GpuArcVideoServiceHost listens events of
ArcBridgeService and pass to GpuArcVideoService.
    
BUG=b/25057601
TEST=video from arc playbacks
==========

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

Committed patchset #21 (id:480001)

[commit-bot: I haz the power, 2016-04-16 02:42:17]

Description was changed from

==========
Implements GpuArcVideoService and its host
    
In the GPU process, GpuArcVideoService (arc::VideoHost) is registered
via content's gpu client hook. It also manages additional IPC channel
per accelerator and dispatches calls to ArcVideoAccelerator.
    
In the browser process, GpuArcVideoServiceHost listens events of
ArcBridgeService and pass to GpuArcVideoService.
    
BUG=b/25057601
TEST=video from arc playbacks
==========

to

==========
Implements GpuArcVideoService and its host

In the GPU process, GpuArcVideoService (arc::VideoHost) is registered
via content's gpu client hook. It also manages additional IPC channel
per accelerator and dispatches calls to ArcVideoAccelerator.

In the browser process, GpuArcVideoServiceHost listens events of
ArcBridgeService and pass to GpuArcVideoService.

BUG=b/25057601
TEST=video from arc playbacks

Committed: https://crrev.com/e5654af3fec812e02719216a0014698b235cd52f
Cr-Commit-Position: refs/heads/master@{#387799}
==========

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

Patchset 21 (id:??) landed as
https://crrev.com/e5654af3fec812e02719216a0014698b235cd52f
Cr-Commit-Position: refs/heads/master@{#387799}