Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2705213005: Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux.

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 3 weeks ago by Julien Isorce
Modified:
2 months ago
Reviewers:
reveman, sadrul, spang
CC:
chromium-reviews, piman+watch_chromium.orgigalia.com, tonikitoo
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add NativePixmapDmabufStub to finalize glCreateImageCHROMIUM on Linux. This class is just a stub to hold dmabuf in order to be passed to GLImageNativePixmap::Initialize(NativePixmap*). This complete enablement of glCreateImageCHROMIUM with linux dma buf on linux. BUG=584248 R=reveman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 6

Patch Set 3 : Just rebase #

Patch Set 4 : Addressed some review remarks and fixed build #

Patch Set 5 : Rebase and addressed remarks #

Total comments: 4

Patch Set 6 : Rename NativePixmapDmabufStub to NativePixmapDmabuf #

Patch Set 7 : Rebase #

Patch Set 8 : Rebase #

Patch Set 9 : Rebase #

Patch Set 10 : Rebase #

Patch Set 11 : Rebase #

Patch Set 12 : Rebase #

Total comments: 3

Patch Set 13 : Make GbmPixmap inherit NativePixmapDmabuf #

Patch Set 14 : Make ~NativePixmapDmaBuf protected #

Patch Set 15 : Make GetSize non inline #

Patch Set 16 : Make the helper a member instead of a parent #

Patch Set 17 : Forgot friend class base::RefCountedThreadSafe #

Total comments: 4

Patch Set 18 : Go back to previous solution (Patch Set 13) make the helper a parent instead of member #

Total comments: 1

Patch Set 19 : Go back to solution NativePximapDmabufHelper is a member of GbmBufer while GbmPixmap inherits Nativ… #

Patch Set 20 : Just rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+274 lines, -97 lines) Patch
M gpu/ipc/service/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -0 lines 0 comments Download
M gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 18 2 chunks +8 lines, -3 lines 0 comments Download
M ui/gfx/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -0 lines 0 comments Download
A ui/gfx/linux/native_pixmap_dmabuf.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 18 1 chunk +86 lines, -0 lines 0 comments Download
A ui/gfx/linux/native_pixmap_dmabuf.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 18 1 chunk +127 lines, -0 lines 0 comments Download
M ui/ozone/platform/drm/gpu/gbm_buffer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +10 lines, -16 lines 0 comments Download
M ui/ozone/platform/drm/gpu/gbm_buffer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 5 chunks +37 lines, -78 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 98 (77 generated)
reveman
https://codereview.chromium.org/2705213005/diff/20001/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc File gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc (right): https://codereview.chromium.org/2705213005/diff/20001/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc#newcode106 gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc:106: pixmap = new ui::NativePixmapDmaBufStub(size, format, should we be using ...
4 months ago (2017-04-12 18:21:37 UTC) #4
Julien Isorce
Thx for the review, here are my replies: https://codereview.chromium.org/2705213005/diff/20001/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc File gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc (right): https://codereview.chromium.org/2705213005/diff/20001/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc#newcode106 gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc:106: pixmap ...
4 months ago (2017-04-13 12:46:02 UTC) #9
Julien Isorce
> https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/BUILD.gn#newcode496 > ui/gfx/BUILD.gn:496: "//gpu/ipc/service:ipc_service_sources", > On 2017/04/12 18:21:36, reveman wrote: > > why is ...
4 months ago (2017-04-14 09:59:42 UTC) #15
spang1
On 2017/04/14 09:59:42, Julien Isorce wrote: > > > https://codereview.chromium.org/2705213005/diff/20001/ui/gfx/BUILD.gn#newcode496 > > ui/gfx/BUILD.gn:496: "//gpu/ipc/service:ipc_service_sources", > ...
4 months ago (2017-04-18 18:59:44 UTC) #16
Julien Isorce
On 2017/04/18 18:59:44, spang1 wrote: > On 2017/04/14 09:59:42, Julien Isorce wrote: > > > ...
3 months, 4 weeks ago (2017-04-19 12:06:23 UTC) #21
Julien Isorce
Please take a look to patch set 5, thx!
3 months, 4 weeks ago (2017-04-19 12:07:56 UTC) #22
spang
lgtm https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pixmap_dmabuf_stub.cc File ui/gfx/linux/native_pixmap_dmabuf_stub.cc (right): https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pixmap_dmabuf_stub.cc#newcode79 ui/gfx/linux/native_pixmap_dmabuf_stub.cc:79: { Wrong whitespace, please run clang-format / git ...
3 months, 3 weeks ago (2017-04-21 15:11:49 UTC) #23
Julien Isorce
Thx for the review. Remarks addressed in Patch Set 6. https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pixmap_dmabuf_stub.cc File ui/gfx/linux/native_pixmap_dmabuf_stub.cc (right): https://codereview.chromium.org/2705213005/diff/80001/ui/gfx/linux/native_pixmap_dmabuf_stub.cc#newcode79 ...
3 months, 3 weeks ago (2017-04-24 12:26:00 UTC) #24
Julien Isorce
Hi @reveman, please take a look. Thx!
3 months, 3 weeks ago (2017-04-27 13:49:43 UTC) #33
Julien Isorce
Hi sadrul, PTAL at ui/gfx/*. Thank you!
3 months, 1 week ago (2017-05-11 09:00:22 UTC) #37
sadrul
On 2017/05/11 09:00:22, Julien Isorce wrote: > Hi sadrul, PTAL at ui/gfx/*. Thank you! reveman@ ...
3 months ago (2017-05-12 17:10:20 UTC) #40
Julien Isorce
On 2017/05/12 17:10:20, sadrul wrote: > On 2017/05/11 09:00:22, Julien Isorce wrote: > > Hi ...
3 months ago (2017-05-18 15:01:40 UTC) #41
Julien Isorce
Hi @reveman, gentle ping to review this CL. Thx.
2 months, 1 week ago (2017-06-05 14:49:07 UTC) #58
reveman
https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pixmap_dmabuf.h File ui/gfx/linux/native_pixmap_dmabuf.h (right): https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pixmap_dmabuf.h#newcode1 ui/gfx/linux/native_pixmap_dmabuf.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. ...
2 months, 1 week ago (2017-06-05 17:44:42 UTC) #59
Julien Isorce
https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pixmap_dmabuf.h File ui/gfx/linux/native_pixmap_dmabuf.h (right): https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pixmap_dmabuf.h#newcode1 ui/gfx/linux/native_pixmap_dmabuf.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. ...
2 months, 1 week ago (2017-06-05 20:23:58 UTC) #60
Julien Isorce
https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pixmap_dmabuf.h File ui/gfx/linux/native_pixmap_dmabuf.h (right): https://codereview.chromium.org/2705213005/diff/210001/ui/gfx/linux/native_pixmap_dmabuf.h#newcode1 ui/gfx/linux/native_pixmap_dmabuf.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. ...
2 months, 1 week ago (2017-06-07 20:10:49 UTC) #81
reveman
https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pixmap_dmabuf.h File ui/gfx/linux/native_pixmap_dmabuf.h (right): https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pixmap_dmabuf.h#newcode21 ui/gfx/linux/native_pixmap_dmabuf.h:21: class GFX_EXPORT NativePixmapDmabufHelper why a helper instead of NativePixmapDmabufBase ...
2 months, 1 week ago (2017-06-07 21:02:37 UTC) #82
Julien Isorce
https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pixmap_dmabuf.h File ui/gfx/linux/native_pixmap_dmabuf.h (right): https://codereview.chromium.org/2705213005/diff/310001/ui/gfx/linux/native_pixmap_dmabuf.h#newcode21 ui/gfx/linux/native_pixmap_dmabuf.h:21: class GFX_EXPORT NativePixmapDmabufHelper On 2017/06/07 21:02:37, reveman wrote: > ...
2 months, 1 week ago (2017-06-07 21:29:53 UTC) #83
Julien Isorce
On 2017/06/07 21:02:37, reveman wrote: > why a helper instead of NativePixmapDmabufBase that we derive ...
2 months, 1 week ago (2017-06-08 11:07:33 UTC) #88
reveman
https://codereview.chromium.org/2705213005/diff/310002/ui/ozone/platform/drm/gpu/gbm_buffer.h File ui/ozone/platform/drm/gpu/gbm_buffer.h (right): https://codereview.chromium.org/2705213005/diff/310002/ui/ozone/platform/drm/gpu/gbm_buffer.h#newcode24 ui/ozone/platform/drm/gpu/gbm_buffer.h:24: class GbmBuffer : public GbmBufferBase, public gfx::NativePixmapDmabufHelper { why ...
2 months, 1 week ago (2017-06-08 13:40:32 UTC) #89
Julien Isorce
2 months, 1 week ago (2017-06-08 14:01:31 UTC) #90
On 2017/06/08 13:40:32, reveman wrote:
>
https://codereview.chromium.org/2705213005/diff/310002/ui/ozone/platform/drm/...
> ui/ozone/platform/drm/gpu/gbm_buffer.h:24: class GbmBuffer : public
> GbmBufferBase, public gfx::NativePixmapDmabufHelper {
> why do we need to inherit this here? can't we just remove most of the
functions
> now that gfx::NativePixmapDmaBuf implement them?

Hi David, thx for your comments. But I am a bit lost, it looks to me that
yesterday you said the opposite :) ?
"why a helper instead of NativePixmapDmabufBase that we derive from"
"why do we need to inherit this here? "
I guess I have miss understood your point so let me clarify:

Prior this CL here is the existing design:

DrmBuffer inherits DrmBufferBase which inherits ozone::Refcounted<ScanoutBuffer>
DrmBuffer is a scoped_refptr member of DrmPixmap which implements the interface
RefCounted<gfx::NativePixmap>

Then this CL introduces gfx::NativePixmapDmabuf which implements the interface
RefCounted<gfx::NativePixmap>.

Now making DrmPixmap inherits gfx::NativePixmapDmabuf is not straight forward to
me. It is actually all about the relation
between NativePixmapHelper and DrmBuffer.
Currently I have 2 solutions for this relation: Patch Set 16 (member) or Patch
Set 18 (parent).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b