ozone: Introduce ClientPixmap and ClientPixmapFactory for non-GPU processes.

ozone: Introduce ClientPixmap and ClientPixmapFactory for non-GPU processes.

It's part of "native GpuMemoryBuffer on ChromeOS Freon" implementation.
 * Design doc: https://docs.google.com/document/d/1qpLLo4mBkzHBh5cuBtBjJZAzXK2X9BgBJtpESh-mNn8

Renderer needs to use Ozone platform interface because of native
GpuMemoryBuffer support. However 99% of OzonePlatform isn't usable
from the Renderer. So this CL introduces ClientPixmapFactory.

ClientPixmapFactory allows external implementations to
hook into Chromium to provide a client pixmap for non-GPU processes.

GPU process creates NativePixmap and sends GpuMemoryBufferHandle to Browser
or Renderer. The non-GPU processes will import a client pixmap from the handle in
https://codereview.chromium.org/1134993003/

To demonstrate why this interface is needed, make
GpuMemoryBufferFactoryOzoneNativeBuffer use this interface.
IsGpuMemoryBufferConfigurationSupported() must answer if the configuration is
supported with respect to each Ozone platform, but currently the answer is hardcoded,
which is true only on Ozone GBM. This CL makes each Ozone platform answer this question.

Browser and Renderer have the singleton ClientPixmapFactory instance like Android and MacOS.

For more information why a pixmap is created by SurfaceFactoryOzone, instead of ClientPixmapFactory,

(1) The privileged (GPU process) API.
  This is SurfaceFactoryOzone. It's used to allocate buffer objects and swap onto
  a surface/display, etc. By design there's only one process using this API. It's
  extremely driver specific under the hood, including in userspace.
  CreateNativePixmap goes here.

(2) The unprivileged API.
  This is ClientPixmapFactory. It can only take handles to existing objects and
  map them, and it should work under the sandbox. It's only driver specific
  in the kernel; the userland part doesn't require loading drivers.
  ImportClientPixmap goes here.

TEST=content_unittests --gtest_filter=GpuMemoryBuffer* --ozone-platform=gbm --ozone-use-surfaceless
Run amd64_generic_freon image
BUG=475633
Committed: https://crrev.com/16f5a8a44f4a2c10086263775e5be20669810867
Cr-Commit-Position: refs/heads/master@{#342094}

[dshwang, 2015-05-15 19:25:15]

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

[dshwang, 2015-05-15 19:25:15]

spang, could you review ui/ozone?
sievers, could you review content/?

reveman, when I address your comment in
https://codereview.chromium.org/1134993003/, I realize this CL is prerequisite.
Could you review overall direction of this CL?

[spang, 2015-05-15 20:44:17]

I think we should initialize a smaller object than OzonePlatform since 99% of it
isn't usable from the renderer.

Longer term I think even browser / gpu interfaces should be better separated,
but let's not make it worse by throwing renderer into the mix.

[spang, 2015-05-15 20:44:19]

I think we should initialize a smaller object than OzonePlatform since 99% of it
isn't usable from the renderer.

Longer term I think even browser / gpu interfaces should be better separated,
but let's not make it worse by throwing renderer into the mix.

[dshwang, 2015-05-19 11:29:57]

On 2015/05/15 20:44:19, spang wrote:
> I think we should initialize a smaller object than OzonePlatform since 99% of
it
> isn't usable from the renderer.
> 
> Longer term I think even browser / gpu interfaces should be better separated,
> but let's not make it worse by throwing renderer into the mix.

Thank you for advice. I'll add OzoneClient for renderer and browser process.
I'll create platform OzeonClient in the same manner how
OzonePlatform::CreateInstance() creates each OzonePlatform.
Following is strawman proposal. WDYT?

// Base class for Ozone client implementations. Render process and UI process
// use OzoneClient instead of OzonePlatform to access Ozone specific
// features. e.g. share a surface created by GPU process
//
// UI process uses both OzoneClient and OzonePlatform but the purpose is
// different. UI process initializes subsystems (e.g. events, surface) via
// OzonePlatform, while OzoneClient is used for ozone client purpose.
// (e.g. surface sharing)
//
// Each Ozone platforms override this class and implement the virtual
// GetFooFactoryOzone() methods to provide implementations of the
// various ozone interfaces.
//
// A platform is free to use different implementations of each
// interface depending on the context. You can, for example, create
// different objects depending on the underlying hardware, command
// line flags, or whatever is appropriate for the platform.
class OZONE_EXPORT OzoneClient {
 public:
  static OzoneClient* GetInstance();

  // Factory getters to override in subclasses. The returned objects will be
  // injected into the appropriate layer at startup. Subclasses should not
  // inject these objects themselves. Ownership is retained by OzoneClient.
  virtual SurfaceClientFactoryOzone* GetSurfaceClientFactoryOzone();

 private:
  DISALLOW_COPY_AND_ASSIGN(OzoneClient);
};

[dshwang, 2015-05-19 18:42:34]

Hi, I introduce OzoneClient because OzonePlatform is too big for Renderer.
I explain how Renderer will use this interface in design doc.
Please see 'Control flow' section in the "native GpuMemoryBuffer on ChromeOS"
document;
https://docs.google.com/document/d/1qpLLo4mBkzHBh5cuBtBjJZAzXK2X9BgBJtpESh-mNn8

Could you review again?

[dshwang, 2015-05-20 14:31:10]

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

[dshwang, 2015-05-26 07:50:37]

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

[dshwang, 2015-05-26 17:30:49]

spang, alexst, Renderer needs to access ozone interface, so this CL introduces
OzoneClient. could you review?

[spang, 2015-06-01 22:46:41]

sorry for the delay

https://codereview.chromium.org/1128113011/diff/60001/ui/ozone/ozone.gyp
File ui/ozone/ozone.gyp (right):

https://codereview.chromium.org/1128113011/diff/60001/ui/ozone/ozone.gyp#newc...
ui/ozone/ozone.gyp:190: '--typename=OzoneClient',
No need to add another invocation of the script.

You can pass multiple typenames & includes to the other one.

https://codereview.chromium.org/1128113011/diff/60001/ui/ozone/public/ozone_c...
File ui/ozone/public/ozone_client.h (right):

https://codereview.chromium.org/1128113011/diff/60001/ui/ozone/public/ozone_c...
ui/ozone/public/ozone_client.h:40: static void InitializeForRenderer();
Why are these functions separate but do the same thing?

I wasn't suggesting to copy the existing pattern of having a singleton with
multiple initializers depending on process. This is what I want to avoid.

If the object has only one purpose it doesn't matter which process we're in. The
processes that want one will create one, and the processes that don't need one
won't.

I think we should just remove OzoneClient, and do something like:

// ui/ozone/public/native_pixmap_client.h
class NativePixmapClient {
public:
  static scoped_ptr<NativePixmapClient> Create();

  bool Initialize( /* TBD args - need a way to pass in vgem fd */ );
 
  static scoped_refptr<NativePixmap> ImportNativePixmap(
      base::FileDescriptor handle,
      const gfx::Size& size,
      BufferFormat format);
};

// ui/ozone/public/native_pixmap_client.cc
scoped_ptr<NativePixmapClient> NativePixmapClient::Create() {
  return PlatformObject<NativePixmapClient>::Create();
}

The renderer would make one of these and use it to import buffers. We need to
discuss how to initialize it with the vgem fd obviously.

I probably got some of the details wrong, but does this seem doable?

[dshwang, 2015-06-03 14:11:51]

Hi spang, Thank you for reviewing. I replace OzoneClient to NativePixmapClient
as you reviewed. OzoneClient was introduced for more general purpose, but it's
over-generalization. NativePixmapClient is good idea!
Could you review again?

https://codereview.chromium.org/1128113011/diff/60001/ui/ozone/ozone.gyp
File ui/ozone/ozone.gyp (right):

https://codereview.chromium.org/1128113011/diff/60001/ui/ozone/ozone.gyp#newc...
ui/ozone/ozone.gyp:190: '--typename=OzoneClient',
On 2015/06/01 22:46:41, spang wrote:
> No need to add another invocation of the script.
> 
> You can pass multiple typenames & includes to the other one.

Done.

https://codereview.chromium.org/1128113011/diff/60001/ui/ozone/public/ozone_c...
File ui/ozone/public/ozone_client.h (right):

https://codereview.chromium.org/1128113011/diff/60001/ui/ozone/public/ozone_c...
ui/ozone/public/ozone_client.h:40: static void InitializeForRenderer();
On 2015/06/01 22:46:41, spang wrote:
> Why are these functions separate but do the same thing?
> 
> I wasn't suggesting to copy the existing pattern of having a singleton with
> multiple initializers depending on process. This is what I want to avoid.
> 
> If the object has only one purpose it doesn't matter which process we're in.
The
> processes that want one will create one, and the processes that don't need one
> won't.
> 
> I think we should just remove OzoneClient, and do something like:
> 
> // ui/ozone/public/native_pixmap_client.h
> class NativePixmapClient {
> public:
>   static scoped_ptr<NativePixmapClient> Create();
> 
>   bool Initialize( /* TBD args - need a way to pass in vgem fd */ );
>  
>   static scoped_refptr<NativePixmap> ImportNativePixmap(
>       base::FileDescriptor handle,
>       const gfx::Size& size,
>       BufferFormat format);
> };
> 
> // ui/ozone/public/native_pixmap_client.cc
> scoped_ptr<NativePixmapClient> NativePixmapClient::Create() {
>   return PlatformObject<NativePixmapClient>::Create();
> }
> 
> The renderer would make one of these and use it to import buffers. We need to
> discuss how to initialize it with the vgem fd obviously.

Sure, I comments future plan in NativePixmapClientGbm::Initialize()

> I probably got some of the details wrong, but does this seem doable?

Nice idea!

https://codereview.chromium.org/1128113011/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1395: #endif
Need to pass this command line to render process to let render process know
which ozone platform is used to initialize right ozone platform.

https://codereview.chromium.org/1128113011/diff/80001/content/child/child_gpu...
File content/child/child_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/content/child/child_gpu...
content/child/child_gpu_memory_buffer_manager.cc:48:
ui::NativePixmapClient::InitializeIfNeeded();
As you mentioned "The processes that want one will create one, and the processes
that don't need one won't.", this interface is initialized very lazily when it's
needed for Renderer.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/native_pixmap_client_gbm.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/native_pixmap_client_gbm.cc:20: // TODO(dshwang): get
VGEM fd. crbug.com/475633
This method will get VGEM fd from DrmDisplayHostManager in Browser process.
It depends on https://codereview.chromium.org/1151533002/

Render process and Browser process needs different implementation because
NativePixmapClientGbm in Render process needs API mechanism. It might requires
different Initialize methods. Let's discuss more detail in the future CL.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/ozone_p...
File ui/ozone/public/ozone_platform.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/ozone_p...
ui/ozone/public/ozone_platform.cc:47: NativePixmapClient::InitializeIfNeeded();
Browser process always initializes it unlike Renderer.

[dshwang, 2015-06-05 07:54:59]

spang, could you review again? thank you.

[spang, 2015-06-05 19:21:54]

https://codereview.chromium.org/1128113011/diff/80001/content/child/child_gpu...
File content/child/child_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/content/child/child_gpu...
content/child/child_gpu_memory_buffer_manager.cc:48:
ui::NativePixmapClient::InitializeIfNeeded();
On 2015/06/03 14:11:51, dshwang wrote:
> As you mentioned "The processes that want one will create one, and the
processes
> that don't need one won't.", this interface is initialized very lazily when
it's
> needed for Renderer.

Lazy initialization leads to unpredictable initialization order.

Please do it eagerly and only once.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/platform/test/...
File ui/ozone/platform/test/ozone_platform_test.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/platform/test/...
ui/ozone/platform/test/ozone_platform_test.cc:133: return new
NativePixmapClientTest;
Make a function 

// native_pixmap_client.h
OZONE_EXPORT
scoped_ptr<NativePixmapClient> CreateStubNativePixmapClient();

// native_pixmap_client.cc
class StubNativePixmapClient {
  // ...
}

scoped_ptr<NativePixmapClient> CreateStubNativePixmapClient() {
  return new StubNativePixmapClient;
}

and use that for platforms that don't implement it.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.cc:19: NativePixmapClient* g_instance =
nullptr;
I would prefer if this object was managed inside content/common/gpu/...

It doesn't seem like it needs to be shared with anything else.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.h (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.h:32: virtual std::vector<Configuration>
GetSupportedNativePixmapConfigurations()
Is this used from the renderer? (Does it need to move?)

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/ozone_p...
File ui/ozone/public/ozone_platform.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/ozone_p...
ui/ozone/public/ozone_platform.cc:47: NativePixmapClient::InitializeIfNeeded();
On 2015/06/03 14:11:51, dshwang wrote:
> Browser process always initializes it unlike Renderer.

Why does the browser need it? Is there a case where GPU memory buffers are
mapped in the browser?

[reveman, 2015-06-05 20:26:07]

https://codereview.chromium.org/1128113011/diff/80001/content/child/child_gpu...
File content/child/child_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/content/child/child_gpu...
content/child/child_gpu_memory_buffer_manager.cc:48:
ui::NativePixmapClient::InitializeIfNeeded();
On 2015/06/05 at 19:21:53, spang wrote:
> On 2015/06/03 14:11:51, dshwang wrote:
> > As you mentioned "The processes that want one will create one, and the
processes
> > that don't need one won't.", this interface is initialized very lazily when
it's
> > needed for Renderer.
> 
> Lazy initialization leads to unpredictable initialization order.
> 
> Please do it eagerly and only once.

The MacOSX equivalent to NativePixmap is IOSurface and the child process wide
initialization of that is done in content/app/content_main_runner.cc. See
IOSurfaceManager::SetInstance. I think it would be best to do this
NativePixmapClient initialization at the same place.

https://codereview.chromium.org/1128113011/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/1128113011/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:14:
gfx::GpuMemoryBuffer::Format ConvertFormat(
nit: the name ConvertFormat is a bit ambiguous. maybe
FormatFromOzoneBufferFormat instead

https://codereview.chromium.org/1128113011/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:21: default:
nit: avoid a default case if possible

https://codereview.chromium.org/1128113011/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:28:
gfx::GpuMemoryBuffer::Usage ConvertUsage(
nit: maybe UsageFromOzoneBufferUsage

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.cc:19: NativePixmapClient* g_instance =
nullptr;
On 2015/06/05 at 19:21:54, spang wrote:
> I would prefer if this object was managed inside content/common/gpu/...
> 
> It doesn't seem like it needs to be shared with anything else.

I think IOSurfaceManager and SurfaceTextureManager interfaces are good examples
to follow. Those are abstract classes that live in content/common/

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.cc:54: void
NativePixmapClient::CreateInstance() {
why are you not using a normal singleton so that this is thread safe?

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.h (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.h:16: class OZONE_EXPORT NativePixmapClient
{
NativePixmapClient sounds like we'd have one of these instances per NativePixmap
but that's not the case.

This is similar to the IOSurfaceManager interface on MacOS and the
SurfaceTextureManager interface on Android. Would it make sense to call this
NativePixmapManager for consistency?

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.h:33: const;
can this be pure virtual?

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/ozone_p...
File ui/ozone/public/ozone_platform.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/ozone_p...
ui/ozone/public/ozone_platform.cc:47: NativePixmapClient::InitializeIfNeeded();
On 2015/06/05 at 19:21:54, spang wrote:
> On 2015/06/03 14:11:51, dshwang wrote:
> > Browser process always initializes it unlike Renderer.
> 
> Why does the browser need it? Is there a case where GPU memory buffers are
mapped in the browser?

The UI compositor.

[spang, 2015-06-05 22:21:23]

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.h (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.h:16: class OZONE_EXPORT NativePixmapClient
{
On 2015/06/05 20:26:07, reveman wrote:
> NativePixmapClient sounds like we'd have one of these instances per
NativePixmap
> but that's not the case.
> 
> This is similar to the IOSurfaceManager interface on MacOS and the
> SurfaceTextureManager interface on Android. Would it make sense to call this
> NativePixmapManager for consistency?

That sounds good to me.

[dshwang, 2015-06-08 11:15:14]

Thank you for reviewing. I addressed all concerns except for moving
NativePixmapManager to content/
could you review again?

I didn't move NativePixmapManager to content/ because each platform (e.g. gbm,
caca) should implement it. I think we don't want to let content/ know platform
specific, because BUILD.gn and content.gyp in content/ needs to know
ozone_platform_gbm or something.

https://codereview.chromium.org/1128113011/diff/80001/content/child/child_gpu...
File content/child/child_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/content/child/child_gpu...
content/child/child_gpu_memory_buffer_manager.cc:48:
ui::NativePixmapClient::InitializeIfNeeded();
On 2015/06/05 20:26:06, reveman wrote:
> On 2015/06/05 at 19:21:53, spang wrote:
> > On 2015/06/03 14:11:51, dshwang wrote:
> > > As you mentioned "The processes that want one will create one, and the
> processes
> > > that don't need one won't.", this interface is initialized very lazily
when
> it's
> > > needed for Renderer.
> > 
> > Lazy initialization leads to unpredictable initialization order.
> > 
> > Please do it eagerly and only once.
> 
> The MacOSX equivalent to NativePixmap is IOSurface and the child process wide
> initialization of that is done in content/app/content_main_runner.cc. See
> IOSurfaceManager::SetInstance. I think it would be best to do this
> NativePixmapClient initialization at the same place.

Done. Initialize it in render_thread_impl.cc, not content_main_runner.cc,
because gpu process doesn't need it.
btw, the interface doesn't use global or filescope object, so IMO this lazy
initialization isn't related to unpredictable initialization order.

https://codereview.chromium.org/1128113011/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/1128113011/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:14:
gfx::GpuMemoryBuffer::Format ConvertFormat(
On 2015/06/05 20:26:06, reveman wrote:
> nit: the name ConvertFormat is a bit ambiguous. maybe
> FormatFromOzoneBufferFormat instead

Done.

https://codereview.chromium.org/1128113011/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:21: default:
On 2015/06/05 20:26:07, reveman wrote:
> nit: avoid a default case if possible

Done.

https://codereview.chromium.org/1128113011/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:28:
gfx::GpuMemoryBuffer::Usage ConvertUsage(
On 2015/06/05 20:26:06, reveman wrote:
> nit: maybe UsageFromOzoneBufferUsage

Done.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/platform/test/...
File ui/ozone/platform/test/ozone_platform_test.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/platform/test/...
ui/ozone/platform/test/ozone_platform_test.cc:133: return new
NativePixmapClientTest;
On 2015/06/05 19:21:54, spang wrote:
> Make a function 
> 
> // native_pixmap_client.h
> OZONE_EXPORT
> scoped_ptr<NativePixmapClient> CreateStubNativePixmapClient();
> 
> // native_pixmap_client.cc
> class StubNativePixmapClient {
>   // ...
> }
> 
> scoped_ptr<NativePixmapClient> CreateStubNativePixmapClient() {
>   return new StubNativePixmapClient;
> }
> 
> and use that for platforms that don't implement it.

Done.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.cc:19: NativePixmapClient* g_instance =
nullptr;
On 2015/06/05 20:26:07, reveman wrote:
> On 2015/06/05 at 19:21:54, spang wrote:
> > I would prefer if this object was managed inside content/common/gpu/...
> > 
> > It doesn't seem like it needs to be shared with anything else.
> 
> I think IOSurfaceManager and SurfaceTextureManager interfaces are good
examples
> to follow. Those are abstract classes that live in content/common/

IMO it should stay here. NativePixmapManager needs to be implemented for various
platforms. e.g. gbm
I think we don't want to let content/ know which platform is used. WDYT?

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.cc:54: void
NativePixmapClient::CreateInstance() {
On 2015/06/05 20:26:07, reveman wrote:
> why are you not using a normal singleton so that this is thread safe?

Done. nice suggestion.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.h (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.h:16: class OZONE_EXPORT NativePixmapClient
{
On 2015/06/05 22:21:23, spang wrote:
> On 2015/06/05 20:26:07, reveman wrote:
> > NativePixmapClient sounds like we'd have one of these instances per
> NativePixmap
> > but that's not the case.
> > 
> > This is similar to the IOSurfaceManager interface on MacOS and the
> > SurfaceTextureManager interface on Android. Would it make sense to call this
> > NativePixmapManager for consistency?
> 
> That sounds good to me.

Done.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.h:32: virtual std::vector<Configuration>
GetSupportedNativePixmapConfigurations()
On 2015/06/05 19:21:54, spang wrote:
> Is this used from the renderer? (Does it need to move?)

Currently it's used by browser process. Renderer will use it soon.
In addition, each platform (e.g. gbm, drm) supports different configuration.

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.h:33: const;
On 2015/06/05 20:26:07, reveman wrote:
> can this be pure virtual?

Done.

[spang, 2015-06-08 17:47:58]

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.cc:19: NativePixmapClient* g_instance =
nullptr;
On 2015/06/08 11:15:14, dshwang wrote:
> On 2015/06/05 20:26:07, reveman wrote:
> > On 2015/06/05 at 19:21:54, spang wrote:
> > > I would prefer if this object was managed inside content/common/gpu/...
> > > 
> > > It doesn't seem like it needs to be shared with anything else.
> > 
> > I think IOSurfaceManager and SurfaceTextureManager interfaces are good
> examples
> > to follow. Those are abstract classes that live in content/common/
> 
> IMO it should stay here. NativePixmapManager needs to be implemented for
various
> platforms. e.g. gbm
> I think we don't want to let content/ know which platform is used. WDYT?

It's true, we can't move NativePixmapManager implementation.

What I was hoping to move was the instance from the above line (g_instance).
Ideally content would manage its instance(s). We'd expose
NativePixmapManager::Create() rather than NativePixmapManager::GetInstance().

Having it here enforces one instance per address space, but that's not necessary
or even desirable. It may lead to bugs; options like --single-process break when
we do this because then both browser and renderer try to initialize the shared
instance (say, with different copies of the vgem fd).

[dshwang, 2015-06-09 18:37:40]

thank you for reviewing. I have a question. could you feedback?

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
File ui/ozone/public/native_pixmap_client.cc (right):

https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
ui/ozone/public/native_pixmap_client.cc:19: NativePixmapClient* g_instance =
nullptr;
On 2015/06/08 17:47:57, spang wrote:
> On 2015/06/08 11:15:14, dshwang wrote:
> > On 2015/06/05 20:26:07, reveman wrote:
> > > On 2015/06/05 at 19:21:54, spang wrote:
> > > > I would prefer if this object was managed inside content/common/gpu/...
> > > > 
> > > > It doesn't seem like it needs to be shared with anything else.
> > > 
> > > I think IOSurfaceManager and SurfaceTextureManager interfaces are good
> > examples
> > > to follow. Those are abstract classes that live in content/common/
> > 
> > IMO it should stay here. NativePixmapManager needs to be implemented for
> various
> > platforms. e.g. gbm
> > I think we don't want to let content/ know which platform is used. WDYT?
> 
> It's true, we can't move NativePixmapManager implementation.
> 
> What I was hoping to move was the instance from the above line (g_instance).
> Ideally content would manage its instance(s). We'd expose
> NativePixmapManager::Create() rather than NativePixmapManager::GetInstance().
> 
> Having it here enforces one instance per address space, but that's not
necessary
> or even desirable. It may lead to bugs; options like --single-process break
when
> we do this because then both browser and renderer try to initialize the shared
> instance (say, with different copies of the vgem fd).

I understand your concern. However, it's quite complicated problem. I need more
feedback.
If there are a browser process and 3 renderer, You want to create 4
NativePixmapManager instances, even if --single-process is used.
To do it, some unique instance per renderer and browser must hold a
NativePixmapManager instance. e.g. RenderThreadImpl owns a NativePixmapManager
member.
There are two problems
1. NativePixmapManager is ozone specific. RenderThreadImpl will have ozone
specific method with ifdef
2. GpuMemoryBufferImplOzoneNativeBuffer can be used for both browser and
renderer. It would be messy to get right NativePixmapManager instance.

Both are addressable, but code would be ugly. Following code is needed.

--- a/content/child/child_gpu_memory_buffer_manager.cc
+++ b/content/child/child_gpu_memory_buffer_manager.cc
@@ -47,11 +47,18 @@ ChildGpuMemoryBufferManager::AllocateGpuMemoryBuffer(
   if (!success)
     return scoped_ptr<gfx::GpuMemoryBuffer>();
 
+#if defined(USE_OZONE)
+  scoped_ptr<GpuMemoryBufferImpl> buffer(
+      GpuMemoryBufferImplOzoneNativeBuffer::CreateFromHandle(
+          handle, size, format, callback,
+          RenderThreadImpl::current()->GetNativePixmapManager()));
+#else
   scoped_ptr<GpuMemoryBufferImpl> buffer(GpuMemoryBufferImpl::CreateFromHandle(
       handle,
       size,
       format,
       base::Bind(&DeletedGpuMemoryBuffer, sender_, handle.id)));
+#endif
   if (!buffer) {


On the other hands, as considering vgem requirement, it is ok for both browser
and renderer to share singleton instance.
NativePixmapManager initialization will work like as follows:
NativePixmapManager* NativePixmapManager::GetInstance() {
  if (singleton instance exist)
    return the singleton instance;

  create the singleton instance;

  if (browser process) {
    get the vgem fd from DrmDisplayHostManager;
  } else {
    get the vgem fd via IPC;
  }
  return the singleton instance;
}

All NativePixmapManager has to initialize is to ensure vgem fd. In
--single-process mode, Renderer tries to create NativePixmapManager instance but
there is already singleton instance which Browser created. It's ok to share the
singleton instance. I'll add special handling for browser in ozone_platform.cc
in next CL, because browser process needs different initialization.
It will make code clean, but one concern is that we should be aware why
singleton is ok in this case, which seems to make you uneasy.

What do you prefer? I'll go the direction.

[reveman, 2015-06-09 19:31:40]

On 2015/06/09 at 18:37:40, dongseong.hwang wrote:
> thank you for reviewing. I have a question. could you feedback?
> 
>
https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
> File ui/ozone/public/native_pixmap_client.cc (right):
> 
>
https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
> ui/ozone/public/native_pixmap_client.cc:19: NativePixmapClient* g_instance =
nullptr;
> On 2015/06/08 17:47:57, spang wrote:
> > On 2015/06/08 11:15:14, dshwang wrote:
> > > On 2015/06/05 20:26:07, reveman wrote:
> > > > On 2015/06/05 at 19:21:54, spang wrote:
> > > > > I would prefer if this object was managed inside
content/common/gpu/...
> > > > > 
> > > > > It doesn't seem like it needs to be shared with anything else.
> > > > 
> > > > I think IOSurfaceManager and SurfaceTextureManager interfaces are good
> > > examples
> > > > to follow. Those are abstract classes that live in content/common/
> > > 
> > > IMO it should stay here. NativePixmapManager needs to be implemented for
> > various
> > > platforms. e.g. gbm
> > > I think we don't want to let content/ know which platform is used. WDYT?
> > 
> > It's true, we can't move NativePixmapManager implementation.
> > 
> > What I was hoping to move was the instance from the above line (g_instance).
> > Ideally content would manage its instance(s). We'd expose
> > NativePixmapManager::Create() rather than
NativePixmapManager::GetInstance().
> > 
> > Having it here enforces one instance per address space, but that's not
necessary
> > or even desirable. It may lead to bugs; options like --single-process break
when
> > we do this because then both browser and renderer try to initialize the
shared
> > instance (say, with different copies of the vgem fd).
> 
> I understand your concern. However, it's quite complicated problem. I need
more feedback.
> If there are a browser process and 3 renderer, You want to create 4
NativePixmapManager instances, even if --single-process is used.
> To do it, some unique instance per renderer and browser must hold a
NativePixmapManager instance. e.g. RenderThreadImpl owns a NativePixmapManager
member.
> There are two problems
> 1. NativePixmapManager is ozone specific. RenderThreadImpl will have ozone
specific method with ifdef
> 2. GpuMemoryBufferImplOzoneNativeBuffer can be used for both browser and
renderer. It would be messy to get right NativePixmapManager instance.
> 
> Both are addressable, but code would be ugly. Following code is needed.
> 
> --- a/content/child/child_gpu_memory_buffer_manager.cc
> +++ b/content/child/child_gpu_memory_buffer_manager.cc
> @@ -47,11 +47,18 @@ ChildGpuMemoryBufferManager::AllocateGpuMemoryBuffer(
>    if (!success)
>      return scoped_ptr<gfx::GpuMemoryBuffer>();
>  
> +#if defined(USE_OZONE)
> +  scoped_ptr<GpuMemoryBufferImpl> buffer(
> +      GpuMemoryBufferImplOzoneNativeBuffer::CreateFromHandle(
> +          handle, size, format, callback,
> +          RenderThreadImpl::current()->GetNativePixmapManager()));
> +#else
>    scoped_ptr<GpuMemoryBufferImpl>
buffer(GpuMemoryBufferImpl::CreateFromHandle(
>        handle,
>        size,
>        format,
>        base::Bind(&DeletedGpuMemoryBuffer, sender_, handle.id)));
> +#endif
>    if (!buffer) {
> 
> 
> On the other hands, as considering vgem requirement, it is ok for both browser
and renderer to share singleton instance.
> NativePixmapManager initialization will work like as follows:
> NativePixmapManager* NativePixmapManager::GetInstance() {
>   if (singleton instance exist)
>     return the singleton instance;
> 
>   create the singleton instance;
> 
>   if (browser process) {
>     get the vgem fd from DrmDisplayHostManager;
>   } else {
>     get the vgem fd via IPC;
>   }
>   return the singleton instance;
> }
> 
> All NativePixmapManager has to initialize is to ensure vgem fd. In
--single-process mode, Renderer tries to create NativePixmapManager instance but
there is already singleton instance which Browser created. It's ok to share the
singleton instance. I'll add special handling for browser in ozone_platform.cc
in next CL, because browser process needs different initialization.
> It will make code clean, but one concern is that we should be aware why
singleton is ok in this case, which seems to make you uneasy.
> 
> What do you prefer? I'll go the direction.

Please rebase this onto https://codereview.chromium.org/1120873002 and keep it
consistent with IOSurface/SurfaceTexture usage. It supports single process mode,
in-process gpu service, and full multi-process mode with in-process UI
compositor.

[dshwang, 2015-06-10 13:44:47]

On 2015/06/09 19:31:40, reveman wrote:
> On 2015/06/09 at 18:37:40, dongseong.hwang wrote:
> > thank you for reviewing. I have a question. could you feedback?
> > 
> >
>
https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
> > File ui/ozone/public/native_pixmap_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
> > ui/ozone/public/native_pixmap_client.cc:19: NativePixmapClient* g_instance =
> nullptr;
> > On 2015/06/08 17:47:57, spang wrote:
> > > On 2015/06/08 11:15:14, dshwang wrote:
> > > > On 2015/06/05 20:26:07, reveman wrote:
> > > > > On 2015/06/05 at 19:21:54, spang wrote:
> > > > > > I would prefer if this object was managed inside
> content/common/gpu/...
> > > > > > 
> > > > > > It doesn't seem like it needs to be shared with anything else.
> > > > > 
> > > > > I think IOSurfaceManager and SurfaceTextureManager interfaces are good
> > > > examples
> > > > > to follow. Those are abstract classes that live in content/common/
> > > > 
> > > > IMO it should stay here. NativePixmapManager needs to be implemented for
> > > various
> > > > platforms. e.g. gbm
> > > > I think we don't want to let content/ know which platform is used. WDYT?
> > > 
> > > It's true, we can't move NativePixmapManager implementation.
> > > 
> > > What I was hoping to move was the instance from the above line
(g_instance).
> > > Ideally content would manage its instance(s). We'd expose
> > > NativePixmapManager::Create() rather than
> NativePixmapManager::GetInstance().
> > > 
> > > Having it here enforces one instance per address space, but that's not
> necessary
> > > or even desirable. It may lead to bugs; options like --single-process
break
> when
> > > we do this because then both browser and renderer try to initialize the
> shared
> > > instance (say, with different copies of the vgem fd).
> > 
> > I understand your concern. However, it's quite complicated problem. I need
> more feedback.
> > If there are a browser process and 3 renderer, You want to create 4
> NativePixmapManager instances, even if --single-process is used.
> > To do it, some unique instance per renderer and browser must hold a
> NativePixmapManager instance. e.g. RenderThreadImpl owns a NativePixmapManager
> member.
> > There are two problems
> > 1. NativePixmapManager is ozone specific. RenderThreadImpl will have ozone
> specific method with ifdef
> > 2. GpuMemoryBufferImplOzoneNativeBuffer can be used for both browser and
> renderer. It would be messy to get right NativePixmapManager instance.
> > 
> > Both are addressable, but code would be ugly. Following code is needed.
> > 
> > --- a/content/child/child_gpu_memory_buffer_manager.cc
> > +++ b/content/child/child_gpu_memory_buffer_manager.cc
> > @@ -47,11 +47,18 @@ ChildGpuMemoryBufferManager::AllocateGpuMemoryBuffer(
> >    if (!success)
> >      return scoped_ptr<gfx::GpuMemoryBuffer>();
> >  
> > +#if defined(USE_OZONE)
> > +  scoped_ptr<GpuMemoryBufferImpl> buffer(
> > +      GpuMemoryBufferImplOzoneNativeBuffer::CreateFromHandle(
> > +          handle, size, format, callback,
> > +          RenderThreadImpl::current()->GetNativePixmapManager()));
> > +#else
> >    scoped_ptr<GpuMemoryBufferImpl>
> buffer(GpuMemoryBufferImpl::CreateFromHandle(
> >        handle,
> >        size,
> >        format,
> >        base::Bind(&DeletedGpuMemoryBuffer, sender_, handle.id)));
> > +#endif
> >    if (!buffer) {
> > 
> > 
> > On the other hands, as considering vgem requirement, it is ok for both
browser
> and renderer to share singleton instance.
> > NativePixmapManager initialization will work like as follows:
> > NativePixmapManager* NativePixmapManager::GetInstance() {
> >   if (singleton instance exist)
> >     return the singleton instance;
> > 
> >   create the singleton instance;
> > 
> >   if (browser process) {
> >     get the vgem fd from DrmDisplayHostManager;
> >   } else {
> >     get the vgem fd via IPC;
> >   }
> >   return the singleton instance;
> > }
> > 
> > All NativePixmapManager has to initialize is to ensure vgem fd. In
> --single-process mode, Renderer tries to create NativePixmapManager instance
but
> there is already singleton instance which Browser created. It's ok to share
the
> singleton instance. I'll add special handling for browser in ozone_platform.cc
> in next CL, because browser process needs different initialization.
> > It will make code clean, but one concern is that we should be aware why
> singleton is ok in this case, which seems to make you uneasy.
> > 
> > What do you prefer? I'll go the direction.
> 
> Please rebase this onto https://codereview.chromium.org/1120873002 and keep it
> consistent with IOSurface/SurfaceTexture usage. It supports single process
mode,
> in-process gpu service, and full multi-process mode with in-process UI
> compositor.

Thx for pointing out. you implements SurfaceTextureManager/IOSurfaceManager as
singleton in one address space.
In addition, Ozone doesn't require different implementation between IPC and
--single-process unlike IOSurface/SurfaceTexture has BrowserXXXManger and
InProcessXXXManager.
IOSurface/SurfaceTexture with --single-process can optimize to not use IPC and
system calls, but Ozone must communicate with kernel always.
It's because GPU process creates GBM bo, but Render process uses it as VGEM bo.
IOSurface/SurfaceTexture plays a role to both producer and consumer of native
surface, but NativePixmapManager is just consumer of native surface.
I'll rebase it on your CL.

spang, could you feedback your preference between singleton and
RenderThreadImpl::current()->GetNativePixmapManager().
reveman implements SurfaceTextureManager/IOSurfaceManager working as singleton
in https://codereview.chromium.org/1120873002
with --single-process, chromium has only one
SurfaceTextureManager/IOSurfaceManager instance.

[spang, 2015-06-10 16:26:36]

On 2015/06/10 13:44:47, dshwang wrote:
> On 2015/06/09 19:31:40, reveman wrote:
> > On 2015/06/09 at 18:37:40, dongseong.hwang wrote:
> > > thank you for reviewing. I have a question. could you feedback?
> > > 
> > >
> >
>
https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
> > > File ui/ozone/public/native_pixmap_client.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1128113011/diff/80001/ui/ozone/public/native_...
> > > ui/ozone/public/native_pixmap_client.cc:19: NativePixmapClient* g_instance
=
> > nullptr;
> > > On 2015/06/08 17:47:57, spang wrote:
> > > > On 2015/06/08 11:15:14, dshwang wrote:
> > > > > On 2015/06/05 20:26:07, reveman wrote:
> > > > > > On 2015/06/05 at 19:21:54, spang wrote:
> > > > > > > I would prefer if this object was managed inside
> > content/common/gpu/...
> > > > > > > 
> > > > > > > It doesn't seem like it needs to be shared with anything else.
> > > > > > 
> > > > > > I think IOSurfaceManager and SurfaceTextureManager interfaces are
good
> > > > > examples
> > > > > > to follow. Those are abstract classes that live in content/common/
> > > > > 
> > > > > IMO it should stay here. NativePixmapManager needs to be implemented
for
> > > > various
> > > > > platforms. e.g. gbm
> > > > > I think we don't want to let content/ know which platform is used.
WDYT?
> > > > 
> > > > It's true, we can't move NativePixmapManager implementation.
> > > > 
> > > > What I was hoping to move was the instance from the above line
> (g_instance).
> > > > Ideally content would manage its instance(s). We'd expose
> > > > NativePixmapManager::Create() rather than
> > NativePixmapManager::GetInstance().
> > > > 
> > > > Having it here enforces one instance per address space, but that's not
> > necessary
> > > > or even desirable. It may lead to bugs; options like --single-process
> break
> > when
> > > > we do this because then both browser and renderer try to initialize the
> > shared
> > > > instance (say, with different copies of the vgem fd).
> > > 
> > > I understand your concern. However, it's quite complicated problem. I need
> > more feedback.
> > > If there are a browser process and 3 renderer, You want to create 4
> > NativePixmapManager instances, even if --single-process is used.
> > > To do it, some unique instance per renderer and browser must hold a
> > NativePixmapManager instance. e.g. RenderThreadImpl owns a
NativePixmapManager
> > member.
> > > There are two problems
> > > 1. NativePixmapManager is ozone specific. RenderThreadImpl will have ozone
> > specific method with ifdef
> > > 2. GpuMemoryBufferImplOzoneNativeBuffer can be used for both browser and
> > renderer. It would be messy to get right NativePixmapManager instance.
> > > 
> > > Both are addressable, but code would be ugly. Following code is needed.
> > > 
> > > --- a/content/child/child_gpu_memory_buffer_manager.cc
> > > +++ b/content/child/child_gpu_memory_buffer_manager.cc
> > > @@ -47,11 +47,18 @@ ChildGpuMemoryBufferManager::AllocateGpuMemoryBuffer(
> > >    if (!success)
> > >      return scoped_ptr<gfx::GpuMemoryBuffer>();
> > >  
> > > +#if defined(USE_OZONE)
> > > +  scoped_ptr<GpuMemoryBufferImpl> buffer(
> > > +      GpuMemoryBufferImplOzoneNativeBuffer::CreateFromHandle(
> > > +          handle, size, format, callback,
> > > +          RenderThreadImpl::current()->GetNativePixmapManager()));
> > > +#else
> > >    scoped_ptr<GpuMemoryBufferImpl>
> > buffer(GpuMemoryBufferImpl::CreateFromHandle(
> > >        handle,
> > >        size,
> > >        format,
> > >        base::Bind(&DeletedGpuMemoryBuffer, sender_, handle.id)));
> > > +#endif
> > >    if (!buffer) {
> > > 
> > > 
> > > On the other hands, as considering vgem requirement, it is ok for both
> browser
> > and renderer to share singleton instance.
> > > NativePixmapManager initialization will work like as follows:
> > > NativePixmapManager* NativePixmapManager::GetInstance() {
> > >   if (singleton instance exist)
> > >     return the singleton instance;
> > > 
> > >   create the singleton instance;
> > > 
> > >   if (browser process) {
> > >     get the vgem fd from DrmDisplayHostManager;
> > >   } else {
> > >     get the vgem fd via IPC;
> > >   }
> > >   return the singleton instance;
> > > }
> > > 
> > > All NativePixmapManager has to initialize is to ensure vgem fd. In
> > --single-process mode, Renderer tries to create NativePixmapManager instance
> but
> > there is already singleton instance which Browser created. It's ok to share
> the
> > singleton instance. I'll add special handling for browser in
ozone_platform.cc
> > in next CL, because browser process needs different initialization.
> > > It will make code clean, but one concern is that we should be aware why
> > singleton is ok in this case, which seems to make you uneasy.
> > > 
> > > What do you prefer? I'll go the direction.
> > 
> > Please rebase this onto https://codereview.chromium.org/1120873002 and keep
it
> > consistent with IOSurface/SurfaceTexture usage. It supports single process
> mode,
> > in-process gpu service, and full multi-process mode with in-process UI
> > compositor.
> 
> Thx for pointing out. you implements SurfaceTextureManager/IOSurfaceManager as
> singleton in one address space.
> In addition, Ozone doesn't require different implementation between IPC and
> --single-process unlike IOSurface/SurfaceTexture has BrowserXXXManger and
> InProcessXXXManager.
> IOSurface/SurfaceTexture with --single-process can optimize to not use IPC and
> system calls, but Ozone must communicate with kernel always.
> It's because GPU process creates GBM bo, but Render process uses it as VGEM
bo.
> IOSurface/SurfaceTexture plays a role to both producer and consumer of native
> surface, but NativePixmapManager is just consumer of native surface.
> I'll rebase it on your CL.
> 
> spang, could you feedback your preference between singleton and
> RenderThreadImpl::current()->GetNativePixmapManager().
> reveman implements SurfaceTextureManager/IOSurfaceManager working as singleton
> in https://codereview.chromium.org/1120873002
> with --single-process, chromium has only one
> SurfaceTextureManager/IOSurfaceManager instance.

I would suggest having one for each renderer (even with --single-process), but I
guess it doesn't do that for Mac so I'm not sure. reveman@ is the expert for
this, you should ask him.

My main feedback is that I'd prefer if the platform provide functionality only
and leave management of state entirely to content.

Anyway, I think your first priority should be to get a working patch stack.
There's no point in submitting an interface without a working implementation.
Things that are missing:

(1) handoff of vgem fd from browser to renderer is not implemented
(2) from 2nd patch, it looks like deleting the buffer handle isn't sorted out
yet.

I'd rebase the other patch on top of this one and show that this actually works.

[dshwang, 2015-06-10 17:25:32]

On 2015/06/10 16:26:36, spang wrote:
> I would suggest having one for each renderer (even with --single-process), but
I
> guess it doesn't do that for Mac so I'm not sure. reveman@ is the expert for
> this, you should ask him.

Mac and Android creates only one singleton manager instance for all browser,
gpu, and renderer with --single-process

> My main feedback is that I'd prefer if the platform provide functionality only
> and leave management of state entirely to content.

I can create new singleton class using ozone::NativePixmapManager in content/ to
match mac and android.
It will be just wrapper of ozone::NativePixmapManager. Is it ok?

> Anyway, I think your first priority should be to get a working patch stack.
> There's no point in submitting an interface without a working implementation.
> Things that are missing:
> 
> (1) handoff of vgem fd from browser to renderer is not implemented
> (2) from 2nd patch, it looks like deleting the buffer handle isn't sorted out
> yet.
> 
> I'd rebase the other patch on top of this one and show that this actually
works.

Good feedback. I didn't know it. I'll submit all patches soon. Thanks.

[reveman, 2015-06-10 18:19:12]

On 2015/06/10 at 17:25:32, dongseong.hwang wrote:
> On 2015/06/10 16:26:36, spang wrote:
> > I would suggest having one for each renderer (even with --single-process),
but I
> > guess it doesn't do that for Mac so I'm not sure. reveman@ is the expert for
> > this, you should ask him.
> 
> Mac and Android creates only one singleton manager instance for all browser,
gpu, and renderer with --single-process

If you can reuse the browser implementation in single process mode then that's
great. It should be possible when passing ownership with FDs instead of tracking
it using client Ids. It would still be consistent with the
IOSurface/SurfaceTexture mechanism. They just happen to need a different single
process mode implementation because they rely on client Id verification.

> 
> > My main feedback is that I'd prefer if the platform provide functionality
only
> > and leave management of state entirely to content.
> 
> I can create new singleton class using ozone::NativePixmapManager in content/
to match mac and android.
> It will be just wrapper of ozone::NativePixmapManager. Is it ok?

If all the wrapper does is introduce a singleton then I wouldn't bother with
that. Just add a LazyInstance or call
ozone::NativePixmapManager::SetInstance(new XYZNativePixmapManager) in
browser_main_loop.cc/content_main_runner.cc.

> 
> > Anyway, I think your first priority should be to get a working patch stack.
> > There's no point in submitting an interface without a working
implementation.
> > Things that are missing:
> > 
> > (1) handoff of vgem fd from browser to renderer is not implemented
> > (2) from 2nd patch, it looks like deleting the buffer handle isn't sorted
out
> > yet.
> > 
> > I'd rebase the other patch on top of this one and show that this actually
works.

I agree. If you could update the bug with the set of patches needed to get
everything working that would be very useful!

[dshwang, 2015-06-18 15:47:40]

On 2015/06/10 18:19:12, reveman wrote:
> > > Anyway, I think your first priority should be to get a working patch
stack.
> > > There's no point in submitting an interface without a working
> implementation.
> > > Things that are missing:
> > > 
> > > (1) handoff of vgem fd from browser to renderer is not implemented
> > > (2) from 2nd patch, it looks like deleting the buffer handle isn't sorted
> out
> > > yet.
> > > 
> > > I'd rebase the other patch on top of this one and show that this actually
> works.
> 
> I agree. If you could update the bug with the set of patches needed to get
> everything working that would be very useful!

Hi, It's version of working in progress. I submit to ask whether the direction
is fine or not.
This CL includes IPC of vgem fd, now. As you requested, content/ manages
NativePixmapManager lifecycle.

After this CL, the CL of zero-copy on ChromeOS
(https://codereview.chromium.org/1134993003/) can be landed.
Could I listen to your opinion? Thank you in advance.

[dshwang, 2015-06-24 16:32:57]

Patchset #8 (id:140001) has been deleted

[dshwang, 2015-06-24 16:39:04]

Hi, now this CL includes VGEM transfer. I update the CL description as this CL's
change.
After this CL, only the CL, native GpuMemoryBuffer on ChromeOS, remains
https://codereview.chromium.org/1134993003/
spang, reveman, could you review again?

In addition, to run content_unittests,
https://codereview.chromium.org/1208603002/ is needed.
> content_unittests --gtest_filter=GpuMemoryBuffer* --ozone-platform=gbm
--ozone-use-surfaceless

[dshwang, 2015-06-25 07:23:55]

Patchset #8 (id:160001) has been deleted

[reveman, 2015-06-25 14:48:35]

Maybe I'm missing something but this seems much too complicated for what it
actually does.

Here's what's needed if I understand things correctly:

GpuMemoryBufferImplOzoneNativeBuffer needs a OzoneNativeBufferManager or similar
global instance to hide the logic of how buffers are mapped and shared across
process boundaries from content/. Basically the same as IOSurfaceManager and
SurfaceTextureManager but less complicated as we can pass ownership using a file
descriptor. The OzoneNativeBufferManager implementation needs a vgem device fd
to map buffers on the child process side. That's basically the same as how the
IOSurfaceManager needs a token to acquire an IOSurface reference from the
browser.

It seems like a simple global OzoneNativeBufferManager instance that is
implemented in ozone/ but set by content/ at browser/child process startup
should be sufficient. The vgem device fd can be passed to the
OzoneNativeBufferManager instance in the same way we pass a IOSurfaceManager
token to the child process IOSurfaceManager. This would better match the current
convention used by iosurface/surfacetexture implementations. Wdyt?

https://codereview.chromium.org/1128113011/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_backend.h (right):

https://codereview.chromium.org/1128113011/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_backend.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
I don't see why we need to add this. To keep this patch minimal, can you simply
use the current convention established by IOSurface and SurfaceTexture
implementations instead?

https://codereview.chromium.org/1128113011/diff/180001/content/common/gpu/nat...
File content/common/gpu/native_pixmap_manager_ozone_messages.h (right):

https://codereview.chromium.org/1128113011/diff/180001/content/common/gpu/nat...
content/common/gpu/native_pixmap_manager_ozone_messages.h:17:
base::FileDescriptor /* device_fd */)
nit: "Reply" in the name of a request is awkward. I guess this should be called
SyncGetVirtualDevice but see my comment below.

Can we pass this asynchronously to the child process instead of requiring a
round-trip at startup? Take a look at how we pass IOSurfaceManager tokens to
child processes.

https://codereview.chromium.org/1128113011/diff/180001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/180001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:24: class OZONE_EXPORT
NativePixmapManager {
Please have the name of this match the name used in content/. OzoneNativeBuffer
name is used in content right now.

https://codereview.chromium.org/1128113011/diff/180001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:50: NativePixmapManager*
CreateStubNativePixmapManager();
scoped_ptr

[dshwang, 2015-06-25 16:45:03]

On 2015/06/25 14:48:35, reveman wrote:
> Maybe I'm missing something but this seems much too complicated for what it
> actually does.

Thank you for nice reviewing. Although this CL is quite big, you fully catch up.
I appreciate.
 
> Here's what's needed if I understand things correctly:
> 
> GpuMemoryBufferImplOzoneNativeBuffer needs a OzoneNativeBufferManager or
similar
> global instance to hide the logic of how buffers are mapped and shared across
> process boundaries from content/. Basically the same as IOSurfaceManager and
> SurfaceTextureManager but less complicated as we can pass ownership using a
file
> descriptor. The OzoneNativeBufferManager implementation needs a vgem device fd
> to map buffers on the child process side.

CORRECT!!

> That's basically the same as how the
> IOSurfaceManager needs a token to acquire an IOSurface reference from the
> browser.

However, IOSurfaceManager use MACH_PORT to communicate, and
SurfaceTextureManager use Android framework.

> It seems like a simple global OzoneNativeBufferManager instance that is
> implemented in ozone/ but set by content/ at browser/child process startup
> should be sufficient. The vgem device fd can be passed to the
> OzoneNativeBufferManager instance in the same way we pass a IOSurfaceManager
> token to the child process IOSurfaceManager. This would better match the
current
> convention used by iosurface/surfacetexture implementations. Wdyt?

If it's possible, I would be very happy, but as far as I understand, it's
impossible.
The main reason is Ozone needs chromium IPC. If child process has singleton
OzoneNativeBufferManager, what IPC sender OzoneNativeBufferManager should use?
child process can have multiple ChildThreadImpl and RenderThreadImpl instances
and there is no guarantee that the first ChildThreadImpl live longest in child
process. Note ChildThreadImpl/RenderThreadImpl has its own IPC sender. There is
no singleton IPC sender in child process.

It's why I create OzoneNativeBufferManager instance per ChildThreadImpl to reuse
IPC sender of ChildThreadImpl.
If I'm not correct or you have better idea, please advice to me.

https://codereview.chromium.org/1128113011/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_backend.h (right):

https://codereview.chromium.org/1128113011/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_backend.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/06/25 14:48:35, reveman wrote:
> I don't see why we need to add this. To keep this patch minimal, can you
simply
> use the current convention established by IOSurface and SurfaceTexture
> implementations instead?

It's because ChildNativePixmapManager cannot be singleton. Let's discuss if
ChildNativePixmapManager can be singleton like ChildIOSurfaceManager. If
possible, this class is useless as you said.

https://codereview.chromium.org/1128113011/diff/180001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/180001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:24: class OZONE_EXPORT
NativePixmapManager {
On 2015/06/25 14:48:35, reveman wrote:
> Please have the name of this match the name used in content/.
OzoneNativeBuffer
> name is used in content right now.

underline native buffer class for ozone is NativePixmap,
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/public/na...

I think GpuMemoryBufferFactoryOzoneNativeBuffer and
GpuMemoryBufferImplOzoneNativeBuffer should be renamed to
GpuMemoryBufferFactoryOzone and GpuMemoryBufferImplOzone. Those are just ozone
implementation of GpuMemoryBufferFactory and GpuMemoryBufferImpl.

[dshwang, 2015-06-25 17:26:36]

On 2015/06/25 16:45:03, dshwang wrote:
> On 2015/06/25 14:48:35, reveman wrote:
> > Maybe I'm missing something but this seems much too complicated for what it
> > actually does.
> 
> Thank you for nice reviewing. Although this CL is quite big, you fully catch
up.
> I appreciate.
>  
> > Here's what's needed if I understand things correctly:
> > 
> > GpuMemoryBufferImplOzoneNativeBuffer needs a OzoneNativeBufferManager or
> similar
> > global instance to hide the logic of how buffers are mapped and shared
across
> > process boundaries from content/. Basically the same as IOSurfaceManager and
> > SurfaceTextureManager but less complicated as we can pass ownership using a
> file
> > descriptor. The OzoneNativeBufferManager implementation needs a vgem device
fd
> > to map buffers on the child process side.
> 
> CORRECT!!
> 
> > That's basically the same as how the
> > IOSurfaceManager needs a token to acquire an IOSurface reference from the
> > browser.
> 
> However, IOSurfaceManager use MACH_PORT to communicate, and
> SurfaceTextureManager use Android framework.
> 
> > It seems like a simple global OzoneNativeBufferManager instance that is
> > implemented in ozone/ but set by content/ at browser/child process startup
> > should be sufficient. The vgem device fd can be passed to the
> > OzoneNativeBufferManager instance in the same way we pass a IOSurfaceManager
> > token to the child process IOSurfaceManager. This would better match the
> current
> > convention used by iosurface/surfacetexture implementations. Wdyt?
> 
> If it's possible, I would be very happy, but as far as I understand, it's
> impossible.
> The main reason is Ozone needs chromium IPC. If child process has singleton
> OzoneNativeBufferManager, what IPC sender OzoneNativeBufferManager should use?
> child process can have multiple ChildThreadImpl and RenderThreadImpl instances
> and there is no guarantee that the first ChildThreadImpl live longest in child
> process. Note ChildThreadImpl/RenderThreadImpl has its own IPC sender. There
is
> no singleton IPC sender in child process.
> 
> It's why I create OzoneNativeBufferManager instance per ChildThreadImpl to
reuse
> IPC sender of ChildThreadImpl.
> If I'm not correct or you have better idea, please advice to me.

On the other hands, I think MacOS and Android also should follow ozone pattern,
although it's more complicated.
Rationale
1. Singleton is generally not-good
2. (more important) Chromium IPC is implemented by Mach and UNIX domain sockets
in Mac and Linux respectively. IOSurfaceManager uses MACH in ad-hoc manner,
instead of using Chromium IPC. In this sense, OzoneNativeBufferManager would
create and use UNIX domain sockets. I guess it's anti-pattern.

What do you think about ChildGpuMemoryBufferManager owning ChildIOSurfaceManager
as member?

[reveman, 2015-06-25 18:47:36]

On 2015/06/25 at 17:26:36, dongseong.hwang wrote:
> On 2015/06/25 16:45:03, dshwang wrote:
> > On 2015/06/25 14:48:35, reveman wrote:
> > > Maybe I'm missing something but this seems much too complicated for what
it
> > > actually does.
> > 
> > Thank you for nice reviewing. Although this CL is quite big, you fully catch
up.
> > I appreciate.
> >  
> > > Here's what's needed if I understand things correctly:
> > > 
> > > GpuMemoryBufferImplOzoneNativeBuffer needs a OzoneNativeBufferManager or
> > similar
> > > global instance to hide the logic of how buffers are mapped and shared
across
> > > process boundaries from content/. Basically the same as IOSurfaceManager
and
> > > SurfaceTextureManager but less complicated as we can pass ownership using
a
> > file
> > > descriptor. The OzoneNativeBufferManager implementation needs a vgem
device fd
> > > to map buffers on the child process side.
> > 
> > CORRECT!!
> > 
> > > That's basically the same as how the
> > > IOSurfaceManager needs a token to acquire an IOSurface reference from the
> > > browser.
> > 
> > However, IOSurfaceManager use MACH_PORT to communicate, and
> > SurfaceTextureManager use Android framework.
> > 
> > > It seems like a simple global OzoneNativeBufferManager instance that is
> > > implemented in ozone/ but set by content/ at browser/child process startup
> > > should be sufficient. The vgem device fd can be passed to the
> > > OzoneNativeBufferManager instance in the same way we pass a
IOSurfaceManager
> > > token to the child process IOSurfaceManager. This would better match the
> > current
> > > convention used by iosurface/surfacetexture implementations. Wdyt?
> > 
> > If it's possible, I would be very happy, but as far as I understand, it's
> > impossible.
> > The main reason is Ozone needs chromium IPC. If child process has singleton
> > OzoneNativeBufferManager, what IPC sender OzoneNativeBufferManager should
use?
> > child process can have multiple ChildThreadImpl and RenderThreadImpl
instances
> > and there is no guarantee that the first ChildThreadImpl live longest in
child
> > process. Note ChildThreadImpl/RenderThreadImpl has its own IPC sender. There
is
> > no singleton IPC sender in child process.
> > 
> > It's why I create OzoneNativeBufferManager instance per ChildThreadImpl to
reuse
> > IPC sender of ChildThreadImpl.
> > If I'm not correct or you have better idea, please advice to me.
> 
> On the other hands, I think MacOS and Android also should follow ozone
pattern, although it's more complicated.
> Rationale
> 1. Singleton is generally not-good
> 2. (more important) Chromium IPC is implemented by Mach and UNIX domain
sockets in Mac and Linux respectively. IOSurfaceManager uses MACH in ad-hoc
manner, instead of using Chromium IPC. In this sense, OzoneNativeBufferManager
would create and use UNIX domain sockets. I guess it's anti-pattern.
> 
> What do you think about ChildGpuMemoryBufferManager owning
ChildIOSurfaceManager as member?

It's not trivial to do that without some code duplication but we can look at
that as a follow up. It's better to follow existing conventions (good or bad)
for now. By introducing new conventions you're making it much harder to review
the code as I can't tell if the changes are necessary or not.

[dshwang, 2015-06-26 07:38:38]

On 2015/06/25 18:47:36, reveman wrote:
> On 2015/06/25 at 17:26:36, dongseong.hwang wrote:
> > On the other hands, I think MacOS and Android also should follow ozone
> pattern, although it's more complicated.
> > Rationale
> > 1. Singleton is generally not-good
> > 2. (more important) Chromium IPC is implemented by Mach and UNIX domain
> sockets in Mac and Linux respectively. IOSurfaceManager uses MACH in ad-hoc
> manner, instead of using Chromium IPC. In this sense, OzoneNativeBufferManager
> would create and use UNIX domain sockets. I guess it's anti-pattern.
> > 
> > What do you think about ChildGpuMemoryBufferManager owning
> ChildIOSurfaceManager as member?
> 
> It's not trivial to do that without some code duplication but we can look at
> that as a follow up. It's better to follow existing conventions (good or bad)
> for now. By introducing new conventions you're making it much harder to review
> the code as I can't tell if the changes are necessary or not.

Do you want ozone code to use ad-hoc unix domain socket? I can do it but it
needs more code than this CL.

[reveman, 2015-06-26 11:18:15]

On 2015/06/26 at 07:38:38, dongseong.hwang wrote:
> On 2015/06/25 18:47:36, reveman wrote:
> > On 2015/06/25 at 17:26:36, dongseong.hwang wrote:
> > > On the other hands, I think MacOS and Android also should follow ozone
> > pattern, although it's more complicated.
> > > Rationale
> > > 1. Singleton is generally not-good
> > > 2. (more important) Chromium IPC is implemented by Mach and UNIX domain
> > sockets in Mac and Linux respectively. IOSurfaceManager uses MACH in ad-hoc
> > manner, instead of using Chromium IPC. In this sense,
OzoneNativeBufferManager
> > would create and use UNIX domain sockets. I guess it's anti-pattern.
> > > 
> > > What do you think about ChildGpuMemoryBufferManager owning
> > ChildIOSurfaceManager as member?
> > 
> > It's not trivial to do that without some code duplication but we can look at
> > that as a follow up. It's better to follow existing conventions (good or
bad)
> > for now. By introducing new conventions you're making it much harder to
review
> > the code as I can't tell if the changes are necessary or not.
> 
> Do you want ozone code to use ad-hoc unix domain socket? I can do it but it
needs more code than this CL.

No, why you would need that?

[dshwang, 2015-06-26 12:17:57]

On 2015/06/26 11:18:15, reveman wrote:
> On 2015/06/26 at 07:38:38, dongseong.hwang wrote:
> > On 2015/06/25 18:47:36, reveman wrote:
> > > On 2015/06/25 at 17:26:36, dongseong.hwang wrote:
> > > > On the other hands, I think MacOS and Android also should follow ozone
> > > pattern, although it's more complicated.
> > > > Rationale
> > > > 1. Singleton is generally not-good
> > > > 2. (more important) Chromium IPC is implemented by Mach and UNIX domain
> > > sockets in Mac and Linux respectively. IOSurfaceManager uses MACH in
ad-hoc
> > > manner, instead of using Chromium IPC. In this sense,
> OzoneNativeBufferManager
> > > would create and use UNIX domain sockets. I guess it's anti-pattern.
> > > > 
> > > > What do you think about ChildGpuMemoryBufferManager owning
> > > ChildIOSurfaceManager as member?
> > > 
> > > It's not trivial to do that without some code duplication but we can look
at
> > > that as a follow up. It's better to follow existing conventions (good or
> bad)
> > > for now. By introducing new conventions you're making it much harder to
> review
> > > the code as I can't tell if the changes are necessary or not.
> > 
> > Do you want ozone code to use ad-hoc unix domain socket? I can do it but it
> needs more code than this CL.
> 
> No, why you would need that?

to transfer vgem fd without chromium IPC. IOSurfaceManager communicates
browser-child using mach port. ozone don't have mach port.

[reveman, 2015-06-26 14:56:48]

On 2015/06/26 at 12:17:57, dongseong.hwang wrote:
> On 2015/06/26 11:18:15, reveman wrote:
> > On 2015/06/26 at 07:38:38, dongseong.hwang wrote:
> > > On 2015/06/25 18:47:36, reveman wrote:
> > > > On 2015/06/25 at 17:26:36, dongseong.hwang wrote:
> > > > > On the other hands, I think MacOS and Android also should follow ozone
> > > > pattern, although it's more complicated.
> > > > > Rationale
> > > > > 1. Singleton is generally not-good
> > > > > 2. (more important) Chromium IPC is implemented by Mach and UNIX
domain
> > > > sockets in Mac and Linux respectively. IOSurfaceManager uses MACH in
ad-hoc
> > > > manner, instead of using Chromium IPC. In this sense,
> > OzoneNativeBufferManager
> > > > would create and use UNIX domain sockets. I guess it's anti-pattern.
> > > > > 
> > > > > What do you think about ChildGpuMemoryBufferManager owning
> > > > ChildIOSurfaceManager as member?
> > > > 
> > > > It's not trivial to do that without some code duplication but we can
look at
> > > > that as a follow up. It's better to follow existing conventions (good or
> > bad)
> > > > for now. By introducing new conventions you're making it much harder to
> > review
> > > > the code as I can't tell if the changes are necessary or not.
> > > 
> > > Do you want ozone code to use ad-hoc unix domain socket? I can do it but
it
> > needs more code than this CL.
> > 
> > No, why you would need that?
> 
> to transfer vgem fd without chromium IPC. IOSurfaceManager communicates
browser-child using mach port. ozone don't have mach port.

IOSurface ownership can't be transferred using our regular IPC system in
chromium, that's why we need to use mach IPC. This is not the case with vgem.
vgem buffer ownership can be transferred using our regular IPC system in
chromium and that should make the implementation much simpler.

[dshwang, 2015-06-26 15:16:42]

On 2015/06/26 14:56:48, reveman wrote:
> On 2015/06/26 at 12:17:57, dongseong.hwang wrote:
> > On 2015/06/26 11:18:15, reveman wrote:
> > > On 2015/06/26 at 07:38:38, dongseong.hwang wrote:
> > > > On 2015/06/25 18:47:36, reveman wrote:
> > > > > On 2015/06/25 at 17:26:36, dongseong.hwang wrote:
> > > > > > On the other hands, I think MacOS and Android also should follow
ozone
> > > > > pattern, although it's more complicated.
> > > > > > Rationale
> > > > > > 1. Singleton is generally not-good
> > > > > > 2. (more important) Chromium IPC is implemented by Mach and UNIX
> domain
> > > > > sockets in Mac and Linux respectively. IOSurfaceManager uses MACH in
> ad-hoc
> > > > > manner, instead of using Chromium IPC. In this sense,
> > > OzoneNativeBufferManager
> > > > > would create and use UNIX domain sockets. I guess it's anti-pattern.
> > > > > > 
> > > > > > What do you think about ChildGpuMemoryBufferManager owning
> > > > > ChildIOSurfaceManager as member?
> > > > > 
> > > > > It's not trivial to do that without some code duplication but we can
> look at
> > > > > that as a follow up. It's better to follow existing conventions (good
or
> > > bad)
> > > > > for now. By introducing new conventions you're making it much harder
to
> > > review
> > > > > the code as I can't tell if the changes are necessary or not.
> > > > 
> > > > Do you want ozone code to use ad-hoc unix domain socket? I can do it but
> it
> > > needs more code than this CL.
> > > 
> > > No, why you would need that?
> > 
> > to transfer vgem fd without chromium IPC. IOSurfaceManager communicates
> browser-child using mach port. ozone don't have mach port.
> 
> IOSurface ownership can't be transferred using our regular IPC system in
> chromium, that's why we need to use mach IPC. This is not the case with vgem.
> vgem buffer ownership can be transferred using our regular IPC system in
> chromium and that should make the implementation much simpler.

I see. I'll make ChildNativePixmapManager is singleton in child process. It will
use the IPC sender owned by ChildThreadImpl, which exists at the time of
creation of ChildNativePixmapManager. It's fine for ChildNativePixmapManager to
outlive more than ChildThreadImpl because ChildNativePixmapManager uses IPC
sender only once at the creation time. Note this plan is based on the fact that
ChildNativePixmapManager uses IPC only once.
Let me update. Thank you for patient review.

[reveman, 2015-06-26 15:45:28]

On 2015/06/26 at 15:16:42, dongseong.hwang wrote:
> On 2015/06/26 14:56:48, reveman wrote:
> > On 2015/06/26 at 12:17:57, dongseong.hwang wrote:
> > > On 2015/06/26 11:18:15, reveman wrote:
> > > > On 2015/06/26 at 07:38:38, dongseong.hwang wrote:
> > > > > On 2015/06/25 18:47:36, reveman wrote:
> > > > > > On 2015/06/25 at 17:26:36, dongseong.hwang wrote:
> > > > > > > On the other hands, I think MacOS and Android also should follow
ozone
> > > > > > pattern, although it's more complicated.
> > > > > > > Rationale
> > > > > > > 1. Singleton is generally not-good
> > > > > > > 2. (more important) Chromium IPC is implemented by Mach and UNIX
> > domain
> > > > > > sockets in Mac and Linux respectively. IOSurfaceManager uses MACH in
> > ad-hoc
> > > > > > manner, instead of using Chromium IPC. In this sense,
> > > > OzoneNativeBufferManager
> > > > > > would create and use UNIX domain sockets. I guess it's anti-pattern.
> > > > > > > 
> > > > > > > What do you think about ChildGpuMemoryBufferManager owning
> > > > > > ChildIOSurfaceManager as member?
> > > > > > 
> > > > > > It's not trivial to do that without some code duplication but we can
> > look at
> > > > > > that as a follow up. It's better to follow existing conventions
(good or
> > > > bad)
> > > > > > for now. By introducing new conventions you're making it much harder
to
> > > > review
> > > > > > the code as I can't tell if the changes are necessary or not.
> > > > > 
> > > > > Do you want ozone code to use ad-hoc unix domain socket? I can do it
but
> > it
> > > > needs more code than this CL.
> > > > 
> > > > No, why you would need that?
> > > 
> > > to transfer vgem fd without chromium IPC. IOSurfaceManager communicates
> > browser-child using mach port. ozone don't have mach port.
> > 
> > IOSurface ownership can't be transferred using our regular IPC system in
> > chromium, that's why we need to use mach IPC. This is not the case with
vgem.
> > vgem buffer ownership can be transferred using our regular IPC system in
> > chromium and that should make the implementation much simpler.
> 
> I see. I'll make ChildNativePixmapManager is singleton in child process. It
will use the IPC sender owned by ChildThreadImpl, which exists at the time of
creation of ChildNativePixmapManager. It's fine for ChildNativePixmapManager to
outlive more than ChildThreadImpl because ChildNativePixmapManager uses IPC
sender only once at the creation time. Note this plan is based on the fact that
ChildNativePixmapManager uses IPC only once.
> Let me update. Thank you for patient review.

Instead of having some Child/Browser manager implementations, can we simply have
static ui::OzoneNativeBufferManager::Set/GetInstance and a
OzoneNativeBufferManager::set_virtual_device() function that we use much in the
same way we use set_token in the IOSurfaceManager case?

[dshwang, 2015-07-06 18:19:58]

Patchset #8 (id:180001) has been deleted

[dshwang, 2015-07-06 18:21:24]

Patchset #8 (id:200001) has been deleted

[dshwang, 2015-07-06 18:22:18]

Patchset #8 (id:220001) has been deleted

[dshwang, 2015-07-06 18:45:01]

sorry for delaying update. Could you give me feedback again?

Make ChildNativePixmapManager singleton in child process. It use the IPC sender
owned by ChildThreadImpl, which exists at the time of
creation of ChildNativePixmapManager. It's fine for ChildNativePixmapManager to
outlive more than ChildThreadImpl because ChildNativePixmapManager uses IPC
sender only once at the creation time.

I made mistake to delete previous the patch set, so I re-upload the previous
patch set and then upload the current patch set.

https://codereview.chromium.org/1128113011/diff/260001/content/child/child_gp...
File content/child/child_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/260001/content/child/child_gp...
content/child/child_gpu_memory_buffer_manager.cc:50:
ChildNativePixmapManager::CreateSingleton(sender_.get()));
Make the singleton ChildNativePixmapManager here while MacOS creates the
singleton ChildIOSurfaceManager in content_main_runner.cc. It's because
ChildNativePixmapManager needs IPC sender to initialize.

We cannot make it in ChildGpuMemoryBufferManager constructor because IPC sender
is not yet connected at the time.

The earliest time in which we can create it is
ChildThreadImpl::OnChannelConnected, but it looks more intrusive than here from
my perspective.

https://codereview.chromium.org/1128113011/diff/260001/content/common/gpu/nat...
File content/common/gpu/native_pixmap_manager_ozone_messages.h (right):

https://codereview.chromium.org/1128113011/diff/260001/content/common/gpu/nat...
content/common/gpu/native_pixmap_manager_ozone_messages.h:15:
IPC_SYNC_MESSAGE_CONTROL0_1(BrowserNativePixmapManagerMsg_SyncGetVirtualDevice,
> nit: "Reply" in the name of a request is awkward. I guess this should be
called
> SyncGetVirtualDevice but see my comment below.

Done. renamed.

> Can we pass this asynchronously to the child process instead of requiring a
> round-trip at startup? Take a look at how we pass IOSurfaceManager tokens to
> child processes.

I couldn't find the way. BrowserIOSurfaceManager::Initialize registers mach
token into kernel, and then BrowserIOSurfaceManager::LookupServicePort in
renderer looks up the token from kernel.
In this sense, Browser needs to register VGEM FD into linux kernel. There is no
way. In addition, Browser and Renderer cannot communicate until the channel is
connected (i.e. ChildThreadImpl::OnChannelConnected())

https://codereview.chromium.org/1128113011/diff/260001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/260001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:24: class OZONE_EXPORT
NativePixmapManager {
On 2015/06/25 14:48:35, reveman wrote:
> Please have the name of this match the name used in content/.
OzoneNativeBuffer
> name is used in content right now.

As I mentioned earlier, I'll rename GpuMemoryBufferImplOzoneNativeBuffer to
GpuMemoryBufferImplOzone, instead of renaming here.
It's because
1. the term "NativePixmap" is widely used in ozone code already.
2. GpuMemoryBufferImplOzone is ozone implementation of GpuMemoryBufferImpl.
Prefix "NativeBuffer" is redundant.

[dshwang, 2015-07-08 18:20:58]

Patchset #10 (id:280001) has been deleted

[dshwang, 2015-07-13 13:56:22]

spang, reveman, could you review again?

[dshwang, 2015-07-15 17:54:21]

On 2015/07/13 13:56:22, dshwang wrote:
> spang, reveman, could you review again?

spang, reveman, could you review again? thank you in advance.

[dshwang, 2015-07-21 16:15:03]

spang, reveman, could you review again? 

To speed up review, I separate controversial VGEM and IPC part to
https://codereview.chromium.org/1248713002/
Now this CL purely introduces NativePixmapManager class.

I rebase it on https://codereview.chromium.org/1240353002/. after the CL landed,
I'll kick the trybot.

[dshwang, 2015-07-21 16:34:27]

Patchset #12 (id:340001) has been deleted

[dshwang, 2015-07-21 18:53:56]

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

[reveman, 2015-07-22 16:59:55]

I don't think ChildNativePixmapManager or BrowserNativePixmapManager
implementations are useful. Just instantiate the ui::NativePixmapManager and set
it from content/.

https://codereview.chromium.org/1128113011/diff/360001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1128113011/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:46:
std::vector<GpuMemoryBufferFactory::Configuration>* configurations) {
not sure this helper function is worth much. just fold the for loop below into
GetSupportedConfigurations instead?

https://codereview.chromium.org/1128113011/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:47: for
(auto& ozone_conf : ozone_configurations) {
nit: s/ozone_conf/ozone_configuration/ or maybe
native_pixmap_configurations+native_pixmap_configuration instead

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/caca...
File ui/ozone/platform/caca/ozone_platform_caca.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/caca...
ui/ozone/platform/caca/ozone_platform_caca.h:17: NativePixmapManager*
CreateNativePixmapManagerCaca();
why not in native_pixmap_manager_caca.h?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/cast...
File ui/ozone/platform/cast/ozone_platform_cast.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/cast...
ui/ozone/platform/cast/ozone_platform_cast.h:17: NativePixmapManager*
CreateNativePixmapManagerCast();
why not in native_pixmap_manager_cast.h?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/ozone_platform_drm.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/ozone_platform_drm.h:21: NativePixmapManager*
CreateNativePixmapManagerDrm();
why are these in this file rather than native_pixmap_manager_drm.h?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/eglt...
File ui/ozone/platform/egltest/ozone_platform_egltest.cc (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/eglt...
ui/ozone/platform/egltest/ozone_platform_egltest.cc:414: return
CreateStubNativePixmapManager();
should we add a test implementation instead of using a stub implementation for
tests?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/eglt...
File ui/ozone/platform/egltest/ozone_platform_egltest.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/eglt...
ui/ozone/platform/egltest/ozone_platform_egltest.h:17: NativePixmapManager*
CreateNativePixmapManagerEgltest();
why not add native_pixmap_manager_egltest.h for this?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/test...
File ui/ozone/platform/test/ozone_platform_test.cc (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/test...
ui/ozone/platform/test/ozone_platform_test.cc:124: return
CreateStubNativePixmapManager();
add test implementation like OzonePlatform instead of using a stub for tests?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/test...
File ui/ozone/platform/test/ozone_platform_test.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/test...
ui/ozone/platform/test/ozone_platform_test.h:17: NativePixmapManager*
CreateNativePixmapManagerTest();
native_pixmap_manager_test.h?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.cc:13: #include
"ui/ozone/public/ozone_switches.h"
do you need all these includes? base/command_line.h etc seem unused

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:23: // and Render process.
system specific surface implementation?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:46: virtual void Initialize(const
base::FileDescriptor& device_fd) = 0;
do you really need this when you have the static factory method above?

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:53: NativePixmapManager*
CreateStubNativePixmapManager();
Does this deserve it's own file? Who owns the pointer returned?

[dshwang, 2015-07-23 14:02:28]

On 2015/07/22 16:59:55, reveman wrote:
> I don't think ChildNativePixmapManager or BrowserNativePixmapManager
> implementations are useful. Just instantiate the ui::NativePixmapManager and
set
> it from content/.

Thank you for reviewing. ChildNativePixmapManager or BrowserNativePixmapManager
is needed for next CL. As you suggested, I move them to next CL. This CL just
instantiate the ui::NativePixmapManager and set it from content/.

I add native_pixmap_manager_<platform>.(h|cc) to implement
CreateNativePixmapManager<Platform>().

Could you review again?

https://codereview.chromium.org/1128113011/diff/360001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1128113011/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:46:
std::vector<GpuMemoryBufferFactory::Configuration>* configurations) {
On 2015/07/22 16:59:55, reveman wrote:
> not sure this helper function is worth much. just fold the for loop below into
> GetSupportedConfigurations instead?

Done.

https://codereview.chromium.org/1128113011/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:47: for
(auto& ozone_conf : ozone_configurations) {
On 2015/07/22 16:59:55, reveman wrote:
> nit: s/ozone_conf/ozone_configuration/ or maybe
> native_pixmap_configurations+native_pixmap_configuration instead

Done.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/caca...
File ui/ozone/platform/caca/ozone_platform_caca.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/caca...
ui/ozone/platform/caca/ozone_platform_caca.h:17: NativePixmapManager*
CreateNativePixmapManagerCaca();
On 2015/07/22 16:59:55, reveman wrote:
> why not in native_pixmap_manager_caca.h?

Done.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/cast...
File ui/ozone/platform/cast/ozone_platform_cast.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/cast...
ui/ozone/platform/cast/ozone_platform_cast.h:17: NativePixmapManager*
CreateNativePixmapManagerCast();
On 2015/07/22 16:59:55, reveman wrote:
> why not in native_pixmap_manager_cast.h?

Done.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/ozone_platform_drm.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/ozone_platform_drm.h:21: NativePixmapManager*
CreateNativePixmapManagerDrm();
On 2015/07/22 16:59:55, reveman wrote:
> why are these in this file rather than native_pixmap_manager_drm.h?

Done.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/eglt...
File ui/ozone/platform/egltest/ozone_platform_egltest.cc (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/eglt...
ui/ozone/platform/egltest/ozone_platform_egltest.cc:414: return
CreateStubNativePixmapManager();
On 2015/07/22 16:59:55, reveman wrote:
> should we add a test implementation instead of using a stub implementation for
> tests?

Move CreateStubNativePixmapManager() to
ui/ozone/common/stub_native_pixmap_manager.h like
ui/ozone/common/stub_overlay_manager.h
stub implementation is pattern of ozone. e.g. StubOverlayManager,
StubGpuPlatformSupportHost, StubInputController, StubKeyboardLayoutEngine,
StubInputController
In addition, spang suggested to do this.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/eglt...
File ui/ozone/platform/egltest/ozone_platform_egltest.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/eglt...
ui/ozone/platform/egltest/ozone_platform_egltest.h:17: NativePixmapManager*
CreateNativePixmapManagerEgltest();
On 2015/07/22 16:59:55, reveman wrote:
> why not add native_pixmap_manager_egltest.h for this?

Done.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/test...
File ui/ozone/platform/test/ozone_platform_test.cc (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/test...
ui/ozone/platform/test/ozone_platform_test.cc:124: return
CreateStubNativePixmapManager();
On 2015/07/22 16:59:55, reveman wrote:
> add test implementation like OzonePlatform instead of using a stub for tests?

Done.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/test...
File ui/ozone/platform/test/ozone_platform_test.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/platform/test...
ui/ozone/platform/test/ozone_platform_test.h:17: NativePixmapManager*
CreateNativePixmapManagerTest();
On 2015/07/22 16:59:55, reveman wrote:
> native_pixmap_manager_test.h?

Done.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.cc:13: #include
"ui/ozone/public/ozone_switches.h"
On 2015/07/22 16:59:55, reveman wrote:
> do you need all these includes? base/command_line.h etc seem unused

Done.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:23: // and Render process.
On 2015/07/22 16:59:55, reveman wrote:
> system specific surface implementation?

s/system specific surface implementation/native surface/

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:46: virtual void Initialize(const
base::FileDescriptor& device_fd) = 0;
On 2015/07/22 16:59:55, reveman wrote:
> do you really need this when you have the static factory method above?

It's needed for next CL. I move this to next CL.

https://codereview.chromium.org/1128113011/diff/360001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:53: NativePixmapManager*
CreateStubNativePixmapManager();
On 2015/07/22 16:59:55, reveman wrote:
> Does this deserve it's own file? Who owns the pointer returned?

Make sense. I move it to ui/ozone/common/stub_native_pixmap_manager.h
I did follow existing pattern in public/. e.g. CreateStubGpuPlatformSupport(),
CreateStubGpuPlatformSupportHost(), CreateStubInputController()
I'll move them to common/ later.

[reveman, 2015-07-23 14:46:13]

https://codereview.chromium.org/1128113011/diff/380001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/browser/browse...
content/browser/browser_main_loop.cc:621:
DCHECK(!ui::NativePixmapManager::GetInstance());
nit: remove DCHECK. handled by SetInstance already.

https://codereview.chromium.org/1128113011/diff/380001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1400: #endif
what does this have to do with this patch?

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:45:
DCHECK(ui::NativePixmapManager::GetInstance());
nit: you don't need a !nullptr DCHECK when you dereference it below.
Dereferencing it will already cause an easy to diagnose crash on all platforms.

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:93: if
(!ozone_native_pixmap_factory_.CreateGpuMemoryBuffer(
Can ozone_native_pixmap_factory_ be replaced by NativePixmapManager? It's
confusing to have two different interfaces for ozone native pixmaps and it makes
sense to have the interface that is used to determine what formats are available
to also be used for allocating buffers of those formats.

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
content/test/content_test_suite.cc:114: if (!is_child_process) {
why do you need this "!is_child_process" check?

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
content/test/content_test_suite.cc:115:
DCHECK(!ui::NativePixmapManager::GetInstance());
nit: remove this DCHECK, SetInstance already does this.

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
content/test/content_test_suite.cc:116:
ui::NativePixmapManager::SetInstance(ui::NativePixmapManager::Create());
should we be using CreateNativePixmapManagerTest() here?

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.cc:36: scoped_ptr<NativePixmapManager>
manager =
just "return PlatformObject<NativePixmapManager>::Create();" instead?

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:16: // provide the handle of a native
surface for Browser and Render process.
do you mean native pixmap and not native surface?

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:20: static void
SetInstance(scoped_ptr<NativePixmapManager> instance);
s/scoped_ptr<NativePixmapManager> instance/NativePixmapManager* instance/

SetInstance shouldn't take ownership instance. Ownership and lifetime management
should stay with the caller.

[dshwang, 2015-07-23 16:42:09]

Thank you for quick reviewing. I addressed all comments.

https://codereview.chromium.org/1128113011/diff/380001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/browser/browse...
content/browser/browser_main_loop.cc:621:
DCHECK(!ui::NativePixmapManager::GetInstance());
On 2015/07/23 14:46:12, reveman wrote:
> nit: remove DCHECK. handled by SetInstance already.

Done.

https://codereview.chromium.org/1128113011/diff/380001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1400: #endif
On 2015/07/23 14:46:12, reveman wrote:
> what does this have to do with this patch?

yes, renderer creates NativePixmapManager. at the time, ozone decides right
platform using this commend line.

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:45:
DCHECK(ui::NativePixmapManager::GetInstance());
On 2015/07/23 14:46:12, reveman wrote:
> nit: you don't need a !nullptr DCHECK when you dereference it below.
> Dereferencing it will already cause an easy to diagnose crash on all
platforms.

Done.

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:93: if
(!ozone_native_pixmap_factory_.CreateGpuMemoryBuffer(
On 2015/07/23 14:46:12, reveman wrote:
> Can ozone_native_pixmap_factory_ be replaced by NativePixmapManager? It's
> confusing to have two different interfaces for ozone native pixmaps and it
makes
> sense to have the interface that is used to determine what formats are
available
> to also be used for allocating buffers of those formats.

No, both have different purpose.

Note this class (GpuMemoryBufferFactory) is gpu process implementation except
for IsGpuMemoryBufferConfigurationSupported() and
GetSupportedGpuMemoryBufferConfigurations() methods.
ozone has corresponding gpu process implementation, which is
|ozone_native_pixmap_factory_|.
However, ozone should handle IsGpuMemoryBufferConfigurationSupported() and
GetSupportedGpuMemoryBufferConfigurations() methods in browser and render
process. It's why this CL introduces NativePixmapManager for renderer and
browser.
Android and MacOS don't need this consideration because their backend is only
one.

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
content/test/content_test_suite.cc:114: if (!is_child_process) {
On 2015/07/23 14:46:12, reveman wrote:
> why do you need this "!is_child_process" check?

good question. why gfx::GLSurfaceTestSupport::InitializeOneOff() needs
"!is_child_process" check? InitializeOneOff() internally initialize ozone. So I
initialize NativePixmapManager when ozone is initialized.
I don't fully understand what is |is_child_process| purpose.

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
content/test/content_test_suite.cc:115:
DCHECK(!ui::NativePixmapManager::GetInstance());
On 2015/07/23 14:46:12, reveman wrote:
> nit: remove this DCHECK, SetInstance already does this.

Done.

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
content/test/content_test_suite.cc:116:
ui::NativePixmapManager::SetInstance(ui::NativePixmapManager::Create());
On 2015/07/23 14:46:12, reveman wrote:
> should we be using CreateNativePixmapManagerTest() here?

CreateNativePixmapManagerTest() is not public for another module.
I'll create TestNativePixmapManager in next CL like previous patch set.

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.cc:36: scoped_ptr<NativePixmapManager>
manager =
On 2015/07/23 14:46:12, reveman wrote:
> just "return PlatformObject<NativePixmapManager>::Create();" instead?

Done.

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:16: // provide the handle of a native
surface for Browser and Render process.
On 2015/07/23 14:46:12, reveman wrote:
> do you mean native pixmap and not native surface?

I though native pixmap is the handle of a native surface. native surface could
be dma_buf, ION or etc.
To be succinct, I change "the handle of a native surface" to "native pixmap".

https://codereview.chromium.org/1128113011/diff/380001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:20: static void
SetInstance(scoped_ptr<NativePixmapManager> instance);
On 2015/07/23 14:46:12, reveman wrote:
> s/scoped_ptr<NativePixmapManager> instance/NativePixmapManager* instance/
> 
> SetInstance shouldn't take ownership instance. Ownership and lifetime
management
> should stay with the caller.

oh, interesting. done.

[reveman, 2015-07-23 18:25:23]

https://codereview.chromium.org/1128113011/diff/380001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1400: #endif
On 2015/07/23 at 16:42:09, dshwang wrote:
> On 2015/07/23 14:46:12, reveman wrote:
> > what does this have to do with this patch?
> 
> yes, renderer creates NativePixmapManager. at the time, ozone decides right
platform using this commend line.

In that case, you need to add it here too:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:93: if
(!ozone_native_pixmap_factory_.CreateGpuMemoryBuffer(
On 2015/07/23 at 16:42:09, dshwang wrote:
> On 2015/07/23 14:46:12, reveman wrote:
> > Can ozone_native_pixmap_factory_ be replaced by NativePixmapManager? It's
> > confusing to have two different interfaces for ozone native pixmaps and it
makes
> > sense to have the interface that is used to determine what formats are
available
> > to also be used for allocating buffers of those formats.
> 
> No, both have different purpose.
> 
> Note this class (GpuMemoryBufferFactory) is gpu process implementation except
for IsGpuMemoryBufferConfigurationSupported() and
GetSupportedGpuMemoryBufferConfigurations() methods.
> ozone has corresponding gpu process implementation, which is
|ozone_native_pixmap_factory_|.
> However, ozone should handle IsGpuMemoryBufferConfigurationSupported() and
GetSupportedGpuMemoryBufferConfigurations() methods in browser and render
process. It's why this CL introduces NativePixmapManager for renderer and
browser.
> Android and MacOS don't need this consideration because their backend is only
one.

I don't follow why this needs to be two different interfaces. From content/'s
pov one interface for determining what configs are supported, allocating native
pixmaps and acquiring a native buffer from a file descriptor would make a lot of
sense. It would be similar to the IOSurface API but without the mach_port_t
mess.

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
content/test/content_test_suite.cc:114: if (!is_child_process) {
On 2015/07/23 at 16:42:09, dshwang wrote:
> On 2015/07/23 14:46:12, reveman wrote:
> > why do you need this "!is_child_process" check?
> 
> good question. why gfx::GLSurfaceTestSupport::InitializeOneOff() needs
"!is_child_process" check? InitializeOneOff() internally initialize ozone. So I
initialize NativePixmapManager when ozone is initialized.
> I don't fully understand what is |is_child_process| purpose.

Well it's probably called later in that case. Maybe this is too early to call
ui::NativePixmapManager::Create() if you need ozone to be initialized.

https://codereview.chromium.org/1128113011/diff/380002/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1128113011/diff/380002/content/app/content_ma...
content/app/content_main_runner.cc:691: if
(!ui::NativePixmapManager::GetInstance() &&
why the GetInstance check?

https://codereview.chromium.org/1128113011/diff/380002/content/app/content_ma...
content/app/content_main_runner.cc:694:
ui::NativePixmapManager::Create().release());
Please don't leak the manager. At least use a
base::LazyInstance<scoped_ptr<ui::NativePixmapManager>> if there's no
appropriate owner of this instance.

https://codereview.chromium.org/1128113011/diff/380002/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1128113011/diff/380002/content/browser/browse...
content/browser/browser_main_loop.cc:622:
ui::NativePixmapManager::Create().release());
Please add a member variable for the manager instead of leaking it.

https://codereview.chromium.org/1128113011/diff/380002/content/test/content_t...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1128113011/diff/380002/content/test/content_t...
content/test/content_test_suite.cc:116:
ui::NativePixmapManager::Create().release());
Don't leak the manager instance here as well.

https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:16: // provide a native pixmap for
Browser and Render process.
This doesn't match the interface. All this interface currently does is provide a
function that allows us to query supported configurations. What's missing?

Also, Browser/Render process are content/ concepts that this code is not
supposed to be aware of. Can you explain what this interface provides without
using those terms?

[dshwang, 2015-07-24 12:19:00]

Thank you for careful review!
I reply my opinion of your unified interface concern below.
In addition, add NativePixmapManager member to not leak.

https://codereview.chromium.org/1128113011/diff/380001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1400: #endif
On 2015/07/23 18:25:23, reveman wrote:
> On 2015/07/23 at 16:42:09, dshwang wrote:
> > On 2015/07/23 14:46:12, reveman wrote:
> > > what does this have to do with this patch?
> > 
> > yes, renderer creates NativePixmapManager. at the time, ozone decides right
> platform using this commend line.
> 
> In that case, you need to add it here too:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

thx for consideration, but chrome_restart_request already has the commend line
for browser and gpu

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:93: if
(!ozone_native_pixmap_factory_.CreateGpuMemoryBuffer(
On 2015/07/23 18:25:23, reveman wrote:
> On 2015/07/23 at 16:42:09, dshwang wrote:
> > On 2015/07/23 14:46:12, reveman wrote:
> > > Can ozone_native_pixmap_factory_ be replaced by NativePixmapManager? It's
> > > confusing to have two different interfaces for ozone native pixmaps and it
> makes
> > > sense to have the interface that is used to determine what formats are
> available
> > > to also be used for allocating buffers of those formats.
> > 
> > No, both have different purpose.
> > 
> > Note this class (GpuMemoryBufferFactory) is gpu process implementation
except
> for IsGpuMemoryBufferConfigurationSupported() and
> GetSupportedGpuMemoryBufferConfigurations() methods.
> > ozone has corresponding gpu process implementation, which is
> |ozone_native_pixmap_factory_|.
> > However, ozone should handle IsGpuMemoryBufferConfigurationSupported() and
> GetSupportedGpuMemoryBufferConfigurations() methods in browser and render
> process. It's why this CL introduces NativePixmapManager for renderer and
> browser.
> > Android and MacOS don't need this consideration because their backend is
only
> one.
> 
> I don't follow why this needs to be two different interfaces. From content/'s
> pov one interface for determining what configs are supported, allocating
native
> pixmaps and acquiring a native buffer from a file descriptor would make a lot
of
> sense. It would be similar to the IOSurface API but without the mach_port_t
> mess.

No, this class should have content/gpu pov, not content/ pov. actually
GpuMemoryBufferFactory and GpuMemoryBufferFactory<Platform> should belong to
content/gpu, not content/common/gpu/. The reason why those classes stay in
content/common/ is that browser (and potentially renderer also) abuses
GpuMemoryBufferFactory::GetSupportedTypes static method.
It's fine for browser on Android and MacOS, because those platform determine
supported configuration in compile time. However, ozone should determine the
configuration in runtime.
OzonePlatform interface is for gpu process. currently browser process slightly
abuses OzonePlatform interface, but it will be get rid of in the future.
Now we understand Ozone needs non-GPU process interface to support configuration
and native pixmap, and it's why this CL introduces NativePixmapManager.

I'll move GpuMemoryBufferFactory to content/gpu and remain only
GpuMemoryBufferFactory::GetSupportedTypes() in content/common in another CL in
the future.

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1128113011/diff/380001/content/test/content_t...
content/test/content_test_suite.cc:114: if (!is_child_process) {
On 2015/07/23 18:25:23, reveman wrote:
> On 2015/07/23 at 16:42:09, dshwang wrote:
> > On 2015/07/23 14:46:12, reveman wrote:
> > > why do you need this "!is_child_process" check?
> > 
> > good question. why gfx::GLSurfaceTestSupport::InitializeOneOff() needs
> "!is_child_process" check? InitializeOneOff() internally initialize ozone. So
I
> initialize NativePixmapManager when ozone is initialized.
> > I don't fully understand what is |is_child_process| purpose.
> 
> Well it's probably called later in that case. Maybe this is too early to call
> ui::NativePixmapManager::Create() if you need ozone to be initialized.

I'm fixing unittests for ozone in https://codereview.chromium.org/1208603002/
I'll handle "is_child_process" case later if needed.

https://codereview.chromium.org/1128113011/diff/380002/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1128113011/diff/380002/content/app/content_ma...
content/app/content_main_runner.cc:691: if
(!ui::NativePixmapManager::GetInstance() &&
On 2015/07/23 18:25:23, reveman wrote:
> why the GetInstance check?

I assumed that this routine can be called several times with --single-process. I
was wrong, so I remove GetInstance check.

https://codereview.chromium.org/1128113011/diff/380002/content/app/content_ma...
content/app/content_main_runner.cc:694:
ui::NativePixmapManager::Create().release());
On 2015/07/23 18:25:23, reveman wrote:
> Please don't leak the manager. At least use a
> base::LazyInstance<scoped_ptr<ui::NativePixmapManager>> if there's no
> appropriate owner of this instance.

Done. use member.

https://codereview.chromium.org/1128113011/diff/380002/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1128113011/diff/380002/content/browser/browse...
content/browser/browser_main_loop.cc:622:
ui::NativePixmapManager::Create().release());
On 2015/07/23 18:25:23, reveman wrote:
> Please add a member variable for the manager instead of leaking it.

Done.

https://codereview.chromium.org/1128113011/diff/380002/content/test/content_t...
File content/test/content_test_suite.cc (right):

https://codereview.chromium.org/1128113011/diff/380002/content/test/content_t...
content/test/content_test_suite.cc:116:
ui::NativePixmapManager::Create().release());
On 2015/07/23 18:25:23, reveman wrote:
> Don't leak the manager instance here as well.

Done.

https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:16: // provide a native pixmap for
Browser and Render process.
On 2015/07/23 18:25:23, reveman wrote:
> This doesn't match the interface. All this interface currently does is provide
a
> function that allows us to query supported configurations. What's missing?

Future CL will add "scoped_refptr<NativePixmap> CreateFromFileDescriptor()"
method;
https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/public/native...
The CreateFromFileDescriptor() method is main motivation of this class.

> Also, Browser/Render process are content/ concepts that this code is not
> supposed to be aware of. Can you explain what this interface provides without
> using those terms?

s/Browser and Render process/non-GPU processes/

ozone is aware of gpu process;
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/public/gp...
It's because ozone is gpu process backend.

[reveman, 2015-07-24 14:54:07]

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:93: if
(!ozone_native_pixmap_factory_.CreateGpuMemoryBuffer(
On 2015/07/24 at 12:19:00, dshwang wrote:
> On 2015/07/23 18:25:23, reveman wrote:
> > On 2015/07/23 at 16:42:09, dshwang wrote:
> > > On 2015/07/23 14:46:12, reveman wrote:
> > > > Can ozone_native_pixmap_factory_ be replaced by NativePixmapManager?
It's
> > > > confusing to have two different interfaces for ozone native pixmaps and
it
> > makes
> > > > sense to have the interface that is used to determine what formats are
> > available
> > > > to also be used for allocating buffers of those formats.
> > > 
> > > No, both have different purpose.
> > > 
> > > Note this class (GpuMemoryBufferFactory) is gpu process implementation
except
> > for IsGpuMemoryBufferConfigurationSupported() and
> > GetSupportedGpuMemoryBufferConfigurations() methods.
> > > ozone has corresponding gpu process implementation, which is
> > |ozone_native_pixmap_factory_|.
> > > However, ozone should handle IsGpuMemoryBufferConfigurationSupported() and
> > GetSupportedGpuMemoryBufferConfigurations() methods in browser and render
> > process. It's why this CL introduces NativePixmapManager for renderer and
> > browser.
> > > Android and MacOS don't need this consideration because their backend is
only
> > one.
> > 
> > I don't follow why this needs to be two different interfaces. From
content/'s
> > pov one interface for determining what configs are supported, allocating
native
> > pixmaps and acquiring a native buffer from a file descriptor would make a
lot of
> > sense. It would be similar to the IOSurface API but without the mach_port_t
> > mess.
> 
> No, this class should have content/gpu pov, not content/ pov. actually
GpuMemoryBufferFactory and GpuMemoryBufferFactory<Platform> should belong to
content/gpu, not content/common/gpu/. The reason why those classes stay in
content/common/ is that browser (and potentially renderer also) abuses
GpuMemoryBufferFactory::GetSupportedTypes static method.
> It's fine for browser on Android and MacOS, because those platform determine
supported configuration in compile time. However, ozone should determine the
configuration in runtime.
> OzonePlatform interface is for gpu process. currently browser process slightly
abuses OzonePlatform interface, but it will be get rid of in the future.
> Now we understand Ozone needs non-GPU process interface to support
configuration and native pixmap, and it's why this CL introduces
NativePixmapManager.
> 
> I'll move GpuMemoryBufferFactory to content/gpu and remain only
GpuMemoryBufferFactory::GetSupportedTypes() in content/common in another CL in
the future.

Please don't do that.

Video encoding changes being worked on by a different team will require us to
create GMB instances on the GPU process side so we'll need a manager there too.
For simplicity, I'd like just one (non-process specific) interface for
allocating and creating native pixmap instances as that's what all other
platforms provide.

Btw, the code in ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h/cc
should be moved to
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h/cc now that
that class changed name and it's not supposed to manage native "buffers" anymore
but instead native "pixmaps". The reason we introduced it in the past was to
avoid exposing the "pixmap" concept to content/ but as part of giving up on that
we should move that code to content/.

https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:16: // provide a native pixmap for
Browser and Render process.
On 2015/07/24 at 12:19:00, dshwang wrote:
> On 2015/07/23 18:25:23, reveman wrote:
> > This doesn't match the interface. All this interface currently does is
provide a
> > function that allows us to query supported configurations. What's missing?
> 
> Future CL will add "scoped_refptr<NativePixmap> CreateFromFileDescriptor()"
method;
>
https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/public/native...
> The CreateFromFileDescriptor() method is main motivation of this class.
> 
> > Also, Browser/Render process are content/ concepts that this code is not
> > supposed to be aware of. Can you explain what this interface provides
without
> > using those terms?
> 
> s/Browser and Render process/non-GPU processes/
> 
> ozone is aware of gpu process;
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/public/gp...
> It's because ozone is gpu process backend.

Please include all functions (with NOTIMPLEMENTED statements if necessary) in
this interface as that makes it easier to understand what it's used for.
Otherwise we have a description that doesn't match the code.

Note: I'd really like to see both a CreateNativePixmapFromFileDescriptor() and
CreateNativePixmap() function as part of this interface so it can replace the
current ui::GpuMemoryBufferFactoryOzoneNativeBuffer code.

[dshwang, 2015-07-24 18:20:18]

Thank you for careful reviewing.
During resolving you comment, I submit
https://codereview.chromium.org/1258713002/
I'll rebase this CL on the CL.

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
(right):

https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:93: if
(!ozone_native_pixmap_factory_.CreateGpuMemoryBuffer(
On 2015/07/24 14:54:07, reveman wrote:
> On 2015/07/24 at 12:19:00, dshwang wrote:
> > On 2015/07/23 18:25:23, reveman wrote:
> > > On 2015/07/23 at 16:42:09, dshwang wrote:
> > > > On 2015/07/23 14:46:12, reveman wrote:
> > > > > Can ozone_native_pixmap_factory_ be replaced by NativePixmapManager?
> It's
> > > > > confusing to have two different interfaces for ozone native pixmaps
and
> it
> > > makes
> > > > > sense to have the interface that is used to determine what formats are
> > > available
> > > > > to also be used for allocating buffers of those formats.
> > > > 
> > > > No, both have different purpose.
> > > > 
> > > > Note this class (GpuMemoryBufferFactory) is gpu process implementation
> except
> > > for IsGpuMemoryBufferConfigurationSupported() and
> > > GetSupportedGpuMemoryBufferConfigurations() methods.
> > > > ozone has corresponding gpu process implementation, which is
> > > |ozone_native_pixmap_factory_|.
> > > > However, ozone should handle IsGpuMemoryBufferConfigurationSupported()
and
> > > GetSupportedGpuMemoryBufferConfigurations() methods in browser and render
> > > process. It's why this CL introduces NativePixmapManager for renderer and
> > > browser.
> > > > Android and MacOS don't need this consideration because their backend is
> only
> > > one.
> > > 
> > > I don't follow why this needs to be two different interfaces. From
> content/'s
> > > pov one interface for determining what configs are supported, allocating
> native
> > > pixmaps and acquiring a native buffer from a file descriptor would make a
> lot of
> > > sense. It would be similar to the IOSurface API but without the
mach_port_t
> > > mess.
> > 
> > No, this class should have content/gpu pov, not content/ pov. actually
> GpuMemoryBufferFactory and GpuMemoryBufferFactory<Platform> should belong to
> content/gpu, not content/common/gpu/. The reason why those classes stay in
> content/common/ is that browser (and potentially renderer also) abuses
> GpuMemoryBufferFactory::GetSupportedTypes static method.
> > It's fine for browser on Android and MacOS, because those platform determine
> supported configuration in compile time. However, ozone should determine the
> configuration in runtime.
> > OzonePlatform interface is for gpu process. currently browser process
slightly
> abuses OzonePlatform interface, but it will be get rid of in the future.
> > Now we understand Ozone needs non-GPU process interface to support
> configuration and native pixmap, and it's why this CL introduces
> NativePixmapManager.
> > 
> > I'll move GpuMemoryBufferFactory to content/gpu and remain only
> GpuMemoryBufferFactory::GetSupportedTypes() in content/common in another CL in
> the future.
> 
> Please don't do that.
> 
> Video encoding changes being worked on by a different team will require us to
> create GMB instances on the GPU process side so we'll need a manager there
too.

Ok. However, I cannot understand why moving factory to content/gpu affects badly
creating GMB instances on the GPU process side.

> For simplicity, I'd like just one (non-process specific) interface for
> allocating and creating native pixmap instances as that's what all other
> platforms provide.

On Ozone GBM, it's impossible.
Renderer and Browser create NativePixmap by VGEM device, while GPU process
create NativePixmap by GPU device (i.e. drm/card0).
It's why GPU process use OzonePlatform interface like
  scoped_refptr<ui::NativePixmap> pixmap =
      ui::OzonePlatform::GetInstance()
          ->GetSurfaceFactoryOzone()
          ->CreateNativePixmap(surface_handle, size, GetOzoneFormatFor(format),
                               GetOzoneUsageFor(usage));
, while Renderer and Browser use NativePixmapManager interface like
  scoped_refptr<ui::NativePixmap> pixmap =
        native_pixmap_configurations =
            ui::NativePixmapManager::GetInstance()
                ->CreateNativePixmapFromFileDescriptor();

NativePixmapManager should not know GPU device.

> Btw, the code in
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h/cc
> should be moved to
> content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h/cc now that
> that class changed name and it's not supposed to manage native "buffers"
anymore
> but instead native "pixmaps". The reason we introduced it in the past was to
> avoid exposing the "pixmap" concept to content/ but as part of giving up on
that
> we should move that code to content/.

That's good suggestion. I submit https://codereview.chromium.org/1258713002/
I'll rebase this CL base on above CL.

https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:16: // provide a native pixmap for
Browser and Render process.
On 2015/07/24 14:54:07, reveman wrote:
> On 2015/07/24 at 12:19:00, dshwang wrote:
> > On 2015/07/23 18:25:23, reveman wrote:
> > > This doesn't match the interface. All this interface currently does is
> provide a
> > > function that allows us to query supported configurations. What's missing?
> > 
> > Future CL will add "scoped_refptr<NativePixmap> CreateFromFileDescriptor()"
> method;
> >
>
https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/public/native...
> > The CreateFromFileDescriptor() method is main motivation of this class.
> > 
> > > Also, Browser/Render process are content/ concepts that this code is not
> > > supposed to be aware of. Can you explain what this interface provides
> without
> > > using those terms?
> > 
> > s/Browser and Render process/non-GPU processes/
> > 
> > ozone is aware of gpu process;
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/public/gp...
> > It's because ozone is gpu process backend.
> 
> Please include all functions (with NOTIMPLEMENTED statements if necessary) in
> this interface as that makes it easier to understand what it's used for.
> Otherwise we have a description that doesn't match the code.

Ok, I'll do it in next patch.

> 
> Note: I'd really like to see both a CreateNativePixmapFromFileDescriptor() and
> CreateNativePixmap() function as part of this interface so it can replace the
> current ui::GpuMemoryBufferFactoryOzoneNativeBuffer code.

As I explained above, CreateNativePixmap() function should not be part of this
interface because NativePixmapManager should not know real GPU device.

[spang, 2015-07-24 23:54:35]

On 2015/07/24 18:20:18, dshwang wrote:
> Thank you for careful reviewing.
> During resolving you comment, I submit
> https://codereview.chromium.org/1258713002/
> I'll rebase this CL on the CL.
> 
>
https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
> File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
> (right):
> 
>
https://codereview.chromium.org/1128113011/diff/380001/content/common/gpu/gpu...
> content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:93: if
> (!ozone_native_pixmap_factory_.CreateGpuMemoryBuffer(
> On 2015/07/24 14:54:07, reveman wrote:
> > On 2015/07/24 at 12:19:00, dshwang wrote:
> > > On 2015/07/23 18:25:23, reveman wrote:
> > > > On 2015/07/23 at 16:42:09, dshwang wrote:
> > > > > On 2015/07/23 14:46:12, reveman wrote:
> > > > > > Can ozone_native_pixmap_factory_ be replaced by NativePixmapManager?
> > It's
> > > > > > confusing to have two different interfaces for ozone native pixmaps
> and
> > it
> > > > makes
> > > > > > sense to have the interface that is used to determine what formats
are
> > > > available
> > > > > > to also be used for allocating buffers of those formats.
> > > > > 
> > > > > No, both have different purpose.
> > > > > 
> > > > > Note this class (GpuMemoryBufferFactory) is gpu process implementation
> > except
> > > > for IsGpuMemoryBufferConfigurationSupported() and
> > > > GetSupportedGpuMemoryBufferConfigurations() methods.
> > > > > ozone has corresponding gpu process implementation, which is
> > > > |ozone_native_pixmap_factory_|.
> > > > > However, ozone should handle IsGpuMemoryBufferConfigurationSupported()
> and
> > > > GetSupportedGpuMemoryBufferConfigurations() methods in browser and
render
> > > > process. It's why this CL introduces NativePixmapManager for renderer
and
> > > > browser.
> > > > > Android and MacOS don't need this consideration because their backend
is
> > only
> > > > one.
> > > > 
> > > > I don't follow why this needs to be two different interfaces. From
> > content/'s
> > > > pov one interface for determining what configs are supported, allocating
> > native
> > > > pixmaps and acquiring a native buffer from a file descriptor would make
a
> > lot of
> > > > sense. It would be similar to the IOSurface API but without the
> mach_port_t
> > > > mess.
> > > 
> > > No, this class should have content/gpu pov, not content/ pov. actually
> > GpuMemoryBufferFactory and GpuMemoryBufferFactory<Platform> should belong to
> > content/gpu, not content/common/gpu/. The reason why those classes stay in
> > content/common/ is that browser (and potentially renderer also) abuses
> > GpuMemoryBufferFactory::GetSupportedTypes static method.
> > > It's fine for browser on Android and MacOS, because those platform
determine
> > supported configuration in compile time. However, ozone should determine the
> > configuration in runtime.
> > > OzonePlatform interface is for gpu process. currently browser process
> slightly
> > abuses OzonePlatform interface, but it will be get rid of in the future.
> > > Now we understand Ozone needs non-GPU process interface to support
> > configuration and native pixmap, and it's why this CL introduces
> > NativePixmapManager.
> > > 
> > > I'll move GpuMemoryBufferFactory to content/gpu and remain only
> > GpuMemoryBufferFactory::GetSupportedTypes() in content/common in another CL
in
> > the future.
> > 
> > Please don't do that.
> > 
> > Video encoding changes being worked on by a different team will require us
to
> > create GMB instances on the GPU process side so we'll need a manager there
> too.
> 
> Ok. However, I cannot understand why moving factory to content/gpu affects
badly
> creating GMB instances on the GPU process side.
> 
> > For simplicity, I'd like just one (non-process specific) interface for
> > allocating and creating native pixmap instances as that's what all other
> > platforms provide.
> 
> On Ozone GBM, it's impossible.
> Renderer and Browser create NativePixmap by VGEM device, while GPU process
> create NativePixmap by GPU device (i.e. drm/card0).
> It's why GPU process use OzonePlatform interface like
>   scoped_refptr<ui::NativePixmap> pixmap =
>       ui::OzonePlatform::GetInstance()
>           ->GetSurfaceFactoryOzone()
>           ->CreateNativePixmap(surface_handle, size,
GetOzoneFormatFor(format),
>                                GetOzoneUsageFor(usage));
> , while Renderer and Browser use NativePixmapManager interface like
>   scoped_refptr<ui::NativePixmap> pixmap =
>         native_pixmap_configurations =
>             ui::NativePixmapManager::GetInstance()
>                 ->CreateNativePixmapFromFileDescriptor();
> 
> NativePixmapManager should not know GPU device.
> 
> > Btw, the code in
> ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h/cc
> > should be moved to
> > content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h/cc now
that
> > that class changed name and it's not supposed to manage native "buffers"
> anymore
> > but instead native "pixmaps". The reason we introduced it in the past was to
> > avoid exposing the "pixmap" concept to content/ but as part of giving up on
> that
> > we should move that code to content/.
> 
> That's good suggestion. I submit https://codereview.chromium.org/1258713002/
> I'll rebase this CL base on above CL.
> 
>
https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
> File ui/ozone/public/native_pixmap_manager.h (right):
> 
>
https://codereview.chromium.org/1128113011/diff/380002/ui/ozone/public/native...
> ui/ozone/public/native_pixmap_manager.h:16: // provide a native pixmap for
> Browser and Render process.
> On 2015/07/24 14:54:07, reveman wrote:
> > On 2015/07/24 at 12:19:00, dshwang wrote:
> > > On 2015/07/23 18:25:23, reveman wrote:
> > > > This doesn't match the interface. All this interface currently does is
> > provide a
> > > > function that allows us to query supported configurations. What's
missing?
> > > 
> > > Future CL will add "scoped_refptr<NativePixmap>
CreateFromFileDescriptor()"
> > method;
> > >
> >
>
https://codereview.chromium.org/1134993003/diff/200001/ui/ozone/public/native...
> > > The CreateFromFileDescriptor() method is main motivation of this class.
> > > 
> > > > Also, Browser/Render process are content/ concepts that this code is not
> > > > supposed to be aware of. Can you explain what this interface provides
> > without
> > > > using those terms?
> > > 
> > > s/Browser and Render process/non-GPU processes/
> > > 
> > > ozone is aware of gpu process;
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/public/gp...
> > > It's because ozone is gpu process backend.
> > 
> > Please include all functions (with NOTIMPLEMENTED statements if necessary)
in
> > this interface as that makes it easier to understand what it's used for.
> > Otherwise we have a description that doesn't match the code.
> 
> Ok, I'll do it in next patch.
> 
> > 
> > Note: I'd really like to see both a CreateNativePixmapFromFileDescriptor()
and
> > CreateNativePixmap() function as part of this interface so it can replace
the
> > current ui::GpuMemoryBufferFactoryOzoneNativeBuffer code.
> 
> As I explained above, CreateNativePixmap() function should not be part of this
> interface because NativePixmapManager should not know real GPU device.

Yeah, there's two APIs here

(1) The privileged (GPU process) API.
 
 This is SurfaceFactoryOzone. It's used to allocate buffer objects and swap onto
a surface/display, etc. By design there's only one process using this API. It's
extremely driver specific under the hood, including in userspace.

  CreateNativePixmap goes here.

(2) The unprivileged API.

  This is the one introduced here in this patch, so currently proposed as
"NativePixmapManager".

 It can only take handles to existing objects and map them, and it should work
under the sandbox. It's only driver specific in the kernel; the userland part
doesn't require loading drivers.

  CreateNativePixmapFromFileDescriptor goes here.
  (really, we should s/Create/Import/)

I don't think it makes sense to conflate these into one interface. To do
anything that fits under (1) you need to own the master display device, but (2)
requires a less privileged device and a buffer handle. Ideally we wouldn't even
have the less privileged device, but the dmabuf API don't enable this currently.

[dshwang, 2015-07-27 07:47:12]

On 2015/07/24 23:54:35, spang wrote:
> On 2015/07/24 18:20:18, dshwang wrote:
> > > Note: I'd really like to see both a CreateNativePixmapFromFileDescriptor()
> and
> > > CreateNativePixmap() function as part of this interface so it can replace
> the
> > > current ui::GpuMemoryBufferFactoryOzoneNativeBuffer code.
> > 
> > As I explained above, CreateNativePixmap() function should not be part of
this
> > interface because NativePixmapManager should not know real GPU device.
> 
> Yeah, there's two APIs here
> 
> (1) The privileged (GPU process) API.
>  
>  This is SurfaceFactoryOzone. It's used to allocate buffer objects and swap
onto
> a surface/display, etc. By design there's only one process using this API.
It's
> extremely driver specific under the hood, including in userspace.
> 
>   CreateNativePixmap goes here.
> 
> (2) The unprivileged API.
> 
>   This is the one introduced here in this patch, so currently proposed as
> "NativePixmapManager".
> 
>  It can only take handles to existing objects and map them, and it should work
> under the sandbox. It's only driver specific in the kernel; the userland part
> doesn't require loading drivers.
> 
>   CreateNativePixmapFromFileDescriptor goes here.
>   (really, we should s/Create/Import/)
> 
> I don't think it makes sense to conflate these into one interface. To do
> anything that fits under (1) you need to own the master display device, but
(2)
> requires a less privileged device and a buffer handle. Ideally we wouldn't
even
> have the less privileged device, but the dmabuf API don't enable this
currently.

Thank you for nice explanation. I add this explanation to CL's description.
Rebase this CL based on crrev.com/1258713002 as well as address nits.

[dshwang, 2015-07-27 07:50:45]

Patchset #16 (id:430001) has been deleted

[reveman, 2015-07-27 19:52:21]

Ok, I'm fine with keeping the SurfaceFactory interface for allocating pixmaps
even though I have a hard time seeing the value of it now that we have a
NativePixmapManager interface. I would have expected different implementations
of the NativePixmapManager interface based on the privileges allowed in a
specific process rather than two different APIs. Having one interface to ask
what configurations are supported/creating instances from handles and a
different one for allocating the new pixmaps is harder to understand from
contents/ pov imo.

[vignatti (out of this project), 2015-07-27 20:14:05]

On 2015/07/24 23:54:35, spang wrote:
> I don't think it makes sense to conflate these into one interface. To do
> anything that fits under (1) you need to own the master display device, but
(2)
> requires a less privileged device and a buffer handle. Ideally we wouldn't
even
> have the less privileged device, but the dmabuf API don't enable this
currently.

FWIW I'm right now investigating how to fix the dma-buf API, so a process could
mmap a gem buffer by simply passing its prime fd.

[spang, 2015-07-27 22:08:06]

On 2015/07/27 19:52:21, reveman wrote:
> Ok, I'm fine with keeping the SurfaceFactory interface for allocating pixmaps
> even though I have a hard time seeing the value of it now that we have a
> NativePixmapManager interface. I would have expected different implementations
> of the NativePixmapManager interface based on the privileges allowed in a
> specific process rather than two different APIs. Having one interface to ask
> what configurations are supported/creating instances from handles and a
> different one for allocating the new pixmaps is harder to understand from
> contents/ pov imo.

Hm, what would NativePixmapManager::CreateNativePixmap() do in the renderer, if
we used the same interface? It would have to simply NOTREACHED().

I honestly think it is a design error if we cannot determine what functionality
we have just based on the typename.

[spang, 2015-07-27 22:13:00]

On 2015/07/27 22:08:06, spang wrote:
> On 2015/07/27 19:52:21, reveman wrote:
> > Ok, I'm fine with keeping the SurfaceFactory interface for allocating
pixmaps
> > even though I have a hard time seeing the value of it now that we have a
> > NativePixmapManager interface. I would have expected different
implementations
> > of the NativePixmapManager interface based on the privileges allowed in a
> > specific process rather than two different APIs. Having one interface to ask
> > what configurations are supported/creating instances from handles and a
> > different one for allocating the new pixmaps is harder to understand from
> > contents/ pov imo.
> 
> Hm, what would NativePixmapManager::CreateNativePixmap() do in the renderer,
if
> we used the same interface? It would have to simply NOTREACHED().
> 
> I honestly think it is a design error if we cannot determine what
functionality
> we have just based on the typename.

Actually that's probably too strong, but I think we should at least avoid the
case of reusing an interface in contexts where a key part of its functionality
will actually never be usable.

[reveman, 2015-07-28 15:23:47]

On 2015/07/27 at 22:08:06, spang wrote:
> On 2015/07/27 19:52:21, reveman wrote:
> > Ok, I'm fine with keeping the SurfaceFactory interface for allocating
pixmaps
> > even though I have a hard time seeing the value of it now that we have a
> > NativePixmapManager interface. I would have expected different
implementations
> > of the NativePixmapManager interface based on the privileges allowed in a
> > specific process rather than two different APIs. Having one interface to ask
> > what configurations are supported/creating instances from handles and a
> > different one for allocating the new pixmaps is harder to understand from
> > contents/ pov imo.
> 
> Hm, what would NativePixmapManager::CreateNativePixmap() do in the renderer,
if we used the same interface? It would have to simply NOTREACHED().

In single process mode it would succeed. Maybe even in multi-process mode if the
sandbox was disabled. I guess it would return null on failure.

> 
> I honestly think it is a design error if we cannot determine what
functionality we have just based on the typename.

I'm fine with two separate interfaces if that's preferred. I think we just have
two slightly different points of view here. I think of this as the
"NativePixmap" API, where some parts of the API require less sandbox
restrictions than others. Much like we have one IOSurface API and some parts of
it will fail in the renderer sandbox.

[dshwang, 2015-07-28 16:54:12]

On 2015/07/28 15:23:47, reveman wrote:
> On 2015/07/27 at 22:08:06, spang wrote:
> > On 2015/07/27 19:52:21, reveman wrote:
> > > Ok, I'm fine with keeping the SurfaceFactory interface for allocating
> pixmaps
> > > even though I have a hard time seeing the value of it now that we have a
> > > NativePixmapManager interface. I would have expected different
> implementations
> > > of the NativePixmapManager interface based on the privileges allowed in a
> > > specific process rather than two different APIs. Having one interface to
ask
> > > what configurations are supported/creating instances from handles and a
> > > different one for allocating the new pixmaps is harder to understand from
> > > contents/ pov imo.
> > 
> > Hm, what would NativePixmapManager::CreateNativePixmap() do in the renderer,
> if we used the same interface? It would have to simply NOTREACHED().
> 
> In single process mode it would succeed. Maybe even in multi-process mode if
the
> sandbox was disabled. I guess it would return null on failure.
> 
> > 
> > I honestly think it is a design error if we cannot determine what
> functionality we have just based on the typename.
> 
> I'm fine with two separate interfaces if that's preferred. I think we just
have
> two slightly different points of view here. I think of this as the
> "NativePixmap" API, where some parts of the API require less sandbox
> restrictions than others. Much like we have one IOSurface API and some parts
of
> it will fail in the renderer sandbox.

Thank you for all opinion. Both have its pros and cons.
Ozone developers prefers that SurfaceFactoryOzone has CreateNativePixmap() and
NativePixmapManager has ImportNativePixmap(). So we will go this way.
I think the name of NativePixmapManager makes this confusion.
NativePixmapManager can be renamed to NativePixmapImporter because
NativePixmapManager doesn't create NativePixmap, although Android and IOS uses
the name of XXXManager.
WDYT?

https://codereview.chromium.org/1258713002/ is at end of reviewing. So could you
review this CL again? Thank you!

[reveman, 2015-07-28 18:07:57]

lgtm with nits

https://codereview.chromium.org/1128113011/diff/450001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/450001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:45: base::FileDescriptor handle,
nit: const base::FileDescriptor& handle

https://codereview.chromium.org/1128113011/diff/450001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_manager.h:46: gfx::Size size,
nit: const gfx::Size& size

[reveman, 2015-07-28 18:09:28]

On 2015/07/28 at 16:54:12, dongseong.hwang wrote:
> On 2015/07/28 15:23:47, reveman wrote:
> > On 2015/07/27 at 22:08:06, spang wrote:
> > > On 2015/07/27 19:52:21, reveman wrote:
> > > > Ok, I'm fine with keeping the SurfaceFactory interface for allocating
> > pixmaps
> > > > even though I have a hard time seeing the value of it now that we have a
> > > > NativePixmapManager interface. I would have expected different
> > implementations
> > > > of the NativePixmapManager interface based on the privileges allowed in
a
> > > > specific process rather than two different APIs. Having one interface to
ask
> > > > what configurations are supported/creating instances from handles and a
> > > > different one for allocating the new pixmaps is harder to understand
from
> > > > contents/ pov imo.
> > > 
> > > Hm, what would NativePixmapManager::CreateNativePixmap() do in the
renderer,
> > if we used the same interface? It would have to simply NOTREACHED().
> > 
> > In single process mode it would succeed. Maybe even in multi-process mode if
the
> > sandbox was disabled. I guess it would return null on failure.
> > 
> > > 
> > > I honestly think it is a design error if we cannot determine what
> > functionality we have just based on the typename.
> > 
> > I'm fine with two separate interfaces if that's preferred. I think we just
have
> > two slightly different points of view here. I think of this as the
> > "NativePixmap" API, where some parts of the API require less sandbox
> > restrictions than others. Much like we have one IOSurface API and some parts
of
> > it will fail in the renderer sandbox.
> 
> Thank you for all opinion. Both have its pros and cons.
> Ozone developers prefers that SurfaceFactoryOzone has CreateNativePixmap() and
NativePixmapManager has ImportNativePixmap(). So we will go this way.
> I think the name of NativePixmapManager makes this confusion.
> NativePixmapManager can be renamed to NativePixmapImporter because
NativePixmapManager doesn't create NativePixmap, although Android and IOS uses
the name of XXXManager.
> WDYT?

I think calling it NativePixmapManager is fine. It creates NativePixmap
instances after all. Just doesn't allocate the storage for them.

[spang, 2015-07-28 18:17:34]

lgtm

[spang, 2015-07-28 23:36:15]

On 2015/07/28 18:17:34, spang wrote:
> lgtm

Actually, thinking about this more and looking at the next CL, I'm not 100% sure
about naming this "NativePixmapManager".

I think the clearest path to fit into the existing framework is to have a
NativePixmapManager that's analogous to IOSurfaceManager, which lives in
content/. It would be involved in transporting the buffer via IPC (just like
IOSurfaceManager does with its mach transaction).

But we should not also have a platform object with the same name. I'm not sure
about alternative, though.

I think we should hold off just a bit longer on this until the following patch
is in better shape.

[dshwang, 2015-07-29 14:48:42]

On 2015/07/28 23:36:15, spang wrote:
> On 2015/07/28 18:17:34, spang wrote:
> > lgtm
> 
> Actually, thinking about this more and looking at the next CL, I'm not 100%
sure
> about naming this "NativePixmapManager".
> 
> I think the clearest path to fit into the existing framework is to have a
> NativePixmapManager that's analogous to IOSurfaceManager, which lives in
> content/. It would be involved in transporting the buffer via IPC (just like
> IOSurfaceManager does with its mach transaction).
> 
> But we should not also have a platform object with the same name. I'm not sure
> about alternative, though.
> 
> I think we should hold off just a bit longer on this until the following patch
> is in better shape.

After rethinking, I feel exposing NativePixmap to renderer is not good. So far,
NativePixmap is used in only gpu process and all methods are not needed for
renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
In addition, Map()/Unmap() methods which next CL will add are not needed for gpu
process also.
The main purpose of NativePixmap is to abstract the backend of GLImage in Ozone.
I think we need to invent new concept/name for renderer. The name should be
related to NativePixmap because the counter part in gpu process is NativePixmap.
FYI, GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer() creates
GpuMemoryBufferHandle from NativePixmap in gpu process.

My suggestion is as follows;
Introduce "Pixmap" pure abstract class which defines Format and Usage enum,
which both gpu process and renderer need.
Rename NativePixmap to GLPixmap and only gpu process uses GLPixmap. GLPixmap
inherits Pixmap.
Introduce NativePixmap which renderer use. NativePixmap inherits Pixmap and has
Map/Unmap methods.

In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap should be renamed to
GpuMemoryBuffer(Factory|Impl)Ozone because GpuMemoryBufferFactory uses
"GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap". Alternative renaming is
GpuMemoryBufferFACTORYOzoneGLPixmap and GpuMemoryBufferIMPLOzoneNativePixmap but
looks inconsistent and burdensome.

WDYT?

[reveman, 2015-07-29 14:58:35]

On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> On 2015/07/28 23:36:15, spang wrote:
> > On 2015/07/28 18:17:34, spang wrote:
> > > lgtm
> > 
> > Actually, thinking about this more and looking at the next CL, I'm not 100%
sure
> > about naming this "NativePixmapManager".
> > 
> > I think the clearest path to fit into the existing framework is to have a
> > NativePixmapManager that's analogous to IOSurfaceManager, which lives in
> > content/. It would be involved in transporting the buffer via IPC (just like
> > IOSurfaceManager does with its mach transaction).
> > 
> > But we should not also have a platform object with the same name. I'm not
sure
> > about alternative, though.
> > 
> > I think we should hold off just a bit longer on this until the following
patch
> > is in better shape.
> 
> After rethinking, I feel exposing NativePixmap to renderer is not good. So
far, NativePixmap is used in only gpu process and all methods are not needed for
renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> In addition, Map()/Unmap() methods which next CL will add are not needed for
gpu process also.
> The main purpose of NativePixmap is to abstract the backend of GLImage in
Ozone.
> I think we need to invent new concept/name for renderer. The name should be
related to NativePixmap because the counter part in gpu process is NativePixmap.
> FYI, GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer() creates
GpuMemoryBufferHandle from NativePixmap in gpu process.
> 
> My suggestion is as follows;
> Introduce "Pixmap" pure abstract class which defines Format and Usage enum,
which both gpu process and renderer need.
> Rename NativePixmap to GLPixmap and only gpu process uses GLPixmap. GLPixmap
inherits Pixmap.
> Introduce NativePixmap which renderer use. NativePixmap inherits Pixmap and
has Map/Unmap methods.
> 
> In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap should be renamed
to GpuMemoryBuffer(Factory|Impl)Ozone because GpuMemoryBufferFactory uses
"GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap". Alternative renaming is
GpuMemoryBufferFACTORYOzoneGLPixmap and GpuMemoryBufferIMPLOzoneNativePixmap but
looks inconsistent and burdensome.
> 
> WDYT?

In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of buffer and not
a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the correct name unless
this buffer type ends up being called simply "ozone" which is clearly not going
to be the case.

Can the class on the GPU process side that is needed to implement the GLImage
functionality be called "Surface"? SurfaceFactoryOzone is creating it after all.
NativePixmap is then available for the class name on the renderer side.

[dshwang, 2015-07-29 15:26:17]

On 2015/07/29 14:58:35, reveman wrote:
> On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > On 2015/07/28 23:36:15, spang wrote:
> > > On 2015/07/28 18:17:34, spang wrote:
> > > > lgtm
> > > 
> > > Actually, thinking about this more and looking at the next CL, I'm not
100%
> sure
> > > about naming this "NativePixmapManager".
> > > 
> > > I think the clearest path to fit into the existing framework is to have a
> > > NativePixmapManager that's analogous to IOSurfaceManager, which lives in
> > > content/. It would be involved in transporting the buffer via IPC (just
like
> > > IOSurfaceManager does with its mach transaction).
> > > 
> > > But we should not also have a platform object with the same name. I'm not
> sure
> > > about alternative, though.
> > > 
> > > I think we should hold off just a bit longer on this until the following
> patch
> > > is in better shape.
> > 
> > After rethinking, I feel exposing NativePixmap to renderer is not good. So
> far, NativePixmap is used in only gpu process and all methods are not needed
for
> renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > In addition, Map()/Unmap() methods which next CL will add are not needed for
> gpu process also.
> > The main purpose of NativePixmap is to abstract the backend of GLImage in
> Ozone.
> > I think we need to invent new concept/name for renderer. The name should be
> related to NativePixmap because the counter part in gpu process is
NativePixmap.
> > FYI, GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
creates
> GpuMemoryBufferHandle from NativePixmap in gpu process.
> > 
> > My suggestion is as follows;
> > Introduce "Pixmap" pure abstract class which defines Format and Usage enum,
> which both gpu process and renderer need.
> > Rename NativePixmap to GLPixmap and only gpu process uses GLPixmap. GLPixmap
> inherits Pixmap.
> > Introduce NativePixmap which renderer use. NativePixmap inherits Pixmap and
> has Map/Unmap methods.
> > 
> > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap should be
renamed
> to GpuMemoryBuffer(Factory|Impl)Ozone because GpuMemoryBufferFactory uses
> "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap". Alternative renaming
is
> GpuMemoryBufferFACTORYOzoneGLPixmap and GpuMemoryBufferIMPLOzoneNativePixmap
but
> looks inconsistent and burdensome.
> > 
> > WDYT?
> 
> In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of buffer and
not
> a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the correct name unless
> this buffer type ends up being called simply "ozone" which is clearly not
going
> to be the case.
> 
> Can the class on the GPU process side that is needed to implement the GLImage
> functionality be called "Surface"? SurfaceFactoryOzone is creating it after
all.
> NativePixmap is then available for the class name on the renderer side.

The main job of SurfaceFactoryOzone is to create SurfaceOzoneEGL, which is
backend of GLSurface.
SurfaceFactoryOzone creates NativePixmap as side job, because it can do, because
one of SurfaceOzoneEGL needs NativePixmap.
So "Surface" name is already occupied.

If we uses "Pixmap" name in gpu side, we can rename to
GpuMemoryBuffer(Factory|Impl)OzonePixmap.
gpu side will use "GLPixmap" and renderer will use "NativePixmap". If you
suggest better name, I'll appreciate!

[reveman, 2015-07-29 15:32:04]

On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> On 2015/07/29 14:58:35, reveman wrote:
> > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > On 2015/07/28 23:36:15, spang wrote:
> > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > lgtm
> > > > 
> > > > Actually, thinking about this more and looking at the next CL, I'm not
100%
> > sure
> > > > about naming this "NativePixmapManager".
> > > > 
> > > > I think the clearest path to fit into the existing framework is to have
a
> > > > NativePixmapManager that's analogous to IOSurfaceManager, which lives in
> > > > content/. It would be involved in transporting the buffer via IPC (just
like
> > > > IOSurfaceManager does with its mach transaction).
> > > > 
> > > > But we should not also have a platform object with the same name. I'm
not
> > sure
> > > > about alternative, though.
> > > > 
> > > > I think we should hold off just a bit longer on this until the following
> > patch
> > > > is in better shape.
> > > 
> > > After rethinking, I feel exposing NativePixmap to renderer is not good. So
> > far, NativePixmap is used in only gpu process and all methods are not needed
for
> > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > In addition, Map()/Unmap() methods which next CL will add are not needed
for
> > gpu process also.
> > > The main purpose of NativePixmap is to abstract the backend of GLImage in
> > Ozone.
> > > I think we need to invent new concept/name for renderer. The name should
be
> > related to NativePixmap because the counter part in gpu process is
NativePixmap.
> > > FYI, GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
creates
> > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > 
> > > My suggestion is as follows;
> > > Introduce "Pixmap" pure abstract class which defines Format and Usage
enum,
> > which both gpu process and renderer need.
> > > Rename NativePixmap to GLPixmap and only gpu process uses GLPixmap.
GLPixmap
> > inherits Pixmap.
> > > Introduce NativePixmap which renderer use. NativePixmap inherits Pixmap
and
> > has Map/Unmap methods.
> > > 
> > > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap should be
renamed
> > to GpuMemoryBuffer(Factory|Impl)Ozone because GpuMemoryBufferFactory uses
> > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap". Alternative renaming
is
> > GpuMemoryBufferFACTORYOzoneGLPixmap and GpuMemoryBufferIMPLOzoneNativePixmap
but
> > looks inconsistent and burdensome.
> > > 
> > > WDYT?
> > 
> > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of buffer and
not
> > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the correct name
unless
> > this buffer type ends up being called simply "ozone" which is clearly not
going
> > to be the case.
> > 
> > Can the class on the GPU process side that is needed to implement the
GLImage
> > functionality be called "Surface"? SurfaceFactoryOzone is creating it after
all.
> > NativePixmap is then available for the class name on the renderer side.
> 
> The main job of SurfaceFactoryOzone is to create SurfaceOzoneEGL, which is
backend of GLSurface.
> SurfaceFactoryOzone creates NativePixmap as side job, because it can do,
because one of SurfaceOzoneEGL needs NativePixmap.
> So "Surface" name is already occupied.
> 
> If we uses "Pixmap" name in gpu side, we can rename to
GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> gpu side will use "GLPixmap" and renderer will use "NativePixmap". If you
suggest better name, I'll appreciate!

Not a fan of GLPixmap. The GL prefix makes it look like it's assocaited and
dependent on a GL context. Maybe OzoneNativeBuffer (on service side) and
OzoneNativePixmap (on client side)?

[dshwang, 2015-07-29 15:52:15]

On 2015/07/29 15:32:04, reveman wrote:
> On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> > On 2015/07/29 14:58:35, reveman wrote:
> > > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > > On 2015/07/28 23:36:15, spang wrote:
> > > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > > lgtm
> > > > > 
> > > > > Actually, thinking about this more and looking at the next CL, I'm not
> 100%
> > > sure
> > > > > about naming this "NativePixmapManager".
> > > > > 
> > > > > I think the clearest path to fit into the existing framework is to
have
> a
> > > > > NativePixmapManager that's analogous to IOSurfaceManager, which lives
in
> > > > > content/. It would be involved in transporting the buffer via IPC
(just
> like
> > > > > IOSurfaceManager does with its mach transaction).
> > > > > 
> > > > > But we should not also have a platform object with the same name. I'm
> not
> > > sure
> > > > > about alternative, though.
> > > > > 
> > > > > I think we should hold off just a bit longer on this until the
following
> > > patch
> > > > > is in better shape.
> > > > 
> > > > After rethinking, I feel exposing NativePixmap to renderer is not good.
So
> > > far, NativePixmap is used in only gpu process and all methods are not
needed
> for
> > > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > > In addition, Map()/Unmap() methods which next CL will add are not needed
> for
> > > gpu process also.
> > > > The main purpose of NativePixmap is to abstract the backend of GLImage
in
> > > Ozone.
> > > > I think we need to invent new concept/name for renderer. The name should
> be
> > > related to NativePixmap because the counter part in gpu process is
> NativePixmap.
> > > > FYI, GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
> creates
> > > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > > 
> > > > My suggestion is as follows;
> > > > Introduce "Pixmap" pure abstract class which defines Format and Usage
> enum,
> > > which both gpu process and renderer need.
> > > > Rename NativePixmap to GLPixmap and only gpu process uses GLPixmap.
> GLPixmap
> > > inherits Pixmap.
> > > > Introduce NativePixmap which renderer use. NativePixmap inherits Pixmap
> and
> > > has Map/Unmap methods.
> > > > 
> > > > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap should be
> renamed
> > > to GpuMemoryBuffer(Factory|Impl)Ozone because GpuMemoryBufferFactory uses
> > > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap". Alternative
renaming
> is
> > > GpuMemoryBufferFACTORYOzoneGLPixmap and
GpuMemoryBufferIMPLOzoneNativePixmap
> but
> > > looks inconsistent and burdensome.
> > > > 
> > > > WDYT?
> > > 
> > > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of buffer
and
> not
> > > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the correct name
> unless
> > > this buffer type ends up being called simply "ozone" which is clearly not
> going
> > > to be the case.
> > > 
> > > Can the class on the GPU process side that is needed to implement the
> GLImage
> > > functionality be called "Surface"? SurfaceFactoryOzone is creating it
after
> all.
> > > NativePixmap is then available for the class name on the renderer side.
> > 
> > The main job of SurfaceFactoryOzone is to create SurfaceOzoneEGL, which is
> backend of GLSurface.
> > SurfaceFactoryOzone creates NativePixmap as side job, because it can do,
> because one of SurfaceOzoneEGL needs NativePixmap.
> > So "Surface" name is already occupied.
> > 
> > If we uses "Pixmap" name in gpu side, we can rename to
> GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> > gpu side will use "GLPixmap" and renderer will use "NativePixmap". If you
> suggest better name, I'll appreciate!
> 
> Not a fan of GLPixmap. The GL prefix makes it look like it's assocaited and
> dependent on a GL context. Maybe OzoneNativeBuffer (on service side) and
> OzoneNativePixmap (on client side)?

I see. I will use NativeBuffer and NativePixmap. NativePixmap is widely used in
service side already, so NativePixmap will be used in service side and
NativeBuffer will be used in client side.
To be consistent, I'll rename to GpuMemoryBufferFactoryNativePixmap and
GpuMemoryBufferImplNativeBuffer.
Does it sound good?

[reveman, 2015-07-29 16:20:14]

On 2015/07/29 at 15:52:15, dongseong.hwang wrote:
> On 2015/07/29 15:32:04, reveman wrote:
> > On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> > > On 2015/07/29 14:58:35, reveman wrote:
> > > > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > > > On 2015/07/28 23:36:15, spang wrote:
> > > > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > > > lgtm
> > > > > > 
> > > > > > Actually, thinking about this more and looking at the next CL, I'm
not
> > 100%
> > > > sure
> > > > > > about naming this "NativePixmapManager".
> > > > > > 
> > > > > > I think the clearest path to fit into the existing framework is to
have
> > a
> > > > > > NativePixmapManager that's analogous to IOSurfaceManager, which
lives in
> > > > > > content/. It would be involved in transporting the buffer via IPC
(just
> > like
> > > > > > IOSurfaceManager does with its mach transaction).
> > > > > > 
> > > > > > But we should not also have a platform object with the same name.
I'm
> > not
> > > > sure
> > > > > > about alternative, though.
> > > > > > 
> > > > > > I think we should hold off just a bit longer on this until the
following
> > > > patch
> > > > > > is in better shape.
> > > > > 
> > > > > After rethinking, I feel exposing NativePixmap to renderer is not
good. So
> > > > far, NativePixmap is used in only gpu process and all methods are not
needed
> > for
> > > > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > > > In addition, Map()/Unmap() methods which next CL will add are not
needed
> > for
> > > > gpu process also.
> > > > > The main purpose of NativePixmap is to abstract the backend of GLImage
in
> > > > Ozone.
> > > > > I think we need to invent new concept/name for renderer. The name
should
> > be
> > > > related to NativePixmap because the counter part in gpu process is
> > NativePixmap.
> > > > > FYI, GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
> > creates
> > > > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > > > 
> > > > > My suggestion is as follows;
> > > > > Introduce "Pixmap" pure abstract class which defines Format and Usage
> > enum,
> > > > which both gpu process and renderer need.
> > > > > Rename NativePixmap to GLPixmap and only gpu process uses GLPixmap.
> > GLPixmap
> > > > inherits Pixmap.
> > > > > Introduce NativePixmap which renderer use. NativePixmap inherits
Pixmap
> > and
> > > > has Map/Unmap methods.
> > > > > 
> > > > > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap should be
> > renamed
> > > > to GpuMemoryBuffer(Factory|Impl)Ozone because GpuMemoryBufferFactory
uses
> > > > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap". Alternative
renaming
> > is
> > > > GpuMemoryBufferFACTORYOzoneGLPixmap and
GpuMemoryBufferIMPLOzoneNativePixmap
> > but
> > > > looks inconsistent and burdensome.
> > > > > 
> > > > > WDYT?
> > > > 
> > > > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of buffer
and
> > not
> > > > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the correct name
> > unless
> > > > this buffer type ends up being called simply "ozone" which is clearly
not
> > going
> > > > to be the case.
> > > > 
> > > > Can the class on the GPU process side that is needed to implement the
> > GLImage
> > > > functionality be called "Surface"? SurfaceFactoryOzone is creating it
after
> > all.
> > > > NativePixmap is then available for the class name on the renderer side.
> > > 
> > > The main job of SurfaceFactoryOzone is to create SurfaceOzoneEGL, which is
> > backend of GLSurface.
> > > SurfaceFactoryOzone creates NativePixmap as side job, because it can do,
> > because one of SurfaceOzoneEGL needs NativePixmap.
> > > So "Surface" name is already occupied.
> > > 
> > > If we uses "Pixmap" name in gpu side, we can rename to
> > GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> > > gpu side will use "GLPixmap" and renderer will use "NativePixmap". If you
> > suggest better name, I'll appreciate!
> > 
> > Not a fan of GLPixmap. The GL prefix makes it look like it's assocaited and
> > dependent on a GL context. Maybe OzoneNativeBuffer (on service side) and
> > OzoneNativePixmap (on client side)?
> 
> I see. I will use NativeBuffer and NativePixmap. NativePixmap is widely used
in service side already, so NativePixmap will be used in service side and
NativeBuffer will be used in client side.
> To be consistent, I'll rename to GpuMemoryBufferFactoryNativePixmap and
GpuMemoryBufferImplNativeBuffer.
> Does it sound good?

Please keep GpuMemoryBufferFactory and GpuMemoryBufferImpl suffix the same.
GpuMemoryBufferImplNativePixmap that uses a NativeBuffer instance internally is
fine. Otherwise, this sgtm if spang is OK with it.

[spang, 2015-07-29 17:19:16]

On 2015/07/29 16:20:14, reveman wrote:
> On 2015/07/29 at 15:52:15, dongseong.hwang wrote:
> > On 2015/07/29 15:32:04, reveman wrote:
> > > On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> > > > On 2015/07/29 14:58:35, reveman wrote:
> > > > > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > > > > On 2015/07/28 23:36:15, spang wrote:
> > > > > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > > > > lgtm
> > > > > > > 
> > > > > > > Actually, thinking about this more and looking at the next CL, I'm
> not
> > > 100%
> > > > > sure
> > > > > > > about naming this "NativePixmapManager".
> > > > > > > 
> > > > > > > I think the clearest path to fit into the existing framework is to
> have
> > > a
> > > > > > > NativePixmapManager that's analogous to IOSurfaceManager, which
> lives in
> > > > > > > content/. It would be involved in transporting the buffer via IPC
> (just
> > > like
> > > > > > > IOSurfaceManager does with its mach transaction).
> > > > > > > 
> > > > > > > But we should not also have a platform object with the same name.
> I'm
> > > not
> > > > > sure
> > > > > > > about alternative, though.
> > > > > > > 
> > > > > > > I think we should hold off just a bit longer on this until the
> following
> > > > > patch
> > > > > > > is in better shape.
> > > > > > 
> > > > > > After rethinking, I feel exposing NativePixmap to renderer is not
> good. So
> > > > > far, NativePixmap is used in only gpu process and all methods are not
> needed
> > > for
> > > > > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > > > > In addition, Map()/Unmap() methods which next CL will add are not
> needed
> > > for
> > > > > gpu process also.
> > > > > > The main purpose of NativePixmap is to abstract the backend of
GLImage
> in
> > > > > Ozone.
> > > > > > I think we need to invent new concept/name for renderer. The name
> should
> > > be
> > > > > related to NativePixmap because the counter part in gpu process is
> > > NativePixmap.
> > > > > > FYI,
GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
> > > creates
> > > > > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > > > > 
> > > > > > My suggestion is as follows;
> > > > > > Introduce "Pixmap" pure abstract class which defines Format and
Usage
> > > enum,
> > > > > which both gpu process and renderer need.
> > > > > > Rename NativePixmap to GLPixmap and only gpu process uses GLPixmap.
> > > GLPixmap
> > > > > inherits Pixmap.
> > > > > > Introduce NativePixmap which renderer use. NativePixmap inherits
> Pixmap
> > > and
> > > > > has Map/Unmap methods.
> > > > > > 
> > > > > > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap should
be
> > > renamed
> > > > > to GpuMemoryBuffer(Factory|Impl)Ozone because GpuMemoryBufferFactory
> uses
> > > > > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap". Alternative
> renaming
> > > is
> > > > > GpuMemoryBufferFACTORYOzoneGLPixmap and
> GpuMemoryBufferIMPLOzoneNativePixmap
> > > but
> > > > > looks inconsistent and burdensome.
> > > > > > 
> > > > > > WDYT?
> > > > > 
> > > > > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of
buffer
> and
> > > not
> > > > > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the correct name
> > > unless
> > > > > this buffer type ends up being called simply "ozone" which is clearly
> not
> > > going
> > > > > to be the case.
> > > > > 
> > > > > Can the class on the GPU process side that is needed to implement the
> > > GLImage
> > > > > functionality be called "Surface"? SurfaceFactoryOzone is creating it
> after
> > > all.
> > > > > NativePixmap is then available for the class name on the renderer
side.
> > > > 
> > > > The main job of SurfaceFactoryOzone is to create SurfaceOzoneEGL, which
is
> > > backend of GLSurface.
> > > > SurfaceFactoryOzone creates NativePixmap as side job, because it can do,
> > > because one of SurfaceOzoneEGL needs NativePixmap.
> > > > So "Surface" name is already occupied.
> > > > 
> > > > If we uses "Pixmap" name in gpu side, we can rename to
> > > GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> > > > gpu side will use "GLPixmap" and renderer will use "NativePixmap". If
you
> > > suggest better name, I'll appreciate!
> > > 
> > > Not a fan of GLPixmap. The GL prefix makes it look like it's assocaited
and
> > > dependent on a GL context. Maybe OzoneNativeBuffer (on service side) and
> > > OzoneNativePixmap (on client side)?
> > 
> > I see. I will use NativeBuffer and NativePixmap. NativePixmap is widely used
> in service side already, so NativePixmap will be used in service side and
> NativeBuffer will be used in client side.
> > To be consistent, I'll rename to GpuMemoryBufferFactoryNativePixmap and
> GpuMemoryBufferImplNativeBuffer.
> > Does it sound good?
> 
> Please keep GpuMemoryBufferFactory and GpuMemoryBufferImpl suffix the same.
> GpuMemoryBufferImplNativePixmap that uses a NativeBuffer instance internally
is
> fine. Otherwise, this sgtm if spang is OK with it.

FWIW I was also thinking about whether it really made sense to reuse the
NativePixmap interface on the renderer side. So I do think it is worth exploring
adding a new type.

I think NativeBuffer vs NativePixmap is not very self documenting, though. What
aboud ZeroCopyPixmap? It's a NativePixmap that has been exported for zero-copy
upload, and also has transporting across processes as a fundamental capability.

It could look something like

class ZeroCopyPixmap {
  virtual bool Map(void **data) = 0;
  virtual void Unmap() = 0;
  virtual void GetStride(int* data) = 0;
}

I was also thinking we should have a ParamTraits<ZeroCopyPixmap> so that
content/ need not worry about how many file descriptors we need to transport, or
how metadata should be handled (like our stride issue).

Having a 2nd type does clarify the factory naming issue a bit:
ZeroCopyPixmapFactory can be the object responsible for constructing them (and
deserializing them from IPC::Message). It doesn't create NativePixmaps - that's
SurfaceFactoryOzone.

[dshwang, 2015-07-29 17:54:56]

On 2015/07/29 17:19:16, spang wrote:
> On 2015/07/29 16:20:14, reveman wrote:
> > On 2015/07/29 at 15:52:15, dongseong.hwang wrote:
> > > On 2015/07/29 15:32:04, reveman wrote:
> > > > On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> > > > > On 2015/07/29 14:58:35, reveman wrote:
> > > > > > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > > > > > On 2015/07/28 23:36:15, spang wrote:
> > > > > > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > > > > > lgtm
> > > > > > > > 
> > > > > > > > Actually, thinking about this more and looking at the next CL,
I'm
> > not
> > > > 100%
> > > > > > sure
> > > > > > > > about naming this "NativePixmapManager".
> > > > > > > > 
> > > > > > > > I think the clearest path to fit into the existing framework is
to
> > have
> > > > a
> > > > > > > > NativePixmapManager that's analogous to IOSurfaceManager, which
> > lives in
> > > > > > > > content/. It would be involved in transporting the buffer via
IPC
> > (just
> > > > like
> > > > > > > > IOSurfaceManager does with its mach transaction).
> > > > > > > > 
> > > > > > > > But we should not also have a platform object with the same
name.
> > I'm
> > > > not
> > > > > > sure
> > > > > > > > about alternative, though.
> > > > > > > > 
> > > > > > > > I think we should hold off just a bit longer on this until the
> > following
> > > > > > patch
> > > > > > > > is in better shape.
> > > > > > > 
> > > > > > > After rethinking, I feel exposing NativePixmap to renderer is not
> > good. So
> > > > > > far, NativePixmap is used in only gpu process and all methods are
not
> > needed
> > > > for
> > > > > > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > > > > > In addition, Map()/Unmap() methods which next CL will add are not
> > needed
> > > > for
> > > > > > gpu process also.
> > > > > > > The main purpose of NativePixmap is to abstract the backend of
> GLImage
> > in
> > > > > > Ozone.
> > > > > > > I think we need to invent new concept/name for renderer. The name
> > should
> > > > be
> > > > > > related to NativePixmap because the counter part in gpu process is
> > > > NativePixmap.
> > > > > > > FYI,
> GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
> > > > creates
> > > > > > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > > > > > 
> > > > > > > My suggestion is as follows;
> > > > > > > Introduce "Pixmap" pure abstract class which defines Format and
> Usage
> > > > enum,
> > > > > > which both gpu process and renderer need.
> > > > > > > Rename NativePixmap to GLPixmap and only gpu process uses
GLPixmap.
> > > > GLPixmap
> > > > > > inherits Pixmap.
> > > > > > > Introduce NativePixmap which renderer use. NativePixmap inherits
> > Pixmap
> > > > and
> > > > > > has Map/Unmap methods.
> > > > > > > 
> > > > > > > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap should
> be
> > > > renamed
> > > > > > to GpuMemoryBuffer(Factory|Impl)Ozone because GpuMemoryBufferFactory
> > uses
> > > > > > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap". Alternative
> > renaming
> > > > is
> > > > > > GpuMemoryBufferFACTORYOzoneGLPixmap and
> > GpuMemoryBufferIMPLOzoneNativePixmap
> > > > but
> > > > > > looks inconsistent and burdensome.
> > > > > > > 
> > > > > > > WDYT?
> > > > > > 
> > > > > > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of
> buffer
> > and
> > > > not
> > > > > > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the correct
name
> > > > unless
> > > > > > this buffer type ends up being called simply "ozone" which is
clearly
> > not
> > > > going
> > > > > > to be the case.
> > > > > > 
> > > > > > Can the class on the GPU process side that is needed to implement
the
> > > > GLImage
> > > > > > functionality be called "Surface"? SurfaceFactoryOzone is creating
it
> > after
> > > > all.
> > > > > > NativePixmap is then available for the class name on the renderer
> side.
> > > > > 
> > > > > The main job of SurfaceFactoryOzone is to create SurfaceOzoneEGL,
which
> is
> > > > backend of GLSurface.
> > > > > SurfaceFactoryOzone creates NativePixmap as side job, because it can
do,
> > > > because one of SurfaceOzoneEGL needs NativePixmap.
> > > > > So "Surface" name is already occupied.
> > > > > 
> > > > > If we uses "Pixmap" name in gpu side, we can rename to
> > > > GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> > > > > gpu side will use "GLPixmap" and renderer will use "NativePixmap". If
> you
> > > > suggest better name, I'll appreciate!
> > > > 
> > > > Not a fan of GLPixmap. The GL prefix makes it look like it's assocaited
> and
> > > > dependent on a GL context. Maybe OzoneNativeBuffer (on service side) and
> > > > OzoneNativePixmap (on client side)?
> > > 
> > > I see. I will use NativeBuffer and NativePixmap. NativePixmap is widely
used
> > in service side already, so NativePixmap will be used in service side and
> > NativeBuffer will be used in client side.
> > > To be consistent, I'll rename to GpuMemoryBufferFactoryNativePixmap and
> > GpuMemoryBufferImplNativeBuffer.
> > > Does it sound good?
> > 
> > Please keep GpuMemoryBufferFactory and GpuMemoryBufferImpl suffix the same.
> > GpuMemoryBufferImplNativePixmap that uses a NativeBuffer instance internally
> is
> > fine. Otherwise, this sgtm if spang is OK with it.
> 
> FWIW I was also thinking about whether it really made sense to reuse the
> NativePixmap interface on the renderer side. So I do think it is worth
exploring
> adding a new type.
> 
> I think NativeBuffer vs NativePixmap is not very self documenting, though.
What
> aboud ZeroCopyPixmap? It's a NativePixmap that has been exported for zero-copy
> upload, and also has transporting across processes as a fundamental
capability.
> 
> It could look something like
> 
> class ZeroCopyPixmap {
>   virtual bool Map(void **data) = 0;
>   virtual void Unmap() = 0;
>   virtual void GetStride(int* data) = 0;
> }
> 
> I was also thinking we should have a ParamTraits<ZeroCopyPixmap> so that
> content/ need not worry about how many file descriptors we need to transport,
or
> how metadata should be handled (like our stride issue).

Nice idea! ZeroCopyPixmap is self-explanatory. server side uses NativePixmap and
client side uses ZeroCopyPixmap.

> Having a 2nd type does clarify the factory naming issue a bit:
> ZeroCopyPixmapFactory can be the object responsible for constructing them (and
> deserializing them from IPC::Message). It doesn't create NativePixmaps -
that's
> SurfaceFactoryOzone.

I think ZeroCopyPixmapFactory is better name than ZeroCopyPixmapManager although
Android and MacOSX use <surface name>Manager. reveman, which do you want?

I don't fully catch up about ParamTraits<ZeroCopyPixmap>. Do you suggest to add
ozone specific member to GpuMemoryBufferHandle, which is ZeroCopyPixmap?
I think it's better to just add |stride_| to GpuMemoryBufferHandle.

[reveman, 2015-07-29 18:13:34]

On 2015/07/29 at 17:54:56, dongseong.hwang wrote:
> On 2015/07/29 17:19:16, spang wrote:
> > On 2015/07/29 16:20:14, reveman wrote:
> > > On 2015/07/29 at 15:52:15, dongseong.hwang wrote:
> > > > On 2015/07/29 15:32:04, reveman wrote:
> > > > > On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> > > > > > On 2015/07/29 14:58:35, reveman wrote:
> > > > > > > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > > > > > > On 2015/07/28 23:36:15, spang wrote:
> > > > > > > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > > > > > > lgtm
> > > > > > > > > 
> > > > > > > > > Actually, thinking about this more and looking at the next CL,
I'm
> > > not
> > > > > 100%
> > > > > > > sure
> > > > > > > > > about naming this "NativePixmapManager".
> > > > > > > > > 
> > > > > > > > > I think the clearest path to fit into the existing framework
is to
> > > have
> > > > > a
> > > > > > > > > NativePixmapManager that's analogous to IOSurfaceManager,
which
> > > lives in
> > > > > > > > > content/. It would be involved in transporting the buffer via
IPC
> > > (just
> > > > > like
> > > > > > > > > IOSurfaceManager does with its mach transaction).
> > > > > > > > > 
> > > > > > > > > But we should not also have a platform object with the same
name.
> > > I'm
> > > > > not
> > > > > > > sure
> > > > > > > > > about alternative, though.
> > > > > > > > > 
> > > > > > > > > I think we should hold off just a bit longer on this until the
> > > following
> > > > > > > patch
> > > > > > > > > is in better shape.
> > > > > > > > 
> > > > > > > > After rethinking, I feel exposing NativePixmap to renderer is
not
> > > good. So
> > > > > > > far, NativePixmap is used in only gpu process and all methods are
not
> > > needed
> > > > > for
> > > > > > > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > > > > > > In addition, Map()/Unmap() methods which next CL will add are
not
> > > needed
> > > > > for
> > > > > > > gpu process also.
> > > > > > > > The main purpose of NativePixmap is to abstract the backend of
> > GLImage
> > > in
> > > > > > > Ozone.
> > > > > > > > I think we need to invent new concept/name for renderer. The
name
> > > should
> > > > > be
> > > > > > > related to NativePixmap because the counter part in gpu process is
> > > > > NativePixmap.
> > > > > > > > FYI,
> > GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
> > > > > creates
> > > > > > > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > > > > > > 
> > > > > > > > My suggestion is as follows;
> > > > > > > > Introduce "Pixmap" pure abstract class which defines Format and
> > Usage
> > > > > enum,
> > > > > > > which both gpu process and renderer need.
> > > > > > > > Rename NativePixmap to GLPixmap and only gpu process uses
GLPixmap.
> > > > > GLPixmap
> > > > > > > inherits Pixmap.
> > > > > > > > Introduce NativePixmap which renderer use. NativePixmap inherits
> > > Pixmap
> > > > > and
> > > > > > > has Map/Unmap methods.
> > > > > > > > 
> > > > > > > > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap
should
> > be
> > > > > renamed
> > > > > > > to GpuMemoryBuffer(Factory|Impl)Ozone because
GpuMemoryBufferFactory
> > > uses
> > > > > > > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap".
Alternative
> > > renaming
> > > > > is
> > > > > > > GpuMemoryBufferFACTORYOzoneGLPixmap and
> > > GpuMemoryBufferIMPLOzoneNativePixmap
> > > > > but
> > > > > > > looks inconsistent and burdensome.
> > > > > > > > 
> > > > > > > > WDYT?
> > > > > > > 
> > > > > > > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of
> > buffer
> > > and
> > > > > not
> > > > > > > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the correct
name
> > > > > unless
> > > > > > > this buffer type ends up being called simply "ozone" which is
clearly
> > > not
> > > > > going
> > > > > > > to be the case.
> > > > > > > 
> > > > > > > Can the class on the GPU process side that is needed to implement
the
> > > > > GLImage
> > > > > > > functionality be called "Surface"? SurfaceFactoryOzone is creating
it
> > > after
> > > > > all.
> > > > > > > NativePixmap is then available for the class name on the renderer
> > side.
> > > > > > 
> > > > > > The main job of SurfaceFactoryOzone is to create SurfaceOzoneEGL,
which
> > is
> > > > > backend of GLSurface.
> > > > > > SurfaceFactoryOzone creates NativePixmap as side job, because it can
do,
> > > > > because one of SurfaceOzoneEGL needs NativePixmap.
> > > > > > So "Surface" name is already occupied.
> > > > > > 
> > > > > > If we uses "Pixmap" name in gpu side, we can rename to
> > > > > GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> > > > > > gpu side will use "GLPixmap" and renderer will use "NativePixmap".
If
> > you
> > > > > suggest better name, I'll appreciate!
> > > > > 
> > > > > Not a fan of GLPixmap. The GL prefix makes it look like it's
assocaited
> > and
> > > > > dependent on a GL context. Maybe OzoneNativeBuffer (on service side)
and
> > > > > OzoneNativePixmap (on client side)?
> > > > 
> > > > I see. I will use NativeBuffer and NativePixmap. NativePixmap is widely
used
> > > in service side already, so NativePixmap will be used in service side and
> > > NativeBuffer will be used in client side.
> > > > To be consistent, I'll rename to GpuMemoryBufferFactoryNativePixmap and
> > > GpuMemoryBufferImplNativeBuffer.
> > > > Does it sound good?
> > > 
> > > Please keep GpuMemoryBufferFactory and GpuMemoryBufferImpl suffix the
same.
> > > GpuMemoryBufferImplNativePixmap that uses a NativeBuffer instance
internally
> > is
> > > fine. Otherwise, this sgtm if spang is OK with it.
> > 
> > FWIW I was also thinking about whether it really made sense to reuse the
> > NativePixmap interface on the renderer side. So I do think it is worth
exploring
> > adding a new type.
> > 
> > I think NativeBuffer vs NativePixmap is not very self documenting, though.
What
> > aboud ZeroCopyPixmap? It's a NativePixmap that has been exported for
zero-copy
> > upload, and also has transporting across processes as a fundamental
capability.
> > 
> > It could look something like
> > 
> > class ZeroCopyPixmap {
> >   virtual bool Map(void **data) = 0;
> >   virtual void Unmap() = 0;
> >   virtual void GetStride(int* data) = 0;
> > }
> > 
> > I was also thinking we should have a ParamTraits<ZeroCopyPixmap> so that
> > content/ need not worry about how many file descriptors we need to
transport, or
> > how metadata should be handled (like our stride issue).
> 
> Nice idea! ZeroCopyPixmap is self-explanatory. server side uses NativePixmap
and client side uses ZeroCopyPixmap.
> 
> > Having a 2nd type does clarify the factory naming issue a bit:
> > ZeroCopyPixmapFactory can be the object responsible for constructing them
(and
> > deserializing them from IPC::Message). It doesn't create NativePixmaps -
that's
> > SurfaceFactoryOzone.
> 
> I think ZeroCopyPixmapFactory is better name than ZeroCopyPixmapManager
although Android and MacOSX use <surface name>Manager. reveman, which do you
want?
> 
> I don't fully catch up about ParamTraits<ZeroCopyPixmap>. Do you suggest to
add ozone specific member to GpuMemoryBufferHandle, which is ZeroCopyPixmap?
> I think it's better to just add |stride_| to GpuMemoryBufferHandle.

I think ZeroCopyPixmap will be confused with zero-copy concept in the
compositor. e.g. we would be using zero-copy pixmaps to perform one-copy tile
texture initialization. One-copy and zero-copy terms are currently used to
describe the usage of these buffers so using it as a name of the buffers too is
not the best imo.

Can we just add "client" to the name? NativePixmap/NativeClientPixmap

[spang, 2015-07-29 18:16:37]

On 2015/07/29 18:13:34, reveman wrote:
> On 2015/07/29 at 17:54:56, dongseong.hwang wrote:
> > On 2015/07/29 17:19:16, spang wrote:
> > > On 2015/07/29 16:20:14, reveman wrote:
> > > > On 2015/07/29 at 15:52:15, dongseong.hwang wrote:
> > > > > On 2015/07/29 15:32:04, reveman wrote:
> > > > > > On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> > > > > > > On 2015/07/29 14:58:35, reveman wrote:
> > > > > > > > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > > > > > > > On 2015/07/28 23:36:15, spang wrote:
> > > > > > > > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > > > > > > > lgtm
> > > > > > > > > > 
> > > > > > > > > > Actually, thinking about this more and looking at the next
CL,
> I'm
> > > > not
> > > > > > 100%
> > > > > > > > sure
> > > > > > > > > > about naming this "NativePixmapManager".
> > > > > > > > > > 
> > > > > > > > > > I think the clearest path to fit into the existing framework
> is to
> > > > have
> > > > > > a
> > > > > > > > > > NativePixmapManager that's analogous to IOSurfaceManager,
> which
> > > > lives in
> > > > > > > > > > content/. It would be involved in transporting the buffer
via
> IPC
> > > > (just
> > > > > > like
> > > > > > > > > > IOSurfaceManager does with its mach transaction).
> > > > > > > > > > 
> > > > > > > > > > But we should not also have a platform object with the same
> name.
> > > > I'm
> > > > > > not
> > > > > > > > sure
> > > > > > > > > > about alternative, though.
> > > > > > > > > > 
> > > > > > > > > > I think we should hold off just a bit longer on this until
the
> > > > following
> > > > > > > > patch
> > > > > > > > > > is in better shape.
> > > > > > > > > 
> > > > > > > > > After rethinking, I feel exposing NativePixmap to renderer is
> not
> > > > good. So
> > > > > > > > far, NativePixmap is used in only gpu process and all methods
are
> not
> > > > needed
> > > > > > for
> > > > > > > > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > > > > > > > In addition, Map()/Unmap() methods which next CL will add are
> not
> > > > needed
> > > > > > for
> > > > > > > > gpu process also.
> > > > > > > > > The main purpose of NativePixmap is to abstract the backend of
> > > GLImage
> > > > in
> > > > > > > > Ozone.
> > > > > > > > > I think we need to invent new concept/name for renderer. The
> name
> > > > should
> > > > > > be
> > > > > > > > related to NativePixmap because the counter part in gpu process
is
> > > > > > NativePixmap.
> > > > > > > > > FYI,
> > > GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
> > > > > > creates
> > > > > > > > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > > > > > > > 
> > > > > > > > > My suggestion is as follows;
> > > > > > > > > Introduce "Pixmap" pure abstract class which defines Format
and
> > > Usage
> > > > > > enum,
> > > > > > > > which both gpu process and renderer need.
> > > > > > > > > Rename NativePixmap to GLPixmap and only gpu process uses
> GLPixmap.
> > > > > > GLPixmap
> > > > > > > > inherits Pixmap.
> > > > > > > > > Introduce NativePixmap which renderer use. NativePixmap
inherits
> > > > Pixmap
> > > > > > and
> > > > > > > > has Map/Unmap methods.
> > > > > > > > > 
> > > > > > > > > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap
> should
> > > be
> > > > > > renamed
> > > > > > > > to GpuMemoryBuffer(Factory|Impl)Ozone because
> GpuMemoryBufferFactory
> > > > uses
> > > > > > > > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap".
> Alternative
> > > > renaming
> > > > > > is
> > > > > > > > GpuMemoryBufferFACTORYOzoneGLPixmap and
> > > > GpuMemoryBufferIMPLOzoneNativePixmap
> > > > > > but
> > > > > > > > looks inconsistent and burdensome.
> > > > > > > > > 
> > > > > > > > > WDYT?
> > > > > > > > 
> > > > > > > > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type of
> > > buffer
> > > > and
> > > > > > not
> > > > > > > > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the
correct
> name
> > > > > > unless
> > > > > > > > this buffer type ends up being called simply "ozone" which is
> clearly
> > > > not
> > > > > > going
> > > > > > > > to be the case.
> > > > > > > > 
> > > > > > > > Can the class on the GPU process side that is needed to
implement
> the
> > > > > > GLImage
> > > > > > > > functionality be called "Surface"? SurfaceFactoryOzone is
creating
> it
> > > > after
> > > > > > all.
> > > > > > > > NativePixmap is then available for the class name on the
renderer
> > > side.
> > > > > > > 
> > > > > > > The main job of SurfaceFactoryOzone is to create SurfaceOzoneEGL,
> which
> > > is
> > > > > > backend of GLSurface.
> > > > > > > SurfaceFactoryOzone creates NativePixmap as side job, because it
can
> do,
> > > > > > because one of SurfaceOzoneEGL needs NativePixmap.
> > > > > > > So "Surface" name is already occupied.
> > > > > > > 
> > > > > > > If we uses "Pixmap" name in gpu side, we can rename to
> > > > > > GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> > > > > > > gpu side will use "GLPixmap" and renderer will use "NativePixmap".
> If
> > > you
> > > > > > suggest better name, I'll appreciate!
> > > > > > 
> > > > > > Not a fan of GLPixmap. The GL prefix makes it look like it's
> assocaited
> > > and
> > > > > > dependent on a GL context. Maybe OzoneNativeBuffer (on service side)
> and
> > > > > > OzoneNativePixmap (on client side)?
> > > > > 
> > > > > I see. I will use NativeBuffer and NativePixmap. NativePixmap is
widely
> used
> > > > in service side already, so NativePixmap will be used in service side
and
> > > > NativeBuffer will be used in client side.
> > > > > To be consistent, I'll rename to GpuMemoryBufferFactoryNativePixmap
and
> > > > GpuMemoryBufferImplNativeBuffer.
> > > > > Does it sound good?
> > > > 
> > > > Please keep GpuMemoryBufferFactory and GpuMemoryBufferImpl suffix the
> same.
> > > > GpuMemoryBufferImplNativePixmap that uses a NativeBuffer instance
> internally
> > > is
> > > > fine. Otherwise, this sgtm if spang is OK with it.
> > > 
> > > FWIW I was also thinking about whether it really made sense to reuse the
> > > NativePixmap interface on the renderer side. So I do think it is worth
> exploring
> > > adding a new type.
> > > 
> > > I think NativeBuffer vs NativePixmap is not very self documenting, though.
> What
> > > aboud ZeroCopyPixmap? It's a NativePixmap that has been exported for
> zero-copy
> > > upload, and also has transporting across processes as a fundamental
> capability.
> > > 
> > > It could look something like
> > > 
> > > class ZeroCopyPixmap {
> > >   virtual bool Map(void **data) = 0;
> > >   virtual void Unmap() = 0;
> > >   virtual void GetStride(int* data) = 0;
> > > }
> > > 
> > > I was also thinking we should have a ParamTraits<ZeroCopyPixmap> so that
> > > content/ need not worry about how many file descriptors we need to
> transport, or
> > > how metadata should be handled (like our stride issue).
> > 
> > Nice idea! ZeroCopyPixmap is self-explanatory. server side uses NativePixmap
> and client side uses ZeroCopyPixmap.
> > 
> > > Having a 2nd type does clarify the factory naming issue a bit:
> > > ZeroCopyPixmapFactory can be the object responsible for constructing them
> (and
> > > deserializing them from IPC::Message). It doesn't create NativePixmaps -
> that's
> > > SurfaceFactoryOzone.
> > 
> > I think ZeroCopyPixmapFactory is better name than ZeroCopyPixmapManager
> although Android and MacOSX use <surface name>Manager. reveman, which do you
> want?
> > 
> > I don't fully catch up about ParamTraits<ZeroCopyPixmap>. Do you suggest to
> add ozone specific member to GpuMemoryBufferHandle, which is ZeroCopyPixmap?
> > I think it's better to just add |stride_| to GpuMemoryBufferHandle.
> 
> I think ZeroCopyPixmap will be confused with zero-copy concept in the
> compositor. e.g. we would be using zero-copy pixmaps to perform one-copy tile
> texture initialization. One-copy and zero-copy terms are currently used to
> describe the usage of these buffers so using it as a name of the buffers too
is
> not the best imo.
> 
> Can we just add "client" to the name? NativePixmap/NativeClientPixmap

That's fine with me, although I'd guess that was something related to NaCL. We
could also permute the words to get "ClientNativePixmap" or
"NativePixmapClient".

[reveman, 2015-07-29 18:35:08]

On 2015/07/29 at 18:16:37, spang wrote:
> On 2015/07/29 18:13:34, reveman wrote:
> > On 2015/07/29 at 17:54:56, dongseong.hwang wrote:
> > > On 2015/07/29 17:19:16, spang wrote:
> > > > On 2015/07/29 16:20:14, reveman wrote:
> > > > > On 2015/07/29 at 15:52:15, dongseong.hwang wrote:
> > > > > > On 2015/07/29 15:32:04, reveman wrote:
> > > > > > > On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> > > > > > > > On 2015/07/29 14:58:35, reveman wrote:
> > > > > > > > > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > > > > > > > > On 2015/07/28 23:36:15, spang wrote:
> > > > > > > > > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > > > > > > > > lgtm
> > > > > > > > > > > 
> > > > > > > > > > > Actually, thinking about this more and looking at the next
CL,
> > I'm
> > > > > not
> > > > > > > 100%
> > > > > > > > > sure
> > > > > > > > > > > about naming this "NativePixmapManager".
> > > > > > > > > > > 
> > > > > > > > > > > I think the clearest path to fit into the existing
framework
> > is to
> > > > > have
> > > > > > > a
> > > > > > > > > > > NativePixmapManager that's analogous to IOSurfaceManager,
> > which
> > > > > lives in
> > > > > > > > > > > content/. It would be involved in transporting the buffer
via
> > IPC
> > > > > (just
> > > > > > > like
> > > > > > > > > > > IOSurfaceManager does with its mach transaction).
> > > > > > > > > > > 
> > > > > > > > > > > But we should not also have a platform object with the
same
> > name.
> > > > > I'm
> > > > > > > not
> > > > > > > > > sure
> > > > > > > > > > > about alternative, though.
> > > > > > > > > > > 
> > > > > > > > > > > I think we should hold off just a bit longer on this until
the
> > > > > following
> > > > > > > > > patch
> > > > > > > > > > > is in better shape.
> > > > > > > > > > 
> > > > > > > > > > After rethinking, I feel exposing NativePixmap to renderer
is
> > not
> > > > > good. So
> > > > > > > > > far, NativePixmap is used in only gpu process and all methods
are
> > not
> > > > > needed
> > > > > > > for
> > > > > > > > > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > > > > > > > > In addition, Map()/Unmap() methods which next CL will add
are
> > not
> > > > > needed
> > > > > > > for
> > > > > > > > > gpu process also.
> > > > > > > > > > The main purpose of NativePixmap is to abstract the backend
of
> > > > GLImage
> > > > > in
> > > > > > > > > Ozone.
> > > > > > > > > > I think we need to invent new concept/name for renderer. The
> > name
> > > > > should
> > > > > > > be
> > > > > > > > > related to NativePixmap because the counter part in gpu
process is
> > > > > > > NativePixmap.
> > > > > > > > > > FYI,
> > > > GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
> > > > > > > creates
> > > > > > > > > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > > > > > > > > 
> > > > > > > > > > My suggestion is as follows;
> > > > > > > > > > Introduce "Pixmap" pure abstract class which defines Format
and
> > > > Usage
> > > > > > > enum,
> > > > > > > > > which both gpu process and renderer need.
> > > > > > > > > > Rename NativePixmap to GLPixmap and only gpu process uses
> > GLPixmap.
> > > > > > > GLPixmap
> > > > > > > > > inherits Pixmap.
> > > > > > > > > > Introduce NativePixmap which renderer use. NativePixmap
inherits
> > > > > Pixmap
> > > > > > > and
> > > > > > > > > has Map/Unmap methods.
> > > > > > > > > > 
> > > > > > > > > > In addition, GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap
> > should
> > > > be
> > > > > > > renamed
> > > > > > > > > to GpuMemoryBuffer(Factory|Impl)Ozone because
> > GpuMemoryBufferFactory
> > > > > uses
> > > > > > > > > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap".
> > Alternative
> > > > > renaming
> > > > > > > is
> > > > > > > > > GpuMemoryBufferFACTORYOzoneGLPixmap and
> > > > > GpuMemoryBufferIMPLOzoneNativePixmap
> > > > > > > but
> > > > > > > > > looks inconsistent and burdensome.
> > > > > > > > > > 
> > > > > > > > > > WDYT?
> > > > > > > > > 
> > > > > > > > > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the type
of
> > > > buffer
> > > > > and
> > > > > > > not
> > > > > > > > > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the
correct
> > name
> > > > > > > unless
> > > > > > > > > this buffer type ends up being called simply "ozone" which is
> > clearly
> > > > > not
> > > > > > > going
> > > > > > > > > to be the case.
> > > > > > > > > 
> > > > > > > > > Can the class on the GPU process side that is needed to
implement
> > the
> > > > > > > GLImage
> > > > > > > > > functionality be called "Surface"? SurfaceFactoryOzone is
creating
> > it
> > > > > after
> > > > > > > all.
> > > > > > > > > NativePixmap is then available for the class name on the
renderer
> > > > side.
> > > > > > > > 
> > > > > > > > The main job of SurfaceFactoryOzone is to create
SurfaceOzoneEGL,
> > which
> > > > is
> > > > > > > backend of GLSurface.
> > > > > > > > SurfaceFactoryOzone creates NativePixmap as side job, because it
can
> > do,
> > > > > > > because one of SurfaceOzoneEGL needs NativePixmap.
> > > > > > > > So "Surface" name is already occupied.
> > > > > > > > 
> > > > > > > > If we uses "Pixmap" name in gpu side, we can rename to
> > > > > > > GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> > > > > > > > gpu side will use "GLPixmap" and renderer will use
"NativePixmap".
> > If
> > > > you
> > > > > > > suggest better name, I'll appreciate!
> > > > > > > 
> > > > > > > Not a fan of GLPixmap. The GL prefix makes it look like it's
> > assocaited
> > > > and
> > > > > > > dependent on a GL context. Maybe OzoneNativeBuffer (on service
side)
> > and
> > > > > > > OzoneNativePixmap (on client side)?
> > > > > > 
> > > > > > I see. I will use NativeBuffer and NativePixmap. NativePixmap is
widely
> > used
> > > > > in service side already, so NativePixmap will be used in service side
and
> > > > > NativeBuffer will be used in client side.
> > > > > > To be consistent, I'll rename to GpuMemoryBufferFactoryNativePixmap
and
> > > > > GpuMemoryBufferImplNativeBuffer.
> > > > > > Does it sound good?
> > > > > 
> > > > > Please keep GpuMemoryBufferFactory and GpuMemoryBufferImpl suffix the
> > same.
> > > > > GpuMemoryBufferImplNativePixmap that uses a NativeBuffer instance
> > internally
> > > > is
> > > > > fine. Otherwise, this sgtm if spang is OK with it.
> > > > 
> > > > FWIW I was also thinking about whether it really made sense to reuse the
> > > > NativePixmap interface on the renderer side. So I do think it is worth
> > exploring
> > > > adding a new type.
> > > > 
> > > > I think NativeBuffer vs NativePixmap is not very self documenting,
though.
> > What
> > > > aboud ZeroCopyPixmap? It's a NativePixmap that has been exported for
> > zero-copy
> > > > upload, and also has transporting across processes as a fundamental
> > capability.
> > > > 
> > > > It could look something like
> > > > 
> > > > class ZeroCopyPixmap {
> > > >   virtual bool Map(void **data) = 0;
> > > >   virtual void Unmap() = 0;
> > > >   virtual void GetStride(int* data) = 0;
> > > > }
> > > > 
> > > > I was also thinking we should have a ParamTraits<ZeroCopyPixmap> so that
> > > > content/ need not worry about how many file descriptors we need to
> > transport, or
> > > > how metadata should be handled (like our stride issue).
> > > 
> > > Nice idea! ZeroCopyPixmap is self-explanatory. server side uses
NativePixmap
> > and client side uses ZeroCopyPixmap.
> > > 
> > > > Having a 2nd type does clarify the factory naming issue a bit:
> > > > ZeroCopyPixmapFactory can be the object responsible for constructing
them
> > (and
> > > > deserializing them from IPC::Message). It doesn't create NativePixmaps -
> > that's
> > > > SurfaceFactoryOzone.
> > > 
> > > I think ZeroCopyPixmapFactory is better name than ZeroCopyPixmapManager
> > although Android and MacOSX use <surface name>Manager. reveman, which do you
> > want?
> > > 
> > > I don't fully catch up about ParamTraits<ZeroCopyPixmap>. Do you suggest
to
> > add ozone specific member to GpuMemoryBufferHandle, which is ZeroCopyPixmap?
> > > I think it's better to just add |stride_| to GpuMemoryBufferHandle.
> > 
> > I think ZeroCopyPixmap will be confused with zero-copy concept in the
> > compositor. e.g. we would be using zero-copy pixmaps to perform one-copy
tile
> > texture initialization. One-copy and zero-copy terms are currently used to
> > describe the usage of these buffers so using it as a name of the buffers too
is
> > not the best imo.
> > 
> > Can we just add "client" to the name? NativePixmap/NativeClientPixmap
> 
> That's fine with me, although I'd guess that was something related to NaCL. We
could also permute the words to get "ClientNativePixmap" or
"NativePixmapClient".

ClientNativePixmap sgtm. NativePixmapClient can be confused with a user of
NativePixmaps. e.g. the TileManager/TileManagerClient use case.

[dshwang, 2015-07-30 11:06:11]

On 2015/07/29 18:35:08, reveman wrote:
> On 2015/07/29 at 18:16:37, spang wrote:
> > On 2015/07/29 18:13:34, reveman wrote:
> > > On 2015/07/29 at 17:54:56, dongseong.hwang wrote:
> > > > On 2015/07/29 17:19:16, spang wrote:
> > > > > On 2015/07/29 16:20:14, reveman wrote:
> > > > > > On 2015/07/29 at 15:52:15, dongseong.hwang wrote:
> > > > > > > On 2015/07/29 15:32:04, reveman wrote:
> > > > > > > > On 2015/07/29 at 15:26:17, dongseong.hwang wrote:
> > > > > > > > > On 2015/07/29 14:58:35, reveman wrote:
> > > > > > > > > > On 2015/07/29 at 14:48:42, dongseong.hwang wrote:
> > > > > > > > > > > On 2015/07/28 23:36:15, spang wrote:
> > > > > > > > > > > > On 2015/07/28 18:17:34, spang wrote:
> > > > > > > > > > > > > lgtm
> > > > > > > > > > > > 
> > > > > > > > > > > > Actually, thinking about this more and looking at the
next
> CL,
> > > I'm
> > > > > > not
> > > > > > > > 100%
> > > > > > > > > > sure
> > > > > > > > > > > > about naming this "NativePixmapManager".
> > > > > > > > > > > > 
> > > > > > > > > > > > I think the clearest path to fit into the existing
> framework
> > > is to
> > > > > > have
> > > > > > > > a
> > > > > > > > > > > > NativePixmapManager that's analogous to
IOSurfaceManager,
> > > which
> > > > > > lives in
> > > > > > > > > > > > content/. It would be involved in transporting the
buffer
> via
> > > IPC
> > > > > > (just
> > > > > > > > like
> > > > > > > > > > > > IOSurfaceManager does with its mach transaction).
> > > > > > > > > > > > 
> > > > > > > > > > > > But we should not also have a platform object with the
> same
> > > name.
> > > > > > I'm
> > > > > > > > not
> > > > > > > > > > sure
> > > > > > > > > > > > about alternative, though.
> > > > > > > > > > > > 
> > > > > > > > > > > > I think we should hold off just a bit longer on this
until
> the
> > > > > > following
> > > > > > > > > > patch
> > > > > > > > > > > > is in better shape.
> > > > > > > > > > > 
> > > > > > > > > > > After rethinking, I feel exposing NativePixmap to renderer
> is
> > > not
> > > > > > good. So
> > > > > > > > > > far, NativePixmap is used in only gpu process and all
methods
> are
> > > not
> > > > > > needed
> > > > > > > > for
> > > > > > > > > > renderer. e.g. ScheduleOverlayPlane(), GetScaledPixmap().
> > > > > > > > > > > In addition, Map()/Unmap() methods which next CL will add
> are
> > > not
> > > > > > needed
> > > > > > > > for
> > > > > > > > > > gpu process also.
> > > > > > > > > > > The main purpose of NativePixmap is to abstract the
backend
> of
> > > > > GLImage
> > > > > > in
> > > > > > > > > > Ozone.
> > > > > > > > > > > I think we need to invent new concept/name for renderer.
The
> > > name
> > > > > > should
> > > > > > > > be
> > > > > > > > > > related to NativePixmap because the counter part in gpu
> process is
> > > > > > > > NativePixmap.
> > > > > > > > > > > FYI,
> > > > > GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer()
> > > > > > > > creates
> > > > > > > > > > GpuMemoryBufferHandle from NativePixmap in gpu process.
> > > > > > > > > > > 
> > > > > > > > > > > My suggestion is as follows;
> > > > > > > > > > > Introduce "Pixmap" pure abstract class which defines
Format
> and
> > > > > Usage
> > > > > > > > enum,
> > > > > > > > > > which both gpu process and renderer need.
> > > > > > > > > > > Rename NativePixmap to GLPixmap and only gpu process uses
> > > GLPixmap.
> > > > > > > > GLPixmap
> > > > > > > > > > inherits Pixmap.
> > > > > > > > > > > Introduce NativePixmap which renderer use. NativePixmap
> inherits
> > > > > > Pixmap
> > > > > > > > and
> > > > > > > > > > has Map/Unmap methods.
> > > > > > > > > > > 
> > > > > > > > > > > In addition,
GpuMemoryBuffer(Factory|Impl)OzoneNativePixmap
> > > should
> > > > > be
> > > > > > > > renamed
> > > > > > > > > > to GpuMemoryBuffer(Factory|Impl)Ozone because
> > > GpuMemoryBufferFactory
> > > > > > uses
> > > > > > > > > > "GLPixmap" and GpuMemoryBufferImpl uses "NativePixmap".
> > > Alternative
> > > > > > renaming
> > > > > > > > is
> > > > > > > > > > GpuMemoryBufferFACTORYOzoneGLPixmap and
> > > > > > GpuMemoryBufferIMPLOzoneNativePixmap
> > > > > > > > but
> > > > > > > > > > looks inconsistent and burdensome.
> > > > > > > > > > > 
> > > > > > > > > > > WDYT?
> > > > > > > > > > 
> > > > > > > > > > In GpuMemoryBuffer(Factory|Impl)NAME, NAME should be the
type
> of
> > > > > buffer
> > > > > > and
> > > > > > > > not
> > > > > > > > > > a platform. GpuMemoryBuffer(Factory|Impl)Ozone is not the
> correct
> > > name
> > > > > > > > unless
> > > > > > > > > > this buffer type ends up being called simply "ozone" which
is
> > > clearly
> > > > > > not
> > > > > > > > going
> > > > > > > > > > to be the case.
> > > > > > > > > > 
> > > > > > > > > > Can the class on the GPU process side that is needed to
> implement
> > > the
> > > > > > > > GLImage
> > > > > > > > > > functionality be called "Surface"? SurfaceFactoryOzone is
> creating
> > > it
> > > > > > after
> > > > > > > > all.
> > > > > > > > > > NativePixmap is then available for the class name on the
> renderer
> > > > > side.
> > > > > > > > > 
> > > > > > > > > The main job of SurfaceFactoryOzone is to create
> SurfaceOzoneEGL,
> > > which
> > > > > is
> > > > > > > > backend of GLSurface.
> > > > > > > > > SurfaceFactoryOzone creates NativePixmap as side job, because
it
> can
> > > do,
> > > > > > > > because one of SurfaceOzoneEGL needs NativePixmap.
> > > > > > > > > So "Surface" name is already occupied.
> > > > > > > > > 
> > > > > > > > > If we uses "Pixmap" name in gpu side, we can rename to
> > > > > > > > GpuMemoryBuffer(Factory|Impl)OzonePixmap.
> > > > > > > > > gpu side will use "GLPixmap" and renderer will use
> "NativePixmap".
> > > If
> > > > > you
> > > > > > > > suggest better name, I'll appreciate!
> > > > > > > > 
> > > > > > > > Not a fan of GLPixmap. The GL prefix makes it look like it's
> > > assocaited
> > > > > and
> > > > > > > > dependent on a GL context. Maybe OzoneNativeBuffer (on service
> side)
> > > and
> > > > > > > > OzoneNativePixmap (on client side)?
> > > > > > > 
> > > > > > > I see. I will use NativeBuffer and NativePixmap. NativePixmap is
> widely
> > > used
> > > > > > in service side already, so NativePixmap will be used in service
side
> and
> > > > > > NativeBuffer will be used in client side.
> > > > > > > To be consistent, I'll rename to
GpuMemoryBufferFactoryNativePixmap
> and
> > > > > > GpuMemoryBufferImplNativeBuffer.
> > > > > > > Does it sound good?
> > > > > > 
> > > > > > Please keep GpuMemoryBufferFactory and GpuMemoryBufferImpl suffix
the
> > > same.
> > > > > > GpuMemoryBufferImplNativePixmap that uses a NativeBuffer instance
> > > internally
> > > > > is
> > > > > > fine. Otherwise, this sgtm if spang is OK with it.
> > > > > 
> > > > > FWIW I was also thinking about whether it really made sense to reuse
the
> > > > > NativePixmap interface on the renderer side. So I do think it is worth
> > > exploring
> > > > > adding a new type.
> > > > > 
> > > > > I think NativeBuffer vs NativePixmap is not very self documenting,
> though.
> > > What
> > > > > aboud ZeroCopyPixmap? It's a NativePixmap that has been exported for
> > > zero-copy
> > > > > upload, and also has transporting across processes as a fundamental
> > > capability.
> > > > > 
> > > > > It could look something like
> > > > > 
> > > > > class ZeroCopyPixmap {
> > > > >   virtual bool Map(void **data) = 0;
> > > > >   virtual void Unmap() = 0;
> > > > >   virtual void GetStride(int* data) = 0;
> > > > > }
> > > > > 
> > > > > I was also thinking we should have a ParamTraits<ZeroCopyPixmap> so
that
> > > > > content/ need not worry about how many file descriptors we need to
> > > transport, or
> > > > > how metadata should be handled (like our stride issue).
> > > > 
> > > > Nice idea! ZeroCopyPixmap is self-explanatory. server side uses
> NativePixmap
> > > and client side uses ZeroCopyPixmap.
> > > > 
> > > > > Having a 2nd type does clarify the factory naming issue a bit:
> > > > > ZeroCopyPixmapFactory can be the object responsible for constructing
> them
> > > (and
> > > > > deserializing them from IPC::Message). It doesn't create NativePixmaps
-
> > > that's
> > > > > SurfaceFactoryOzone.
> > > > 
> > > > I think ZeroCopyPixmapFactory is better name than ZeroCopyPixmapManager
> > > although Android and MacOSX use <surface name>Manager. reveman, which do
you
> > > want?
> > > > 
> > > > I don't fully catch up about ParamTraits<ZeroCopyPixmap>. Do you suggest
> to
> > > add ozone specific member to GpuMemoryBufferHandle, which is
ZeroCopyPixmap?
> > > > I think it's better to just add |stride_| to GpuMemoryBufferHandle.
> > > 
> > > I think ZeroCopyPixmap will be confused with zero-copy concept in the
> > > compositor. e.g. we would be using zero-copy pixmaps to perform one-copy
> tile
> > > texture initialization. One-copy and zero-copy terms are currently used to
> > > describe the usage of these buffers so using it as a name of the buffers
too
> is
> > > not the best imo.
> > > 
> > > Can we just add "client" to the name? NativePixmap/NativeClientPixmap
> > 
> > That's fine with me, although I'd guess that was something related to NaCL.
We
> could also permute the words to get "ClientNativePixmap" or
> "NativePixmapClient".
> 
> ClientNativePixmap sgtm. NativePixmapClient can be confused with a user of
> NativePixmaps. e.g. the TileManager/TileManagerClient use case.

ClientNativePixmap sgtm, but a bit long. how about ClientPixmap and
ClientPixmapFactory?

[dshwang, 2015-07-30 15:11:21]

Patchset #17 (id:470001) has been deleted

[dshwang, 2015-07-30 15:13:21]

Patchset #17 (id:490001) has been deleted

[dshwang, 2015-07-30 15:21:12]

Hi, I introduce ClientPixmap for non-gpu process.
class OZONE_EXPORT ClientPixmap {
 public:
  virtual void* Map() = 0;
  virtual void Unmap() = 0;
  virtual int GetStride() const = 0;
};

ClientPixmapManager (previously NativePixmapManager) creates ClientPixmap for
non-gpu process.

To show how this API is actually used, I update following CLs already.
https://codereview.chromium.org/1248713002/ ozone: ClientPixmapManager passes
VGEM fd from browser to renderer.
https://codereview.chromium.org/1134993003/ ozone: Implement zero/one-copy
texture for Ozone GBM.

Could you review again?

[reveman, 2015-07-30 16:14:11]

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
File ui/ozone/public/client_pixmap.h (right):

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
ui/ozone/public/client_pixmap.h:13: // in non-GPU processes, but can also be
read by the GPU.
"non-GPU processes", do we need to be this specific here? CPU access to native
pixmaps in the GPU process will mostly like use this class too.

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
ui/ozone/public/client_pixmap.h:15: class OZONE_EXPORT ClientPixmap {
Why the "Native" prefix in one case but not the other? Can you make it
consistent? Pixmap+ClientPixmap or NativePixmap+ClientNativePixmap

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/pixmap...
File ui/ozone/public/pixmap_types.h (right):

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/pixmap...
ui/ozone/public/pixmap_types.h:13: PIXMAP_FORMAT_UNKNOWN,
Realize this is not new but it would be nice to avoid invalid enum values if
possible. Can we remove this?

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/pixmap...
ui/ozone/public/pixmap_types.h:16: PIXMAP_FORMAT_LAST = PIXMAP_FORMAT_RGBX_8888
Is PIXMAP_FORMAT_LAST used for IPC? If not, can we remove it?

[dshwang, 2015-07-30 16:50:15]

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
File ui/ozone/public/client_pixmap.h (right):

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
ui/ozone/public/client_pixmap.h:13: // in non-GPU processes, but can also be
read by the GPU.
On 2015/07/30 16:14:10, reveman wrote:
> "non-GPU processes", do we need to be this specific here? CPU access to native
> pixmaps in the GPU process will mostly like use this class too.

That's true, though we don't have the use case yet. I'll remove "non-GPU
processes"

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
ui/ozone/public/client_pixmap.h:15: class OZONE_EXPORT ClientPixmap {
On 2015/07/30 16:14:10, reveman wrote:
> Why the "Native" prefix in one case but not the other? Can you make it
> consistent? Pixmap+ClientPixmap or NativePixmap+ClientNativePixmap

Ok, I remain this up to spang. spang, which do you prefer?

[spang, 2015-07-30 19:19:04]

On 2015/07/30 16:50:15, dshwang wrote:
>
https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
> File ui/ozone/public/client_pixmap.h (right):
> 
>
https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
> ui/ozone/public/client_pixmap.h:13: // in non-GPU processes, but can also be
> read by the GPU.
> On 2015/07/30 16:14:10, reveman wrote:
> > "non-GPU processes", do we need to be this specific here? CPU access to
native
> > pixmaps in the GPU process will mostly like use this class too.
> 
> That's true, though we don't have the use case yet. I'll remove "non-GPU
> processes"
> 
>
https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
> ui/ozone/public/client_pixmap.h:15: class OZONE_EXPORT ClientPixmap {
> On 2015/07/30 16:14:10, reveman wrote:
> > Why the "Native" prefix in one case but not the other? Can you make it
> > consistent? Pixmap+ClientPixmap or NativePixmap+ClientNativePixmap
> 
> Ok, I remain this up to spang. spang, which do you prefer?

I think ClientNativePixmap is short enough, and then we don't have to rename the
existing NativePixmap interface.

[dshwang, 2015-07-31 10:41:04]

I renamed to ClientNativePixmap. Could you review again?

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
File ui/ozone/public/client_pixmap.h (right):

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/client...
ui/ozone/public/client_pixmap.h:15: class OZONE_EXPORT ClientPixmap {
On 2015/07/30 16:50:15, dshwang wrote:
> On 2015/07/30 16:14:10, reveman wrote:
> > Why the "Native" prefix in one case but not the other? Can you make it
> > consistent? Pixmap+ClientPixmap or NativePixmap+ClientNativePixmap
> 
> Ok, I remain this up to spang. spang, which do you prefer?

On 2015/07/30 19:19:04, spang wrote:
> I think ClientNativePixmap is short enough, and then we don't have to rename
the
> existing NativePixmap interface.

Ok, NativePixmap and ClientNativePixmap are chosen.

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/pixmap...
File ui/ozone/public/pixmap_types.h (right):

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/pixmap...
ui/ozone/public/pixmap_types.h:13: PIXMAP_FORMAT_UNKNOWN,
On 2015/07/30 16:14:11, reveman wrote:
> Realize this is not new but it would be nice to avoid invalid enum values if
> possible. Can we remove this?

Done. removed.

https://codereview.chromium.org/1128113011/diff/510001/ui/ozone/public/pixmap...
ui/ozone/public/pixmap_types.h:16: PIXMAP_FORMAT_LAST = PIXMAP_FORMAT_RGBX_8888
On 2015/07/30 16:14:11, reveman wrote:
> Is PIXMAP_FORMAT_LAST used for IPC? If not, can we remove it?

It's used for IPC in ozone_gpu_messages.h

[dshwang, 2015-07-31 11:23:49]

Patchset #18 (id:530001) has been deleted

[dshwang, 2015-07-31 11:26:31]

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

[dshwang, 2015-07-31 11:27:18]

dongseong.hwang@intel.com changed reviewers:
+ dcheng@chromium.org, sievers@chromium.org
- piman@chromium.org

[dshwang, 2015-07-31 11:38:06]

Patchset #18 (id:550001) has been deleted

[dshwang, 2015-07-31 11:38:43]

Patchset #7 (id:120001) has been deleted

[dshwang, 2015-07-31 11:39:05]

Patchset #3 (id:40001) has been deleted

[reveman, 2015-07-31 15:50:15]

lgtm with nits

https://codereview.chromium.org/1128113011/diff/570001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/570001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:45: virtual
scoped_ptr<ClientNativePixmap> ImportClientNativePixmap(
nit: maybe ImportNativePixmap (we're importing a native pixmap not a client
native..) or CreateClientNativePixmapFromFileDescriptor

https://codereview.chromium.org/1128113011/diff/570001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:46: base::FileDescriptor handle,
const base::FileDescriptor& handle

This is necessary as you only forward declare FileDescriptor above

https://codereview.chromium.org/1128113011/diff/570001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:47: gfx::Size size,
nit: const gfx::Size& size

[dshwang, 2015-07-31 16:11:45]

> I think we should hold off just a bit longer on this until the following patch
is in better shape.

spang, could you review again? Do you think we should hold off more?

sievers, could you review content/?
dcheng, could you review ui/ozone/common/gpu/ozone_gpu_message*?

https://codereview.chromium.org/1128113011/diff/570001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/570001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:45: virtual
scoped_ptr<ClientNativePixmap> ImportClientNativePixmap(
On 2015/07/31 15:50:15, reveman wrote:
> nit: maybe ImportNativePixmap (we're importing a native pixmap not a client
> native..) or CreateClientNativePixmapFromFileDescriptor

Done. choose ImportNativePixmap

https://codereview.chromium.org/1128113011/diff/570001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:46: base::FileDescriptor handle,
On 2015/07/31 15:50:15, reveman wrote:
> const base::FileDescriptor& handle
> 
> This is necessary as you only forward declare FileDescriptor above

I missed your last comment. Done.
BTW, type for argument is fine with forward declaration, unlike type for member.

https://codereview.chromium.org/1128113011/diff/570001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:47: gfx::Size size,
On 2015/07/31 15:50:14, reveman wrote:
> nit: const gfx::Size& size

Done.

[dcheng, 2015-07-31 18:09:52]

IPC changes seem OK, but two other comments.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
File ui/ozone/common/stub_client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
ui/ozone/common/stub_client_native_pixmap_manager.h:13: // The caller takes
ownership of the instance.
Returning a scoped_ptr<T> here would be a nice way to express that =)

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/common/client_native_pixmap_manager_gbm.cc (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/common/client_native_pixmap_manager_gbm.cc:21:
std::vector<Configuration> configurations = {
Braced initialization of std::vector isn't allowed yet: it uses
std::initializer_list, which is a C++11 library feature.

[dshwang, 2015-07-31 18:59:29]

thx dcheng for reviewing. could you review again?

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
File ui/ozone/common/stub_client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
ui/ozone/common/stub_client_native_pixmap_manager.h:13: // The caller takes
ownership of the instance.
On 2015/07/31 18:09:52, dcheng wrote:
> Returning a scoped_ptr<T> here would be a nice way to express that =)

It's used in "ClientNativePixmapManager*
CreateClientNativePixmapManager<Platform>()" function, which is generated by
script.
If it returns scoped_ptr<T>, the function will become a bit weird.
ClientNativePixmapManager* CreateClientNativePixmapManager<Platform>() {
  return CreateStubClientNativePixmapManager().release();
}

It's nice suggestion. I'll change all of them with script change in another CL.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/common/client_native_pixmap_manager_gbm.cc (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/common/client_native_pixmap_manager_gbm.cc:21:
std::vector<Configuration> configurations = {
On 2015/07/31 18:09:52, dcheng wrote:
> Braced initialization of std::vector isn't allowed yet: it uses
> std::initializer_list, which is a C++11 library feature.

as google-styleguide, braced initializer list is allowed. chrome style guide is
different?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initia...

[dcheng, 2015-07-31 19:02:39]

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
File ui/ozone/common/stub_client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
ui/ozone/common/stub_client_native_pixmap_manager.h:13: // The caller takes
ownership of the instance.
On 2015/07/31 at 18:59:29, dshwang wrote:
> On 2015/07/31 18:09:52, dcheng wrote:
> > Returning a scoped_ptr<T> here would be a nice way to express that =)
> 
> It's used in "ClientNativePixmapManager*
CreateClientNativePixmapManager<Platform>()" function, which is generated by
script.
> If it returns scoped_ptr<T>, the function will become a bit weird.
> ClientNativePixmapManager* CreateClientNativePixmapManager<Platform>() {
>   return CreateStubClientNativePixmapManager().release();
> }
> 
> It's nice suggestion. I'll change all of them with script change in another
CL.

Why not just do it in this patch, since it's introducing it?

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/common/client_native_pixmap_manager_gbm.cc (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/common/client_native_pixmap_manager_gbm.cc:21:
std::vector<Configuration> configurations = {
On 2015/07/31 at 18:59:29, dshwang wrote:
> On 2015/07/31 18:09:52, dcheng wrote:
> > Braced initialization of std::vector isn't allowed yet: it uses
> > std::initializer_list, which is a C++11 library feature.
> 
> as google-styleguide, braced initializer list is allowed. chrome style guide
is different?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initia...

https://chromium-cpp.appspot.com/

[spang, 2015-08-01 02:14:56]

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
content/app/content_main_runner.cc:674: if (process_type ==
switches::kRendererProcess) {
This doesn't work for me. You have to also do it for kZygoteProcess.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
File ui/ozone/common/stub_client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
ui/ozone/common/stub_client_native_pixmap_manager.h:13: // The caller takes
ownership of the instance.
On 2015/07/31 19:02:39, dcheng wrote:
> On 2015/07/31 at 18:59:29, dshwang wrote:
> > On 2015/07/31 18:09:52, dcheng wrote:
> > > Returning a scoped_ptr<T> here would be a nice way to express that =)
> > 
> > It's used in "ClientNativePixmapManager*
> CreateClientNativePixmapManager<Platform>()" function, which is generated by
> script.
> > If it returns scoped_ptr<T>, the function will become a bit weird.
> > ClientNativePixmapManager* CreateClientNativePixmapManager<Platform>() {
> >   return CreateStubClientNativePixmapManager().release();
> > }
> > 
> > It's nice suggestion. I'll change all of them with script change in another
> CL.
> 
> Why not just do it in this patch, since it's introducing it?

Because it will domino to all the places this is called, and those places have a
defined signature (to match some generated code). I'd prefer to avoid the churn
and leave it alone personally.

It does get wrapped in a scoped_ptr eventually, always (at the call to
make_scoped_ptr() in ui/ozone/platform_object_internal.h).

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap.h:19: virtual void* Map() = 0;
bool Map(void **data)

for consistency with GMB interface

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap.h:21: virtual int GetStride() const = 0;
void GetStride(void *stride)

same reason

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:26: class OZONE_EXPORT
ClientNativePixmapManager {
I know this is a bit burdensome, but can you name it ClientNativePixmapFactory?

It's not like IOSurfaceManager - it doesn't keep a map of ClientNativePixmaps,
just creates them and then forgets about them. It creates ClientNativePixmap
objects, so it's a ClientNativePixmapFactory.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:41: virtual
std::vector<Configuration> GetSupportedConfigurations() const = 0;
I don't think it belongs here.

The client doesn't get to decide what kind of buffer to create, this is a
service side concern and only used by the GMB factory. So it stays on
SurfaceFactoryOzone.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_types.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_types.h:12: enum NativePixmapFormat {
enum class NativePixmapFormat {
  BGRA_8888,
  RGBX_8888,
  LAST = RGBX_8888
};

ui::NativePixmapFormat::BGRA_8888

but note that I have another change pending that makes this part unnecessary:

  https://codereview.chromium.org/1269503007/

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_types.h:18: enum NativePixmapUsage {
enum class NativePixmapUsage {
  MAP,
  PERSISTENT_MAP,
  SCANOUT,
  LAST = SCANOUT
};

ui::NativePixmapUsage::MAP

[reveman, 2015-08-01 03:03:13]

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:26: class OZONE_EXPORT
ClientNativePixmapManager {
On 2015/08/01 at 02:14:56, spang wrote:
> I know this is a bit burdensome, but can you name it
ClientNativePixmapFactory?
> 
> It's not like IOSurfaceManager - it doesn't keep a map of ClientNativePixmaps,
just creates them and then forgets about them. It creates ClientNativePixmap
objects, so it's a ClientNativePixmapFactory.

sgtm

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:41: virtual
std::vector<Configuration> GetSupportedConfigurations() const = 0;
On 2015/08/01 at 02:14:56, spang wrote:
> I don't think it belongs here.
> 
> The client doesn't get to decide what kind of buffer to create, this is a
service side concern and only used by the GMB factory. So it stays on
SurfaceFactoryOzone.

+1

[dshwang, 2015-08-03 12:51:49]

rename to ClientNativePixmapFactory, and rebase to
https://codereview.chromium.org/1269503007/
reveman, spang, dcheng, sievers, could you review again?

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
content/app/content_main_runner.cc:659: !process_type.empty() && (process_type
== switches::kRendererProcess ||
spang, in the same sense, kZygoteProcess is needed here.

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
content/app/content_main_runner.cc:674: if (process_type ==
switches::kRendererProcess) {
On 2015/08/01 02:14:56, spang wrote:
> This doesn't work for me. You have to also do it for kZygoteProcess.

Why do you think so? Zygote don't need to handle native GpuMemoryBuffer

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
File ui/ozone/common/stub_client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/common/stub_c...
ui/ozone/common/stub_client_native_pixmap_manager.h:13: // The caller takes
ownership of the instance.
On 2015/08/01 02:14:56, spang wrote:
> On 2015/07/31 19:02:39, dcheng wrote:
> > On 2015/07/31 at 18:59:29, dshwang wrote:
> > > On 2015/07/31 18:09:52, dcheng wrote:
> > > > Returning a scoped_ptr<T> here would be a nice way to express that =)
> > > 
> > > It's used in "ClientNativePixmapManager*
> > CreateClientNativePixmapManager<Platform>()" function, which is generated by
> > script.
> > > If it returns scoped_ptr<T>, the function will become a bit weird.
> > > ClientNativePixmapManager* CreateClientNativePixmapManager<Platform>() {
> > >   return CreateStubClientNativePixmapManager().release();
> > > }
> > > 
> > > It's nice suggestion. I'll change all of them with script change in
another
> > CL.
> > 
> > Why not just do it in this patch, since it's introducing it?
> 
> Because it will domino to all the places this is called, and those places have
a
> defined signature (to match some generated code). I'd prefer to avoid the
churn
> and leave it alone personally.
> 
> It does get wrapped in a scoped_ptr eventually, always (at the call to
> make_scoped_ptr() in ui/ozone/platform_object_internal.h).

Thank you for explanation.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/common/client_native_pixmap_manager_gbm.cc (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/common/client_native_pixmap_manager_gbm.cc:21:
std::vector<Configuration> configurations = {
On 2015/07/31 19:02:39, dcheng wrote:
> On 2015/07/31 at 18:59:29, dshwang wrote:
> > On 2015/07/31 18:09:52, dcheng wrote:
> > > Braced initialization of std::vector isn't allowed yet: it uses
> > > std::initializer_list, which is a C++11 library feature.
> > 
> > as google-styleguide, braced initializer list is allowed. chrome style guide
> is different?
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initia...
> 
> https://chromium-cpp.appspot.com/

I see. Thx. Done.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap.h:19: virtual void* Map() = 0;
On 2015/08/01 02:14:56, spang wrote:
> bool Map(void **data)
> 
> for consistency with GMB interface

Done.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap.h:21: virtual int GetStride() const = 0;
On 2015/08/01 02:14:56, spang wrote:
> void GetStride(void *stride)
> 
> same reason

Done.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:26: class OZONE_EXPORT
ClientNativePixmapManager {
On 2015/08/01 03:03:13, reveman wrote:
> On 2015/08/01 at 02:14:56, spang wrote:
> > I know this is a bit burdensome, but can you name it
> ClientNativePixmapFactory?
> > 
> > It's not like IOSurfaceManager - it doesn't keep a map of
ClientNativePixmaps,
> just creates them and then forgets about them. It creates ClientNativePixmap
> objects, so it's a ClientNativePixmapFactory.
> 
> sgtm

Done.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:41: virtual
std::vector<Configuration> GetSupportedConfigurations() const = 0;
On 2015/08/01 03:03:13, reveman wrote:
> On 2015/08/01 at 02:14:56, spang wrote:
> > I don't think it belongs here.
> > 
> > The client doesn't get to decide what kind of buffer to create, this is a
> service side concern and only used by the GMB factory. So it stays on
> SurfaceFactoryOzone.
> 
> +1

That makes sense but it requires huge change.
Note that this is used in
BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferXXXX, and browser doesn't
create SurfaceFactoryOzone.
Currently fallback decision maker is browser, not gpu process. browser decides
either native GpuMemoryBuffer or GpuMemoryBufferImplSharedMemory via
GetSupportedConfigurations().
If we want to move GetSupportedConfigurations() to SurfaceFactoryOzone, we
should move fallback logic to gpu process earlier. It might be future
refactoring task.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/native...
File ui/ozone/public/native_pixmap_types.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_types.h:12: enum NativePixmapFormat {
On 2015/08/01 02:14:56, spang wrote:
> enum class NativePixmapFormat {
>   BGRA_8888,
>   RGBX_8888,
>   LAST = RGBX_8888
> };
> 
> ui::NativePixmapFormat::BGRA_8888
> 
> but note that I have another change pending that makes this part unnecessary:
> 
>   https://codereview.chromium.org/1269503007/

Good to know. I rebase this CL based on your CL. Let me wait for landing your
CL.

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/native...
ui/ozone/public/native_pixmap_types.h:18: enum NativePixmapUsage {
On 2015/08/01 02:14:56, spang wrote:
> enum class NativePixmapUsage {
>   MAP,
>   PERSISTENT_MAP,
>   SCANOUT,
>   LAST = SCANOUT
> };
> 
> ui::NativePixmapUsage::MAP

Done.

[spang, 2015-08-03 18:24:51]

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
content/app/content_main_runner.cc:659: !process_type.empty() && (process_type
== switches::kRendererProcess ||
On 2015/08/03 12:51:48, dshwang wrote:
> spang, in the same sense, kZygoteProcess is needed here.

I'm not sure if the zygote is used on mac..

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
content/app/content_main_runner.cc:674: if (process_type ==
switches::kRendererProcess) {
On 2015/08/03 12:51:48, dshwang wrote:
> On 2015/08/01 02:14:56, spang wrote:
> > This doesn't work for me. You have to also do it for kZygoteProcess.
> 
> Why do you think so? Zygote don't need to handle native GpuMemoryBuffer

Zygote runs this initialization once, and then forks multiple times to create
renderers.

I'm pretty sure if the zygote is enabled, and you only check "kRendererProcess"
here, the code will never run.

At least, that's what I experienced. Have you tried it to see if it runs?

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_manager.h (right):

https://codereview.chromium.org/1128113011/diff/590001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_manager.h:41: virtual
std::vector<Configuration> GetSupportedConfigurations() const = 0;
On 2015/08/03 12:51:48, dshwang wrote:
> On 2015/08/01 03:03:13, reveman wrote:
> > On 2015/08/01 at 02:14:56, spang wrote:
> > > I don't think it belongs here.
> > > 
> > > The client doesn't get to decide what kind of buffer to create, this is a
> > service side concern and only used by the GMB factory. So it stays on
> > SurfaceFactoryOzone.
> > 
> > +1
> 
> That makes sense but it requires huge change.
> Note that this is used in
> BrowserGpuMemoryBufferManager::AllocateGpuMemoryBufferXXXX, and browser
doesn't
> create SurfaceFactoryOzone.
> Currently fallback decision maker is browser, not gpu process. browser decides
> either native GpuMemoryBuffer or GpuMemoryBufferImplSharedMemory via
> GetSupportedConfigurations().
> If we want to move GetSupportedConfigurations() to SurfaceFactoryOzone, we
> should move fallback logic to gpu process earlier. It might be future
> refactoring task.

Ah, ok, I see your point about selecting native vs shared memory in the browser.
It doesn't seem ideal for it to work that way because it doesn't allow for any
hardware dependence on what types can be supported, but I guess we can leave
this for later.

[dshwang, 2015-08-04 13:05:21]

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1128113011/diff/590001/content/app/content_ma...
content/app/content_main_runner.cc:674: if (process_type ==
switches::kRendererProcess) {
On 2015/08/03 18:24:51, spang wrote:
> On 2015/08/03 12:51:48, dshwang wrote:
> > On 2015/08/01 02:14:56, spang wrote:
> > > This doesn't work for me. You have to also do it for kZygoteProcess.
> > 
> > Why do you think so? Zygote don't need to handle native GpuMemoryBuffer
> 
> Zygote runs this initialization once, and then forks multiple times to create
> renderers.
> 
> I'm pretty sure if the zygote is enabled, and you only check
"kRendererProcess"
> here, the code will never run.

It run well in render process, no matter zygote enabled.
Zygote process gets --type=zygote and Render process gets --type=renderer.
ClientNativePixmapManager instance must be created in only renderer. 

> At least, that's what I experienced. Have you tried it to see if it runs?

Yes, it works well with zygote process. If allow kZygoteProcess here, zygote
creates ClientNativePixmapManagerTEST instance because we don't pass
"--ozone-platform=gbm" to zygote.

[spang, 2015-08-04 15:55:13]

lgtm

Although I think the zygote issue is real, and will become apparent shortly.

[dshwang, 2015-08-04 16:21:10]

On 2015/08/04 15:55:13, spang wrote:
> lgtm
> 
> Although I think the zygote issue is real, and will become apparent shortly.

Thank you for re-lgtm. I think zygote has no issue. In addition, the code in
content_main_runner.cc:674 will be removed in next CL;
https://codereview.chromium.org/1248713002/
reveman, do you have same concern about zygote?

reveman and spang gave lgtm.
sievers, could you review content/?
dcheng, could you review ui/ozone/common/gpu/ozone_gpu_message*?

[dshwang, 2015-08-05 09:14:29]

dongseong.hwang@intel.com changed reviewers:
- dcheng@chromium.org, sievers@chromium.org

[dshwang, 2015-08-05 09:15:34]

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

[dshwang, 2015-08-05 09:16:46]

On 2015/08/04 16:21:10, dshwang wrote:
> reveman and spang gave lgtm.
> sievers, could you review content/?
> dcheng, could you review ui/ozone/common/gpu/ozone_gpu_message*?

dcheng, sorry for noise. The change in ozone_gpu_message* was removed, so don't
need to review.

piman, sievers, could you review content/?

[dshwang, 2015-08-05 12:11:50]

piman, sievers, could you review content/?

https://codereview.chromium.org/1128113011/diff/630001/content/renderer/rende...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/1128113011/diff/630001/content/renderer/rende...
content/renderer/renderer_main.cc:114:
ui::ClientNativePixmapFactory::SetInstance(g_pixmap_factory.Get().get());
On 2015/08/04 15:55:13, spang wrote:
> lgtm
> 
> Although I think the zygote issue is real, and will become apparent shortly.

you are right. It's not yet completely switched to "renderer" in
ContentMainRunnerImpl::Initialize(), so kZygoteProcess is needed in the
function.
So I move ClientNativePixmapFactory::SetInstance() for renderer to here.

[no sievers, 2015-08-05 18:27:02]

content lgtm

https://codereview.chromium.org/1128113011/diff/630001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_factory.cc (right):

https://codereview.chromium.org/1128113011/diff/630001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_factory.cc:41:
ClientNativePixmapFactory::~ClientNativePixmapFactory() {}
nit: It's a bit of an interesting pattern - the singleton here with the external
scoped ownership. Should we call SetInstance(nullptr) here at least?

[dshwang, 2015-08-05 18:46:34]

thx for reviewing!

https://codereview.chromium.org/1128113011/diff/630001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_factory.cc (right):

https://codereview.chromium.org/1128113011/diff/630001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_factory.cc:41:
ClientNativePixmapFactory::~ClientNativePixmapFactory() {}
On 2015/08/05 18:27:02, sievers wrote:
> nit: It's a bit of an interesting pattern - the singleton here with the
external
> scoped ownership. Should we call SetInstance(nullptr) here at least?

This pattern is originated from SurfaceTextureManager and IOSurfaceManager. All
singletons live until the process die. so SetInstance(nullptr) is actually not
needed.
If reveman agree on this idea, I'll add SetInstance(nullptr) to above all
desctuctors in another CLs.

[dshwang, 2015-08-05 18:46:52]

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

[dshwang, 2015-08-05 18:46:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1128113011/#ps630001
(title: "handle zygote")

[commit-bot: I haz the power, 2015-08-05 18:48:39]

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

[commit-bot: I haz the power, 2015-08-05 19:50:54]

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

[commit-bot: I haz the power, 2015-08-05 19:50:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2015-08-05 19:54:03]

https://codereview.chromium.org/1128113011/diff/630001/ui/ozone/public/client...
File ui/ozone/public/client_native_pixmap_factory.cc (right):

https://codereview.chromium.org/1128113011/diff/630001/ui/ozone/public/client...
ui/ozone/public/client_native_pixmap_factory.cc:41:
ClientNativePixmapFactory::~ClientNativePixmapFactory() {}
On 2015/08/05 at 18:46:34, dshwang wrote:
> On 2015/08/05 18:27:02, sievers wrote:
> > nit: It's a bit of an interesting pattern - the singleton here with the
external
> > scoped ownership. Should we call SetInstance(nullptr) here at least?
> 
> This pattern is originated from SurfaceTextureManager and IOSurfaceManager.
All singletons live until the process die. so SetInstance(nullptr) is actually
not needed.
> If reveman agree on this idea, I'll add SetInstance(nullptr) to above all
desctuctors in another CLs.

The reason for Set/GetInstance is to push the ownership and lifetime management
to content/ instead of having it in ui/. E.g. content/ can create 10
ClientNativePixmapFactory instances but only set one as the current instance.
ui/ doesn't care.

[spang, 2015-08-05 20:07:25]

On 2015/08/05 19:54:03, reveman wrote:
>
https://codereview.chromium.org/1128113011/diff/630001/ui/ozone/public/client...
> File ui/ozone/public/client_native_pixmap_factory.cc (right):
> 
>
https://codereview.chromium.org/1128113011/diff/630001/ui/ozone/public/client...
> ui/ozone/public/client_native_pixmap_factory.cc:41:
> ClientNativePixmapFactory::~ClientNativePixmapFactory() {}
> On 2015/08/05 at 18:46:34, dshwang wrote:
> > On 2015/08/05 18:27:02, sievers wrote:
> > > nit: It's a bit of an interesting pattern - the singleton here with the
> external
> > > scoped ownership. Should we call SetInstance(nullptr) here at least?
> > 
> > This pattern is originated from SurfaceTextureManager and IOSurfaceManager.
> All singletons live until the process die. so SetInstance(nullptr) is actually
> not needed.
> > If reveman agree on this idea, I'll add SetInstance(nullptr) to above all
> desctuctors in another CLs.
> 
> The reason for Set/GetInstance is to push the ownership and lifetime
management
> to content/ instead of having it in ui/. E.g. content/ can create 10
> ClientNativePixmapFactory instances but only set one as the current instance.
> ui/ doesn't care.

I would even go further and move the pointer itself (and SetInstance() /
GetInstance()) to content, so that it not only creates it but also directly
controls who can access it.

We don't have any need to access the global instance from within ui/ or below.

[dshwang, 2015-08-06 07:16:49]

Patchset #20 (id:650001) has been deleted

[dshwang, 2015-08-06 07:17:17]

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

[dshwang, 2015-08-06 07:17:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sievers@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1128113011/#ps670001
(title: "fix extensions_unittests")

[commit-bot: I haz the power, 2015-08-06 07:17:29]

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

[commit-bot: I haz the power, 2015-08-06 09:04:04]

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

[commit-bot: I haz the power, 2015-08-06 09:04:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dshwang, 2015-08-06 10:11:27]

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

[commit-bot: I haz the power, 2015-08-06 10:11:47]

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

[commit-bot: I haz the power, 2015-08-06 11:00:40]

Committed patchset #20 (id:670001)

[commit-bot: I haz the power, 2015-08-06 11:01:13]

Patchset 20 (id:??) landed as
https://crrev.com/16f5a8a44f4a2c10086263775e5be20669810867
Cr-Commit-Position: refs/heads/master@{#342094}