Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2477043002:
exo: Add drm support to motion_events. (Closed)
|
Created:
4 years, 1 month ago by Daniele Castagna Modified:
4 years, 1 month ago Reviewers:
reveman CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Add drm support to motion_events.
Add support to render using dmabufs to motion_events wayland
test client.
BUG=661010
TEST=XDG_RUNTIME_DIR=/var/run/chrome /tmp/wayland_motion_events --linux_dmabuf_v1 on kevin.
Committed: https://crrev.com/67808baf1012d79550204d4cb25c4e3a26caa416
Cr-Commit-Position: refs/heads/master@{#430224}
Patch Set 1 #Patch Set 2 : Rebase on master. #
Total comments: 33
Patch Set 3 : Address comments and rebase on skia code. #Patch Set 4 : Make it compile on linux. #Patch Set 5 : Command line must be initialized for Ozone too. #Patch Set 6 : Rebase on master. #Patch Set 7 : fix drm objects order of destruction. #Patch Set 8 : Fix destruction order. #
Total comments: 35
Patch Set 9 : Address comments. #Patch Set 10 : Make linux compile. #
Total comments: 11
Patch Set 11 : Last round of nits. #
Messages
Total messages: 36 (25 generated)
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... File components/exo/wayland/BUILD.gn (right): https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/BUILD.gn:108: "-lGLESv2", do we need these? https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:128: std::unique_ptr<gbm_bo> bo = nullptr; remove " = nullptr" https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:130: EGLImageKHR egl_image = 0; can we have a unique_ptr for this too? https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:131: unsigned gl_texture = 0; s/gl_texture/texture/ can we use GLuint instead of unsigned? and can we add a ScopedTexture wrapper so we can use unique_ptr? https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:132: unsigned fb = 0; s/fb/framebuffer/ and GLuint + ScopedFramebuffer as above https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:253: static_cast<std::unique_ptr<wl_buffer>*>(data); nit: maybe cleaner to just pass BufferState* as data? https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:258: LOG(ERROR) << "Linux buffer params failed callback"; nit: s/failed callback/failed/ https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:267: if (base::CommandLine::ForCurrentProcess()->HasSwitch("linux_dmabuf_v1")) { s/linux_dmabuf_v1/use-drm/ and move the string to a constant above similar to how we maintain switches in chrome https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:268: #if defined(OZONE_PLATFORM_GBM) move the switch check into this ifdef and just silently ignore it here in the non-GBM case. If we want to bail out if flag is used without gbm then let's do that as early as possible. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:280: zwp_linux_buffer_params_v1* params = are we leaking this object? https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:287: wl_display_roundtrip(display); please move this round trip so we only do it once after creating all buffers. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:294: const EGLint khr_image_attrs[] = {EGL_DMA_BUF_PLANE0_FD_EXT, nit: not sure about 'const' here https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:310: NULL /* no client buffer */, khr_image_attrs); s/NULL/nullptr/ https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:315: (GLeglImageOES)buffer_state->egl_image); static_cast https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:345: bool use_dmabuf = use_drm and maybe a good idea to add this to the main loop context. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:394: base::ScopedFD fd(open("/dev/dri/renderD128", O_RDWR)); lets move "/dev/dri/renderD128" to a constant at the top of the file. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:395: if (fd.get() < 0) { we probably want this in the GBM ifdef below or it will fail on linux in some cases
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:40001) has been deleted
PTAL https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... File components/exo/wayland/BUILD.gn (right): https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/BUILD.gn:108: "-lGLESv2", On 2016/11/04 at 22:34:14, reveman wrote: > do we need these? No, removed. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:128: std::unique_ptr<gbm_bo> bo = nullptr; On 2016/11/04 at 22:34:14, reveman wrote: > remove " = nullptr" Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:130: EGLImageKHR egl_image = 0; On 2016/11/04 at 22:34:15, reveman wrote: > can we have a unique_ptr for this too? Added a ScopedEglImage that is slightly nicer than the unique_ptr since EGLImageKHR is defined as a void* and the destructor needs another parameter. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:131: unsigned gl_texture = 0; On 2016/11/04 at 22:34:14, reveman wrote: > s/gl_texture/texture/ > > can we use GLuint instead of unsigned? and can we add a ScopedTexture wrapper so we can use unique_ptr? Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:132: unsigned fb = 0; On 2016/11/04 at 22:34:14, reveman wrote: > s/fb/framebuffer/ > > and GLuint + ScopedFramebuffer as above This is gone now that we're using skia. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:253: static_cast<std::unique_ptr<wl_buffer>*>(data); On 2016/11/04 at 22:34:14, reveman wrote: > nit: maybe cleaner to just pass BufferState* as data? Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:258: LOG(ERROR) << "Linux buffer params failed callback"; On 2016/11/04 at 22:34:14, reveman wrote: > nit: s/failed callback/failed/ Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:267: if (base::CommandLine::ForCurrentProcess()->HasSwitch("linux_dmabuf_v1")) { On 2016/11/04 at 22:34:14, reveman wrote: > s/linux_dmabuf_v1/use-drm/ and move the string to a constant above similar to how we maintain switches in chrome Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:268: #if defined(OZONE_PLATFORM_GBM) On 2016/11/04 at 22:34:14, reveman wrote: > move the switch check into this ifdef and just silently ignore it here in the non-GBM case. If we want to bail out if flag is used without gbm then let's do that as early as possible. Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:280: zwp_linux_buffer_params_v1* params = On 2016/11/04 at 22:34:14, reveman wrote: > are we leaking this object? Fixed. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:287: wl_display_roundtrip(display); On 2016/11/04 at 22:34:14, reveman wrote: > please move this round trip so we only do it once after creating all buffers. Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:294: const EGLint khr_image_attrs[] = {EGL_DMA_BUF_PLANE0_FD_EXT, On 2016/11/04 at 22:34:14, reveman wrote: > nit: not sure about 'const' here Removed. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:315: (GLeglImageOES)buffer_state->egl_image); On 2016/11/04 at 22:34:14, reveman wrote: > static_cast Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:345: bool use_dmabuf = On 2016/11/04 at 22:34:14, reveman wrote: > use_drm and maybe a good idea to add this to the main loop context. Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:394: base::ScopedFD fd(open("/dev/dri/renderD128", O_RDWR)); On 2016/11/04 at 22:34:14, reveman wrote: > lets move "/dev/dri/renderD128" to a constant at the top of the file. Done. https://codereview.chromium.org/2477043002/diff/20001/components/exo/wayland/... components/exo/wayland/clients/motion_events.cc:395: if (fd.get() < 0) { On 2016/11/04 at 22:34:14, reveman wrote: > we probably want this in the GBM ifdef below or it will fail on linux in some cases Done.
Description was changed from ========== exo: Add drm support to motion_events. Add support to render using dmabufs to motion_events wayland test client. BUG= TEST=XDG_RUNTIME_DIR=/var/run/chrome /tmp/wayland_motion_events --linux_dmabuf_v1 on kevin. ========== to ========== exo: Add drm support to motion_events. Add support to render using dmabufs to motion_events wayland test client. BUG=661010 TEST=XDG_RUNTIME_DIR=/var/run/chrome /tmp/wayland_motion_events --linux_dmabuf_v1 on kevin. ==========
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:140001) has been deleted
lgtm with nits https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... File components/exo/wayland/clients/motion_events.cc (left): https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:221: nit: keep this blank line https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:53: #if defined(OZONE_PLATFORM_GBM) nit: move ifdef-ed deleters below other deleters, or kill the blankline at 57 https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:88: // DRI render node path nit: s/path/path./ https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:148: ScopedEglImage(int fd, EGLint khr_image_attrs[]) { fd is unused https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:151: NULL /* no client buffer */, khr_image_attrs); s/NULL/nullptr/ https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:155: }; Let's use base::ScopedGeneric for EGLImage and texture as discussed. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:155: }; Let's use base::ScopedGeneric for EGLImage and texture as discussed. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:161: bool busy = false; nit: move these 3 below https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:164: nit: remove this blankline https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:171: sk_sp<SkImage> sk_image; unused https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:289: static GrGLFuncPtr egl_get_gl_proc(void* ctx, const char name[]) { nit: remove static, move to unnamed namespace, s/egl_get_gl_proc/GrGLGetProcEGL/? https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:290: GrGLFuncPtr ptr = eglGetProcAddress(name); nit: remove temporary variable https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:290: GrGLFuncPtr ptr = eglGetProcAddress(name); nit: remove temporary variable https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:340: sk_sp<GrContext> gr_context_; nit: blankline after this https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:391: LOG(ERROR) << "Can't open drm device '" << kDriRenderNode << ";"; s/";"/"'"/ https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:448: wl_shell_surface_set_title(shell_surface.get(), "Test Client"); nit: while here, can you change this to "Motion Event Client"? https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:448: wl_shell_surface_set_title(shell_surface.get(), "Test Client"); nit: while here, can you change this to "Motion Event Client"? and maybe add a constant for it https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:448: wl_shell_surface_set_title(shell_surface.get(), "Test Client"); while here, "Motion Event Client" and maybe add a constant for this at the top https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:556: glFinish(); maybe move this into the if-scope as it's a bit weird to call this for each frame when using sw raster..
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... File components/exo/wayland/clients/motion_events.cc (left): https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:221: On 2016/11/06 at 22:18:21, reveman wrote: > nit: keep this blank line Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:53: #if defined(OZONE_PLATFORM_GBM) On 2016/11/06 at 22:18:21, reveman wrote: > nit: move ifdef-ed deleters below other deleters, or kill the blankline at 57 Moved below. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:88: // DRI render node path On 2016/11/06 at 22:18:20, reveman wrote: > nit: s/path/path./ Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:148: ScopedEglImage(int fd, EGLint khr_image_attrs[]) { On 2016/11/06 at 22:18:21, reveman wrote: > fd is unused Gone with the ScopedEglImage. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:151: NULL /* no client buffer */, khr_image_attrs); On 2016/11/06 at 22:18:20, reveman wrote: > s/NULL/nullptr/ Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:155: }; On 2016/11/06 at 22:18:20, reveman wrote: > Let's use base::ScopedGeneric for EGLImage and texture as discussed. Ok. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:161: bool busy = false; On 2016/11/06 at 22:18:21, reveman wrote: > nit: move these 3 below Moved below. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:164: On 2016/11/06 at 22:18:20, reveman wrote: > nit: remove this blankline Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:171: sk_sp<SkImage> sk_image; On 2016/11/06 at 22:18:21, reveman wrote: > unused Removed. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:289: static GrGLFuncPtr egl_get_gl_proc(void* ctx, const char name[]) { On 2016/11/06 at 22:18:21, reveman wrote: > nit: remove static, move to unnamed namespace, s/egl_get_gl_proc/GrGLGetProcEGL/? Rmoved this function for a lambda. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:290: GrGLFuncPtr ptr = eglGetProcAddress(name); On 2016/11/06 at 22:18:21, reveman wrote: > nit: remove temporary variable Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:340: sk_sp<GrContext> gr_context_; On 2016/11/06 at 22:18:21, reveman wrote: > nit: blankline after this Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:391: LOG(ERROR) << "Can't open drm device '" << kDriRenderNode << ";"; On 2016/11/06 at 22:18:20, reveman wrote: > s/";"/"'"/ Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:448: wl_shell_surface_set_title(shell_surface.get(), "Test Client"); On 2016/11/06 at 22:18:21, reveman wrote: > while here, "Motion Event Client" and maybe add a constant for this at the top Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:556: glFinish(); On 2016/11/06 at 22:18:21, reveman wrote: > maybe move this into the if-scope as it's a bit weird to call this for each frame when using sw raster.. Done. https://codereview.chromium.org/2477043002/diff/180001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:611: glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width_, height_, 0, GL_RGBA, Removed the TexImage2D that was useless.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:147: typedef base::ScopedGeneric<GLuint, DeleteTextureTraits> ScopedTexture; nit: using ScopedTexture = ... https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:155: typedef base::ScopedGeneric<EGLImageKHR, DeleteEglImageTraits> ScopedEglImage; nit: using ScopedEglImage = ... https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:578: LOG(ERROR) << "Can't create gbm buffer."; s/buffer./buffer/ for consistency https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:627: desc.fConfig = kBGRA_8888_GrPixelConfig; nit: move to constant at the top with other BGRA constants https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:651: static_cast<uint8_t*>(buffer->shared_memory.memory()), stride()); nit: add a DCHECK(buffer->sk_surface) here too
https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... File components/exo/wayland/clients/motion_events.cc (right): https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:147: typedef base::ScopedGeneric<GLuint, DeleteTextureTraits> ScopedTexture; On 2016/11/07 at 02:44:34, reveman wrote: > nit: using ScopedTexture = ... Done. https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:155: typedef base::ScopedGeneric<EGLImageKHR, DeleteEglImageTraits> ScopedEglImage; On 2016/11/07 at 02:44:34, reveman wrote: > nit: using ScopedEglImage = ... Done. https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:289: This line is gone. https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:578: LOG(ERROR) << "Can't create gbm buffer."; On 2016/11/07 at 02:44:34, reveman wrote: > s/buffer./buffer/ for consistency Done. https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:627: desc.fConfig = kBGRA_8888_GrPixelConfig; On 2016/11/07 at 02:44:34, reveman wrote: > nit: move to constant at the top with other BGRA constants Done. https://codereview.chromium.org/2477043002/diff/220001/components/exo/wayland... components/exo/wayland/clients/motion_events.cc:651: static_cast<uint8_t*>(buffer->shared_memory.memory()), stride()); On 2016/11/07 at 02:44:34, reveman wrote: > nit: add a DCHECK(buffer->sk_surface) here too Done.
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2477043002/#ps240001 (title: "Last round of nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== exo: Add drm support to motion_events. Add support to render using dmabufs to motion_events wayland test client. BUG=661010 TEST=XDG_RUNTIME_DIR=/var/run/chrome /tmp/wayland_motion_events --linux_dmabuf_v1 on kevin. ========== to ========== exo: Add drm support to motion_events. Add support to render using dmabufs to motion_events wayland test client. BUG=661010 TEST=XDG_RUNTIME_DIR=/var/run/chrome /tmp/wayland_motion_events --linux_dmabuf_v1 on kevin. Committed: https://crrev.com/67808baf1012d79550204d4cb25c4e3a26caa416 Cr-Commit-Position: refs/heads/master@{#430224} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/67808baf1012d79550204d4cb25c4e3a26caa416 Cr-Commit-Position: refs/heads/master@{#430224}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:240001) has been created in https://codereview.chromium.org/2481713002/ by perkj@chromium.org. The reason for reverting is: Seems to break PrecacheFetcherTest.DailyQuot componnentest on Mac 10.9 and 10.10 https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/31355. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
