Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1709443002:
Clear IOSurfaces immediately after creating them. (Closed)
DescriptionClear IOSurfaces immediately after creating them.
Calling IOSurfaceLock() followed by IOSurfaceUnlock() appears sufficient.
BUG=584760
Committed: https://crrev.com/c789639d94066ccc3c935d31b8c6a756815fbb8a
Cr-Commit-Position: refs/heads/master@{#376054}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comments from reveman@. #Patch Set 3 : Fix flag. #
Total comments: 2
Patch Set 4 : Comments from reveman. #
Total comments: 4
Patch Set 5 : Comments from avi. #Messages
Total messages: 30 (13 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709443002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709443002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
erikchen@chromium.org changed reviewers: + reveman@chromium.org
reveman: Please review No more failures on the vms, after switching the test to render with shaders/textures.
https://codereview.chromium.org/1709443002/diff/1/ui/gfx/mac/io_surface.cc File ui/gfx/mac/io_surface.cc (right): https://codereview.chromium.org/1709443002/diff/1/ui/gfx/mac/io_surface.cc#ne... ui/gfx/mac/io_surface.cc:154: // https://code.google.com/p/chromium/issues/detail?id=584760#c17 This bug cannot be accessed outside google. Can we include an explanation here instead of just referring to the bug? https://codereview.chromium.org/1709443002/diff/1/ui/gfx/mac/io_surface.cc#ne... ui/gfx/mac/io_surface.cc:155: IOReturn r = IOSurfaceLock(surface, kIOSurfaceLockAvoidSync, NULL); I feel like I might have asked this before so sorry if I'm repeating myself. Can we just do this here instead: IOReturn r = IOSurfaceLock(surface, 0, nullptr); DCHECK_EQ(r, kIOReturnSuccess); IOSurfaceUnlock(surface, 0, nullptr); I'd prefer to keep this as simple as possible and avoid shipping code that is not tested by the unit test you added. https://codereview.chromium.org/1709443002/diff/1/ui/gfx/mac/io_surface.cc#ne... ui/gfx/mac/io_surface.cc:158: IOSurfaceUnlock(surface, kIOSurfaceLockAvoidSync, NULL); nit: nullptr here and above
https://codereview.chromium.org/1709443002/diff/1/ui/gfx/mac/io_surface.cc File ui/gfx/mac/io_surface.cc (right): https://codereview.chromium.org/1709443002/diff/1/ui/gfx/mac/io_surface.cc#ne... ui/gfx/mac/io_surface.cc:154: // https://code.google.com/p/chromium/issues/detail?id=584760#c17 On 2016/02/17 18:37:37, reveman wrote: > This bug cannot be accessed outside google. Can we include an explanation here > instead of just referring to the bug? Done. https://codereview.chromium.org/1709443002/diff/1/ui/gfx/mac/io_surface.cc#ne... ui/gfx/mac/io_surface.cc:155: IOReturn r = IOSurfaceLock(surface, kIOSurfaceLockAvoidSync, NULL); On 2016/02/17 18:37:37, reveman wrote: > I feel like I might have asked this before so sorry if I'm repeating myself. > > Can we just do this here instead: > > IOReturn r = IOSurfaceLock(surface, 0, nullptr); > DCHECK_EQ(r, kIOReturnSuccess); > IOSurfaceUnlock(surface, 0, nullptr); > > I'd prefer to keep this as simple as possible and avoid shipping code that is > not tested by the unit test you added. Done. https://codereview.chromium.org/1709443002/diff/1/ui/gfx/mac/io_surface.cc#ne... ui/gfx/mac/io_surface.cc:158: IOSurfaceUnlock(surface, kIOSurfaceLockAvoidSync, NULL); On 2016/02/17 18:37:37, reveman wrote: > nit: nullptr here and above Done.
lgtm % typo nit https://codereview.chromium.org/1709443002/diff/40001/ui/gfx/mac/io_surface.cc File ui/gfx/mac/io_surface.cc (right): https://codereview.chromium.org/1709443002/diff/40001/ui/gfx/mac/io_surface.c... ui/gfx/mac/io_surface.cc:153: // Zero-initialize the texture. Calling IOSurfaceLock/IOSurfaceUnlock appears nit: s/texture/IOSurface/
https://codereview.chromium.org/1709443002/diff/40001/ui/gfx/mac/io_surface.cc File ui/gfx/mac/io_surface.cc (right): https://codereview.chromium.org/1709443002/diff/40001/ui/gfx/mac/io_surface.c... ui/gfx/mac/io_surface.cc:153: // Zero-initialize the texture. Calling IOSurfaceLock/IOSurfaceUnlock appears On 2016/02/17 19:21:51, reveman wrote: > nit: s/texture/IOSurface/ Done.
erikchen@chromium.org changed reviewers: + kbr@chromium.org
kbr: Please review ui/gl/test/gl_image_test_template.h.
reveman@chromium.org changed reviewers: - kbr@chromium.org
Btw, the unit test doesn't do exactly what you expect right now. The code was broken before your test though and I'm just about to upload a fix for it. Maybe we should hold off on landing this until we've landed a fix for the test. Currently the test will only check that the first texel is initialized to zero and not the rest of the texture.
kbr@chromium.org changed reviewers: + kbr@chromium.org
lgtm
On 2016/02/17 19:31:56, Ken Russell wrote: > lgtm reveman's CL has landed: https://codereview.chromium.org/1708733002/
The CQ bit was checked by erikchen@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/1709443002/#ps60001 (title: "Comments from reveman.")
The CQ bit was unchecked by erikchen@chromium.org
erikchen@chromium.org changed reviewers: + avi@chromium.org
avi: Please review ui/gfx/mac/io_surface.cc
LGTM with nits. https://codereview.chromium.org/1709443002/diff/60001/ui/gfx/mac/io_surface.cc File ui/gfx/mac/io_surface.cc (right): https://codereview.chromium.org/1709443002/diff/60001/ui/gfx/mac/io_surface.c... ui/gfx/mac/io_surface.cc:155: // https://code.google.com/p/chromium/issues/detail?id=584760#c17 You can abbreviate it as https://crbug.com/584760#c17 and put it on the previous line. https://codereview.chromium.org/1709443002/diff/60001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1709443002/diff/60001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:173: // https://crbug.com/584760. Drop the comment? It's no longer accurate.
https://codereview.chromium.org/1709443002/diff/60001/ui/gfx/mac/io_surface.cc File ui/gfx/mac/io_surface.cc (right): https://codereview.chromium.org/1709443002/diff/60001/ui/gfx/mac/io_surface.c... ui/gfx/mac/io_surface.cc:155: // https://code.google.com/p/chromium/issues/detail?id=584760#c17 On 2016/02/17 23:07:25, Avi wrote: > You can abbreviate it as > > https://crbug.com/584760#c17 > > and put it on the previous line. Done. https://codereview.chromium.org/1709443002/diff/60001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1709443002/diff/60001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:173: // https://crbug.com/584760. On 2016/02/17 23:07:25, Avi wrote: > Drop the comment? It's no longer accurate. Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, avi@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1709443002/#ps80001 (title: "Comments from avi.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709443002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709443002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Clear IOSurfaces immediately after creating them. Calling IOSurfaceLock() followed by IOSurfaceUnlock() appears sufficient. BUG=584760 ========== to ========== Clear IOSurfaces immediately after creating them. Calling IOSurfaceLock() followed by IOSurfaceUnlock() appears sufficient. BUG=584760 Committed: https://crrev.com/c789639d94066ccc3c935d31b8c6a756815fbb8a Cr-Commit-Position: refs/heads/master@{#376054} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c789639d94066ccc3c935d31b8c6a756815fbb8a Cr-Commit-Position: refs/heads/master@{#376054} |
