Implement GpuArcVideoService for arc video accelerator

Implement GpuArcVideoService for arc video accelerator

GpuArcVideoService creates new channel and dispatch IPC to
ArcVideoAccelerator.

BUG=b/25057601

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

[kcwu, 2015-11-20 07:28:13]

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

[Owen Lin, 2015-11-23 05:31:07]

Just first batch. Need to understand more to do the review.

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_arc_accelerator_host.cc (right):

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.cc:90: while
(pending_requests_.empty()) {
while (!empty)?

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_arc_accelerator_host.h (right):

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:19: // kcwu: Should this
class be folded into GpuProcessHost?
TODO(kcwu)?
Sounds good to me.

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:33: void
CreateArcAccelerator(uint32_t device_type, const CreatedCallback& cb);
Why need to pass device_type. I would we will do the initialization later. For
the ArcBridgeService, the job is only creating the channel.

Maybe rename it to CreateArcVideoAcceleratorChannel?

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_process_launch_causes.h (right):

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_process_launch_causes.h:11: // TODO(kcwu)
I think we need this in the same CL.

[kcwu, 2015-11-23 08:11:48]

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_arc_accelerator_host.cc (right):

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.cc:90: while
(pending_requests_.empty()) {
On 2015/11/23 05:31:06, Owen Lin wrote:
> while (!empty)?

Done.

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_arc_accelerator_host.h (right):

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:19: // kcwu: Should this
class be folded into GpuProcessHost?
On 2015/11/23 05:31:06, Owen Lin wrote:
> TODO(kcwu)?
> Sounds good to me.

Now the object calling relationship is
(browser side) GpuArcAcceleratorHost -> GpuChannelHost ->GpuProcessHost
(gpu side) GpuChannelManager -> GpuChannel -> GpuArcAccelerator

If moving GpuArcAcceleratorHost into GpuProcessHost or child of GpuProcessHost
Pros:
 - GpuArcAcceleratorHost does similar to GpuChannelHost, so it sounds good to be
parallel to GpuChannelHost.
Cons:
 - Adding responsibility of GpuProcessHost and GpuChannelManager. I don't know
whether it is good design.
 - We may need to duplicate some code what GpuChannelHost does, to handle
something like BrowserGpuChannelHostFactory.

I am not sure what is better design.

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:33: void
CreateArcAccelerator(uint32_t device_type, const CreatedCallback& cb);
On 2015/11/23 05:31:06, Owen Lin wrote:
> Why need to pass device_type. I would we will do the initialization later. For
> the ArcBridgeService, the job is only creating the channel.
> 
> Maybe rename it to CreateArcVideoAcceleratorChannel?

The main reason is I want to reduce one IPC round trip for initialize. If the
initial latency is not a concern, I agreed it is cleaner to have a dedicate IPC
message for initialize.

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_process_launch_causes.h (right):

https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_process_launch_causes.h:11: // TODO(kcwu)
On 2015/11/23 05:31:06, Owen Lin wrote:
> I think we need this in the same CL.

Yes, it was "TODO in this CL'.
Done.

[Pawel Osciak, 2015-11-24 10:41:14]

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:825:
IPC_MESSAGE_CONTROL1(GpuMsg_CreateArcAccelerator, int32_t /* device_type */)
Perhaps we should use an enum for device type? IPC will do a range check on it
for us then.

Here and for other such values below perhaps...

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:826:
IPC_MESSAGE_CONTROL0(GpuMsg_ShutdownArcAccelerators)
Would this allow one instance to shut down all the others? If so, this would be
an issue if we are aiming at instance isolation?

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:832:
IPC_SYNC_MESSAGE_CONTROL5_1(ArcAcceleratorMsg_BindSharedBuffer,
s/SharedBuffer/SharedMemory/ ?

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:836: size_t /* offset */,
off_t?

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:838: uint32_t /* result */)
Do we need more results than a bool success/false?

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:849: uint32_t /* port */,
Do we need port if we specify both in and out counts?

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:859: uint32_t /* result */)
Do we need an Error notification message that would be sent back to client,
similar to NotifyError in VDA?

[kcwu, 2015-11-24 13:02:43]

Hi Pawel,

Could you take a look the discussion first at line 19 and 33 of
https://codereview.chromium.org/1451353002/diff/20001/content/common/gpu/clie...
?

They are more about design decision.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:826:
IPC_MESSAGE_CONTROL0(GpuMsg_ShutdownArcAccelerators)
On 2015/11/24 10:41:14, Pawel Osciak wrote:
> Would this allow one instance to shut down all the others? If so, this would
be
> an issue if we are aiming at instance isolation?

This message is only used when Chrome browser process want to shutdown gpu
process. IPC messages sending by instance directly are start at line 832.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:832:
IPC_SYNC_MESSAGE_CONTROL5_1(ArcAcceleratorMsg_BindSharedBuffer,
On 2015/11/24 10:41:14, Pawel Osciak wrote:
> s/SharedBuffer/SharedMemory/ ?

The message name and following types just follow the ArcVideoAccelerator
interface.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:849: uint32_t /* port */,
On 2015/11/24 10:41:14, Pawel Osciak wrote:
> Do we need port if we specify both in and out counts?

Ah, my variable name seems confusing.
This is a sync message, |in_count| is the request, |out_count| is the response.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:859: uint32_t /* result */)
On 2015/11/24 10:41:14, Pawel Osciak wrote:
> Do we need an Error notification message that would be sent back to client,
> similar to NotifyError in VDA?

Yes, there is a new callback onError(). I will add it when I sync ipc messages
with the new interface.

[Owen Lin, 2015-11-26 03:54:41]

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_arc_accelerator_host.cc (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.cc:91:
pending_requests_.front().Run(IPC::ChannelHandle(), 0);
I thought result "0" means success, but it is actually aborted.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_arc_accelerator_host.h (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:19: // kcwu: Should this
class be folded into GpuProcessHost?
I changed my mind. Let's keep this class. But how about changing the name? The
name is kind of misleading. It is not an accelerator at all.

Let's make it a GpuArcVideoService which is a service lived in GPU process to
server video requests from Arc.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:35: void
CreateArcAccelerator(uint32_t device_type, const CreatedCallback& cb);
I would prefer keep it cleaner for know. If we do have performance issue we can
measure the improvement and justified the change.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:49:
scoped_refptr<GpuChannelHost> gpu_channel_host_;
As we discussed, Can we just use GpuProcessHost? What's the benefit of using
GpuChannel?

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.h:233: void OnShutdownArcAccelerators();
Can we route the message to GpuArcVideoService? Only create is from GpuChannel
or GpuProcess.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.h:284: scoped_ptr<GpuArcAccelerator>
arc_accelerator_;
Please consider move this to GpuProcess.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/gpu_arc_accelerator.h (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/gpu_arc_accelerator.h:33: class GpuArcAccelerator :
public base::NonThreadSafe {
How about refactor this class into two seperate classes?

GpuArcVideoService and GpuArcAccelerator.

GpuArcVideoService is listen on the GpuChannel and is responsible for the
channel creation.
Each GpuArcAccelerator is corresponding to one ArcVideoAccelerator and listen on
the new created channel.

[kcwu, 2015-11-26 10:34:43]

major changes:
1. I moved GpuArcVideoServiceHost to content/browser for better dependency, but
I am still not sure whether I handled the path and dependency correctly.

2. https://codereview.chromium.org/1456133005 is merged into this CL.

3. GpuArcAccelerator is renamed to GpuArcVideoService.
4. ipc messages followed Owen's new interface

Remain tasks:
1. Considering to use GpuProcessHost directly instead of GpuChannelHost.
2. migrate to mojo

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_arc_accelerator_host.cc (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.cc:91:
pending_requests_.front().Run(IPC::ChannelHandle(), 0);
On 2015/11/26 03:54:40, Owen Lin wrote:
> I thought result "0" means success, but it is actually aborted.

Acknowledged.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_arc_accelerator_host.h (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:19: // kcwu: Should this
class be folded into GpuProcessHost?
On 2015/11/26 03:54:41, Owen Lin wrote:
> I changed my mind. Let's keep this class. But how about changing the name? The
> name is kind of misleading. It is not an accelerator at all.
> 
> Let's make it a GpuArcVideoService which is a service lived in GPU process to
> server video requests from Arc.

Done.
Nice idea. This name is better.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:35: void
CreateArcAccelerator(uint32_t device_type, const CreatedCallback& cb);
On 2015/11/26 03:54:40, Owen Lin wrote:
> I would prefer keep it cleaner for know. If we do have performance issue we
can
> measure the improvement and justified the change. 

Done.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_arc_accelerator_host.h:49:
scoped_refptr<GpuChannelHost> gpu_channel_host_;
On 2015/11/26 03:54:41, Owen Lin wrote:
> As we discussed, Can we just use GpuProcessHost? What's the benefit of using
> GpuChannel?

GpuChannelHost and BrowserGpuChannelHostFactory do some life-cycle, threading,
and state management.
For example, all major methods of GpuProcessHost need to be invoked on IO thread
while we are on UI thread. And we have to take more care about gpu crash and
relaunch, etc.

I estimate using GpuProcessHost will need slightly more code. I will try and
compare later.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.h:233: void OnShutdownArcAccelerators();
On 2015/11/26 03:54:41, Owen Lin wrote:
> Can we route the message to GpuArcVideoService? Only create is from GpuChannel
> or GpuProcess.

No. This is for terminating GpuArcVideoService. For example, Android is shutting
down or the whole chrome is closing. We need a method to terminate
GpuArcVideoService itself, not only each accelerator.

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/gpu_arc_accelerator.h (right):

https://codereview.chromium.org/1451353002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/gpu_arc_accelerator.h:33: class GpuArcAccelerator :
public base::NonThreadSafe {
On 2015/11/26 03:54:41, Owen Lin wrote:
> How about refactor this class into two seperate classes?
> 
> GpuArcVideoService and GpuArcAccelerator.
> 
> GpuArcVideoService is listen on the GpuChannel and is responsible for the
> channel creation.
> Each GpuArcAccelerator is corresponding to one ArcVideoAccelerator and listen
on
> the new created channel.

The latter responsibility is called "Client" (a bad name, I think) now. Let me
rename it as GpuArcVideoService::AcceleratorStub.

[kcwu, 2015-11-27 11:50:50]

The CL is updated to use GpuProcessHost.

[kcwu, 2015-11-27 12:03:43]

Description was changed from

==========
Implement GpuArcAccelerator, the ipc handler for arc accelerator

GpuArcAccelerator creates new socket and dispatch IPC to
ArcVideoAccelerator.

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

to

==========
Implement GpuArcVideoService for arc video accelerator

GpuArcVideoService creates new channel and dispatch IPC to
ArcVideoAccelerator.

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

[Owen Lin, 2015-12-07 09:05:23]

https://codereview.chromium.org/1451353002/diff/80001/components/arc/arc_brid...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1451353002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:71: virtual void
OnCreateArcVideoAcceleratorConnection() {}
How about rename it as OnRequestArcVideoAcceleratorConnection()?
// Called whenever there is a request to create a video accelerator connection.

In addition, Should we use the observer pattern to support the function?
Observer sounds like an optional. But it is not the case.

How about a ConnectionProvider or ConnectionFactory?

For example:

class ArcBridgeService {
public:
  class ConnectionProvider {
    IPC::ChannelHandle buildConnection();
  };

  bool requestConnection(enum ConnectionType, IPC::ChannelHandle &output) {
  }

  void registerConnectionProvider(ConnectionType, ConnectionProvider) {
  }
};

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_arc_video_service_host.cc:19: const
GpuProcessHost::CreateArcVideoAcceleratorChannelCallback& callback) {
Can we unify the naming? Is connection == channel?

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_arc_video_service_host.cc:36:
weak_factory_.InvalidateWeakPtrs();
I thought it will be called automatically.

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_arc_video_service_host.cc:57: case
arc::ArcBridgeService::State::STOPPING:
When will this happened? Will it happens when we restart the android container?

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:847: if
(create_arc_video_accelerator_channel_requests_.empty())
Would you like to log the error situation?

https://codereview.chromium.org/1451353002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/gpu_arc_video_service.cc:221: void
GpuArcVideoService::RemoveAllClientsOnArcThread(
Can we just depends on OnChannelError() to remove invalid client?

https://codereview.chromium.org/1451353002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/gpu_arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/gpu_arc_video_service.h:50: void CreateChannel();
Can we have a callback? So that we don't need to depends on gpu_channel_manager.

[kcwu, 2015-12-10 10:17:37]

rebased with new ArcBridgeService; addressed Owen's comments

https://codereview.chromium.org/1451353002/diff/80001/components/arc/arc_brid...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1451353002/diff/80001/components/arc/arc_brid...
components/arc/arc_bridge_service.h:71: virtual void
OnCreateArcVideoAcceleratorConnection() {}
On 2015/12/07 09:05:23, Owen Lin wrote:
> How about rename it as OnRequestArcVideoAcceleratorConnection()?
> // Called whenever there is a request to create a video accelerator
connection.
> 
> In addition, Should we use the observer pattern to support the function?
> Observer sounds like an optional. But it is not the case.
> 
> How about a ConnectionProvider or ConnectionFactory?
> 
> For example:
> 
> class ArcBridgeService {
> public:
>   class ConnectionProvider {
>     IPC::ChannelHandle buildConnection();
>   };
> 
>   bool requestConnection(enum ConnectionType, IPC::ChannelHandle &output) {
>   }
> 
>   void registerConnectionProvider(ConnectionType, ConnectionProvider) {
>   }
> };
> 

Revised to use new ArcBridgeService's new observer. PTAL.

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_arc_video_service_host.cc:19: const
GpuProcessHost::CreateArcVideoAcceleratorChannelCallback& callback) {
On 2015/12/07 09:05:23, Owen Lin wrote:
> Can we unify the naming? Is connection == channel?

Done.

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_arc_video_service_host.cc:36:
weak_factory_.InvalidateWeakPtrs();
On 2015/12/07 09:05:23, Owen Lin wrote:
> I thought it will be called automatically. 

Done.

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_arc_video_service_host.cc:57: case
arc::ArcBridgeService::State::STOPPING:
On 2015/12/07 09:05:23, Owen Lin wrote:
> When will this happened? Will it happens when we restart the android
container?

Yes

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1451353002/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:847: if
(create_arc_video_accelerator_channel_requests_.empty())
On 2015/12/07 09:05:23, Owen Lin wrote:
> Would you like to log the error situation?

Done.

https://codereview.chromium.org/1451353002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/gpu_arc_video_service.cc:221: void
GpuArcVideoService::RemoveAllClientsOnArcThread(
On 2015/12/07 09:05:23, Owen Lin wrote:
> Can we just depends on OnChannelError() to remove invalid client?

If doing so, the client inside arc instance must be destroyed before this class.
I don't know the order yet and don't want to depend on that.

https://codereview.chromium.org/1451353002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/gpu_arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/gpu_arc_video_service.h:50: void CreateChannel();
On 2015/12/07 09:05:23, Owen Lin wrote:
> Can we have a callback? So that we don't need to depends on
gpu_channel_manager.

Revised to use only IPC::Sender interface.

[Owen Lin, 2015-12-15 03:04:06]

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:121: virtual void
OnRequestArcVideoAcceleratorChannel() {}
I still don't think we should use the name/pattern "Observer" here.

Observer should be optional to arc_bridge_service and can be multiple. It is
perfect to notify the state change inside the container. But I think, here is
not the case. (What if we add two listeners?) We need exactly ONE "Service" to
handle the request.

Please change it to a service provider, e.g., VideoServiceProvider or simply
VideoService. Also change AddVideoServiceObserver to SetVideoService(...). How
do you think?

https://codereview.chromium.org/1451353002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/100001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.h:53: void CreateChannel();
I mean if we have a callback for this function. It is easy to understand (and
more flexible) How the channel is returned. You can send the message inside the
callback.

typedef Callback<void(Handle)> CreateChannelCallback;

void CreateChannel(const CreateChannelCallback& callback);

[Pawel Osciak, 2015-12-16 10:17:58]

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:121: virtual void
OnRequestArcVideoAcceleratorChannel() {}
On 2015/12/15 03:04:06, Owen Lin wrote:
> I still don't think we should use the name/pattern "Observer" here.
> 
> Observer should be optional to arc_bridge_service and can be multiple. It is
> perfect to notify the state change inside the container. But I think, here is
> not the case. (What if we add two listeners?) We need exactly ONE "Service" to
> handle the request.
> 
> Please change it to a service provider, e.g., VideoServiceProvider or simply
> VideoService. Also change AddVideoServiceObserver to SetVideoService(...). How
> do you think?

+1, I'd prefer this as well.

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_impl.cc:165: const IPC::ChannelHandle& handle)
{
Do we need to verify that handle is not IPC::ChannelHandle() created by
~GpuArcVideoServiceHost()?

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_impl.cc:184: return false;
Do we need to close also before returning from l.170?

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

https://codereview.chromium.org/1451353002/diff/100001/components/arc/common/...
components/arc/common/arc_bridge.mojom:144:
OnRequestArcVideoAcceleratorChannel();
Please document.

https://codereview.chromium.org/1451353002/diff/100001/components/arc/common/...
components/arc/common/arc_bridge.mojom:183:
NotifyVideoAcceleratorChannelCreated(handle channel_handle);
Please document.

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:49: DCHECK(bridge_service);
Should we actually if(bridge_service) here (and in l.91)? In destructor we if(),
but here we only DCHECK?

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:93:
ignore_result(bridge_service->NotifyVideoAcceleratorChannelCreated(handle));
Is there any other caller that doesn't ignore_result? If not, should we just
make it void?

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:20: class
GpuArcVideoServiceHost
Please add a short description of what this class does.

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:42: int num_pending_request_;
Please document.

s/num_pending_request_/num_pending_requests_/

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.h (right):

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.h:160: void
CreateArcVideoAcceleratorChannel(
Please document.

[kcwu, 2015-12-16 14:05:33]

addressed all comments and split out ArcVideoAccelerator interface. PTAL

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service.h:121: virtual void
OnRequestArcVideoAcceleratorChannel() {}
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> On 2015/12/15 03:04:06, Owen Lin wrote:
> > I still don't think we should use the name/pattern "Observer" here.
> > 
> > Observer should be optional to arc_bridge_service and can be multiple. It is
> > perfect to notify the state change inside the container. But I think, here
is
> > not the case. (What if we add two listeners?) We need exactly ONE "Service"
to
> > handle the request.
> > 
> > Please change it to a service provider, e.g., VideoServiceProvider or simply
> > VideoService. Also change AddVideoServiceObserver to SetVideoService(...).
How
> > do you think?
> 
> +1, I'd prefer this as well.

Done.

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_impl.cc:165: const IPC::ChannelHandle& handle)
{
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Do we need to verify that handle is not IPC::ChannelHandle() created by
> ~GpuArcVideoServiceHost()?

Why? Both ArcBridgeServiceImpl and GpuArcVideoServiceHost are in trusted browser
process.

https://codereview.chromium.org/1451353002/diff/100001/components/arc/arc_bri...
components/arc/arc_bridge_service_impl.cc:184: return false;
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Do we need to close also before returning from l.170?

No need to close since it is already wrapped into scoper in line 177. Line 183
is removed.

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

https://codereview.chromium.org/1451353002/diff/100001/components/arc/common/...
components/arc/common/arc_bridge.mojom:144:
OnRequestArcVideoAcceleratorChannel();
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Please document.

Done.

https://codereview.chromium.org/1451353002/diff/100001/components/arc/common/...
components/arc/common/arc_bridge.mojom:183:
NotifyVideoAcceleratorChannelCreated(handle channel_handle);
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Please document.

Done.

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:49: DCHECK(bridge_service);
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Should we actually if(bridge_service) here (and in l.91)? In destructor we
if(),
> but here we only DCHECK?

Basically it is bug if bridge_service is null, hence the DCHECK.

I usually treat dtor as special case -- I prefer do-nothing-object (merely
created and destroyed but no methods called) don't crash. This may be helpful
for writing tests. Does it make sense?

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:93:
ignore_result(bridge_service->NotifyVideoAcceleratorChannelCreated(handle));
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Is there any other caller that doesn't ignore_result? If not, should we just
> make it void?

Done.

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:20: class
GpuArcVideoServiceHost
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Please add a short description of what this class does.

Done.

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:42: int num_pending_request_;
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Please document.
> 
> s/num_pending_request_/num_pending_requests_/

Done.

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.h (right):

https://codereview.chromium.org/1451353002/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.h:160: void
CreateArcVideoAcceleratorChannel(
On 2015/12/16 10:17:58, Pawel Osciak wrote:
> Please document.

Done.

https://codereview.chromium.org/1451353002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/100001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.h:53: void CreateChannel();
On 2015/12/15 03:04:06, Owen Lin wrote:
> I mean if we have a callback for this function. It is easy to understand (and
> more flexible) How the channel is returned. You can send the message inside
the
> callback.
> 
> typedef Callback<void(Handle)> CreateChannelCallback;
> 
> void CreateChannel(const CreateChannelCallback& callback);

Done.

[Luis Héctor Chávez, 2015-12-16 16:37:22]

https://codereview.chromium.org/1451353002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right):

https://codereview.chromium.org/1451353002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:89:
scoped_ptr<content::GpuArcVideoServiceHost> gpu_arc_video_service_host_;
Is it possible to inject the GpuArcVideoServiceHost into the ArcserviceManager
so ChromeBrowserMain does not have to directly own it?

You can create an opaque type in components/arc with just a constructor,
destructor and an Initialize method.

https://codereview.chromium.org/1451353002/diff/180001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1451353002/diff/180001/components/arc/common/...
components/arc/common/arc_bridge.mojom:228:
OnRequestArcVideoAcceleratorChannel();
Instead of adding methods directly to ArcBridge, can you create a pair of
interfaces in video.mojom?

that should contain something like:

interface VideoHost {
  RequestArcVideoAcceleratorChannel() => (handle channel_handle);
};

interface VideoInstance {
  Init(VideoHost host_ptr);
};

And then only add OnVideoInstanceReady(VideoInstance instance_ptr); here?

That way ArcBridgeService does not have to know anything about video or how to
send the IPC channel.

https://codereview.chromium.org/1451353002/diff/180001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/180001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:24: public
arc::ArcBridgeService::VideoService {
instead of an arc::ArcBridgeService::VideoService, can this be arc::VideoHost?

[kcwu, 2015-12-22 11:58:25]

https://codereview.chromium.org/1451353002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right):

https://codereview.chromium.org/1451353002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.h:89:
scoped_ptr<content::GpuArcVideoServiceHost> gpu_arc_video_service_host_;
On 2015/12/16 16:37:22, Luis Héctor Chávez wrote:
> Is it possible to inject the GpuArcVideoServiceHost into the ArcserviceManager
> so ChromeBrowserMain does not have to directly own it?
> 
> You can create an opaque type in components/arc with just a constructor,
> destructor and an Initialize method.

Done.
I am not fully sure what you mean. PTAL.

https://codereview.chromium.org/1451353002/diff/180001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1451353002/diff/180001/components/arc/common/...
components/arc/common/arc_bridge.mojom:228:
OnRequestArcVideoAcceleratorChannel();
On 2015/12/16 16:37:22, Luis Héctor Chávez wrote:
> Instead of adding methods directly to ArcBridge, can you create a pair of
> interfaces in video.mojom?
> 
> that should contain something like:
> 
> interface VideoHost {
>   RequestArcVideoAcceleratorChannel() => (handle channel_handle);
> };
> 
> interface VideoInstance {
>   Init(VideoHost host_ptr);
> };
> 
> And then only add OnVideoInstanceReady(VideoInstance instance_ptr); here?
> 
> That way ArcBridgeService does not have to know anything about video or how to
> send the IPC channel.

Done.
Since your refactoring was reverted, I try to do the similar thing and need to
rebase with your refactoring.

https://codereview.chromium.org/1451353002/diff/180001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/180001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:24: public
arc::ArcBridgeService::VideoService {
On 2015/12/16 16:37:22, Luis Héctor Chávez wrote:
> instead of an arc::ArcBridgeService::VideoService, can this be arc::VideoHost?

Done.

[Luis Héctor Chávez, 2015-12-23 00:02:53]

You probably want to rebase now.

https://codereview.chromium.org/1451353002/diff/200001/components/arc.gypi
File components/arc.gypi (right):

https://codereview.chromium.org/1451353002/diff/200001/components/arc.gypi#ne...
components/arc.gypi:32: 'arc/arc_video_service.h',
Can you move these to arc/video/?

https://codereview.chromium.org/1451353002/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/1451353002/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service_impl.cc:207: // refactoring.
This is done.

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

https://codereview.chromium.org/1451353002/diff/200001/components/arc/common/...
components/arc/common/arc_bridge.mojom:231: OnVideoInstanceReady(VideoInstance
instance_ptr);
OnVideoInstanceReady@107.

https://codereview.chromium.org/1451353002/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:52:
bridge_service->AddObserver(this);
if (arc_bridge_service->video_instance())
  OnVideoInstanceReady();

https://codereview.chromium.org/1451353002/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:85:
base::Bind(&GpuArcVideoServiceHost::ReplyChannelCreated,
How about you change ReplyChannelCreated to also receive a callback as first
parameter and do

base::Bind(&GpuArcVideoServiceHost::ReplyChannelCreated,
           weak_factory_.GetWeakPtr(),
           callback);

?

That way you don't need to store the callbacks in a queue.

[kcwu, 2015-12-23 01:05:23]

rebased

https://codereview.chromium.org/1451353002/diff/200001/components/arc.gypi
File components/arc.gypi (right):

https://codereview.chromium.org/1451353002/diff/200001/components/arc.gypi#ne...
components/arc.gypi:32: 'arc/arc_video_service.h',
On 2015/12/23 00:02:53, Luis Héctor Chávez wrote:
> Can you move these to arc/video/?

Done.

https://codereview.chromium.org/1451353002/diff/200001/components/arc/arc_bri...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/1451353002/diff/200001/components/arc/arc_bri...
components/arc/arc_bridge_service_impl.cc:207: // refactoring.
On 2015/12/23 00:02:53, Luis Héctor Chávez wrote:
> This is done.

Done.

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

https://codereview.chromium.org/1451353002/diff/200001/components/arc/common/...
components/arc/common/arc_bridge.mojom:231: OnVideoInstanceReady(VideoInstance
instance_ptr);
On 2015/12/23 00:02:53, Luis Héctor Chávez wrote:
> OnVideoInstanceReady@107.

Done.

https://codereview.chromium.org/1451353002/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:52:
bridge_service->AddObserver(this);
On 2015/12/23 00:02:53, Luis Héctor Chávez wrote:
> if (arc_bridge_service->video_instance())
>   OnVideoInstanceReady();

Done.

https://codereview.chromium.org/1451353002/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:85:
base::Bind(&GpuArcVideoServiceHost::ReplyChannelCreated,
On 2015/12/23 00:02:53, Luis Héctor Chávez wrote:
> How about you change ReplyChannelCreated to also receive a callback as first
> parameter and do
> 
> base::Bind(&GpuArcVideoServiceHost::ReplyChannelCreated,
>            weak_factory_.GetWeakPtr(),
>            callback);
> 
> ?
> 
> That way you don't need to store the callbacks in a queue.

Done.

[lhc(google), 2015-12-23 01:13:24]

lhchavez@google.com changed reviewers:
+ hidehiko@chromium.org, lhchavez@google.com

[lhc(google), 2015-12-23 01:13:25]

components/arc lgtm. Defer OWNER approval to hidehiko@.

[Pawel Osciak, 2015-12-23 06:24:01]

https://chromiumcodereview.appspot.com/1451353002/diff/100001/components/arc/...
File components/arc/arc_bridge_service_impl.cc (right):

https://chromiumcodereview.appspot.com/1451353002/diff/100001/components/arc/...
components/arc/arc_bridge_service_impl.cc:165: const IPC::ChannelHandle& handle)
{
On 2015/12/16 14:05:32, kcwu wrote:
> On 2015/12/16 10:17:58, Pawel Osciak wrote:
> > Do we need to verify that handle is not IPC::ChannelHandle() created by
> > ~GpuArcVideoServiceHost()?
> 
> Why? Both ArcBridgeServiceImpl and GpuArcVideoServiceHost are in trusted
browser
> process.

Yes, but if creation failed we may get an empty handle here, do we need to check
for that?

https://chromiumcodereview.appspot.com/1451353002/diff/220001/components/arc/...
File components/arc/video/arc_video_service.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/components/arc/...
components/arc/video/arc_video_service.h:11: // acceleration service.
We should probably mention that the purpose of this class is to create a channel
into the GPU process only, which then handles video decoding/encoding, without
going through ArcVideoService anymore?

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/browser...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/browser...
content/browser/gpu/gpu_arc_video_service_host.h:22: // GPU process and sends
the created channel back to ArcBridgeService.
s/GpuArcVideoService/GpuArcVideoService to create a video accelerator channel/

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/browser...
content/browser/gpu/gpu_arc_video_service_host.h:43: void ReplyChannelCreated(
Perhaps OnChannelCreatedReply or HandleChannelCreatedReply ?

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/browser...
content/browser/gpu/gpu_arc_video_service_host.h:46: void Shutdown();
Nit: please empty line above.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
File content/common/gpu/DEPS (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/DEPS:5: "+media/video/arc_video_accelerator.h",
This should not go into this CL I think?

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
File content/common/gpu/gpu_channel_manager.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/gpu_channel_manager.h:197: scoped_ptr<GpuArcVideoService>
arc_video_service_;
I'm wondering if we should make this and other things ifdef CHROMEOS?

Also, perhaps s/arc_video_service_/gpu_arc_video_service_/

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
File content/common/gpu/media/gpu_arc_video_service.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/media/gpu_arc_video_service.h:29: // For each creation
requests from GpuChannelManager, GpuArcVideoService will
s/requests/request/

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/media/gpu_arc_video_service.h:31: //
|arc_video_accelerator_thread_|.
It would be preferable to make all ArcVideoAccelerator methods callable on GPU
main thread, and nonblocking, because GpuArcVideoDecodeAccelerator has to call
VDA implementations on GPU main as well.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/media/gpu_arc_video_service.h:37: // |shutdown_event| should
signal an event when this process shutdown in
s/shutdown/is about to be shut down/ ?

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/media/gpu_arc_video_service.h:49: // Creates a new
accelerator stub. The creation result will be send back via
s/send/sent/

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/content...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/content...
content/content_common.gypi:354: 'common/gpu/media/gpu_arc_video_service.cc',
Perhaps in chromeos=1?

[kcwu, 2015-12-23 09:09:43]

https://chromiumcodereview.appspot.com/1451353002/diff/100001/components/arc/...
File components/arc/arc_bridge_service_impl.cc (right):

https://chromiumcodereview.appspot.com/1451353002/diff/100001/components/arc/...
components/arc/arc_bridge_service_impl.cc:165: const IPC::ChannelHandle& handle)
{
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> On 2015/12/16 14:05:32, kcwu wrote:
> > On 2015/12/16 10:17:58, Pawel Osciak wrote:
> > > Do we need to verify that handle is not IPC::ChannelHandle() created by
> > > ~GpuArcVideoServiceHost()?
> > 
> > Why? Both ArcBridgeServiceImpl and GpuArcVideoServiceHost are in trusted
> browser
> > process.
> 
> Yes, but if creation failed we may get an empty handle here, do we need to
check
> for that?

No, I want to let the remote caller handle the failure.

Looks like the handle could be empty. I found existing code may do that.
https://code.google.com/p/chromium/codesearch#chromium/src/components/font_se...

I will double check after my code works.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/components/arc/...
File components/arc/video/arc_video_service.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/components/arc/...
components/arc/video/arc_video_service.h:11: // acceleration service.
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> We should probably mention that the purpose of this class is to create a
channel
> into the GPU process only, which then handles video decoding/encoding, without
> going through ArcVideoService anymore?

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/browser...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/browser...
content/browser/gpu/gpu_arc_video_service_host.h:22: // GPU process and sends
the created channel back to ArcBridgeService.
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> s/GpuArcVideoService/GpuArcVideoService to create a video accelerator channel/

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/browser...
content/browser/gpu/gpu_arc_video_service_host.h:43: void ReplyChannelCreated(
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> Perhaps OnChannelCreatedReply or HandleChannelCreatedReply ?

Acknowledged.
Removed.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/browser...
content/browser/gpu/gpu_arc_video_service_host.h:46: void Shutdown();
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> Nit: please empty line above.

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
File content/common/gpu/DEPS (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/DEPS:5: "+media/video/arc_video_accelerator.h",
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> This should not go into this CL I think?

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
File content/common/gpu/gpu_channel_manager.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/gpu_channel_manager.h:197: scoped_ptr<GpuArcVideoService>
arc_video_service_;
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> I'm wondering if we should make this and other things ifdef CHROMEOS?
> 
> Also, perhaps s/arc_video_service_/gpu_arc_video_service_/

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
File content/common/gpu/media/gpu_arc_video_service.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/media/gpu_arc_video_service.h:29: // For each creation
requests from GpuChannelManager, GpuArcVideoService will
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> s/requests/request/

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/media/gpu_arc_video_service.h:31: //
|arc_video_accelerator_thread_|.
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> It would be preferable to make all ArcVideoAccelerator methods callable on GPU
> main thread, and nonblocking, because GpuArcVideoDecodeAccelerator has to call
> VDA implementations on GPU main as well.

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/media/gpu_arc_video_service.h:37: // |shutdown_event| should
signal an event when this process shutdown in
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> s/shutdown/is about to be shut down/ ?

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/common/...
content/common/gpu/media/gpu_arc_video_service.h:49: // Creates a new
accelerator stub. The creation result will be send back via
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> s/send/sent/

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/content...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/1451353002/diff/220001/content/content...
content/content_common.gypi:354: 'common/gpu/media/gpu_arc_video_service.cc',
On 2015/12/23 06:24:01, Pawel Osciak wrote:
> Perhaps in chromeos=1?

Done.

[kcwu, 2015-12-23 10:24:38]

trybot failed to build on some targets, for example
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
I don't understand what's wrong. Could you give me a hand?

[kcwu, 2015-12-24 17:30:03]

dependency fixed. gn build and component build are both okay to build.

[Pawel Osciak, 2015-12-28 08:58:03]

https://codereview.chromium.org/1451353002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1451353002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:398:
make_scoped_ptr(content::CreateArcVideoService())));
Could CreateArcVideoService return a scoper instead?

https://codereview.chromium.org/1451353002/diff/280001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/280001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:42: void ReplyChannelCreated(
Did you intend to remove this? If not, perhaps OnChannelCreatedReply or
HandleChannelCreatedReply?

[kcwu, 2015-12-28 09:31:04]

https://codereview.chromium.org/1451353002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1451353002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:398:
make_scoped_ptr(content::CreateArcVideoService())));
On 2015/12/28 08:58:03, Pawel Osciak wrote:
> Could CreateArcVideoService return a scoper instead?

Done.

https://codereview.chromium.org/1451353002/diff/280001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/280001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:42: void ReplyChannelCreated(
On 2015/12/28 08:58:03, Pawel Osciak wrote:
> Did you intend to remove this? If not, perhaps OnChannelCreatedReply or
> HandleChannelCreatedReply?

Done.

[kcwu, 2015-12-28 10:57:15]

kcwu@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org, holte@chromium.org,
stevenjb@chromium.org

[kcwu, 2015-12-28 10:57:16]

Owner's review, please
stevenjb: chrome/browser/chromeos/
holte: tools/metrics/histograms/histograms.xml
dcheng: components/arc/common and content/common/gpu/gpu_messages.h
avi: content/
hidehiko: components/
posciak: content/common/gpu/media/

Thanks

[Avi (use Gerrit), 2015-12-28 15:03:58]

https://codereview.chromium.org/1451353002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/DEPS (right):

https://codereview.chromium.org/1451353002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/DEPS:9: "+content/browser",
No. You are not allowed to access any part of content other than content/public
from outside of content.

[stevenjb, 2015-12-28 16:16:00]

https://codereview.chromium.org/1451353002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/DEPS (right):

https://codereview.chromium.org/1451353002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/DEPS:9: "+content/browser",
On 2015/12/28 15:03:58, Avi wrote:
> No. You are not allowed to access any part of content other than
content/public
> from outside of content.

You shouldn't need this DEPS change since you are including from
content/public/browser and chrome/browser already has +content/public/browser.

[Luis Héctor Chávez, 2015-12-28 23:57:38]

https://codereview.chromium.org/1451353002/diff/300001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1451353002/diff/300001/components/arc/common/...
components/arc/common/arc_bridge.mojom:39:
OnVideoInstanceReady@107(VideoInstance instance_ptr);
Can you add [MinVersion=1] before the method name?

If by the time this is ready to be checked in,
https://codereview.chromium.org/1495723004/ has already landed, make that
[MinVersion=2].

[dcheng, 2015-12-29 00:43:23]

A few general comments. Also, a content/browser/gpu OWNER should review this
patch.

https://codereview.chromium.org/1451353002/diff/300001/components/arc/arc_ser...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/1451353002/diff/300001/components/arc/arc_ser...
components/arc/arc_service_manager.cc:31:
arc_video_service_(std::move(arc_video_service)) {
#include <utility> please.

https://codereview.chromium.org/1451353002/diff/300001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/300001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:22: class
GpuArcVideoServiceHost : public base::NonThreadSafe,
Use base::ThreadChecker instead of inheriting.

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:18: public base::NonThreadSafe
{
Ditto, use ThreadChecker.

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:21:
AcceleratorStub(GpuArcVideoService* owner) : owner_(owner) {}
explicit

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:65: GpuArcVideoService*
owner_;
GpuArcVideoService* const

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.h:61:
DISALLOW_IMPLICIT_CONSTRUCTORS(GpuArcVideoService);
DISALLOW_COPY_AND_ASSIGN instead is much more common.

[kcwu, 2015-12-29 03:36:32]

https://codereview.chromium.org/1451353002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/DEPS (right):

https://codereview.chromium.org/1451353002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/DEPS:9: "+content/browser",
On 2015/12/28 15:03:58, Avi wrote:
> No. You are not allowed to access any part of content other than
content/public
> from outside of content.

Ah, I forgot to remove this line after fixed the dependency.
Done.

https://codereview.chromium.org/1451353002/diff/300001/components/arc/arc_ser...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/1451353002/diff/300001/components/arc/arc_ser...
components/arc/arc_service_manager.cc:31:
arc_video_service_(std::move(arc_video_service)) {
On 2015/12/29 00:43:23, dcheng wrote:
> #include <utility> please.

Done.

https://codereview.chromium.org/1451353002/diff/300001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1451353002/diff/300001/components/arc/common/...
components/arc/common/arc_bridge.mojom:39:
OnVideoInstanceReady@107(VideoInstance instance_ptr);
On 2015/12/28 23:57:38, Luis Héctor Chávez wrote:
> Can you add [MinVersion=1] before the method name?
> 
> If by the time this is ready to be checked in,
> https://codereview.chromium.org/1495723004/ has already landed, make that
> [MinVersion=2].

Done.

https://codereview.chromium.org/1451353002/diff/300001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/300001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:22: class
GpuArcVideoServiceHost : public base::NonThreadSafe,
On 2015/12/29 00:43:23, dcheng wrote:
> Use base::ThreadChecker instead of inheriting.

Done.

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:18: public base::NonThreadSafe
{
On 2015/12/29 00:43:23, dcheng wrote:
> Ditto, use ThreadChecker.

Done.

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:21:
AcceleratorStub(GpuArcVideoService* owner) : owner_(owner) {}
On 2015/12/29 00:43:23, dcheng wrote:
> explicit

Done.

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:65: GpuArcVideoService*
owner_;
On 2015/12/29 00:43:23, dcheng wrote:
> GpuArcVideoService* const

Done.

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/300001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.h:61:
DISALLOW_IMPLICIT_CONSTRUCTORS(GpuArcVideoService);
On 2015/12/29 00:43:23, dcheng wrote:
> DISALLOW_COPY_AND_ASSIGN instead is much more common.

Done.

[kcwu, 2015-12-29 03:48:14]

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

[kcwu, 2015-12-29 03:48:15]

add jbauman for content/browser/gpu and content/common/gpu owner review

[Owen Lin, 2015-12-29 05:31:42]

https://codereview.chromium.org/1451353002/diff/300001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/300001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:55: if
(bridge_service->video_instance())
Please add comment to explain the two cases.

https://codereview.chromium.org/1451353002/diff/320001/components/arc/video/a...
File components/arc/video/arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/320001/components/arc/video/a...
components/arc/video/arc_video_service.h:15: class ArcVideoService {
The document of this class doesn't match to the code. Please see:

https://code.google.com/p/chromium/codesearch#chromium/src/components/arc/pow...

I think this class is in the same role as ArcPowerBridge. To sync with others,
please rename it to ArcVideoBridge.

Besides, I would also suggest that delegating the job of VideoHost to
GpuArcVideoServiceHost, but leave the other implementations here.

More specific:

class ArcVideoBridge : public VideoHost, 
                       public ArcBridgeService::Observer {
  public Initialize() {
     ...
  }

  public onStateChanged() override {
    ... 
  }
}

class GpuArcVideoServiceHost : public ArcVideoBridge {
  public OnRequestArcVideoAcceleratorChannel() override {
  }
}

The responsibilities are clear: ArcVideoBridge handles ArcBridgeService related
things, like registering and state observing. GpuArcVideoServiceHost implements
ArcVideoBridge and mainly provides the VideoHost functions from the GPU process.

https://codereview.chromium.org/1451353002/diff/320001/content/public/browser...
File content/public/browser/arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/320001/content/public/browser...
content/public/browser/arc_video_service.h:17: CONTENT_EXPORT
scoped_ptr<arc::ArcVideoService> CreateArcVideoService();
Can we move this into ArcVideoBridge.h and just implement the function in
GpuArcVideoServiceHost?

class ArcVideoBrdige {
public:
  static scoped_ptr<ArcVideoService> CreateInstance();
};

In gpu_arc_video_service_host.cc:
namespace arc {
  scoped_ptr<ArcVideoService> ArcVideoBridge::CreateInstance() {
    ...
  }
}

[kcwu, 2015-12-29 13:24:44]

https://codereview.chromium.org/1451353002/diff/300001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/300001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:55: if
(bridge_service->video_instance())
On 2015/12/29 05:31:42, Owen Lin wrote:
> Please add comment to explain the two cases.

Done.

https://codereview.chromium.org/1451353002/diff/320001/components/arc/video/a...
File components/arc/video/arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/320001/components/arc/video/a...
components/arc/video/arc_video_service.h:15: class ArcVideoService {
On 2015/12/29 05:31:42, Owen Lin wrote:
> The document of this class doesn't match to the code. Please see:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/arc/pow...
> 
> I think this class is in the same role as ArcPowerBridge. To sync with others,
> please rename it to ArcVideoBridge.
> 
> Besides, I would also suggest that delegating the job of VideoHost to
> GpuArcVideoServiceHost, but leave the other implementations here.
> 
> More specific:
> 
> class ArcVideoBridge : public VideoHost, 
>                        public ArcBridgeService::Observer {
>   public Initialize() {
>      ...
>   }
> 
>   public onStateChanged() override {
>     ... 
>   }
> }
> 
> class GpuArcVideoServiceHost : public ArcVideoBridge {
>   public OnRequestArcVideoAcceleratorChannel() override {
>   }
> }
> 
> The responsibilities are clear: ArcVideoBridge handles ArcBridgeService
related
> things, like registering and state observing. GpuArcVideoServiceHost
implements
> ArcVideoBridge and mainly provides the VideoHost functions from the GPU
process.

Done.

https://codereview.chromium.org/1451353002/diff/320001/content/public/browser...
File content/public/browser/arc_video_service.h (right):

https://codereview.chromium.org/1451353002/diff/320001/content/public/browser...
content/public/browser/arc_video_service.h:17: CONTENT_EXPORT
scoped_ptr<arc::ArcVideoService> CreateArcVideoService();
On 2015/12/29 05:31:42, Owen Lin wrote:
> Can we move this into ArcVideoBridge.h and just implement the function in
> GpuArcVideoServiceHost?
> 
> class ArcVideoBrdige {
> public:
>   static scoped_ptr<ArcVideoService> CreateInstance();
> };
> 
> In gpu_arc_video_service_host.cc:
> namespace arc {
>   scoped_ptr<ArcVideoService> ArcVideoBridge::CreateInstance() {
>     ...
>   }
> }

Acknowledged.
I revised the code in the other way.

[stevenjb, 2015-12-29 16:59:34]

c/b/chromeos lgtm

[Steven Holte, 2015-12-30 01:27:40]

histograms.xml lgtm

[jbauman, 2015-12-30 01:29:29]

https://codereview.chromium.org/1451353002/diff/340001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://codereview.chromium.org/1451353002/diff/340001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.cc:48: io_task_runner_->PostTask(
What threads can this be called on? HandleChannelCreatedReply will be called on
the IO thread by the GPUProcessHost, so it seems like this must be called on
that thread as well, and this PostTask should be unnecessary.

https://codereview.chromium.org/1451353002/diff/340001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/340001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:56: bool
OnMessageReceived(const IPC::Message& msg) override {
This seems reasonable, but have you considered using mojo instead of Chrome IPC
for this?

[Owen Lin, 2015-12-30 03:08:49]

lgtm

[Pawel Osciak, 2015-12-30 07:39:15]

content/common/gpu/media/ lgtm and a few nits

https://chromiumcodereview.appspot.com/1451353002/diff/340001/components/arc/...
File components/arc/arc_service_manager.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/340001/components/arc/...
components/arc/arc_service_manager.h:25: scoped_ptr<ArcVideoBridge>
video_service);
s/video_service/video_bridge/

https://chromiumcodereview.appspot.com/1451353002/diff/340001/components/arc/...
File components/arc/video/arc_video_bridge.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/340001/components/arc/...
components/arc/video/arc_video_bridge.h:21:
ArcVideoBridge(scoped_ptr<VideoHostDelegate> video_host_delegate);
explicit

https://chromiumcodereview.appspot.com/1451353002/diff/340001/content/common/...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://chromiumcodereview.appspot.com/1451353002/diff/340001/content/common/...
content/common/gpu/media/gpu_arc_video_service.cc:77:
accelerator_stubs_.clear();
Nit: do we need to call this explicitly? Should happen in map::~map<>

[kcwu, 2015-12-30 09:40:33]

https://chromiumcodereview.appspot.com/1451353002/diff/340001/components/arc/...
File components/arc/arc_service_manager.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/340001/components/arc/...
components/arc/arc_service_manager.h:25: scoped_ptr<ArcVideoBridge>
video_service);
On 2015/12/30 07:39:15, Pawel Osciak wrote:
> s/video_service/video_bridge/

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/340001/components/arc/...
File components/arc/video/arc_video_bridge.h (right):

https://chromiumcodereview.appspot.com/1451353002/diff/340001/components/arc/...
components/arc/video/arc_video_bridge.h:21:
ArcVideoBridge(scoped_ptr<VideoHostDelegate> video_host_delegate);
On 2015/12/30 07:39:15, Pawel Osciak wrote:
> explicit

Done.

https://chromiumcodereview.appspot.com/1451353002/diff/340001/content/browser...
File content/browser/gpu/gpu_arc_video_service_host.cc (right):

https://chromiumcodereview.appspot.com/1451353002/diff/340001/content/browser...
content/browser/gpu/gpu_arc_video_service_host.cc:48: io_task_runner_->PostTask(
On 2015/12/30 01:29:29, jbauman wrote:
> What threads can this be called on? HandleChannelCreatedReply will be called
on
> the IO thread by the GPUProcessHost, so it seems like this must be called on
> that thread as well, and this PostTask should be unnecessary.

Ah, HandleChannelCreatedReply is called on the IO thread and my DCHECK was
wrong. I revised it.

Regarding threads, GpuProcessHost::Get() must be called on IO thread, so
PostTask to IO thread is necessary.

https://chromiumcodereview.appspot.com/1451353002/diff/340001/content/common/...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://chromiumcodereview.appspot.com/1451353002/diff/340001/content/common/...
content/common/gpu/media/gpu_arc_video_service.cc:56: bool
OnMessageReceived(const IPC::Message& msg) override {
On 2015/12/30 01:29:29, jbauman wrote:
> This seems reasonable, but have you considered using mojo instead of Chrome
IPC
> for this?

Yes, I will migrate to mojo later.

https://chromiumcodereview.appspot.com/1451353002/diff/340001/content/common/...
content/common/gpu/media/gpu_arc_video_service.cc:77:
accelerator_stubs_.clear();
On 2015/12/30 07:39:15, Pawel Osciak wrote:
> Nit: do we need to call this explicitly? Should happen in map::~map<>

Done.

[jbauman, 2015-12-30 22:34:22]

content/browser/gpu and content/common/gpu lgtm

[hidehiko, 2016-01-05 08:41:05]

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

https://codereview.chromium.org/1451353002/diff/360001/components/arc/common/...
components/arc/common/arc_bridge.mojom:39: [MinVersion=1]
OnVideoInstanceReady@107(VideoInstance instance_ptr);
Probably 106? If it is necessary to be skipped, please comment.

https://codereview.chromium.org/1451353002/diff/360001/components/arc/video/a...
File components/arc/video/arc_video_bridge.cc (right):

https://codereview.chromium.org/1451353002/diff/360001/components/arc/video/a...
components/arc/video/arc_video_bridge.cc:37: video_host_delegate_->Shutdown();
Shutdown() sounds like we will never reuse this (specifically under Chrome's
context). However, we will (specifically after
https://codereview.chromium.org/1548833002/).
How about OnStopping() or something?

https://codereview.chromium.org/1451353002/diff/360001/components/arc/video/a...
File components/arc/video/arc_video_bridge.h (right):

https://codereview.chromium.org/1451353002/diff/360001/components/arc/video/a...
components/arc/video/arc_video_bridge.h:33: mojo::Binding<arc::VideoHost>
binding_;
DISALLOW_COPY_AND_ASSIGN.

[Luis Héctor Chávez, 2016-01-05 17:58:13]

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

https://codereview.chromium.org/1451353002/diff/360001/components/arc/common/...
components/arc/common/arc_bridge.mojom:39: [MinVersion=1]
OnVideoInstanceReady@107(VideoInstance instance_ptr);
On 2016/01/05 08:41:05, hidehiko wrote:
> Probably 106? If it is necessary to be skipped, please comment.

107 is the right number, since 106 is reserved for the auth interface. I'm
tracking all in-flight changes and assigning each new interface a unique
ordinal.

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

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

https://codereview.chromium.org/1451353002/diff/360001/components/arc/common/...
components/arc/common/arc_bridge.mojom:39: [MinVersion=1]
OnVideoInstanceReady@107(VideoInstance instance_ptr);
On 2016/01/05 17:58:13, Luis Héctor Chávez wrote:
> On 2016/01/05 08:41:05, hidehiko wrote:
> > Probably 106? If it is necessary to be skipped, please comment.
> 
> 107 is the right number, since 106 is reserved for the auth interface. I'm
> tracking all in-flight changes and assigning each new interface a unique
> ordinal.

Acknowledged.

https://codereview.chromium.org/1451353002/diff/360001/components/arc/video/a...
File components/arc/video/arc_video_bridge.cc (right):

https://codereview.chromium.org/1451353002/diff/360001/components/arc/video/a...
components/arc/video/arc_video_bridge.cc:37: video_host_delegate_->Shutdown();
On 2016/01/05 08:41:05, hidehiko wrote:
> Shutdown() sounds like we will never reuse this (specifically under Chrome's
> context). However, we will (specifically after
> https://codereview.chromium.org/1548833002/).
> How about OnStopping() or something?

Done.

https://codereview.chromium.org/1451353002/diff/360001/components/arc/video/a...
File components/arc/video/arc_video_bridge.h (right):

https://codereview.chromium.org/1451353002/diff/360001/components/arc/video/a...
components/arc/video/arc_video_bridge.h:33: mojo::Binding<arc::VideoHost>
binding_;
On 2016/01/05 08:41:05, hidehiko wrote:
> DISALLOW_COPY_AND_ASSIGN.

Done.

[kcwu, 2016-01-06 06:27:37]

@avi and @dcheng, could you take another look?
thanks

[Avi (use Gerrit), 2016-01-06 16:24:10]

Mostly OK.

https://codereview.chromium.org/1451353002/diff/420001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1451353002/diff/420001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:854: IPC::ChannelHandle /* handle to channel
*/)
Every other set of messages in this file has commentary, are split into messages
and host messages, etc.

Please comment these up.

Also, shouldn't these be #if defined (OS_CHROMEOS)?

[hidehiko, 2016-01-07 06:41:56]

components/arc LGTM.

[kcwu, 2016-01-07 08:16:12]

https://codereview.chromium.org/1451353002/diff/420001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1451353002/diff/420001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:854: IPC::ChannelHandle /* handle to channel
*/)
On 2016/01/06 16:24:10, Avi wrote:
> Every other set of messages in this file has commentary, are split into
messages
> and host messages, etc.
> 
> Please comment these up.
> 
> Also, shouldn't these be #if defined (OS_CHROMEOS)?

Done.
Also add #if defined(OS_CHROMEOS) in gpu_process_host.{cc,h} where uses these
messages.

[Avi (use Gerrit), 2016-01-07 15:00:30]

LGTM

[Luis Héctor Chávez, 2016-01-07 17:08:26]

https://codereview.chromium.org/1451353002/diff/440001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1451353002/diff/440001/components/arc/common/...
components/arc/common/arc_bridge.mojom:39: [MinVersion=1]
OnVideoInstanceReady@107(VideoInstance instance_ptr);
FYI Another change went in with MinVersion=1. Need to bump it to MinVersion=2.

[dcheng, 2016-01-08 09:11:15]

https://codereview.chromium.org/1451353002/diff/420001/components/arc/video/a...
File components/arc/video/arc_video_bridge.cc (right):

https://codereview.chromium.org/1451353002/diff/420001/components/arc/video/a...
components/arc/video/arc_video_bridge.cc:18: if (bridge_service) {
We expect this to always be non-null right?

https://codereview.chromium.org/1451353002/diff/420001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/420001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:30: // GpuArcVideoService, on
GPU process.
Nit: in the GPU process.

https://codereview.chromium.org/1451353002/diff/440001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/440001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:34: #if defined(OS_POSIX)
Isn't ARC CrOS specific? When will this not be true?

[kcwu, 2016-01-08 10:32:09]

https://codereview.chromium.org/1451353002/diff/420001/components/arc/video/a...
File components/arc/video/arc_video_bridge.cc (right):

https://codereview.chromium.org/1451353002/diff/420001/components/arc/video/a...
components/arc/video/arc_video_bridge.cc:18: if (bridge_service) {
On 2016/01/08 09:11:15, dcheng wrote:
> We expect this to always be non-null right?

Done.

https://codereview.chromium.org/1451353002/diff/420001/content/browser/gpu/gp...
File content/browser/gpu/gpu_arc_video_service_host.h (right):

https://codereview.chromium.org/1451353002/diff/420001/content/browser/gpu/gp...
content/browser/gpu/gpu_arc_video_service_host.h:30: // GpuArcVideoService, on
GPU process.
On 2016/01/08 09:11:15, dcheng wrote:
> Nit: in the GPU process.

Done.

https://codereview.chromium.org/1451353002/diff/440001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1451353002/diff/440001/components/arc/common/...
components/arc/common/arc_bridge.mojom:39: [MinVersion=1]
OnVideoInstanceReady@107(VideoInstance instance_ptr);
On 2016/01/07 17:08:26, Luis Héctor Chávez wrote:
> FYI Another change went in with MinVersion=1. Need to bump it to MinVersion=2.

Done.

https://codereview.chromium.org/1451353002/diff/440001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/440001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:34: #if defined(OS_POSIX)
On 2016/01/08 09:11:15, dcheng wrote:
> Isn't ARC CrOS specific? When will this not be true?

Yes. Removed the #if condition.

[mdempsky, 2016-01-09 01:07:04]

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

[mdempsky, 2016-01-09 01:07:05]

https://codereview.chromium.org/1451353002/diff/480001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/480001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:49: // RemoveClient will
delete |this|.
Why not just do "delete this;"?  RemoveClient doesn't seem to be called
otherwise, and then you can get rid of RemoveClient and accelerator_stubs_.

[kcwu, 2016-01-09 03:22:04]

https://codereview.chromium.org/1451353002/diff/480001/content/common/gpu/med...
File content/common/gpu/media/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/1451353002/diff/480001/content/common/gpu/med...
content/common/gpu/media/gpu_arc_video_service.cc:49: // RemoveClient will
delete |this|.
On 2016/01/09 01:07:05, mdempsky wrote:
> Why not just do "delete this;"?  RemoveClient doesn't seem to be called
> otherwise, and then you can get rid of RemoveClient and accelerator_stubs_.

We have to keep accelerator_stubs_ because
GpuChannelManager::OnShutdownArcVideoService() will delete GpuArcVideoService
and implicitly delete accelerator_stubs_.

[dcheng, 2016-01-12 02:03:12]

lgtm

[kcwu, 2016-01-12 03:32:35]

The CQ bit was checked by kcwu@chromium.org

[kcwu, 2016-01-12 03:32:36]

The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@google.com,
holte@chromium.org, stevenjb@chromium.org, owenlin@chromium.org,
posciak@chromium.org, jbauman@chromium.org, hidehiko@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1451353002/#ps480001
(title: "addressed dcheng's comments")

[commit-bot: I haz the power, 2016-01-12 03:33:54]

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

[commit-bot: I haz the power, 2016-01-12 04:51:46]

Description was changed from

==========
Implement GpuArcVideoService for arc video accelerator

GpuArcVideoService creates new channel and dispatch IPC to
ArcVideoAccelerator.

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

to

==========
Implement GpuArcVideoService for arc video accelerator

GpuArcVideoService creates new channel and dispatch IPC to
ArcVideoAccelerator.

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

[commit-bot: I haz the power, 2016-01-12 04:51:47]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2016-01-12 04:52:57]

Description was changed from

==========
Implement GpuArcVideoService for arc video accelerator

GpuArcVideoService creates new channel and dispatch IPC to
ArcVideoAccelerator.

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

to

==========
Implement GpuArcVideoService for arc video accelerator

GpuArcVideoService creates new channel and dispatch IPC to
ArcVideoAccelerator.

BUG=b/25057601

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

[commit-bot: I haz the power, 2016-01-12 04:52:58]

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

[jam, 2016-01-21 17:35:39]

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

[jam, 2016-01-21 17:35:40]

https://codereview.chromium.org/1451353002/diff/480001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1451353002/diff/480001/content/browser/DEPS#n...
content/browser/DEPS:3: # depend on components which we share with the mojo
html_viewer.
see the comment right above where you added this. this isn't allowed. content
can't include components that aren't shared with html_viewer.

[jam, 2016-01-21 17:41:12]

also, looking at this I don't understand why content has to know about arc. this
is a chrome os browser feature. content shouldn't have knowledge about this.

Given these issues, I think this change should be reverted and we should have
more discussions on how this can be implemented in a way that keeps the layering
of content.