zero-copy: Clarify to allocate/destroy GpuMemoryBuffer on any thread and use it on the main thread o

zero-copy: Clarify to allocate/destroy GpuMemoryBuffer on any thread and use it on the main thread only

Add many thread check to clarify it.

BUG=473125
Committed: https://crrev.com/d041a28b7aa47ff1d0ff3cca7387a643729b0287
Cr-Commit-Position: refs/heads/master@{#324382}

[dshwang, 2015-04-01 19:03:41]

dongseong.hwang@intel.com changed reviewers:
+ reveman@chromium.org, spang@chromium.org

[dshwang, 2015-04-01 19:03:42]

reveman@, there is the way to avoid thread safe ref counted.
This CL fixes all potential thread bugs of gpu memory buffer.

I guess that IO thread create/delete gpu memory buffer in order to return
quickly, but it can causes very difficult bugs. We prefer correctness to
performance.

[spang, 2015-04-01 19:05:23]

spang@chromium.org changed reviewers:
+ alexst@chromium.org

[reveman, 2015-04-01 19:22:21]

lgtm % alexst review and moving the thread checker

https://codereview.chromium.org/1050923003/diff/1/ui/ozone/public/surface_fac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1050923003/diff/1/ui/ozone/public/surface_fac...
ui/ozone/public/surface_factory_ozone.h:171: base::ThreadChecker
thread_checker_;
A thread checker is good but please move it to the implementation instead of the
interface.

[dshwang, 2015-04-02 07:56:05]

On 2015/04/01 19:22:21, reveman wrote:
> lgtm % alexst review and moving the thread checker

Thank you for reviewing.

alexst@, could you review it in terms of GpuMemoryBuffer design?
spang@, could you review ui/ozone/ ?

https://codereview.chromium.org/1050923003/diff/1/ui/ozone/public/surface_fac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1050923003/diff/1/ui/ozone/public/surface_fac...
ui/ozone/public/surface_factory_ozone.h:171: base::ThreadChecker
thread_checker_;
On 2015/04/01 19:22:20, reveman wrote:
> A thread checker is good but please move it to the implementation instead of
the
> interface.

This thread check is added to check SurfaceFactoryOzone::GetInstance() which is
implemented in this class, instead of subclass.
If moving thread check to subclass, I think there is no way to check
SurfaceFactoryOzone::GetInstance()

[reveman, 2015-04-02 12:35:39]

https://codereview.chromium.org/1050923003/diff/1/ui/ozone/public/surface_fac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1050923003/diff/1/ui/ozone/public/surface_fac...
ui/ozone/public/surface_factory_ozone.h:171: base::ThreadChecker
thread_checker_;
On 2015/04/02 07:56:05, dshwang wrote:
> On 2015/04/01 19:22:20, reveman wrote:
> > A thread checker is good but please move it to the implementation instead of
> the
> > interface.
> 
> This thread check is added to check SurfaceFactoryOzone::GetInstance() which
is
> implemented in this class, instead of subclass.
> If moving thread check to subclass, I think there is no way to check
> SurfaceFactoryOzone::GetInstance() 

GetInstance() is static. The thread usage restriction is a restriction of the
implementation of this interface, not the interface itself.

In general, it's nice to keep this as an interface because it allows an
implementation to cleanly implement not just this interface. When you add a
member here it becomes more of a base class and I would expect a "is-a"
relationship between this class and a derived class.

[dshwang, 2015-04-02 14:24:49]

dongseong.hwang@intel.com changed reviewers:
+ lcwu@chromium.org, sievers@chromium.org

[dshwang, 2015-04-02 14:26:27]

alexst@, could you review it in terms of GpuMemoryBuffer design?
spang@, could you review ui/ ?
sievers@, could you review content/ ?
lcwu1@, could you review chromecast/ ?

https://codereview.chromium.org/1050923003/diff/1/ui/ozone/public/surface_fac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/1050923003/diff/1/ui/ozone/public/surface_fac...
ui/ozone/public/surface_factory_ozone.h:171: base::ThreadChecker
thread_checker_;
On 2015/04/02 12:35:39, reveman wrote:
> On 2015/04/02 07:56:05, dshwang wrote:
> > On 2015/04/01 19:22:20, reveman wrote:
> > > A thread checker is good but please move it to the implementation instead
of
> > the
> > > interface.
> > 
> > This thread check is added to check SurfaceFactoryOzone::GetInstance() which
> is
> > implemented in this class, instead of subclass.
> > If moving thread check to subclass, I think there is no way to check
> > SurfaceFactoryOzone::GetInstance() 
> 
> GetInstance() is static. The thread usage restriction is a restriction of the
> implementation of this interface, not the interface itself.
> 
> In general, it's nice to keep this as an interface because it allows an
> implementation to cleanly implement not just this interface. When you add a
> member here it becomes more of a base class and I would expect a "is-a"
> relationship between this class and a derived class.

Alright, I move thread check to each implementation.

[reveman, 2015-04-02 14:41:58]

reveman@chromium.org changed reviewers:
- lcwu@chromium.org, sievers@chromium.org, spang@chromium.org

[reveman, 2015-04-02 14:41:59]

-lcwu
-sievers
-spang

Please wait for alexst@'s feedback before you move forward with this. I think he
has some important performance concerns that might prevent us from being able to
make this change.

[dshwang, 2015-04-02 15:07:36]

On 2015/04/02 14:41:59, reveman wrote:
> -lcwu
> -sievers
> -spang
> 
> Please wait for alexst@'s feedback before you move forward with this. I think
he
> has some important performance concerns that might prevent us from being able
to
> make this change.

As I mentioned #1, I thought there is might be performance issue, because now
GpuMemoryBuffer creation needs synchronous communication IO-to-Main-to-IO thread
in Gpu process.
However, IMHO, it's inevitable cost to make sure thread correctness for each
platform backend of GpuMemoryBuffer.
For example, currently platform backend of GpuMemoryBuffer is deleted sometimes
by main thread, and sometimes by IO thread, because both GL Image destructor and
DestroyGpuMemoryBuffer message can delete it, even though IO thread always
create the backend. We cannot guarantee it's correct in all platforms.

In addition, one-copy and zero-copy reuse resources by ResourcePool, so I think
occasional GpuMemoryBuffer creation is not very performance sensitive.
I'm waiting for alexst@'s feedback.

[reveman, 2015-04-02 15:30:06]

On 2015/04/02 at 15:07:36, dongseong.hwang wrote:
> On 2015/04/02 14:41:59, reveman wrote:
> > -lcwu
> > -sievers
> > -spang
> > 
> > Please wait for alexst@'s feedback before you move forward with this. I
think he
> > has some important performance concerns that might prevent us from being
able to
> > make this change.
> 
> As I mentioned #1, I thought there is might be performance issue, because now
GpuMemoryBuffer creation needs synchronous communication IO-to-Main-to-IO thread
in Gpu process.
> However, IMHO, it's inevitable cost to make sure thread correctness for each
platform backend of GpuMemoryBuffer.
> For example, currently platform backend of GpuMemoryBuffer is deleted
sometimes by main thread, and sometimes by IO thread, because both GL Image
destructor and DestroyGpuMemoryBuffer message can delete it, even though IO
thread always create the backend. We cannot guarantee it's correct in all
platforms.

This performance issue would affect the UI compositor and not just renderers.
That's why we have to be careful here. alexst@ can provide more details.

I think we can require implementations to support use on multiple threads.
GpuMemoryBuffers are meant to be used with multiple processes. I have a hard
time seeing how an implementation that support multiple processes would not be
able to support multiple threads.

> 
> In addition, one-copy and zero-copy reuse resources by ResourcePool, so I
think occasional GpuMemoryBuffer creation is not very performance sensitive.
> I'm waiting for alexst@'s feedback.

[alexst (slow to review), 2015-04-02 16:41:15]

On 2015/04/02 15:30:06, reveman wrote:
> On 2015/04/02 at 15:07:36, dongseong.hwang wrote:
> > On 2015/04/02 14:41:59, reveman wrote:
> > > -lcwu
> > > -sievers
> > > -spang
> > > 
> > > Please wait for alexst@'s feedback before you move forward with this. I
> think he
> > > has some important performance concerns that might prevent us from being
> able to
> > > make this change.
> > 
> > As I mentioned #1, I thought there is might be performance issue, because
now
> GpuMemoryBuffer creation needs synchronous communication IO-to-Main-to-IO
thread
> in Gpu process.
> > However, IMHO, it's inevitable cost to make sure thread correctness for each
> platform backend of GpuMemoryBuffer.
> > For example, currently platform backend of GpuMemoryBuffer is deleted
> sometimes by main thread, and sometimes by IO thread, because both GL Image
> destructor and DestroyGpuMemoryBuffer message can delete it, even though IO
> thread always create the backend. We cannot guarantee it's correct in all
> platforms.
> 
> This performance issue would affect the UI compositor and not just renderers.
> That's why we have to be careful here. alexst@ can provide more details.
> 
> I think we can require implementations to support use on multiple threads.
> GpuMemoryBuffers are meant to be used with multiple processes. I have a hard
> time seeing how an implementation that support multiple processes would not be
> able to support multiple threads.
> 
> > 
> > In addition, one-copy and zero-copy reuse resources by ResourcePool, so I
> think occasional GpuMemoryBuffer creation is not very performance sensitive.
> > I'm waiting for alexst@'s feedback.

I am not necessarily a fan of this thread split code myself, I don't think
moving everything to main thread is the right answer.

Some context is important here because it was a deliberate decision to allocate
on IO.

While it isn't so critical if a renderer sits there blocked while GPU main is
processing, this call is also made from the UI thread in the browser compositor,
and that particular thread is to remain fast at all times. Bouncing off only gpu
IO is the way to ensure that. The alternative allows for a compromised or
otherwise slow renderer to block the browser process from displaying UI or even
deadlocking if something wedges GPU main. This should never happen.

We've talked about alternatives along the lines of 
-allocating only a sync point on IO thread and
 posting a task to main for actual allocation and
 later using it as a fence for glimage creation
-splitting off browser and renderer allocations
-splitting of scanout vs zero copy buffers
-even forbidding zero copy buffers allocated from the browser compositor

All these introduce a fair bit more complexity and IO allocation was the
trade-off we made.

I'd also like to echo Dave's point, GpuMemoryBuffers are meant to be used with
multiple processes, multiple threads are a subset of that and I think requiring
this of the implementation is not unreasonable.

[dshwang, 2015-04-06 17:35:13]

Patchset #3 (id:40001) has been deleted

[dshwang, 2015-04-06 17:57:45]

On 2015/04/02 16:41:15, alexst wrote:
> I am not necessarily a fan of this thread split code myself, I don't think
> moving everything to main thread is the right answer.
> 
> Some context is important here because it was a deliberate decision to
allocate
> on IO.
> 
> While it isn't so critical if a renderer sits there blocked while GPU main is
> processing, this call is also made from the UI thread in the browser
compositor,
> and that particular thread is to remain fast at all times. Bouncing off only
gpu
> IO is the way to ensure that. The alternative allows for a compromised or
> otherwise slow renderer to block the browser process from displaying UI or
even
> deadlocking if something wedges GPU main. This should never happen.
> 
> We've talked about alternatives along the lines of 
> -allocating only a sync point on IO thread and
>  posting a task to main for actual allocation and
>  later using it as a fence for glimage creation
> -splitting off browser and renderer allocations
> -splitting of scanout vs zero copy buffers
> -even forbidding zero copy buffers allocated from the browser compositor
> 
> All these introduce a fair bit more complexity and IO allocation was the
> trade-off we made.
> 
> I'd also like to echo Dave's point, GpuMemoryBuffers are meant to be used with
> multiple processes, multiple threads are a subset of that and I think
requiring
> this of the implementation is not unreasonable.

Thank you for explanation behind design. I understand why IO thread must create
GpuMemoryBuffer.

I change the goal of this CL. This CL moves GpuMemoryBuffer to main thread after
creation in IO thread.
It's because current impl is thread-error-prone.

For example, NativePixmap for Ozone GpuMemoryBuffer has several methods;
GetEGLClientBuffer(), GetDmaBufFd(), GetDmaBufPitch(), ~NativePixmap()
Those aren't implemented as thread-safe. This CL makes those be called in only
main thread.

For another example, SurfaceTexture for Android GpuMemoryBuffer has
UpdateTexImage(), ReleaseTexImage(), ~SurfaceTexture()
This CL makes those be called in only main thread also.

Those methods except for destructor are already called in the main thread. This
CL clarifies those supposed to be called in only main thread.

Currently, destructor can be called in both main thread and io thread. If GL
Image refers to final instance the backend instance (e.g.
glCreateGpuMemoryBufferImageCHROMIUM), main thread destorys the backend.
Otherwise (e.g. glCreateImageCHROMIUM), IO thread destroys the backend.
This CL makes destructor of the backend always be called in main thread.

[reveman, 2015-04-06 18:20:20]

On 2015/04/06 at 17:57:45, dongseong.hwang wrote:
> On 2015/04/02 16:41:15, alexst wrote:
> > I am not necessarily a fan of this thread split code myself, I don't think
> > moving everything to main thread is the right answer.
> > 
> > Some context is important here because it was a deliberate decision to
allocate
> > on IO.
> > 
> > While it isn't so critical if a renderer sits there blocked while GPU main
is
> > processing, this call is also made from the UI thread in the browser
compositor,
> > and that particular thread is to remain fast at all times. Bouncing off only
gpu
> > IO is the way to ensure that. The alternative allows for a compromised or
> > otherwise slow renderer to block the browser process from displaying UI or
even
> > deadlocking if something wedges GPU main. This should never happen.
> > 
> > We've talked about alternatives along the lines of 
> > -allocating only a sync point on IO thread and
> >  posting a task to main for actual allocation and
> >  later using it as a fence for glimage creation
> > -splitting off browser and renderer allocations
> > -splitting of scanout vs zero copy buffers
> > -even forbidding zero copy buffers allocated from the browser compositor
> > 
> > All these introduce a fair bit more complexity and IO allocation was the
> > trade-off we made.
> > 
> > I'd also like to echo Dave's point, GpuMemoryBuffers are meant to be used
with
> > multiple processes, multiple threads are a subset of that and I think
requiring
> > this of the implementation is not unreasonable.
> 
> Thank you for explanation behind design. I understand why IO thread must
create GpuMemoryBuffer.
> 
> I change the goal of this CL. This CL moves GpuMemoryBuffer to main thread
after creation in IO thread.
> It's because current impl is thread-error-prone.

The use of concepts such as main thread or IO thread is a layering violation for
a lot of this code.

The current requirement for these object are; allocated/destroy on any thread
and use on one thread only, with responsibility to ensure there are no races on
the user. Is there a problem with this?

Some threadchecker instances to validate this behavior wouldn't hurt but I'm
failing to see why we need to make any other changes.

> 
> For example, NativePixmap for Ozone GpuMemoryBuffer has several methods;
GetEGLClientBuffer(), GetDmaBufFd(), GetDmaBufPitch(), ~NativePixmap()
> Those aren't implemented as thread-safe. This CL makes those be called in only
main thread.
> 
> For another example, SurfaceTexture for Android GpuMemoryBuffer has
UpdateTexImage(), ReleaseTexImage(), ~SurfaceTexture()
> This CL makes those be called in only main thread also.
> 
> Those methods except for destructor are already called in the main thread.
This CL clarifies those supposed to be called in only main thread.
> 
> Currently, destructor can be called in both main thread and io thread. If GL
Image refers to final instance the backend instance (e.g.
glCreateGpuMemoryBufferImageCHROMIUM), main thread destorys the backend.
Otherwise (e.g. glCreateImageCHROMIUM), IO thread destroys the backend.
> This CL makes destructor of the backend always be called in main thread.

[dshwang, 2015-04-07 05:03:59]

Patchset #4 (id:80001) has been deleted

[dshwang, 2015-04-07 05:08:00]

On 2015/04/06 18:20:20, reveman wrote:
> The current requirement for these object are; allocated/destroy on any thread
> and use on one thread only, with responsibility to ensure there are no races
on
> the user. Is there a problem with this?

Now I understand code base. It looks good. I thought code was intended to use
these object on any thread.
It makes sense to only allocate/destroy on any thread

> Some threadchecker instances to validate this behavior wouldn't hurt but I'm
> failing to see why we need to make any other changes.

I see. I change the goal of this CL again. Just add thread check to clarify it.

[reveman, 2015-04-07 13:07:05]

sorry, I'm failing to see how all these ui/ozone changes are related. is it not
GLImage creation and usage that we should verify is always happening on the same
thread.

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:162:
DCHECK(thread_checker_.CalledOnValidThread());
I don't think we want a thread checker here as that prevents this function from
being called on any thread.

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc:108:
DCHECK(thread_checker_.CalledOnValidThread());
ditto

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/gpu/gpu_memory...
File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/gpu/gpu_memory...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:169:
DCHECK(thread_checker_.CalledOnValidThread());
ditto

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/gpu/gpu_memory...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:170:
scoped_refptr<NativePixmap> pixmap(nullptr);
what's wrong with the previous code?

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/platform/caca/...
File ui/ozone/platform/caca/caca_window_manager.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/platform/caca/...
ui/ozone/platform/caca/caca_window_manager.cc:121:
DCHECK(thread_checker_.CalledOnValidThread());
how are the DCHECKs in this file related to GpuMemoryBuffer usage on one thread?

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/platform/drm/d...
File ui/ozone/platform/drm/drm_surface_factory.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/platform/drm/d...
ui/ozone/platform/drm/drm_surface_factory.cc:28:
DCHECK(thread_checker_.CalledOnValidThread());
ditto. how is this related?

[dshwang, 2015-04-07 14:04:27]

On 2015/04/07 13:07:05, reveman wrote:
> sorry, I'm failing to see how all these ui/ozone changes are related. is it
not
> GLImage creation and usage that we should verify is always happening on the
same
> thread.

Thx for patient review. I wanted to clarify SurfaceFactoryOzone API must be
called on the main thread except for CreateNativePixmap() API. It looks
not-related to this CL. I'll separate it.

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:162:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/04/07 13:07:05, reveman wrote:
> I don't think we want a thread checker here as that prevents this function
from
> being called on any thread.

We allow CreateGpuMemoryBuffer() to be called on any thread, but we allow
CreateImageForGpuMemoryBuffer() to be called on only main thread because only
client API can create GLImage as following call stack

#2 0x7fe562138d61
content::GpuMemoryBufferFactoryIOSurface::CreateImageForGpuMemoryBuffer()
#3 0x7fe562bd608f content::GpuChannel::CreateImageForGpuMemoryBuffer()
#4 0x7fe562be06f2 content::GpuCommandBufferStub::OnCreateImage()
#5 0x7fe562be5ad5 content::GpuCommandBufferStub::OnMessageReceived()

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_surface_texture.cc:108:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/04/07 13:07:05, reveman wrote:
> ditto

ditto

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/gpu/gpu_memory...
File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/gpu/gpu_memory...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:169:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/04/07 13:07:05, reveman wrote:
> ditto

ditto

https://codereview.chromium.org/1050923003/diff/90020/ui/ozone/gpu/gpu_memory...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:170:
scoped_refptr<NativePixmap> pixmap(nullptr);
On 2015/04/07 13:07:05, reveman wrote:
> what's wrong with the previous code?

nothing wrong. I'll roll back

[dshwang, 2015-04-07 14:14:04]

Patchset #5 (id:110001) has been deleted

[reveman, 2015-04-07 14:22:00]

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:162:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/04/07 14:04:26, dshwang wrote:
> On 2015/04/07 13:07:05, reveman wrote:
> > I don't think we want a thread checker here as that prevents this function
> from
> > being called on any thread.
> 
> We allow CreateGpuMemoryBuffer() to be called on any thread, but we allow
> CreateImageForGpuMemoryBuffer() to be called on only main thread because only
> client API can create GLImage as following call stack
> 
> #2 0x7fe562138d61
> content::GpuMemoryBufferFactoryIOSurface::CreateImageForGpuMemoryBuffer()
> #3 0x7fe562bd608f content::GpuChannel::CreateImageForGpuMemoryBuffer()
> #4 0x7fe562be06f2 content::GpuCommandBufferStub::OnCreateImage()
> #5 0x7fe562be5ad5 content::GpuCommandBufferStub::OnMessageReceived()

Sorry I misread this change.

Yes, image creation is always happening on the same thread. However, there's
nothing about this code that restricts that. Wouldn't it make more sense to have
this thread checker in the GLImage implementations?  The GLImage implementations
with this restriction.

[dshwang, 2015-04-07 14:54:48]

On 2015/04/07 14:22:00, reveman wrote:
>
https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
> File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):
> 
>
https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
> content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:162:
> DCHECK(thread_checker_.CalledOnValidThread());
> On 2015/04/07 14:04:26, dshwang wrote:
> > On 2015/04/07 13:07:05, reveman wrote:
> > > I don't think we want a thread checker here as that prevents this function
> > from
> > > being called on any thread.
> > 
> > We allow CreateGpuMemoryBuffer() to be called on any thread, but we allow
> > CreateImageForGpuMemoryBuffer() to be called on only main thread because
only
> > client API can create GLImage as following call stack
> > 
> > #2 0x7fe562138d61
> > content::GpuMemoryBufferFactoryIOSurface::CreateImageForGpuMemoryBuffer()
> > #3 0x7fe562bd608f content::GpuChannel::CreateImageForGpuMemoryBuffer()
> > #4 0x7fe562be06f2 content::GpuCommandBufferStub::OnCreateImage()
> > #5 0x7fe562be5ad5 content::GpuCommandBufferStub::OnMessageReceived()
> 
> Sorry I misread this change.
> 
> Yes, image creation is always happening on the same thread. However, there's
> nothing about this code that restricts that.

Indeed, but CreateImageForGpuMemoryBuffer() is reason why all
GpuMemoryBufferFactory{Platform} implementation has a mutex.
I add thread check to document why these classes have a lock, rather than
restriction of API.
When CreateImageForGpuMemoryBuffer() will be used in any thread, we can remove
thread check, but I expect it will not happen.

> Wouldn't it make more sense to have
> this thread checker in the GLImage implementations?  The GLImage
implementations
> with this restriction.

GLImage is too obvious belonging to main thread like TextureManager or
something, so I doubt it's worth to add thread check.

[reveman, 2015-04-07 16:19:01]

On 2015/04/07 at 14:54:48, dongseong.hwang wrote:
> On 2015/04/07 14:22:00, reveman wrote:
> >
https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
> > File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):
> > 
> >
https://codereview.chromium.org/1050923003/diff/90020/content/common/gpu/gpu_...
> > content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:162:
> > DCHECK(thread_checker_.CalledOnValidThread());
> > On 2015/04/07 14:04:26, dshwang wrote:
> > > On 2015/04/07 13:07:05, reveman wrote:
> > > > I don't think we want a thread checker here as that prevents this
function
> > > from
> > > > being called on any thread.
> > > 
> > > We allow CreateGpuMemoryBuffer() to be called on any thread, but we allow
> > > CreateImageForGpuMemoryBuffer() to be called on only main thread because
only
> > > client API can create GLImage as following call stack
> > > 
> > > #2 0x7fe562138d61
> > > content::GpuMemoryBufferFactoryIOSurface::CreateImageForGpuMemoryBuffer()
> > > #3 0x7fe562bd608f content::GpuChannel::CreateImageForGpuMemoryBuffer()
> > > #4 0x7fe562be06f2 content::GpuCommandBufferStub::OnCreateImage()
> > > #5 0x7fe562be5ad5 content::GpuCommandBufferStub::OnMessageReceived()
> > 
> > Sorry I misread this change.
> > 
> > Yes, image creation is always happening on the same thread. However, there's
> > nothing about this code that restricts that.
> 
> Indeed, but CreateImageForGpuMemoryBuffer() is reason why all
GpuMemoryBufferFactory{Platform} implementation has a mutex.
> I add thread check to document why these classes have a lock, rather than
restriction of API.
> When CreateImageForGpuMemoryBuffer() will be used in any thread, we can remove
thread check, but I expect it will not happen.

There's nothing in the current factory code that prevents it from being used on
any thread. I'd rather have these thread checks next to the code that can't be
used on any thread as that would make it much easier to understand why they are
needed.

> 
> > Wouldn't it make more sense to have
> > this thread checker in the GLImage implementations?  The GLImage
implementations
> > with this restriction.
> 
> GLImage is too obvious belonging to main thread like TextureManager or
something, so I doubt it's worth to add thread check.

[dshwang, 2015-04-07 16:50:38]

On 2015/04/07 16:19:01, reveman wrote:
> > Indeed, but CreateImageForGpuMemoryBuffer() is reason why all
> GpuMemoryBufferFactory{Platform} implementation has a mutex.
> > I add thread check to document why these classes have a lock, rather than
> restriction of API.
> > When CreateImageForGpuMemoryBuffer() will be used in any thread, we can
remove
> thread check, but I expect it will not happen.
> 
> There's nothing in the current factory code that prevents it from being used
on
> any thread. I'd rather have these thread checks next to the code that can't be
> used on any thread as that would make it much easier to understand why they
are
> needed.

I see. I moved thread check to GLImage code.

[reveman, 2015-04-07 17:34:44]

lgtm

[alexst (slow to review), 2015-04-07 17:51:16]

lgtm

[dshwang, 2015-04-08 06:04:04]

dongseong.hwang@intel.com changed reviewers:
+ sievers@chromium.org

[dshwang, 2015-04-08 06:04:43]

Thanks for reviewing!

sievers@, could you review content/common/gpu/gpu_channel_manager.cc ?

[no sievers, 2015-04-08 18:51:35]

lgtm stamp for adding comment

https://codereview.chromium.org/1050923003/diff/150001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/1050923003/diff/150001/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.cc:63: // Bouncing off only GPU IO is the
way to ensure that.
nit: What is this exactly trying to say? That we are handling this on the IO
thread directly, since it's a synchronous method (and we don't want to block the
client for too long)?

[dshwang, 2015-04-09 04:06:18]

thx for reviewing

https://codereview.chromium.org/1050923003/diff/150001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/1050923003/diff/150001/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.cc:63: // Bouncing off only GPU IO is the
way to ensure that.
On 2015/04/08 18:51:35, sievers wrote:
> nit: What is this exactly trying to say? That we are handling this on the IO
> thread directly, since it's a synchronous method (and we don't want to block
the
> client for too long)?

Main thread also can handle synchronous message while it requires IO->Main->IO
bouncing. I tried to move GpuMsg_CreateGpuMemoryBuffer handling to main thread
in patch set 1, but alexst@ rejected due to above reason.

I changed the comment to be more understandable.

  // GPU IO thread bounces off GpuMsg_CreateGpuMemoryBuffer message, because
  // the UI thread in the browser process must remain fast at all times.

[dshwang, 2015-04-09 04:06:39]

The CQ bit was checked by dongseong.hwang@intel.com

[dshwang, 2015-04-09 04:06:39]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
reveman@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1050923003/#ps170001
(title: "rephrase comment in gpu_channel_manager.cc")

[commit-bot: I haz the power, 2015-04-09 04:07:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1050923003/170001

[commit-bot: I haz the power, 2015-04-09 05:34:43]

Committed patchset #7 (id:170001)

[commit-bot: I haz the power, 2015-04-09 05:35:29]

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