Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2328463004:
Implement WebGL's commit on the main thread (Closed)
|
Created:
4 years, 3 months ago by xidachen Modified:
4 years, 3 months ago Reviewers:
Ken Russell (switch to Gerrit), dcheng, piman, danakj, Justin Novosad, Fady Samuel, xlai (Olivia) CC:
jbroman, ajuma+watch_chromium.org, bajones, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chrishtr, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), haraken, pdr+graphicswatchlist_chromium.org, piman+watch_chromium.org, rwlbuis, Stephen Chennney, Zhenyao Mo Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement WebGL's commit on the main thread.
This CL implements WebGL's commit() API. It uses the frame work established
for the OffscreenCanvasRenderingContext2D. The basic procedure is to
get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The
StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu
mailbox, and a gpu token. We then prepare a CompositorFrame from the
gpu mailbox and the gpu token. It currently works on the main thread
only, making it work on worker thread will come in another CL.
A gpu pixel test is added to make sure that the commit actually commits
to the original HTMLCanvasElement. A layout test is added to ensure
that when an OffscreenCanvas is not constructed from transferControlToOffscreen,
then InvalidStateError must be thrown.
BUG=563852, 619136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/6267a77aa199671dce58a41639b40021db18f7d6
Cr-Commit-Position: refs/heads/master@{#419771}
Patch Set 1 #Patch Set 2 : written as a gpu pixel test #
Total comments: 14
Patch Set 3 : address junov@'s comments #
Total comments: 16
Patch Set 4 : address comments #
Total comments: 11
Patch Set 5 : browser-->renderer IPC for sending back resourceId is ready #Patch Set 6 : rebase #
Total comments: 12
Patch Set 7 : address comments #
Total comments: 13
Patch Set 8 : address comments #
Total comments: 1
Patch Set 9 : update TestExpectation and address junov@'s comments #Patch Set 10 : update gpu pixel test expectation #
Total comments: 4
Patch Set 11 : update trace_test expectations #Patch Set 12 : address dcheng@'s nits #Patch Set 13 : using cc::mojom::MojoCompositorFrameSink #
Total comments: 2
Patch Set 14 : remove unused interface #
Total comments: 1
Patch Set 15 : switch para ordering #
Total comments: 4
Patch Set 16 : nits #Patch Set 17 : fix compile error on win_dbg #Messages
Total messages: 81 (26 generated)
Description was changed from ========== Implement WebGL's commit This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. A layout test is added to make sure that the commit actually commits to the original HTMLCanvasElement. Another layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852 ========== to ========== Implement WebGL's commit This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. A layout test is added to make sure that the commit actually commits to the original HTMLCanvasElement. Another layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
xidachen@chromium.org changed reviewers: + junov@chromium.org
junov@: please take a look before I send it out to broader audience.
Description was changed from ========== Implement WebGL's commit This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. A layout test is added to make sure that the commit actually commits to the original HTMLCanvasElement. Another layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A layout test is added to make sure that the commit actually commits to the original HTMLCanvasElement. Another layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
junov@chromium.org changed reviewers: + kbr@google.com
+kbr for modules/webgl OWNER +danakj to look at use of cc stuff in OffscreenCanvasFrameDispatcherImpl https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:654: if (!getOffscreenCanvas()) I think this case should throw as well. https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:76: unsigned m_textureId = 0; Do we really need this? This texture Id is not usable outside of the WebGL context. https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:54: cc::TransferableResource resource; Here, we should probably assert that 'image' is gpu-backed https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:55: resource.id = image->getTextureId(); I suspect is wrong. The texture Id is useless outside of the GL context it belongs to. Are you sure this is what the resource.id field is meant to be used for? https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:56: resource.format = cc::ResourceFormat::RGBA_8888; we should be extracting format from 'image' instead of using a hard coded value. https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:57: resource.filter = GL_LINEAR; Please add a TODO and create a crbug for this: the filter needs to respect the image-rendering CSS property of the associated canvas element. https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:74: const bool nearestNeighbor = false; TODO: needs to be true when using style "image-rendering: pixelated"
xidachen@chromium.org changed reviewers: + danakj@chromium.org, kbr@chromium.org - kbr@google.com
adding danakj@ to reviewer list. ccing zmo@ and bajones@. https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:654: if (!getOffscreenCanvas()) On 2016/09/09 19:10:50, Justin Novosad wrote: > I think this case should throw as well. Done. https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:76: unsigned m_textureId = 0; On 2016/09/09 19:10:50, Justin Novosad wrote: > Do we really need this? This texture Id is not usable outside of the WebGL > context. But don't we need something like: gl->DeleteTextures(1, &texture_id); at some point after SubmitCompositorFrame? https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:54: cc::TransferableResource resource; On 2016/09/09 19:10:51, Justin Novosad wrote: > Here, we should probably assert that 'image' is gpu-backed Done. https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:55: resource.id = image->getTextureId(); On 2016/09/09 19:10:50, Justin Novosad wrote: > I suspect is wrong. The texture Id is useless outside of the GL context it > belongs to. Are you sure this is what the resource.id field is meant to be used > for? I am not very sure. I looked at this example: https://cs.chromium.org/chromium/src/services/ui/demo/bitmap_uploader.cc?q=bi... In the above example, it kept textureId in a hash map, and call gl->DeleteTextures(1, &texture_id); in a OnResourcesReturned() function call. danakj@: comments? https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:56: resource.format = cc::ResourceFormat::RGBA_8888; On 2016/09/09 19:10:51, Justin Novosad wrote: > we should be extracting format from 'image' instead of using a hard coded value. My impression was that this mailbox is a resource from GPU, and that is always in RGBA_8888 format? https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:57: resource.filter = GL_LINEAR; On 2016/09/09 19:10:51, Justin Novosad wrote: > Please add a TODO and create a crbug for this: the filter needs to respect the > image-rendering CSS property of the associated canvas element. Done. https://codereview.chromium.org/2328463004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:74: const bool nearestNeighbor = false; On 2016/09/09 19:10:50, Justin Novosad wrote: > TODO: needs to be true when using style "image-rendering: pixelated" Done.
xidachen@chromium.org changed reviewers: + xlai@chromium.org
also adding xlai@.
https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:655: exceptionState.throwDOMException(InvalidStateError, "No OffscreenCanvas exist, abort commit()."); Could you add a sub-test to OffscreenCanvas-commit-invalid-call.html for this exception msg? https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:660: exceptionState.throwDOMException(InvalidStateError, "The OffscreenCanvas has no associated HTMLCanvasElement, abort commit()."); Could you change the exception message to "Commit() was called on a context whose OffscreenCanvas is not associated with a canvas element."? This is to be consistent with the 2d context error msg.
https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:31: PassRefPtr<AcceleratedStaticBitmapImage> AcceleratedStaticBitmapImage::createFromWebGLContextImage(sk_sp<SkImage> image, const gpu::Mailbox& mailbox, const gpu::SyncToken& syncToken, GLuint textureId) "textureId" for what? Can you make this name explain more https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:30: static PassRefPtr<AcceleratedStaticBitmapImage> createFromWebGLContextImage(sk_sp<SkImage>, const gpu::Mailbox&, const gpu::SyncToken&, GLuint); please name variables when the name isn't obvious from the type. GLuint provides no info about what it is. https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:75: // keep textureId which is required when preparing cc::TransferableResource Please use capitalizion and punctuation, as all comments should be a proper sentence. Also this comment isn't really explaining things I feel like. Can you say what this texture id is instead? Is it the texture id that is inside the SkImage? You can just say that? What context does this texture id belong to? The context used for the SkImage? The main thread context always? Can you group this member closer to the things it relates to? https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:56: resource.id = image->getTextureId(); The id is not the texture id, it's a ResourceId from ResourceProvider. You can just invent a unique ID if you don't want to use ResourceProvider. The browser will use that number when giving the resource back so you can find it again. This probably means you don't need a texture id in any of the staticbitmapimage stuff. Also, speaking of, where is the browser returning resources? Are you keeping the resource alive while the browser has it somehow? Where are we keeping the |image| alive until then? Did you decide not to use ResourceProvider for a specific reason? It helps with these things. https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:64: resource.is_overlay_candidate = false; Maybe TODO to make these overlay-able? https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:21: void dispatchFrame(PassRefPtr<StaticBitmapImage>) override; Are we still using PassRefPtr in new code or should it just be RefPtrs yet?
Great work and good tests. This looks good overall, but I don't know cc's APIs, so defer to danakj to review that code. A couple of minor comments. https://codereview.chromium.org/2328463004/diff/40001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2328463004/diff/40001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:36: ['win', 'mac', 'linux', 'android'], bug=563852) You can just leave out the OS conditions; they're optional. See the definition of Fail: https://cs.chromium.org/chromium/src/content/test/gpu/gpu_tests/test_expectat... So: self.Fail('Pixel.OffscreenCanvasWebGLGreenBox', bug=563852) https://codereview.chromium.org/2328463004/diff/40001/content/test/gpu/page_s... File content/test/gpu/page_sets/pixel_tests.py (right): https://codereview.chromium.org/2328463004/diff/40001/content/test/gpu/page_s... content/test/gpu/page_sets/pixel_tests.py:173: revision=7, revision=1. This is the first revision of this particular test. https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:655: exceptionState.throwDOMException(InvalidStateError, "No OffscreenCanvas exist, abort commit()."); Also, nit: exist -> exists https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:46: unsigned getTextureId() final { return m_textureId; } Should these use the override keyword? It'd be a useful hint that these are virtual. https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:64: resource.is_overlay_candidate = false; On 2016/09/09 20:51:42, danakj wrote: > Maybe TODO to make these overlay-able? Second that notion -- it'd be ideal to have these canvases go down the same compositing path as normally-rendered HTMLCanvasElements. Fine to do this as a TODO. The fact that the canvas element had transferControlToOffscreen called on it should be a good hint that OffscreenCanvas should use the CHROMIUM_image allocation path (which I assume is needed for this resource to be promoted to an overlay) for its color buffer. https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:71: const bool premultipliedAlpha = true; Is there a TODO to inherit this from the WebGL context's creation settings?
https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:46: unsigned getTextureId() final { return m_textureId; } On 2016/09/09 21:38:17, Ken Russell wrote: > Should these use the override keyword? It'd be a useful hint that these are > virtual. final implies virtual "A virt-specifier-seq shall appear only in the declaration of a virtual member function (10.3)."
https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:46: unsigned getTextureId() final { return m_textureId; } On 2016/09/09 21:43:55, danakj wrote: > On 2016/09/09 21:38:17, Ken Russell wrote: > > Should these use the override keyword? It'd be a useful hint that these are > > virtual. > > final implies virtual > > "A virt-specifier-seq shall appear only in the declaration of a virtual member > function (10.3)." Ah. Thanks for the info.
danakj@: I have addressed most of the comments. I believe there is just last one piece: what is the resource id and how do we handle it? While I was searching for existing code that does prepare a compositor frame, I found this good example: https://cs.chromium.org/chromium/src/services/ui/demo/bitmap_uploader.cc?q=bi... In the above example, it uses a arbitrary number for resource.id, and in their OnResourcesReturned() function call, they do: resource_to_texture_id_map_.erase(resource.id); In my case, I saw that this method: SurfaceResourceHolder::UnrefResources() does the: resource_id_info_map_.erase(count_it); And that gives me the impression that the resource id is automatically erase from the map when it reaches the UnrefResources() call. Am I missing something? For example, do I need to explicitly delete the texture at some point?
On 2016/09/09 20:51:42, danakj wrote: > > Also, speaking of, where is the browser returning resources? Are you keeping the > resource alive while the browser has it somehow? Where are we keeping the > |image| alive until then? > What we need to do here is keep |image| alive on the renderer side at least until the renderer receives confirmation that the mailbox has been consumed by the browser, otherwise you have a race condition that could cause the texture to be deleted before it reaches the browser. In the case where |image| is backed by a texture managed by skia, you need to keep |image| alive until the resource is released by the browser to prevent skia from recycling the texture object while the browser is still using it.
https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:655: exceptionState.throwDOMException(InvalidStateError, "No OffscreenCanvas exists, abort commit()."); Error message would be more readable like this "Commit() was called on a rendering context that was no created from an OffscreenCanvas." https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:665: getOffscreenCanvas()->getOrCreateFrameDispatcher()->dispatchFrame(drawingBuffer()->transferToStaticBitmapImage()); Race condition right here. The return value of drawingBuffer()->transferToStaticBitmapImage() needs to be retained to prevent the texture from being deleted before the browser consumes it. It should probably be retained in the Dispatcher. The texture also needs to be locked (read-only in the renderer) until the browser returns it holding on to it in SkImage form should do the trick. Another problem is that this code will not work correctly if the webgl context was created with the "preserveDrawingBuffer: true" context creation attribute. No need to fix the preserveDrawingBuffer issue in this CL, but at least create a crbug and reference it in a TODO right here. https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:33: void OffscreenCanvasFrameDispatcherImpl::dispatchFrame(PassRefPtr<StaticBitmapImage> image) PassRefPtr is old school. Use RefPtr, and std::move at the call site. https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:62: resource.mailbox_holder = gpu::MailboxHolder(image->getMailbox(), image->getSyncToken(), GL_TEXTURE_2D); Should we call ensureMailbox first? I think there would be no harm in that and it would guard against possible future bugs (with 2D canvas). https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:73: // TOOD(xidachen): this should be inherited from WebGL context's creation settings. crbug? https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:44: virtual unsigned getTextureId() { return 0; } Is this still needed?
Handling the returned resources has been added to OffscreenCanvasFrameDispatcherImpl class. PTAL. https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:655: exceptionState.throwDOMException(InvalidStateError, "No OffscreenCanvas exists, abort commit()."); On 2016/09/12 13:53:25, Justin Novosad wrote: > Error message would be more readable like this "Commit() was called on a > rendering context that was no created from an OffscreenCanvas." Done. https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:33: void OffscreenCanvasFrameDispatcherImpl::dispatchFrame(PassRefPtr<StaticBitmapImage> image) On 2016/09/12 13:53:25, Justin Novosad wrote: > PassRefPtr is old school. Use RefPtr, and std::move at the call site. Done. https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:62: resource.mailbox_holder = gpu::MailboxHolder(image->getMailbox(), image->getSyncToken(), GL_TEXTURE_2D); On 2016/09/12 13:53:25, Justin Novosad wrote: > Should we call ensureMailbox first? I think there would be no harm in that and > it would guard against possible future bugs (with 2D canvas). Done. https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:73: // TOOD(xidachen): this should be inherited from WebGL context's creation settings. On 2016/09/12 13:53:25, Justin Novosad wrote: > crbug? Done. https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:44: virtual unsigned getTextureId() { return 0; } On 2016/09/12 13:53:25, Justin Novosad wrote: > Is this still needed? Done.
On Sat, Sep 10, 2016 at 11:03 AM, <xidachen@chromium.org> wrote: > danakj@: I have addressed most of the comments. I believe there is just > last one > piece: what is the resource id and how do we handle it? > > While I was searching for existing code that does prepare a compositor > frame, I > found this good example: > https://cs.chromium.org/chromium/src/services/ui/demo/ > bitmap_uploader.cc?q=bitmap_upload&sq=package:chromium&l=73 > > In the above example, it uses a arbitrary number for resource.id, and in > their > OnResourcesReturned() function call, they do: > resource_to_texture_id_map_.erase(resource.id); > > In my case, I saw that this method: SurfaceResourceHolder::UnrefResources() > does > the: > resource_id_info_map_.erase(count_it); > And that gives me the impression that the resource id is automatically > erase > from the map when it reaches the UnrefResources() call. > > Am I missing something? For example, do I need to explicitly delete the > texture > at some point? > Yes. You need to keep the texture alive until the resource id is returned from the parent compositor. That's why it is erasing things from the map based on the resource id in the example. The id is what is returned from ResourceProvider, and I think you should use ResourceProvider. It's easier then also. You delete the texture id when the ReleaseCallback you handed to the ResourceProvider is called (or recycle it for use again). -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Sat, Sep 10, 2016 at 11:03 AM, <xidachen@chromium.org> wrote: > danakj@: I have addressed most of the comments. I believe there is just > last one > piece: what is the resource id and how do we handle it? > > While I was searching for existing code that does prepare a compositor > frame, I > found this good example: > https://cs.chromium.org/chromium/src/services/ui/demo/ > bitmap_uploader.cc?q=bitmap_upload&sq=package:chromium&l=73 > > In the above example, it uses a arbitrary number for resource.id, and in > their > OnResourcesReturned() function call, they do: > resource_to_texture_id_map_.erase(resource.id); > > In my case, I saw that this method: SurfaceResourceHolder::UnrefResources() > does > the: > resource_id_info_map_.erase(count_it); > And that gives me the impression that the resource id is automatically > erase > from the map when it reaches the UnrefResources() call. > > Am I missing something? For example, do I need to explicitly delete the > texture > at some point? > Yes. You need to keep the texture alive until the resource id is returned from the parent compositor. That's why it is erasing things from the map based on the resource id in the example. The id is what is returned from ResourceProvider, and I think you should use ResourceProvider. It's easier then also. You delete the texture id when the ReleaseCallback you handed to the ResourceProvider is called (or recycle it for use again). -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:59: resource.id = m_nextResourceId; can you store the id you will use in a local var here and increment there, then use the local var throughout. that will be less fragile. https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:64: image->ensureMailbox(); it feels weird that you're doing this here. why are we getting here with an |image| that doesn't have a mailbox yet? https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:98: for (const cc::mojom::blink::ReturnedResourcePtr& resource : resources) use const auto& https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:99: m_cachedImages.remove(resource->id); Does this delete the texture in the StaticBitmapImage? If so can you comment to explain so (if this was the last ref I guess, can other refs exist?). If not you need to delete it at some point, where would it happen? also TODO to recycle the texture and reuse it for future frames?
https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:59: resource.id = m_nextResourceId; On 2016/09/14 02:28:33, danakj wrote: > can you store the id you will use in a local var here and increment there, then > use the local var throughout. that will be less fragile. Done. https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:64: image->ensureMailbox(); On 2016/09/14 02:28:33, danakj wrote: > it feels weird that you're doing this here. why are we getting here with an > |image| that doesn't have a mailbox yet? It was suggested by junov@ that calling this function should be no harm to the current situation, and would prevent future bugs in 2D canvas cases. https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:98: for (const cc::mojom::blink::ReturnedResourcePtr& resource : resources) On 2016/09/14 02:28:33, danakj wrote: > use const auto& Done. https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:99: m_cachedImages.remove(resource->id); On 2016/09/14 02:28:33, danakj wrote: > Does this delete the texture in the StaticBitmapImage? If so can you comment to > explain so (if this was the last ref I guess, can other refs exist?). If not you > need to delete it at some point, where would it happen? > > also TODO to recycle the texture and reuse it for future frames? I believe this is the last ref to the StaticBitmapImage, and no other refs should exist when we come to this point. junov@: I believe that when a StaticBitmapImage is texture backed, then deleting the last ref will delete its associated texture as well. Could you confirm that?
The ref/release logic looks sound IMHO, but it requires unit testing. https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:665: getOffscreenCanvas()->getOrCreateFrameDispatcher()->dispatchFrame(std::move(drawingBuffer()->transferToStaticBitmapImage())); // TODO(crbug.com/xxx): Make commit() work correctly with { preserveDrawingBuffer : true } https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72: // Increment ref to the |image| by 1, to keep it alive until the browser ReturnResources. Not "increment", just "hold" https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:73: m_cachedImages.add(nextResourceId++, image); std::move(image) https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:99: // the StaticBitmapImage also delete the texture associated with it. This comment is not quite correct. The unref does not necessarily cause deletion of the texture: a) there may be other refs b) skia may reclaim the texture into a pool rather than delete it. What *is* guaranteed here is that the resource will be locked (not re-used or deleted) as long as the ref is held. https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:100: // TODO(xidachen): recycle StaticBitmapImage to be used in to be used in other frames Nothing to do here. Skia takes care of this for you. https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:12: #include "third_party/skia/include/core/SkImage.h" Why does this need to be included in the .h?
https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:665: getOffscreenCanvas()->getOrCreateFrameDispatcher()->dispatchFrame(std::move(drawingBuffer()->transferToStaticBitmapImage())); On 2016/09/14 14:28:13, Justin Novosad wrote: > // TODO(crbug.com/xxx): Make commit() work correctly with { > preserveDrawingBuffer : true } Done. https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72: // Increment ref to the |image| by 1, to keep it alive until the browser ReturnResources. On 2016/09/14 14:28:13, Justin Novosad wrote: > Not "increment", just "hold" Done. https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:73: m_cachedImages.add(nextResourceId++, image); On 2016/09/14 14:28:13, Justin Novosad wrote: > std::move(image) Done. https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:99: // the StaticBitmapImage also delete the texture associated with it. On 2016/09/14 14:28:13, Justin Novosad wrote: > This comment is not quite correct. The unref does not necessarily cause > deletion of the texture: a) there may be other refs b) skia may reclaim the > texture into a pool rather than delete it. What *is* guaranteed here is that > the resource will be locked (not re-used or deleted) as long as the ref is held. Done. https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:100: // TODO(xidachen): recycle StaticBitmapImage to be used in to be used in other frames On 2016/09/14 14:28:13, Justin Novosad wrote: > Nothing to do here. Skia takes care of this for you. Done. https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:12: #include "third_party/skia/include/core/SkImage.h" On 2016/09/14 14:28:13, Justin Novosad wrote: > Why does this need to be included in the .h? I tried to remove this line and put it in .cpp, then I got a access to incomplete type compile error. I think it is because the destructor of StaticBitmapImage needs to delete the sk_sp for SkImage, so it needs to include i here.
The CQ bit was checked by xidachen@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/2328463004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:36: void OffscreenCanvasFrameDispatcherImpl::dispatchFrame(RefPtr<StaticBitmapImage> image) One more Q: why is this a StaticBitmapImage instead of an AcceleratedBitmapImage? Do you intend to support passing non-AcceleratedBitmapImage here? https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:64: image->ensureMailbox(); On 2016/09/14 13:06:43, xidachen wrote: > On 2016/09/14 02:28:33, danakj wrote: > > it feels weird that you're doing this here. why are we getting here with an > > |image| that doesn't have a mailbox yet? > > It was suggested by junov@ that calling this function should be no harm to the > current situation, and would prevent future bugs in 2D canvas cases. Ok I'll defer to junov then, it just feels like an enapsulation violation to me to put this here (and maybe to have this method public at all). But I have to admit I'm not super familiar with the classes involved.
LGTM for the use of cc stuff
https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:36: void OffscreenCanvasFrameDispatcherImpl::dispatchFrame(RefPtr<StaticBitmapImage> image) On 2016/09/14 17:32:04, danakj wrote: > One more Q: why is this a StaticBitmapImage instead of an > AcceleratedBitmapImage? Do you intend to support passing > non-AcceleratedBitmapImage here? danakj@: yes, for example, when we support OffscreenCanvasRenderingContext2D.commit(), it can be a StaticBitmap that is in cpu memory, instead of texture backed.
https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2328463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:36: void OffscreenCanvasFrameDispatcherImpl::dispatchFrame(RefPtr<StaticBitmapImage> image) On 2016/09/14 17:35:57, xidachen wrote: > On 2016/09/14 17:32:04, danakj wrote: > > One more Q: why is this a StaticBitmapImage instead of an > > AcceleratedBitmapImage? Do you intend to support passing > > non-AcceleratedBitmapImage here? > > danakj@: yes, for example, when we support > OffscreenCanvasRenderingContext2D.commit(), it can be a StaticBitmap that is in > cpu memory, instead of texture backed. Ok. *if* you can strongly type that (ie make that a different method) that might be nice. If not then makes sense!
xidachen@chromium.org changed reviewers: + dcheng@chromium.org
adding dcheng@ to take a look at change in: offscreen_canvas_surface.mojom
The CQ bit was unchecked by xidachen@chromium.org
The WebGL changes LGTM. Deferring to the others for correctness of the texture transfer code.
https://codereview.chromium.org/2328463004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h (right): https://codereview.chromium.org/2328463004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:26: void ReturnResources(Vector<cc::mojom::blink::ReturnedResourcePtr> resources) override; Hmmm..Is resourceId the only and single item that will be used throughout the whole implementation? Otherwise, can you use the ReturnResource typemap instead of using the Mojom type directly? It's similar to the way compositor_frame.typemap is brought to Blink.
lgtm with nit https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right): https://codereview.chromium.org/2328463004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:12: #include "third_party/skia/include/core/SkImage.h" On 2016/09/14 14:53:51, xidachen wrote: > On 2016/09/14 14:28:13, Justin Novosad wrote: > > Why does this need to be included in the .h? > > I tried to remove this line and put it in .cpp, then I got a access to > incomplete type compile error. I think it is because the destructor of > StaticBitmapImage needs to delete the sk_sp for SkImage, so it needs to include > i here. Then you should define the destructor in the .cpp file
On 2016/09/14 17:54:47, xlai (Olivia) wrote: > https://codereview.chromium.org/2328463004/diff/140001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h > (right): > > https://codereview.chromium.org/2328463004/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:26: > void ReturnResources(Vector<cc::mojom::blink::ReturnedResourcePtr> resources) > override; > Hmmm..Is resourceId the only and single item that will be used throughout the > whole implementation? Otherwise, can you use the ReturnResource typemap instead > of using the Mojom type directly? It's similar to the way > compositor_frame.typemap is brought to Blink. Right now, the ReturnedResource is just a vector of unsigned int, so I think it should be fine.
If you're just using a vector of integer, then I think there's no need to go through the hassle of adding typemap. Current state lgtm. On 2016/09/14 18:28:13, xidachen wrote: > On 2016/09/14 17:54:47, xlai (Olivia) wrote: > > > https://codereview.chromium.org/2328463004/diff/140001/third_party/WebKit/Sou... > > File > > > third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h > > (right): > > > > > https://codereview.chromium.org/2328463004/diff/140001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:26: > > void ReturnResources(Vector<cc::mojom::blink::ReturnedResourcePtr> resources) > > override; > > Hmmm..Is resourceId the only and single item that will be used throughout the > > whole implementation? Otherwise, can you use the ReturnResource typemap > instead > > of using the Mojom type directly? It's similar to the way > > compositor_frame.typemap is brought to Blink. > > Right now, the ReturnedResource is just a vector of unsigned int, so I think it > should be fine.
gaconyeuemnhieu@gmail.com changed reviewers: + gaconyeuemnhieu@gmail.com
https://codereview.chromium.org/2328463004/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc (right): https://codereview.chromium.org/2328463004/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_frame_receiver_impl.cc:38: DCHECK(!client_.get()); Nit: the .get() should be unnecessary https://codereview.chromium.org/2328463004/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom (right): https://codereview.chromium.org/2328463004/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:33: ReturnResources(array<cc.mojom.ReturnedResource> resources); Would it make sense for this to be a generic CC mojo interface? It seems like we already have SurfaceClient and CompositorFrameSinkClient with the exact same interface.
https://codereview.chromium.org/2328463004/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom (right): https://codereview.chromium.org/2328463004/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:33: ReturnResources(array<cc.mojom.ReturnedResource> resources); On 2016/09/14 20:02:19, dcheng wrote: > Would it make sense for this to be a generic CC mojo interface? It seems like we > already have SurfaceClient and CompositorFrameSinkClient with the exact same > interface. Not sure if this is helpful but this here *is* a CompositorFrameSinkClient, so maybe it should use that interface? Maybe it should use CompositorFrameSink too, not sure if this was considered or is possible.
xidachen@chromium.org changed reviewers: + fsamuel@chromium.org - gaconyeuemnhieu@gmail.com
https://codereview.chromium.org/2328463004/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom (right): https://codereview.chromium.org/2328463004/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:33: ReturnResources(array<cc.mojom.ReturnedResource> resources); On 2016/09/14 20:07:56, danakj wrote: > On 2016/09/14 20:02:19, dcheng wrote: > > Would it make sense for this to be a generic CC mojo interface? It seems like > we > > already have SurfaceClient and CompositorFrameSinkClient with the exact same > > interface. > > Not sure if this is helpful but this here *is* a CompositorFrameSinkClient, so > maybe it should use that interface? Maybe it should use CompositorFrameSink too, > not sure if this was considered or is possible. +Fady: WDYT?
I have a slightly different opinion. I just did some checkup on the two mojom files that declare ReturnResources(array<cc.mojom.ReturnedResource> resources); and they are both in src/services/ui/public/interfaces/. In this CL, the implementation of this function requires access to OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that this could hardly go out of scope of Blink. Does it violate Blink/Chromium dependency if we generate a blink-version of surface.mojom (or display_compositor.mojom) and include it in OffscreenCanvasFrameDispatcherImpl so as to implement the function? I seriously doubt it is. Some of the possible solutions: 1) Try in a separate patch that unifies all these ReturnResources mojom call that can be implemented by both Blink/Chromium. 2) In this CL, instead of return full Resources, just return a vector of integers, because that's the minimal we need. This may probably make the IPC more lightweight, I guess? On 2016/09/15 15:56:28, xidachen wrote: > https://codereview.chromium.org/2328463004/diff/180001/third_party/WebKit/pub... > File > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom > (right): > > https://codereview.chromium.org/2328463004/diff/180001/third_party/WebKit/pub... > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:33: > ReturnResources(array<cc.mojom.ReturnedResource> resources); > On 2016/09/14 20:07:56, danakj wrote: > > On 2016/09/14 20:02:19, dcheng wrote: > > > Would it make sense for this to be a generic CC mojo interface? It seems > like > > we > > > already have SurfaceClient and CompositorFrameSinkClient with the exact same > > > interface. > > > > Not sure if this is helpful but this here *is* a CompositorFrameSinkClient, so > > maybe it should use that interface? Maybe it should use CompositorFrameSink > too, > > not sure if this was considered or is possible. > > +Fady: WDYT?
On Thu, Sep 15, 2016 at 11:14 AM, <xlai@chromium.org> wrote: > I have a slightly different opinion. > > I just did some checkup on the two mojom files that declare > ReturnResources(array<cc.mojom.ReturnedResource> resources); and they are > both > in src/services/ui/public/interfaces/. > > In this CL, the implementation of this function requires access to > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that this > could > hardly go out of scope of Blink. > > Does it violate Blink/Chromium dependency if we generate a blink-version > of > surface.mojom (or display_compositor.mojom) and include it in > OffscreenCanvasFrameDispatcherImpl > so as to implement the function? I seriously doubt it is. > > Some of the possible solutions: > 1) Try in a separate patch that unifies all these ReturnResources mojom > call > that can > be implemented by both Blink/Chromium. > 2) In this CL, instead of return full Resources, just return a vector of > integers, because > that's the minimal we need. This may probably make the IPC more > lightweight, I > guess? > I don't think we should do #2. I'm not familiar enough with mojo things to comment on the rest. As long as the IPC gets here I'm happy. > > > > On 2016/09/15 15:56:28, xidachen wrote: > > > https://codereview.chromium.org/2328463004/diff/180001/ > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > canvas_surface.mojom > > File > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > canvas_surface.mojom > > (right): > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > canvas_surface.mojom#newcode33 > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > canvas_surface.mojom:33: > > ReturnResources(array<cc.mojom.ReturnedResource> resources); > > On 2016/09/14 20:07:56, danakj wrote: > > > On 2016/09/14 20:02:19, dcheng wrote: > > > > Would it make sense for this to be a generic CC mojo interface? It > seems > > like > > > we > > > > already have SurfaceClient and CompositorFrameSinkClient with the > exact > same > > > > interface. > > > > > > Not sure if this is helpful but this here *is* a > CompositorFrameSinkClient, > so > > > maybe it should use that interface? Maybe it should use > CompositorFrameSink > > too, > > > not sure if this was considered or is possible. > > > > +Fady: WDYT? > > > > https://codereview.chromium.org/2328463004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Sep 15, 2016 at 11:14 AM, <xlai@chromium.org> wrote: > I have a slightly different opinion. > > I just did some checkup on the two mojom files that declare > ReturnResources(array<cc.mojom.ReturnedResource> resources); and they are > both > in src/services/ui/public/interfaces/. > > In this CL, the implementation of this function requires access to > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that this > could > hardly go out of scope of Blink. > > Does it violate Blink/Chromium dependency if we generate a blink-version > of > surface.mojom (or display_compositor.mojom) and include it in > OffscreenCanvasFrameDispatcherImpl > so as to implement the function? I seriously doubt it is. > > Some of the possible solutions: > 1) Try in a separate patch that unifies all these ReturnResources mojom > call > that can > be implemented by both Blink/Chromium. > 2) In this CL, instead of return full Resources, just return a vector of > integers, because > that's the minimal we need. This may probably make the IPC more > lightweight, I > guess? > I don't think we should do #2. I'm not familiar enough with mojo things to comment on the rest. As long as the IPC gets here I'm happy. > > > > On 2016/09/15 15:56:28, xidachen wrote: > > > https://codereview.chromium.org/2328463004/diff/180001/ > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > canvas_surface.mojom > > File > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > canvas_surface.mojom > > (right): > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > canvas_surface.mojom#newcode33 > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > canvas_surface.mojom:33: > > ReturnResources(array<cc.mojom.ReturnedResource> resources); > > On 2016/09/14 20:07:56, danakj wrote: > > > On 2016/09/14 20:02:19, dcheng wrote: > > > > Would it make sense for this to be a generic CC mojo interface? It > seems > > like > > > we > > > > already have SurfaceClient and CompositorFrameSinkClient with the > exact > same > > > > interface. > > > > > > Not sure if this is helpful but this here *is* a > CompositorFrameSinkClient, > so > > > maybe it should use that interface? Maybe it should use > CompositorFrameSink > > too, > > > not sure if this was considered or is possible. > > > > +Fady: WDYT? > > > > https://codereview.chromium.org/2328463004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/09/15 18:25:51, danakj wrote: > On Thu, Sep 15, 2016 at 11:14 AM, <mailto:xlai@chromium.org> wrote: > > > I have a slightly different opinion. > > > > I just did some checkup on the two mojom files that declare > > ReturnResources(array<cc.mojom.ReturnedResource> resources); and they are > > both > > in src/services/ui/public/interfaces/. > > > > In this CL, the implementation of this function requires access to > > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that this > > could > > hardly go out of scope of Blink. > > > > Does it violate Blink/Chromium dependency if we generate a blink-version > > of > > surface.mojom (or display_compositor.mojom) and include it in > > OffscreenCanvasFrameDispatcherImpl > > so as to implement the function? I seriously doubt it is. > > > > Some of the possible solutions: > > 1) Try in a separate patch that unifies all these ReturnResources mojom > > call > > that can > > be implemented by both Blink/Chromium. > > 2) In this CL, instead of return full Resources, just return a vector of > > integers, because > > that's the minimal we need. This may probably make the IPC more > > lightweight, I > > guess? > > > > I don't think we should do #2. I'm not familiar enough with mojo things to > comment on the rest. As long as the IPC gets here I'm happy. dcheng@, danakj@: I talked to sadrul@, and apparently the CompositorFrameSinkClient is something in display_compositor, and my understanding is that display_compositor is not yet, but will soon be used in Chromium. Could we proceed with current solution of adding a new interface, and later on unify all of them. With a TODO item obviously.
Sure, #2 is just my suggestion to try to alleviate potential concerns of having additional IPC. Actually, #1 may not be implementable, depending on how loose the Blink/Chromium dependencies can become. Regardless of which, I think it should be attempted outside of this CL and shouldn't let that block this CL. Forget to mention I should add #3 solution--that is to keep the current state--if #1 is not doable. On 2016/09/15 18:25:51, danakj wrote: > On Thu, Sep 15, 2016 at 11:14 AM, <mailto:xlai@chromium.org> wrote: > > > I have a slightly different opinion. > > > > I just did some checkup on the two mojom files that declare > > ReturnResources(array<cc.mojom.ReturnedResource> resources); and they are > > both > > in src/services/ui/public/interfaces/. > > > > In this CL, the implementation of this function requires access to > > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that this > > could > > hardly go out of scope of Blink. > > > > Does it violate Blink/Chromium dependency if we generate a blink-version > > of > > surface.mojom (or display_compositor.mojom) and include it in > > OffscreenCanvasFrameDispatcherImpl > > so as to implement the function? I seriously doubt it is. > > > > Some of the possible solutions: > > 1) Try in a separate patch that unifies all these ReturnResources mojom > > call > > that can > > be implemented by both Blink/Chromium. > > 2) In this CL, instead of return full Resources, just return a vector of > > integers, because > > that's the minimal we need. This may probably make the IPC more > > lightweight, I > > guess? > > > > I don't think we should do #2. I'm not familiar enough with mojo things to > comment on the rest. As long as the IPC gets here I'm happy. > > > > > > > > > > On 2016/09/15 15:56:28, xidachen wrote: > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > canvas_surface.mojom > > > File > > > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > canvas_surface.mojom > > > (right): > > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > canvas_surface.mojom#newcode33 > > > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > canvas_surface.mojom:33: > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); > > > On 2016/09/14 20:07:56, danakj wrote: > > > > On 2016/09/14 20:02:19, dcheng wrote: > > > > > Would it make sense for this to be a generic CC mojo interface? It > > seems > > > like > > > > we > > > > > already have SurfaceClient and CompositorFrameSinkClient with the > > exact > > same > > > > > interface. > > > > > > > > Not sure if this is helpful but this here *is* a > > CompositorFrameSinkClient, > > so > > > > maybe it should use that interface? Maybe it should use > > CompositorFrameSink > > > too, > > > > not sure if this was considered or is possible. > > > > > > +Fady: WDYT? > > > > > > > > https://codereview.chromium.org/2328463004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2016/09/15 18:25:51, danakj wrote: > On Thu, Sep 15, 2016 at 11:14 AM, <mailto:xlai@chromium.org> wrote: > > > I have a slightly different opinion. > > > > I just did some checkup on the two mojom files that declare > > ReturnResources(array<cc.mojom.ReturnedResource> resources); and they are > > both > > in src/services/ui/public/interfaces/. > > > > In this CL, the implementation of this function requires access to > > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that this > > could > > hardly go out of scope of Blink. > > > > Does it violate Blink/Chromium dependency if we generate a blink-version > > of > > surface.mojom (or display_compositor.mojom) and include it in > > OffscreenCanvasFrameDispatcherImpl > > so as to implement the function? I seriously doubt it is. > > > > Some of the possible solutions: > > 1) Try in a separate patch that unifies all these ReturnResources mojom > > call > > that can > > be implemented by both Blink/Chromium. > > 2) In this CL, instead of return full Resources, just return a vector of > > integers, because > > that's the minimal we need. This may probably make the IPC more > > lightweight, I > > guess? > > > > I don't think we should do #2. I'm not familiar enough with mojo things to > comment on the rest. As long as the IPC gets here I'm happy. > > > > > > > > > > On 2016/09/15 15:56:28, xidachen wrote: > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > canvas_surface.mojom > > > File > > > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > canvas_surface.mojom > > > (right): > > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > canvas_surface.mojom#newcode33 > > > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > canvas_surface.mojom:33: > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); > > > On 2016/09/14 20:07:56, danakj wrote: > > > > On 2016/09/14 20:02:19, dcheng wrote: > > > > > Would it make sense for this to be a generic CC mojo interface? It > > seems > > > like > > > > we > > > > > already have SurfaceClient and CompositorFrameSinkClient with the > > exact > > same > > > > > interface. > > > > > > > > Not sure if this is helpful but this here *is* a > > CompositorFrameSinkClient, > > so > > > > maybe it should use that interface? Maybe it should use > > CompositorFrameSink > > > too, > > > > not sure if this was considered or is possible. > > > > > > +Fady: WDYT? > > > > > > > > https://codereview.chromium.org/2328463004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I wholeheartedly agree with Dana's comment. This code should be generalized! Thanks! :-)
On 2016/09/15 20:08:17, Fady Samuel wrote: > On 2016/09/15 18:25:51, danakj wrote: > > On Thu, Sep 15, 2016 at 11:14 AM, <mailto:xlai@chromium.org> wrote: > > > > > I have a slightly different opinion. > > > > > > I just did some checkup on the two mojom files that declare > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); and they are > > > both > > > in src/services/ui/public/interfaces/. > > > > > > In this CL, the implementation of this function requires access to > > > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that this > > > could > > > hardly go out of scope of Blink. > > > > > > Does it violate Blink/Chromium dependency if we generate a blink-version > > > of > > > surface.mojom (or display_compositor.mojom) and include it in > > > OffscreenCanvasFrameDispatcherImpl > > > so as to implement the function? I seriously doubt it is. > > > > > > Some of the possible solutions: > > > 1) Try in a separate patch that unifies all these ReturnResources mojom > > > call > > > that can > > > be implemented by both Blink/Chromium. > > > 2) In this CL, instead of return full Resources, just return a vector of > > > integers, because > > > that's the minimal we need. This may probably make the IPC more > > > lightweight, I > > > guess? > > > > > > > I don't think we should do #2. I'm not familiar enough with mojo things to > > comment on the rest. As long as the IPC gets here I'm happy. > > > > > > > > > > > > > > > > On 2016/09/15 15:56:28, xidachen wrote: > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > > canvas_surface.mojom > > > > File > > > > > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > > canvas_surface.mojom > > > > (right): > > > > > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > > canvas_surface.mojom#newcode33 > > > > > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > > canvas_surface.mojom:33: > > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); > > > > On 2016/09/14 20:07:56, danakj wrote: > > > > > On 2016/09/14 20:02:19, dcheng wrote: > > > > > > Would it make sense for this to be a generic CC mojo interface? It > > > seems > > > > like > > > > > we > > > > > > already have SurfaceClient and CompositorFrameSinkClient with the > > > exact > > > same > > > > > > interface. > > > > > > > > > > Not sure if this is helpful but this here *is* a > > > CompositorFrameSinkClient, > > > so > > > > > maybe it should use that interface? Maybe it should use > > > CompositorFrameSink > > > > too, > > > > > not sure if this was considered or is possible. > > > > > > > > +Fady: WDYT? > > > > > > > > > > > > https://codereview.chromium.org/2328463004/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > I wholeheartedly agree with Dana's comment. This code should be generalized! > Thanks! :-) In order to use the CompositorFrameSinkClient, we have to include services/ dir to the DEPS file of platform/graphics/, I feel like this is a overkill at this moment in order to share few lines of code. I think the DEPS for services/ dir should be discussed first, and then later we can make the change to unify the usage of CompositorFrameSinkClient interface.
On 2016/09/15 19:04:32, xlai (Olivia) wrote: > Sure, #2 is just my suggestion to try to alleviate potential concerns of having > additional IPC. > > Actually, #1 may not be implementable, depending on how loose the Blink/Chromium > dependencies can become. Regardless of which, I think it should be attempted > outside of this CL and shouldn't let that block this CL. I think this is part of the cost of Mojofying things: we need to be figuring out the right layering as we move along. I talked with danakj@ offline, and it sounds like that CompositorFrameSink (and CompositorFrameSinkClient?) are all things cc, services/ui, and Blink will need to use. So it should probably just live somewhere that all three can use... maybe //cc? I don't know how the layering for this works. > > Forget to mention I should add #3 solution--that is to keep the current > state--if > #1 is not doable. > > On 2016/09/15 18:25:51, danakj wrote: > > On Thu, Sep 15, 2016 at 11:14 AM, <mailto:xlai@chromium.org> wrote: > > > > > I have a slightly different opinion. > > > > > > I just did some checkup on the two mojom files that declare > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); and they are > > > both > > > in src/services/ui/public/interfaces/. > > > > > > In this CL, the implementation of this function requires access to > > > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that this > > > could > > > hardly go out of scope of Blink. > > > > > > Does it violate Blink/Chromium dependency if we generate a blink-version > > > of > > > surface.mojom (or display_compositor.mojom) and include it in > > > OffscreenCanvasFrameDispatcherImpl > > > so as to implement the function? I seriously doubt it is. > > > > > > Some of the possible solutions: > > > 1) Try in a separate patch that unifies all these ReturnResources mojom > > > call > > > that can > > > be implemented by both Blink/Chromium. > > > 2) In this CL, instead of return full Resources, just return a vector of > > > integers, because > > > that's the minimal we need. This may probably make the IPC more > > > lightweight, I > > > guess? > > > > > > > I don't think we should do #2. I'm not familiar enough with mojo things to > > comment on the rest. As long as the IPC gets here I'm happy. > > > > > > > > > > > > > > > > On 2016/09/15 15:56:28, xidachen wrote: > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > > canvas_surface.mojom > > > > File > > > > > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > > canvas_surface.mojom > > > > (right): > > > > > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > > canvas_surface.mojom#newcode33 > > > > > > > third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_ > > > canvas_surface.mojom:33: > > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); > > > > On 2016/09/14 20:07:56, danakj wrote: > > > > > On 2016/09/14 20:02:19, dcheng wrote: > > > > > > Would it make sense for this to be a generic CC mojo interface? It > > > seems > > > > like > > > > > we > > > > > > already have SurfaceClient and CompositorFrameSinkClient with the > > > exact > > > same > > > > > > interface. > > > > > > > > > > Not sure if this is helpful but this here *is* a > > > CompositorFrameSinkClient, > > > so > > > > > maybe it should use that interface? Maybe it should use > > > CompositorFrameSink > > > > too, > > > > > not sure if this was considered or is possible. > > > > > > > > +Fady: WDYT? > > > > > > > > > > > > https://codereview.chromium.org/2328463004/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Thu, Sep 15, 2016 at 1:44 PM, <dcheng@chromium.org> wrote: > On 2016/09/15 19:04:32, xlai (Olivia) wrote: > > Sure, #2 is just my suggestion to try to alleviate potential concerns of > having > > additional IPC. > > > > Actually, #1 may not be implementable, depending on how loose the > Blink/Chromium > > dependencies can become. Regardless of which, I think it should be > attempted > > outside of this CL and shouldn't let that block this CL. > > I think this is part of the cost of Mojofying things: we need to be > figuring out > the right layering as we move along. > I talked with danakj@ offline, and it sounds like that > CompositorFrameSink (and > CompositorFrameSinkClient?) are all things cc, services/ui, and Blink will > need > to use. So it should probably just live somewhere that all three can use... > maybe //cc? I don't know how the layering for this works. > I think it should probably live where cc::CompositorFrameSink lives, as it is the successor for it (maybe it should have mojo in its name or something to differentiate the mojo interface vs the virtual c++ interface). > > > > > > Forget to mention I should add #3 solution--that is to keep the current > > state--if > > #1 is not doable. > > > > On 2016/09/15 18:25:51, danakj wrote: > > > On Thu, Sep 15, 2016 at 11:14 AM, <mailto:xlai@chromium.org> wrote: > > > > > > > I have a slightly different opinion. > > > > > > > > I just did some checkup on the two mojom files that declare > > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); and > they are > > > > both > > > > in src/services/ui/public/interfaces/. > > > > > > > > In this CL, the implementation of this function requires access to > > > > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that > this > > > > could > > > > hardly go out of scope of Blink. > > > > > > > > Does it violate Blink/Chromium dependency if we generate a > blink-version > > > > of > > > > surface.mojom (or display_compositor.mojom) and include it in > > > > OffscreenCanvasFrameDispatcherImpl > > > > so as to implement the function? I seriously doubt it is. > > > > > > > > Some of the possible solutions: > > > > 1) Try in a separate patch that unifies all these ReturnResources > mojom > > > > call > > > > that can > > > > be implemented by both Blink/Chromium. > > > > 2) In this CL, instead of return full Resources, just return a > vector of > > > > integers, because > > > > that's the minimal we need. This may probably make the IPC more > > > > lightweight, I > > > > guess? > > > > > > > > > > I don't think we should do #2. I'm not familiar enough with mojo > things to > > > comment on the rest. As long as the IPC gets here I'm happy. > > > > > > > > > > > > > > > > > > > > > > On 2016/09/15 15:56:28, xidachen wrote: > > > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > > > third_party/WebKit/public/platform/modules/ > offscreencanvas/offscreen_ > > > > canvas_surface.mojom > > > > > File > > > > > > > > > third_party/WebKit/public/platform/modules/ > offscreencanvas/offscreen_ > > > > canvas_surface.mojom > > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > > > third_party/WebKit/public/platform/modules/ > offscreencanvas/offscreen_ > > > > canvas_surface.mojom#newcode33 > > > > > > > > > third_party/WebKit/public/platform/modules/ > offscreencanvas/offscreen_ > > > > canvas_surface.mojom:33: > > > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); > > > > > On 2016/09/14 20:07:56, danakj wrote: > > > > > > On 2016/09/14 20:02:19, dcheng wrote: > > > > > > > Would it make sense for this to be a generic CC mojo > interface? It > > > > seems > > > > > like > > > > > > we > > > > > > > already have SurfaceClient and CompositorFrameSinkClient with > the > > > > exact > > > > same > > > > > > > interface. > > > > > > > > > > > > Not sure if this is helpful but this here *is* a > > > > CompositorFrameSinkClient, > > > > so > > > > > > maybe it should use that interface? Maybe it should use > > > > CompositorFrameSink > > > > > too, > > > > > > not sure if this was considered or is possible. > > > > > > > > > > +Fady: WDYT? > > > > > > > > > > > > > > > > https://codereview.chromium.org/2328463004/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google > Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send > an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > https://codereview.chromium.org/2328463004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Sep 15, 2016 at 1:44 PM, <dcheng@chromium.org> wrote: > On 2016/09/15 19:04:32, xlai (Olivia) wrote: > > Sure, #2 is just my suggestion to try to alleviate potential concerns of > having > > additional IPC. > > > > Actually, #1 may not be implementable, depending on how loose the > Blink/Chromium > > dependencies can become. Regardless of which, I think it should be > attempted > > outside of this CL and shouldn't let that block this CL. > > I think this is part of the cost of Mojofying things: we need to be > figuring out > the right layering as we move along. > I talked with danakj@ offline, and it sounds like that > CompositorFrameSink (and > CompositorFrameSinkClient?) are all things cc, services/ui, and Blink will > need > to use. So it should probably just live somewhere that all three can use... > maybe //cc? I don't know how the layering for this works. > I think it should probably live where cc::CompositorFrameSink lives, as it is the successor for it (maybe it should have mojo in its name or something to differentiate the mojo interface vs the virtual c++ interface). > > > > > > Forget to mention I should add #3 solution--that is to keep the current > > state--if > > #1 is not doable. > > > > On 2016/09/15 18:25:51, danakj wrote: > > > On Thu, Sep 15, 2016 at 11:14 AM, <mailto:xlai@chromium.org> wrote: > > > > > > > I have a slightly different opinion. > > > > > > > > I just did some checkup on the two mojom files that declare > > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); and > they are > > > > both > > > > in src/services/ui/public/interfaces/. > > > > > > > > In this CL, the implementation of this function requires access to > > > > OffscreenCanvasFrameDispatcherImpl.m_cachedImages, which means that > this > > > > could > > > > hardly go out of scope of Blink. > > > > > > > > Does it violate Blink/Chromium dependency if we generate a > blink-version > > > > of > > > > surface.mojom (or display_compositor.mojom) and include it in > > > > OffscreenCanvasFrameDispatcherImpl > > > > so as to implement the function? I seriously doubt it is. > > > > > > > > Some of the possible solutions: > > > > 1) Try in a separate patch that unifies all these ReturnResources > mojom > > > > call > > > > that can > > > > be implemented by both Blink/Chromium. > > > > 2) In this CL, instead of return full Resources, just return a > vector of > > > > integers, because > > > > that's the minimal we need. This may probably make the IPC more > > > > lightweight, I > > > > guess? > > > > > > > > > > I don't think we should do #2. I'm not familiar enough with mojo > things to > > > comment on the rest. As long as the IPC gets here I'm happy. > > > > > > > > > > > > > > > > > > > > > > On 2016/09/15 15:56:28, xidachen wrote: > > > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > > > third_party/WebKit/public/platform/modules/ > offscreencanvas/offscreen_ > > > > canvas_surface.mojom > > > > > File > > > > > > > > > third_party/WebKit/public/platform/modules/ > offscreencanvas/offscreen_ > > > > canvas_surface.mojom > > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2328463004/diff/180001/ > > > > third_party/WebKit/public/platform/modules/ > offscreencanvas/offscreen_ > > > > canvas_surface.mojom#newcode33 > > > > > > > > > third_party/WebKit/public/platform/modules/ > offscreencanvas/offscreen_ > > > > canvas_surface.mojom:33: > > > > > ReturnResources(array<cc.mojom.ReturnedResource> resources); > > > > > On 2016/09/14 20:07:56, danakj wrote: > > > > > > On 2016/09/14 20:02:19, dcheng wrote: > > > > > > > Would it make sense for this to be a generic CC mojo > interface? It > > > > seems > > > > > like > > > > > > we > > > > > > > already have SurfaceClient and CompositorFrameSinkClient with > the > > > > exact > > > > same > > > > > > > interface. > > > > > > > > > > > > Not sure if this is helpful but this here *is* a > > > > CompositorFrameSinkClient, > > > > so > > > > > > maybe it should use that interface? Maybe it should use > > > > CompositorFrameSink > > > > > too, > > > > > > not sure if this was considered or is possible. > > > > > > > > > > +Fady: WDYT? > > > > > > > > > > > > > > > > https://codereview.chromium.org/2328463004/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google > Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send > an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > https://codereview.chromium.org/2328463004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A layout test is added to make sure that the commit actually commits to the original HTMLCanvasElement. Another layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A gpu pixel test is added to make sure that the commit actually commits to the original HTMLCanvasElement. A layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
danakj@, dcheng@: the latest patch is now using cc::mojom::MojoCompositorFrameSink, PTAL.
https://codereview.chromium.org/2328463004/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom (right): https://codereview.chromium.org/2328463004/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:25: interface OffscreenCanvasFrameReceiver { This interface is now unused.
Description was changed from ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A gpu pixel test is added to make sure that the commit actually commits to the original HTMLCanvasElement. A layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A gpu pixel test is added to make sure that the commit actually commits to the original HTMLCanvasElement. A layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852, 619136 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2328463004/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom (right): https://codereview.chromium.org/2328463004/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:25: interface OffscreenCanvasFrameReceiver { On 2016/09/19 15:12:10, jbroman wrote: > This interface is now unused. Done.
xidachen@chromium.org changed reviewers: + piman@chromium.org
piman@: could you please take a look at content/ changes? Thanks.
LGTM + nits https://codereview.chromium.org/2328463004/diff/280001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2328463004/diff/280001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:21: if (!GetSurfaceManager()) { nit: lookup the surface manager only once cc::SurfaceManager* manager = GetSurfaceManager(); if (!manager) { [...] } else { manager->InvalidateSurfaceClientId(...); } https://codereview.chromium.org/2328463004/diff/280001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:50: GetSurfaceManager()->RegisterSurfaceClientId(surface_id_.client_id()); nit: manager-> instead of GetSurfaceManager()->
Cool :) Using the cc interface LGTM https://codereview.chromium.org/2328463004/diff/260001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h (right): https://codereview.chromium.org/2328463004/diff/260001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h:27: void SubmitCompositorFrame( Put a comment above these that say what interface/class they are implementing.
mojom lgtm
The CQ bit was checked by xidachen@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...
nits addressed, sending to bots now. Thank you all :). https://codereview.chromium.org/2328463004/diff/280001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2328463004/diff/280001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:21: if (!GetSurfaceManager()) { On 2016/09/19 20:27:07, piman OOO back 2016-09-12 wrote: > nit: lookup the surface manager only once > > cc::SurfaceManager* manager = GetSurfaceManager(); > if (!manager) { > [...] > } else { > manager->InvalidateSurfaceClientId(...); > } Done. https://codereview.chromium.org/2328463004/diff/280001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:50: GetSurfaceManager()->RegisterSurfaceClientId(surface_id_.client_id()); On 2016/09/19 20:27:07, piman OOO back 2016-09-12 wrote: > nit: manager-> instead of GetSurfaceManager()-> Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xidachen@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 xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, junov@chromium.org, xlai@chromium.org, dcheng@chromium.org, piman@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2328463004/#ps320001 (title: "fix compile error on win_dbg")
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.
Description was changed from ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A gpu pixel test is added to make sure that the commit actually commits to the original HTMLCanvasElement. A layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852, 619136 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A gpu pixel test is added to make sure that the commit actually commits to the original HTMLCanvasElement. A layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852, 619136 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A gpu pixel test is added to make sure that the commit actually commits to the original HTMLCanvasElement. A layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852, 619136 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement WebGL's commit on the main thread. This CL implements WebGL's commit() API. It uses the frame work established for the OffscreenCanvasRenderingContext2D. The basic procedure is to get a AcceleratedStaticBitmapImage from WebGL's drawingBuffer. The StaticBitmapImage contains a SkImage that is gpu texture backed, a gpu mailbox, and a gpu token. We then prepare a CompositorFrame from the gpu mailbox and the gpu token. It currently works on the main thread only, making it work on worker thread will come in another CL. A gpu pixel test is added to make sure that the commit actually commits to the original HTMLCanvasElement. A layout test is added to ensure that when an OffscreenCanvas is not constructed from transferControlToOffscreen, then InvalidStateError must be thrown. BUG=563852, 619136 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/6267a77aa199671dce58a41639b40021db18f7d6 Cr-Commit-Position: refs/heads/master@{#419771} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/6267a77aa199671dce58a41639b40021db18f7d6 Cr-Commit-Position: refs/heads/master@{#419771} |
