content: Move all GpuMemoryBuffer allocation to IO thread.

content: Move all GpuMemoryBuffer allocation to IO thread.

Allocation of a GpuMemoryBuffer is still asynchronous on the UI
thread and as a result also for the renderer on the browser side.
However, allocation of a GpuMemoryBuffer still need to be
synchronous for the browser compositor but instead of limiting
the browser compositor to GpuMemoryBuffer implementations that
can be allocated synchronously, we block on the UI thread while
waiting for allocation to be handled asynchronous on the IO
thread. This allows all GpuMemoryBuffer implementations to be
used with the browser compositor.

BUG=380861
Committed: https://crrev.com/1953007c5e879a2e7e53efd0aa0be64263e9c6a0
Cr-Commit-Position: refs/heads/master@{#293871}

[reveman, 2014-09-05 20:44:33]

reveman@chromium.org changed reviewers:
+ alexst@chromium.org, piman@chromium.org

[reveman, 2014-09-05 20:44:34]

Please take a look. I prefer if do this prior to making it possible to use gpu
side allocated buffers in the browser compositor.

This should also make it more efficient for the renderer to allocate buffers as
it doesn't have to wait for the UI thread to respond. It could of course be a
problem if allocation of some specific buffer type is too expensive and block
the IO thread for too long but I think we can move the allocation of that
specific buffer type off to the UI thread in that case. ie.
renderer->browser_io->browser_ui and back in the case of that specific buffer
type.

[piman, 2014-09-05 22:30:38]

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:41: : event(true,
false), width(0), height(0), internalformat(0), usage(0) {}
nit: might as well providing instead a constructor that takes
widht/height/internalformat/usage since there's only one caller that will fill
all those.

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:378:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Couldn't this be called on the compositor thread?

Maybe DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)) ? Since that's the
case that would cause a deadlock.

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:488:
request->event.Signal();
To confirm, if the GPU process dies while a request is pending, we'll go through
this path, correct?

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:493:
gfx::Size(request->width, request->height),
nit: indent looks weird

https://codereview.chromium.org/542083002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/542083002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1331:
CompositorImpl::CreateSurfaceTexture(render_process_id_);
Wondering... is this thread safe? I see a thread checker in the implementation
of CreateSurfaceTexture, and it seems to be talking to Java.

[reveman, 2014-09-05 23:58:43]

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:41: : event(true,
false), width(0), height(0), internalformat(0), usage(0) {}
On 2014/09/05 22:30:37, piman (OOO) wrote:
> nit: might as well providing instead a constructor that takes
> widht/height/internalformat/usage since there's only one caller that will fill
> all those.

Did it this way to be consistent with CreateRequest above. But I agree that we
might as well provide ctors. Change both this and CreateRequest  in latest patch
to keep things consistent.

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:378:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/09/05 22:30:37, piman (OOO) wrote:
> Couldn't this be called on the compositor thread?
> 
> Maybe DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)) ? Since that's
the
> case that would cause a deadlock.

I didn't think of that. Changed DCHECKs as suggested in latest patch.

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:488:
request->event.Signal();
On 2014/09/05 22:30:37, piman (OOO) wrote:
> To confirm, if the GPU process dies while a request is pending, we'll go
through
> this path, correct?

No, unless I misunderstood your question. This code is not really aware of the
usage of the GPU process for allocation of some buffer types. If the GPU process
is dead or dies before it can return a valid handle, we'll still call the
platform dependent GpuMemoryBufferImpl::Create function below. If that decides
to allocate the buffer GPU process side it will call
BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer above which is part of the
GpuMemoryBufferFactoryHost interface. That function calls into GpuProcessHost
which is responsible for returning a NULL handle if the GPU process dies before
processing the IPC.

However, I'm glad you asked as it looks like
GpuProcessHost::SendOutstandingReplies is missing some code for
CreateGpuMemoryBuffer requests. I'll upload another patch to fix this right
away.

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:493:
gfx::Size(request->width, request->height),
On 2014/09/05 22:30:37, piman (OOO) wrote:
> nit: indent looks weird

I agree but this seems to be the way cl format likes it..

https://codereview.chromium.org/542083002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/542083002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1331:
CompositorImpl::CreateSurfaceTexture(render_process_id_);
On 2014/09/05 22:30:38, piman (OOO) wrote:
> Wondering... is this thread safe? I see a thread checker in the implementation
> of CreateSurfaceTexture, and it seems to be talking to Java.

Yes, this should be fine and actually how it used to be originally. Still pretty
confusing, I'd have to admit.

However, now that we have gpu process side allocation of buffers in place it
will be easy for me to refactor this surface texture support so it's consistent
with other buffer types allocated gpu process side and make it work when android
switches to out-of-process gpu service. I'll try to get this done asap. Same
goes for IOSurfaces on mac..

[piman, 2014-09-06 00:28:23]

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:488:
request->event.Signal();
On 2014/09/05 23:58:43, reveman wrote:
> On 2014/09/05 22:30:37, piman (OOO) wrote:
> > To confirm, if the GPU process dies while a request is pending, we'll go
> through
> > this path, correct?
> 
> No, unless I misunderstood your question. This code is not really aware of the
> usage of the GPU process for allocation of some buffer types. If the GPU
process
> is dead or dies before it can return a valid handle, we'll still call the
> platform dependent GpuMemoryBufferImpl::Create function below. If that decides
> to allocate the buffer GPU process side it will call
> BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer above which is part of the
> GpuMemoryBufferFactoryHost interface. That function calls into GpuProcessHost
> which is responsible for returning a NULL handle if the GPU process dies
before
> processing the IPC.

Then I'm confused... Shouldn't we signal the event only after we received the
reply from the GPU process?

> 
> However, I'm glad you asked as it looks like
> GpuProcessHost::SendOutstandingReplies is missing some code for
> CreateGpuMemoryBuffer requests. I'll upload another patch to fix this right
> away.

[reveman, 2014-09-06 20:30:49]

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:488:
request->event.Signal();
On 2014/09/06 00:28:23, piman (OOO) wrote:
> On 2014/09/05 23:58:43, reveman wrote:
> > On 2014/09/05 22:30:37, piman (OOO) wrote:
> > > To confirm, if the GPU process dies while a request is pending, we'll go
> > through
> > > this path, correct?
> > 
> > No, unless I misunderstood your question. This code is not really aware of
the
> > usage of the GPU process for allocation of some buffer types. If the GPU
> process
> > is dead or dies before it can return a valid handle, we'll still call the
> > platform dependent GpuMemoryBufferImpl::Create function below. If that
decides
> > to allocate the buffer GPU process side it will call
> > BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer above which is part of
the
> > GpuMemoryBufferFactoryHost interface. That function calls into
GpuProcessHost
> > which is responsible for returning a NULL handle if the GPU process dies
> before
> > processing the IPC.
> 
> Then I'm confused... Shouldn't we signal the event only after we received the
> reply from the GPU process?

If the format or usage is an invalid value that can't be supported by any
implementation then there's no point in moving forward. We might as well reject
the request early. This could be moved up to AllocateGpuMemoryBuffer instead but
it seemed a bit more consistent to do all these GpuMemoryBufferImpl calls on the
IO thread. Now that I think of it, it might be best to leave it up to each
implementation as we need to check these values there anyhow.

> 
> > 
> > However, I'm glad you asked as it looks like
> > GpuProcessHost::SendOutstandingReplies is missing some code for
> > CreateGpuMemoryBuffer requests. I'll upload another patch to fix this right
> > away.
>

[piman, 2014-09-08 18:33:57]

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:488:
request->event.Signal();
On 2014/09/06 20:30:49, reveman wrote:
> On 2014/09/06 00:28:23, piman (OOO) wrote:
> > On 2014/09/05 23:58:43, reveman wrote:
> > > On 2014/09/05 22:30:37, piman (OOO) wrote:
> > > > To confirm, if the GPU process dies while a request is pending, we'll go
> > > through
> > > > this path, correct?
> > > 
> > > No, unless I misunderstood your question. This code is not really aware of
> the
> > > usage of the GPU process for allocation of some buffer types. If the GPU
> > process
> > > is dead or dies before it can return a valid handle, we'll still call the
> > > platform dependent GpuMemoryBufferImpl::Create function below. If that
> decides
> > > to allocate the buffer GPU process side it will call
> > > BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer above which is part of
> the
> > > GpuMemoryBufferFactoryHost interface. That function calls into
> GpuProcessHost
> > > which is responsible for returning a NULL handle if the GPU process dies
> > before
> > > processing the IPC.
> > 
> > Then I'm confused... Shouldn't we signal the event only after we received
the
> > reply from the GPU process?
> 
> If the format or usage is an invalid value that can't be supported by any
> implementation then there's no point in moving forward. We might as well
reject
> the request early. This could be moved up to AllocateGpuMemoryBuffer instead
but
> it seemed a bit more consistent to do all these GpuMemoryBufferImpl calls on
the
> IO thread. Now that I think of it, it might be best to leave it up to each
> implementation as we need to check these values there anyhow.
> 
> > 
> > > 
> > > However, I'm glad you asked as it looks like
> > > GpuProcessHost::SendOutstandingReplies is missing some code for
> > > CreateGpuMemoryBuffer requests. I'll upload another patch to fix this
right
> > > away.
> > 
> 

ACK about early reject, but what about the general case? Should the event be
signaled in OnGpuMemoryBufferCreated?

[reveman, 2014-09-08 19:58:57]

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/542083002/diff/60001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:488:
request->event.Signal();
On 2014/09/08 18:33:57, piman (OOO) wrote:
> On 2014/09/06 20:30:49, reveman wrote:
> > On 2014/09/06 00:28:23, piman (OOO) wrote:
> > > On 2014/09/05 23:58:43, reveman wrote:
> > > > On 2014/09/05 22:30:37, piman (OOO) wrote:
> > > > > To confirm, if the GPU process dies while a request is pending, we'll
go
> > > > through
> > > > > this path, correct?
> > > > 
> > > > No, unless I misunderstood your question. This code is not really aware
of
> > the
> > > > usage of the GPU process for allocation of some buffer types. If the GPU
> > > process
> > > > is dead or dies before it can return a valid handle, we'll still call
the
> > > > platform dependent GpuMemoryBufferImpl::Create function below. If that
> > decides
> > > > to allocate the buffer GPU process side it will call
> > > > BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer above which is part
of
> > the
> > > > GpuMemoryBufferFactoryHost interface. That function calls into
> > GpuProcessHost
> > > > which is responsible for returning a NULL handle if the GPU process dies
> > > before
> > > > processing the IPC.
> > > 
> > > Then I'm confused... Shouldn't we signal the event only after we received
> the
> > > reply from the GPU process?
> > 
> > If the format or usage is an invalid value that can't be supported by any
> > implementation then there's no point in moving forward. We might as well
> reject
> > the request early. This could be moved up to AllocateGpuMemoryBuffer instead
> but
> > it seemed a bit more consistent to do all these GpuMemoryBufferImpl calls on
> the
> > IO thread. Now that I think of it, it might be best to leave it up to each
> > implementation as we need to check these values there anyhow.
> > 
> > > 
> > > > 
> > > > However, I'm glad you asked as it looks like
> > > > GpuProcessHost::SendOutstandingReplies is missing some code for
> > > > CreateGpuMemoryBuffer requests. I'll upload another patch to fix this
> right
> > > > away.
> > > 
> > 
> 
> ACK about early reject, but what about the general case? Should the event be
> signaled in OnGpuMemoryBufferCreated?

So this patch doesn't actually make it possible to use gpu process side
allocated buffers by the browser compositor. This is still limited to
synchronous allocation using GpuMemoryBufferImpl::Create. This patch simply
moves all allocation to the IO thread so that adding support for gpu process
side allocated buffers can be done cleanly. I'll leave it up to Alex to do that
as part of https://codereview.chromium.org/540443002/.
GpuMemoryBufferImpl::Create basically need to take a callback instead of
returning a buffer and the event will then be signaled when that call back is
called.

I think the fact that this class implements both GpuChannelHostFactory and
GpuMemoryBufferFactoryHost makes things a bit confusing. Not sure what to do
about that..

[piman, 2014-09-08 21:45:51]

On Mon, Sep 8, 2014 at 12:58 PM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.org/542083002/diff/60001/
> content/browser/gpu/browser_gpu_channel_host_factory.cc
> File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):
>
> https://codereview.chromium.org/542083002/diff/60001/
> content/browser/gpu/browser_gpu_channel_host_factory.cc#newcode488
> content/browser/gpu/browser_gpu_channel_host_factory.cc:488:
> request->event.Signal();
> On 2014/09/08 18:33:57, piman (OOO) wrote:
>
>> On 2014/09/06 20:30:49, reveman wrote:
>> > On 2014/09/06 00:28:23, piman (OOO) wrote:
>> > > On 2014/09/05 23:58:43, reveman wrote:
>> > > > On 2014/09/05 22:30:37, piman (OOO) wrote:
>> > > > > To confirm, if the GPU process dies while a request is
>>
> pending, we'll go
>
>> > > > through
>> > > > > this path, correct?
>> > > >
>> > > > No, unless I misunderstood your question. This code is not
>>
> really aware of
>
>> > the
>> > > > usage of the GPU process for allocation of some buffer types. If
>>
> the GPU
>
>> > > process
>> > > > is dead or dies before it can return a valid handle, we'll still
>>
> call the
>
>> > > > platform dependent GpuMemoryBufferImpl::Create function below.
>>
> If that
>
>> > decides
>> > > > to allocate the buffer GPU process side it will call
>> > > > BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer above which
>>
> is part of
>
>> > the
>> > > > GpuMemoryBufferFactoryHost interface. That function calls into
>> > GpuProcessHost
>> > > > which is responsible for returning a NULL handle if the GPU
>>
> process dies
>
>> > > before
>> > > > processing the IPC.
>> > >
>> > > Then I'm confused... Shouldn't we signal the event only after we
>>
> received
>
>> the
>> > > reply from the GPU process?
>> >
>> > If the format or usage is an invalid value that can't be supported
>>
> by any
>
>> > implementation then there's no point in moving forward. We might as
>>
> well
>
>> reject
>> > the request early. This could be moved up to AllocateGpuMemoryBuffer
>>
> instead
>
>> but
>> > it seemed a bit more consistent to do all these GpuMemoryBufferImpl
>>
> calls on
>
>> the
>> > IO thread. Now that I think of it, it might be best to leave it up
>>
> to each
>
>> > implementation as we need to check these values there anyhow.
>> >
>> > >
>> > > >
>> > > > However, I'm glad you asked as it looks like
>> > > > GpuProcessHost::SendOutstandingReplies is missing some code for
>> > > > CreateGpuMemoryBuffer requests. I'll upload another patch to fix
>>
> this
>
>> right
>> > > > away.
>> > >
>> >
>>
>
>  ACK about early reject, but what about the general case? Should the
>>
> event be
>
>> signaled in OnGpuMemoryBufferCreated?
>>
>
> So this patch doesn't actually make it possible to use gpu process side
> allocated buffers by the browser compositor. This is still limited to
> synchronous allocation using GpuMemoryBufferImpl::Create. This patch
> simply moves all allocation to the IO thread so that adding support for
> gpu process side allocated buffers can be done cleanly. I'll leave it up
> to Alex to do that as part of
> https://codereview.chromium.org/540443002/. GpuMemoryBufferImpl::Create
> basically need to take a callback instead of returning a buffer and the
> event will then be signaled when that call back is called.
>

If Alex is ok with picking it up from here, then LGTM. I think it's very
confusing...


>
> I think the fact that this class implements both GpuChannelHostFactory
> and GpuMemoryBufferFactoryHost makes things a bit confusing. Not sure
> what to do about that..
>

Maybe having the GpuMemoryBufferFactoryHost be a tear-off would help.


>
> https://codereview.chromium.org/542083002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-09-08 21:46:20]

lgtm

[reveman, 2014-09-08 21:52:31]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-09-08 21:56:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/542083002/100001

[commit-bot: I haz the power, 2014-09-09 06:27:20]

Committed patchset #6 (id:100001) as 23345ca6ca2a587129ca08495f34e092c33345d3

[commit-bot: I haz the power, 2014-09-10 03:51:11]

Patchset 6 (id:??) landed as
https://crrev.com/1953007c5e879a2e7e53efd0aa0be64263e9c6a0
Cr-Commit-Position: refs/heads/master@{#293871}