Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2360413002:
Implement OffscreenCanvas Unaccelerated 2d commit() on main thread (Closed)
|
Created:
4 years, 3 months ago by xlai (Olivia) Modified:
4 years, 2 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, Rik, f(malita), blink-reviews, danakj+watch_chromium.org, ajuma+watch_chromium.org, Stephen Chennney, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement OffscreenCanvas Unaccelerated 2d commit() on main thread
BUG=563852
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Committed: https://crrev.com/f2246b3d3631e26bdf958bbd59cadd57938826b8
Cr-Commit-Position: refs/heads/master@{#422231}
Patch Set 1 #Patch Set 2 : works #Patch Set 3 : test #
Total comments: 2
Patch Set 4 : Mac #
Total comments: 25
Patch Set 5 : rebase #Patch Set 6 : Address all feedback #
Total comments: 3
Patch Set 7 : Rebase #Patch Set 8 : style #
Total comments: 1
Patch Set 9 : fix #Dependent Patchsets: Messages
Total messages: 34 (11 generated)
Description was changed from ========== Implement OffscreenCanvas Unaccelerated 2d commit() on main thread BUG=563852 ========== to ========== Implement OffscreenCanvas Unaccelerated 2d commit() on main thread BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
xlai@chromium.org changed reviewers: + junov@chromium.org, kbr@chromium.org
junov@chromium.org: Please review changes in OffscreenCanvasFrameDispatcherImpl kbr@chromium.org: Please review changes in gpu pixel test
danakj@chromium.org changed reviewers: + danakj@chromium.org
Some drive-by comments about gpu vs software compositing. https://codereview.chromium.org/2360413002/diff/40001/content/test/gpu/page_s... File content/test/gpu/page_sets/pixel_tests.py (right): https://codereview.chromium.org/2360413002/diff/40001/content/test/gpu/page_s... content/test/gpu/page_sets/pixel_tests.py:58: '--disable-gpu-compositing', Which this verifies software compositing when it's blacklisted, we also fall into software compositing when the gpu process fails to make a context 3 times (see RenderWidgetCompositor for this). In that case you can't rely on canvas being disabled by runtime flags and stuff. So while this makes sense for the pixel test, the code needs to handle more cases and you probably want to unit test that. https://codereview.chromium.org/2360413002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72: if (!image->isTextureBacked() && !RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) { RuntimeEnabledFeatures::accelerated2dCanvasEnabled() doesn't reflect if the compositor is going to expect a software or hardware frame, but I was expecting at least a todo here.
https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72: if (!image->isTextureBacked() && !RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) { You do not need to check RuntimeEnabledFeatures::accelerated2dCanvasEnabled(). The only thing that matters is whether this particular image is in software. For example, if acceleratedCanvases are enabled, but this particular canvas is not accelerated (which can happen for many reasons), then you'd want to use the SharedBitmap path https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:81: resource.mailbox_holder.texture_target = 0; Is this line really necessary? https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:84: m_sharedBitmaps.add(m_nextResourceId++, std::move(bitmap)); I know this is not currently a bug, but could we do the increment (m_nextResourceId++) in only one place. Make the code more readable. https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92: // It guarantees that the resource is not re-used or deleted. Improve this comment to explain why it is not necessary to do this for the non-texture code path https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:126: for (const auto& resource : resources) You could have a single 'for' loop here. Also, for each resource, you can check resource.is_software to know which map it needs to be removed from. You should also put some DCHECKs here to verify that the resource is in fact present in the map where you expect to find it.
https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72: if (!image->isTextureBacked() && !RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) { On 2016/09/23 20:54:52, Justin Novosad wrote: > You do not need to check RuntimeEnabledFeatures::accelerated2dCanvasEnabled(). > The only thing that matters is whether this particular image is in software. > For example, if acceleratedCanvases are enabled, but this particular canvas is > not accelerated (which can happen for many reasons), then you'd want to use the > SharedBitmap path Can the image be not hardware but the compositor is using GPU compositing? Because if that's the case we need to upload to a texture to hand to the compositor.
xidachen@chromium.org changed reviewers: + xidachen@chromium.org
https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:38: self.Fail('Pixel.OffscreenCanvasAccelerated2DWorker', bug=563852) These two entries seem to be duplicated. https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72: if (!image->isTextureBacked() && !RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) { danakj@: Yes, that is possible. I will have a CL implementing that. In that case, my opinion is to upload the |image| from cpu memory to gpu memory, and let CompositorFrame refer to the gpu memory. junov@: In my opinion, there should be two code path for the case when !image->isTextureBacked() depending on whether compositor is gpu or software. https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92: // It guarantees that the resource is not re-used or deleted. On 2016/09/23 20:54:52, Justin Novosad wrote: > Improve this comment to explain why it is not necessary to do this for the > non-texture code path junov@: this is not software path, it is hardware path because it is inside the else block. https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:96: resource.is_overlay_candidate = false; This line should be inside the else block.
https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:96: resource.is_overlay_candidate = false; On 2016/09/24 01:20:20, xidachen wrote: > This line should be inside the else block. Oh, my bad, I believe your intention is to have this line for both software and hardware path. It is fine, then.
The tests will LGTM once a couple of small changes are made. I defer to Dana's, Justin's and Xida's double-checking of that area and for reviews of the rest of the CL. https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:38: self.Fail('Pixel.OffscreenCanvasAccelerated2DWorker', bug=563852) On 2016/09/24 01:20:20, xidachen wrote: > These two entries seem to be duplicated. Yup. https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:47: self.Fail('Pixel.OffscreenCanvasUnaccelerated2DES3', ['mac'], bug=563852) The ES3 version of this suppression isn't needed -- see below. https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/page_s... File content/test/gpu/page_sets/pixel_tests.py (right): https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/page_s... content/test/gpu/page_sets/pixel_tests.py:229: expectations=expectations)) Could you put this in __init__ instead, after the call to self._AddAllPages(expectations, base_name, False)? As in Xida's https://codereview.chromium.org/2365653005/ , since this test requires a special SharedPageState, it won't run in both ES2 and ES3 modes anyway so doesn't need to be in this block.
https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:79: image->imageForCurrentFrame()->readPixels(imageInfo, pixels, imageInfo.minRowBytes(), 0, 0); junov@: I have some thoughts about this line. Right now it is using readPixels() which makes a copy of the SkImage, and then cc::SharedBitmaps refers to that copy. Why don't we use peekPixels()? We know that |image| is not texture backed, so it has to live in cpu memory. We can use peekPixels() and let the cc::SharedBitmaps pointing to the memory that SkImage is holding. peekPixels() doesn't make an extra copy which saves a lot. Does that make sense?
On 2016/09/27 11:22:44, xidachen wrote: > https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp > (right): > > https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:79: > image->imageForCurrentFrame()->readPixels(imageInfo, pixels, > imageInfo.minRowBytes(), 0, 0); > junov@: I have some thoughts about this line. Right now it is using readPixels() > which makes a copy of the SkImage, and then cc::SharedBitmaps refers to that > copy. > > Why don't we use peekPixels()? We know that |image| is not texture backed, so it > has to live in cpu memory. We can use peekPixels() and let the cc::SharedBitmaps > pointing to the memory that SkImage is holding. peekPixels() doesn't make an > extra copy which saves a lot. > > Does that make sense? That does not work because SharedBitmaps are allocated in shared memory so that they can be read by another process. The address pointed to by peekPixels will be in the current process's local address space. That said, a possible future optimization would be to allocate the canvas backings in shared memory right off the bat, to avoid the copy later, but we'd have to be careful about that, to only do it for canvases that use the commit flow (e.g. Only canvas contexts associated with OffscreenCanvases that have an associated surface layer, or something like that)
https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92: // It guarantees that the resource is not re-used or deleted. On 2016/09/24 01:20:20, xidachen wrote: > On 2016/09/23 20:54:52, Justin Novosad wrote: > > Improve this comment to explain why it is not necessary to do this for the > > non-texture code path > > junov@: this is not software path, it is hardware path because it is inside the > else block. Let me rephrase. Please explain, in the comments, why the software path (above) does not need to ref |image|, as we do here in the GPU path.
https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92: // It guarantees that the resource is not re-used or deleted. On 2016/09/27 15:11:15, Justin Novosad wrote: > On 2016/09/24 01:20:20, xidachen wrote: > > On 2016/09/23 20:54:52, Justin Novosad wrote: > > > Improve this comment to explain why it is not necessary to do this for the > > > non-texture code path > > > > junov@: this is not software path, it is hardware path because it is inside > the > > else block. > > Let me rephrase. Please explain, in the comments, why the software path (above) > does not need to ref |image|, as we do here in the GPU path. I think it is needed. Deleting the shared memory before the browser receives the IPC would be equally bad?
https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92: // It guarantees that the resource is not re-used or deleted. On 2016/09/27 19:05:20, danakj wrote: > On 2016/09/27 15:11:15, Justin Novosad wrote: > > On 2016/09/24 01:20:20, xidachen wrote: > > > On 2016/09/23 20:54:52, Justin Novosad wrote: > > > > Improve this comment to explain why it is not necessary to do this for the > > > > non-texture code path > > > > > > junov@: this is not software path, it is hardware path because it is inside > > the > > > else block. > > > > Let me rephrase. Please explain, in the comments, why the software path > (above) > > does not need to ref |image|, as we do here in the GPU path. > > I think it is needed. Deleting the shared memory before the browser receives the > IPC would be equally bad? In my opinion, this is not needed for software path. The code: image->imageForCurrentFrame()->readPixels(...) makes a copy of the pixel data, and put it in bitmap->pixels(), so as long as m_sharedBitmaps keeps the bitmap, it should be fine. The |image| should not be needed anymore because the pixel has been copied. Does that make sense?
On Tue, Sep 27, 2016 at 1:17 PM, <xidachen@chromium.org> wrote: > > https://codereview.chromium.org/2360413002/diff/60001/third_ > party/WebKit/Source/platform/graphics/OffscreenCanvasFrameD > ispatcherImpl.cpp > File > third_party/WebKit/Source/platform/graphics/OffscreenCanvasF > rameDispatcherImpl.cpp > (right): > > https://codereview.chromium.org/2360413002/diff/60001/third_ > party/WebKit/Source/platform/graphics/OffscreenCanvasFrameD > ispatcherImpl.cpp#newcode92 > third_party/WebKit/Source/platform/graphics/OffscreenCanvasF > rameDispatcherImpl.cpp:92: > // It guarantees that the resource is not re-used or deleted. > On 2016/09/27 19:05:20, danakj wrote: > > On 2016/09/27 15:11:15, Justin Novosad wrote: > > > On 2016/09/24 01:20:20, xidachen wrote: > > > > On 2016/09/23 20:54:52, Justin Novosad wrote: > > > > > Improve this comment to explain why it is not necessary to do > this for the > > > > > non-texture code path > > > > > > > > junov@: this is not software path, it is hardware path because it > is inside > > > the > > > > else block. > > > > > > Let me rephrase. Please explain, in the comments, why the software > path > > (above) > > > does not need to ref |image|, as we do here in the GPU path. > > > > I think it is needed. Deleting the shared memory before the browser > receives the > > IPC would be equally bad? > > In my opinion, this is not needed for software path. The code: > image->imageForCurrentFrame()->readPixels(...) > > makes a copy of the pixel data, and put it in bitmap->pixels(), so as > long as m_sharedBitmaps keeps the bitmap, it should be fine. The |image| > should not be needed anymore because the pixel has been copied. Does > that make sense? > Ah okay, yes. Let's make the comments explain this. I was assuming we were giving the SkImage a shared memory backing and shipping that directly, not copying pixels. > > https://codereview.chromium.org/2360413002/ > -- 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 Tue, Sep 27, 2016 at 1:17 PM, <xidachen@chromium.org> wrote: > > https://codereview.chromium.org/2360413002/diff/60001/third_ > party/WebKit/Source/platform/graphics/OffscreenCanvasFrameD > ispatcherImpl.cpp > File > third_party/WebKit/Source/platform/graphics/OffscreenCanvasF > rameDispatcherImpl.cpp > (right): > > https://codereview.chromium.org/2360413002/diff/60001/third_ > party/WebKit/Source/platform/graphics/OffscreenCanvasFrameD > ispatcherImpl.cpp#newcode92 > third_party/WebKit/Source/platform/graphics/OffscreenCanvasF > rameDispatcherImpl.cpp:92: > // It guarantees that the resource is not re-used or deleted. > On 2016/09/27 19:05:20, danakj wrote: > > On 2016/09/27 15:11:15, Justin Novosad wrote: > > > On 2016/09/24 01:20:20, xidachen wrote: > > > > On 2016/09/23 20:54:52, Justin Novosad wrote: > > > > > Improve this comment to explain why it is not necessary to do > this for the > > > > > non-texture code path > > > > > > > > junov@: this is not software path, it is hardware path because it > is inside > > > the > > > > else block. > > > > > > Let me rephrase. Please explain, in the comments, why the software > path > > (above) > > > does not need to ref |image|, as we do here in the GPU path. > > > > I think it is needed. Deleting the shared memory before the browser > receives the > > IPC would be equally bad? > > In my opinion, this is not needed for software path. The code: > image->imageForCurrentFrame()->readPixels(...) > > makes a copy of the pixel data, and put it in bitmap->pixels(), so as > long as m_sharedBitmaps keeps the bitmap, it should be fine. The |image| > should not be needed anymore because the pixel has been copied. Does > that make sense? > Ah okay, yes. Let's make the comments explain this. I was assuming we were giving the SkImage a shared memory backing and shipping that directly, not copying pixels. > > https://codereview.chromium.org/2360413002/ > -- 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.
I've updated the CL and addressed all feedback. https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:47: self.Fail('Pixel.OffscreenCanvasUnaccelerated2DES3', ['mac'], bug=563852) On 2016/09/26 22:29:29, Ken Russell wrote: > The ES3 version of this suppression isn't needed -- see below. Done. https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/page_s... File content/test/gpu/page_sets/pixel_tests.py (right): https://codereview.chromium.org/2360413002/diff/60001/content/test/gpu/page_s... content/test/gpu/page_sets/pixel_tests.py:229: expectations=expectations)) On 2016/09/26 22:29:30, Ken Russell wrote: > Could you put this in __init__ instead, after the call to > self._AddAllPages(expectations, base_name, False)? As in Xida's > https://codereview.chromium.org/2365653005/ , since this test requires a special > SharedPageState, it won't run in both ES2 and ES3 modes anyway so doesn't need > to be in this block. Done. https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:72: if (!image->isTextureBacked() && !RuntimeEnabledFeatures::accelerated2dCanvasEnabled()) { On 2016/09/23 20:54:52, Justin Novosad wrote: > You do not need to check RuntimeEnabledFeatures::accelerated2dCanvasEnabled(). > The only thing that matters is whether this particular image is in software. > For example, if acceleratedCanvases are enabled, but this particular canvas is > not accelerated (which can happen for many reasons), then you'd want to use the > SharedBitmap path Done. https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:81: resource.mailbox_holder.texture_target = 0; On 2016/09/23 20:54:52, Justin Novosad wrote: > Is this line really necessary? Done. https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:84: m_sharedBitmaps.add(m_nextResourceId++, std::move(bitmap)); On 2016/09/23 20:54:52, Justin Novosad wrote: > I know this is not currently a bug, but could we do the increment > (m_nextResourceId++) in only one place. Make the code more readable. Done. Put that in the function getNextResourceIdAndIncrement(). https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:92: // It guarantees that the resource is not re-used or deleted. On 2016/09/27 20:17:23, xidachen wrote: > On 2016/09/27 19:05:20, danakj wrote: > > On 2016/09/27 15:11:15, Justin Novosad wrote: > > > On 2016/09/24 01:20:20, xidachen wrote: > > > > On 2016/09/23 20:54:52, Justin Novosad wrote: > > > > > Improve this comment to explain why it is not necessary to do this for > the > > > > > non-texture code path > > > > > > > > junov@: this is not software path, it is hardware path because it is > inside > > > the > > > > else block. > > > > > > Let me rephrase. Please explain, in the comments, why the software path > > (above) > > > does not need to ref |image|, as we do here in the GPU path. > > > > I think it is needed. Deleting the shared memory before the browser receives > the > > IPC would be equally bad? > > In my opinion, this is not needed for software path. The code: > image->imageForCurrentFrame()->readPixels(...) > > makes a copy of the pixel data, and put it in bitmap->pixels(), so as long as > m_sharedBitmaps keeps the bitmap, it should be fine. The |image| should not be > needed anymore because the pixel has been copied. Does that make sense? Exactly. In the software path, after pixels are finished copying, the |image| is no longer needed so we don't care whether it dies off or not. Only the shared bitmap needs to be kept alive, and that's why I maintain the m_sharedBitmaps. For this part, I am adding a symmetric comment on top of the line m_sharedBitmaps.add(...). I think such a comment is quite clear to tell readers that exactly what references of images need to be hold alive. https://codereview.chromium.org/2360413002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:126: for (const auto& resource : resources) On 2016/09/23 20:54:52, Justin Novosad wrote: > You could have a single 'for' loop here. > Also, for each resource, you can check resource.is_software to know which map it > needs to be removed from. > You should also put some DCHECKs here to verify that the resource is in fact > present in the map where you expect to find it. I can't. The ReturnedResource does not have is_software or any other equivalent attribute that can tell me whether is resource is for software or accelerated mode. But I put it in one for loop now.
lgtm with nits. Also, please wait for Ken's CL to land first: https://codereview.chromium.org/2363343002/ https://codereview.chromium.org/2360413002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp (right): https://codereview.chromium.org/2360413002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:95: m_cachedImages.add(getNextResourceIdAndIncrement(), std::move(image)); My understanding of junov@'s comment is that you do: m_sharedBitmaps.add(m_nextResourceId, ...) and m_cachedImages.add(m_nextResourceId, ...) and then outside of this if--else block, you do a m_nextResourceId++; If there is any early exit in the if--else block, the m_nextResourceId will not increase, so it should be safe.
https://codereview.chromium.org/2360413002/diff/100001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2360413002/diff/100001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/pixel_expectations.py:35: self.Fail('Pixel.OffscreenCanvasUnaccelerated2D', bug=563852) The naming convention's changed in https://codereview.chromium.org/2363343002/ ; should now be "Pixel_OffscreenCanvasUnaccelerated2D". https://codereview.chromium.org/2360413002/diff/100001/content/test/gpu/page_... File content/test/gpu/page_sets/pixel_tests.py (right): https://codereview.chromium.org/2360413002/diff/100001/content/test/gpu/page_... content/test/gpu/page_sets/pixel_tests.py:149: self.AddStory(PixelTestsPage( This new page should now be added to src/content/test/gpu/gpu_tests/pixel_test_pages.py instead, now that https://codereview.chromium.org/2363343002/ has landed. Please let me know if you have any questions about the new format. You no longer need to specify a shared page state, just the browser command line arguments you want per-page.
ken@: can you take a look at the new patch again? I've rebased it based on your recently landed patch https://codereview.chromium.org/2363343002/. I rename the html files because now I know that they are supposed to be shared by both accelerated and unaccelerated cases, and thus the name "accelerated" is no longer appropriate. junov@: gentle ping on reviews on OffscreenCanvasFrameDispatcherImpl
lgtm
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one small test change. https://codereview.chromium.org/2360413002/diff/140001/content/test/gpu/gpu_t... File content/test/gpu/gpu_tests/pixel_test_pages.py (right): https://codereview.chromium.org/2360413002/diff/140001/content/test/gpu/gpu_t... content/test/gpu/gpu_tests/pixel_test_pages.py:190: PixelTestPage( Could you just add this to the above ExperimentalCanvasFeaturesPages set, above? You can just say: browser_args = browser_args + [ '--disable-accelerated-2d-canvas', '--disable-gpu-compositing'] That way you don't need to touch pixel_integration_test at all.
The CQ bit was checked by xlai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xidachen@chromium.org, kbr@chromium.org, junov@chromium.org Link to the patchset: https://codereview.chromium.org/2360413002/#ps160001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Implement OffscreenCanvas Unaccelerated 2d commit() on main thread BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Implement OffscreenCanvas Unaccelerated 2d commit() on main thread BUG=563852 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel Committed: https://crrev.com/f2246b3d3631e26bdf958bbd59cadd57938826b8 Cr-Commit-Position: refs/heads/master@{#422231} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f2246b3d3631e26bdf958bbd59cadd57938826b8 Cr-Commit-Position: refs/heads/master@{#422231} |
