|
|
Created:
3 years, 11 months ago by emircan Modified:
3 years, 10 months ago CC:
chromium-reviews, krit, dshwang, drott+blinkwatch_chromium.org, ajuma+watch-canvas_chromium.org, Stephen Chennney, blink-reviews-html_chromium.org, pdr+graphicswatchlist_chromium.org, jbroman, blink-reviews-platform-graphics_chromium.org, dglazkov+blink, Rik, f(malita), blink-reviews, kinuko+watch, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNotify listeners on OffScreenCanvas changes
This CL adds a hook on commit() calls for OffScreenCanvas so that
the changes are notified to the listeners. setPlaceholderFrame()
calls are used as the hook.
BUG=679610
TEST=Added layout test checking for a single frame capture and content_browsertest
for a continuous capture.
Also tested on html demo
https://cdn.rawgit.com/uysalere/js-demos/master/offscreencanvas_user_commit.html
Review-Url: https://codereview.chromium.org/2632593003
Cr-Commit-Position: refs/heads/master@{#452127}
Committed: https://chromium.googlesource.com/chromium/src/+/8db1658c62cfe49d1be14bc0186d4c20152cfd84
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Remove generate_tests use. #Patch Set 3 : Remove transferFromImageBitmap(). #Patch Set 4 : Rebase #Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 90 (66 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== setplaceholder BUG= ========== to ========== Notify listeners on OffScreenCanvas changes This CL adds a hook on commit() calls for OffScreenCanvas so that the changes are notified to the listeners. setPlaceholderFrame() calls are used as the hook. BUG=679610 TEST=Added layout tests and tested on html demo https://cdn.rawgit.com/uysalere/js-demos/master/offscreencanvas_user_commit.html ==========
emircan@chromium.org changed reviewers: + junov@chromium.org
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. Note that currently testOffScreenCanvasTransferBitmaps() tests fail. I don't see why they would. Let me know if I am missing something there.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2632593003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-offscreencanvas.html (right): https://codereview.chromium.org/2632593003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-offscreencanvas.html:40: generate_tests(testOffScreenCanvasCommits, [ The testharness documentation says you can't use generate_tests with async_test. I haven't looked into why, but this might be why the test is failing on the bots
The CQ bit was checked by emircan@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/2632593003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-offscreencanvas.html (right): https://codereview.chromium.org/2632593003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-offscreencanvas.html:40: generate_tests(testOffScreenCanvasCommits, [ On 2017/01/19 19:06:31, Justin Novosad wrote: > The testharness documentation says you can't use generate_tests with async_test. > I haven't looked into why, but this might be why the test is failing on the > bots Sorry for missing that. I just removed generate_tests usage, but it still fails. After transferFromImageBitmap(), I can see calls to HTMLCanvasElement::didDraw() but there are no calls to HTMLCanvasElement::didFinalizeFrame() where we have the hook.
On 2017/01/19 20:50:52, emircan wrote: > https://codereview.chromium.org/2632593003/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-offscreencanvas.html > (right): > > https://codereview.chromium.org/2632593003/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-offscreencanvas.html:40: > generate_tests(testOffScreenCanvasCommits, [ > On 2017/01/19 19:06:31, Justin Novosad wrote: > > The testharness documentation says you can't use generate_tests with > async_test. > > I haven't looked into why, but this might be why the test is failing on the > > bots > > Sorry for missing that. I just removed generate_tests usage, but it still fails. > After transferFromImageBitmap(), I can see calls to HTMLCanvasElement::didDraw() > but there are no calls to HTMLCanvasElement::didFinalizeFrame() where we have > the hook. Right. I have a WIP CL that refactors that stuff to bring some sanity to those mechanisms (and also fix the issue that canvases not in the DOM don't push updates to media streams). Let me land that first.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by emircan@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...
On 2017/01/19 21:01:29, Justin Novosad wrote: > > Right. I have a WIP CL that refactors that stuff to bring some sanity to those > mechanisms (and also fix the issue that canvases not in the DOM don't push > updates to media streams). Let me land that first. Sure. I just removed transferFromImageBitmap() test cases and the rest works fine. If you want I can try to land this and open a seperate bug. Or I can wait till that lands. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #4 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Notify listeners on OffScreenCanvas changes This CL adds a hook on commit() calls for OffScreenCanvas so that the changes are notified to the listeners. setPlaceholderFrame() calls are used as the hook. BUG=679610 TEST=Added layout tests and tested on html demo https://cdn.rawgit.com/uysalere/js-demos/master/offscreencanvas_user_commit.html ========== to ========== Notify listeners on OffScreenCanvas changes This CL adds a hook on commit() calls for OffScreenCanvas so that the changes are notified to the listeners. setPlaceholderFrame() calls are used as the hook. BUG=679610 TEST=Added layout test checking for a single frame capture and content_browsertest for a continuous capture. Also tested on html demo https://cdn.rawgit.com/uysalere/js-demos/master/offscreencanvas_user_commit.html ==========
I addressed transferFromImageBitmap() issues in this CL: https://codereview.chromium.org/2683343002/ Here I am going to only address offscreen commit() calls. Can we continue the review?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by emircan@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...
Patchset #4 (id:220001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #4 (id:240001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by emircan@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...
Patchset #4 (id:260001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
junov@, I rebased and added the corresponding content_browsertests here as well. Now, this code is hitting a NOTREACHED() here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph.... I attached the stack below as well. Let me know if you think this is related to my tests cases or point to some other problem. [1:1:0215/144702.544094:5275542925727:FATAL:TextureHolder.h(39)] Check failed: false. #0 0x7fdfed36c29b base::debug::StackTrace::StackTrace() #1 0x7fdfed36a8dc base::debug::StackTrace::StackTrace() #2 0x7fdfed3d5baf logging::LogMessage::~LogMessage() #3 0x7fdfe94b5099 blink::TextureHolder::updateSyncToken() #4 0x7fdfe943a05a blink::AcceleratedStaticBitmapImage::updateSyncToken() #5 0x7fdfe94b8e25 blink::OffscreenCanvasFrameDispatcherImpl::ReclaimResources() #6 0x7fdfe99ef0df cc::mojom::blink::MojoCompositorFrameSinkClientStubDispatch::Accept() #7 0x7fdfe94bb1a3 cc::mojom::blink::MojoCompositorFrameSinkClientStub<>::Accept() #8 0x7fdff40e4b25 mojo::InterfaceEndpointClient::HandleValidatedMessage() #9 0x7fdff40e44d1 mojo::InterfaceEndpointClient::HandleIncomingMessageThunk::Accept() #10 0x7fdff40e2155 mojo::FilterChain::Accept() #11 0x7fdff40e65c1 mojo::InterfaceEndpointClient::HandleIncomingMessage() #12 0x7fdff40fc721 mojo::internal::MultiplexRouter::ProcessIncomingMessage() #13 0x7fdff40fbf18 mojo::internal::MultiplexRouter::Accept() #14 0x7fdff40e2155 mojo::FilterChain::Accept() #15 0x7fdff40d7e25 mojo::Connector::ReadSingleMessage() #16 0x7fdff40d8ab9 mojo::Connector::ReadAllAvailableMessages() #17 0x7fdff40d88db mojo::Connector::OnHandleReadyInternal() #18 0x7fdff40d87bb mojo::Connector::OnWatcherHandleReady() #19 0x7fdff40daa7c _ZN4base8internal13FunctorTraitsIMN4mojo9ConnectorEFvjEvE6InvokeIPS3_JjEEEvS5_OT_DpOT0_ #20 0x7fdff40da986
junov@chromium.org changed reviewers: + xlai@chromium.org
Olivia, please a look at this.
On 2017/02/16 15:03:02, Justin Novosad wrote: > Olivia, please a look at this. My guess is that: Because the HTMLCanvasElement::notifyListenersCanvasChanged() will invoke AcceleratedStaticBitmapImage::imageForCurrentFrame() that will consume the mailbox and create a SkiaTextureHolder in the StaticBitmapImage. But SkiaTextureHolder, unlike MailboxTextureHolder, does not implement functions in their common interface that touched on sync_tokens. That's why the UNREACHED() has occurred. Previously, this does not happen for accelerated OffscreenCanvas 2d context because image->ensureMailbox() was called when the CompositorFrame was prepared, which enforced the creation of a TextureMailBoxHolder; but now it was overwritten.
The CQ bit was checked by emircan@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...
On 2017/02/16 17:18:11, xlai (Olivia) wrote: > On 2017/02/16 15:03:02, Justin Novosad wrote: > > Olivia, please a look at this. > > My guess is that: > Because the HTMLCanvasElement::notifyListenersCanvasChanged() will invoke > AcceleratedStaticBitmapImage::imageForCurrentFrame() that will consume the > mailbox and create a SkiaTextureHolder in the StaticBitmapImage. But > SkiaTextureHolder, > unlike MailboxTextureHolder, does not implement functions in their common > interface that touched on sync_tokens. That's why the UNREACHED() has occurred. > Previously, this does not happen for accelerated OffscreenCanvas 2d context > because > image->ensureMailbox() was called when the CompositorFrame was prepared, which > enforced the creation of a TextureMailBoxHolder; but now it was overwritten. I see. I added an empty override function for SkiaTextureHolder::updateSyncToken() to skip the check and see how bots do. I can also just update TextureHolder::updateSyncToken() definition if you prefer.
On 2017/02/16 19:22:36, emircan wrote: > On 2017/02/16 17:18:11, xlai (Olivia) wrote: > > On 2017/02/16 15:03:02, Justin Novosad wrote: > > > Olivia, please a look at this. > > > > My guess is that: > > Because the HTMLCanvasElement::notifyListenersCanvasChanged() will invoke > > AcceleratedStaticBitmapImage::imageForCurrentFrame() that will consume the > > mailbox and create a SkiaTextureHolder in the StaticBitmapImage. But > > SkiaTextureHolder, > > unlike MailboxTextureHolder, does not implement functions in their common > > interface that touched on sync_tokens. That's why the UNREACHED() has > occurred. > > Previously, this does not happen for accelerated OffscreenCanvas 2d context > > because > > image->ensureMailbox() was called when the CompositorFrame was prepared, which > > enforced the creation of a TextureMailBoxHolder; but now it was overwritten. > > I see. I added an empty override function for > SkiaTextureHolder::updateSyncToken() to skip the check and see how bots do. I > can also just update TextureHolder::updateSyncToken() definition if you prefer. Hmmm, you're still overwriting MailboxTextureHolder to SkiaTextureHolder. I think a better fix would be to refactor the imageForCurrentFrame() and TextureHolder. I'll do it another time. You can use this as a temporary fix. Please put a TODO item in AcceleratedStaticBitmapImage::imageForCurrentFrame and file a bug and assign it to me, saying that a refactoring of the TextureHolder and its subclasses is required to make this function not misused in the case when m_textureHolder is MailboxTextureHolder.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
On 2017/02/16 19:51:11, xlai (Olivia) wrote: > Hmmm, you're still overwriting MailboxTextureHolder to > SkiaTextureHolder. I think a better fix would be to refactor the > imageForCurrentFrame() > and TextureHolder. I'll do it another time. > > You can use this as a temporary fix. Please put a TODO item in > AcceleratedStaticBitmapImage::imageForCurrentFrame > and file a bug and assign it > to me, saying that a refactoring of the TextureHolder and its subclasses is > required to > make this function not > misused in the case when m_textureHolder is MailboxTextureHolder. Done. Thanks.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/17 01:42:35, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... I think you still need junov@'s stamp for HTMLCanvasElement.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2017/02/17 02:10:00, xlai (Olivia) wrote: > On 2017/02/17 01:42:35, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > I think you still need junov@'s stamp for HTMLCanvasElement. Thanks. junov@ can you PTAL?
lgtm
The CQ bit was checked by emircan@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by emircan@chromium.org
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
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 320001, "attempt_start_ts": 1487785045080240, "parent_rev": "e3c16997af716ba961740d9598b6942b70e59ccc", "commit_rev": "8db1658c62cfe49d1be14bc0186d4c20152cfd84"}
Message was sent while issue was closed.
Description was changed from ========== Notify listeners on OffScreenCanvas changes This CL adds a hook on commit() calls for OffScreenCanvas so that the changes are notified to the listeners. setPlaceholderFrame() calls are used as the hook. BUG=679610 TEST=Added layout test checking for a single frame capture and content_browsertest for a continuous capture. Also tested on html demo https://cdn.rawgit.com/uysalere/js-demos/master/offscreencanvas_user_commit.html ========== to ========== Notify listeners on OffScreenCanvas changes This CL adds a hook on commit() calls for OffScreenCanvas so that the changes are notified to the listeners. setPlaceholderFrame() calls are used as the hook. BUG=679610 TEST=Added layout test checking for a single frame capture and content_browsertest for a continuous capture. Also tested on html demo https://cdn.rawgit.com/uysalere/js-demos/master/offscreencanvas_user_commit.html Review-Url: https://codereview.chromium.org/2632593003 Cr-Commit-Position: refs/heads/master@{#452127} Committed: https://chromium.googlesource.com/chromium/src/+/8db1658c62cfe49d1be14bc0186d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:320001) as https://chromium.googlesource.com/chromium/src/+/8db1658c62cfe49d1be14bc0186d... |