Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers #24524

bblanke · 2019-10-22T23:36:58Z

Addresses issues raised in the review of PR #24518 and includes changes to 17 tests' metadata for those that now PASS.

Contains fixes in PR #24518:

Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes.

Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #24465 and fix #24536

There are tests for these changes – 17 were updated to PASS

jdm · 2019-10-23T03:18:57Z

In future there is no need to close the existing PR to address review comments. You can make changes to the branch and push them (force pushing if you rewrite history at all) and they will be reflected in the open PR.

bblanke · 2019-10-23T16:24:02Z

Okay; that makes sense!

jdm

This looks really close! I've filed #24536 as some clean up that can be done as a result of this work. Your choice if you want to hop on it or not!

...as/fill-and-stroke-styles/2d.gradient.interpolate.multiple.worker.js.ini

...ta/offscreen-canvas/the-offscreen-canvas/offscreencanvas.resize.html.ini

components/script/dom/webglrenderingcontext.rs

components/script/dom/imagedata.rs

components/script/dom/canvasrenderingcontext2d.rs

components/script/Cargo.toml

components/pixels/lib.rs

jdm · 2019-10-24T14:13:55Z

@bors-servo try=wpt


        Auto merge of #24524 - bblanke:consolidate-size-helpers, r=<try>

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers  Addresses issues raised in the review of PR #24518 and includes changes to 17 tests' metadata for those that now PASS. Contains fixes in PR #24518: Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes. Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24465  - [X] There are tests for these changes – 17 were updated to PASS

bors-servo · 2019-10-24T14:14:08Z

⌛ Trying commit 326f9da with merge 4319de4...

bors-servo · 2019-10-24T14:52:03Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

jdm · 2019-10-28T16:59:07Z

Looks great! This needs to be rebased to deal with the merge conflicts from #24519 unfortunately, which will mean ensuring that any changes you made to CanvasState are moved into the new components/script/canvas_state.rs file. See step 7 in https://github.com/servo/servo/wiki/Github-workflow; if you have difficulties here, please let us know and we can help!

To make the process easier, I recommend squashing your commits into one single commit before starting the rebase against upstream.

bblanke · 2019-10-28T23:54:02Z

@jdm I've made changes to my branch to address your feedback and #24536
Let me know what you think/if I need to do anything to move this toward a merge!

jdm · 2019-10-29T00:01:09Z

@bblanke If you look at the bottom of the pull request, you should see:

Conflicting files
components/script/dom/canvasrenderingcontext2d.rs
components/script/dom/offscreencanvasrenderingcontext2d.rs
components/script/lib.rs

This means that your rebase did not include the changes from #24519, unfortunately. You need to ensure you have fetched a revision from servo/servo that includes 31ff2d4 and then rebase your branch on top of that revision.

bblanke · 2019-11-11T16:03:35Z

@jdm Figured out how to properly rebase! The guide on your wiki is great. Let me know if this works.

bblanke · 2019-11-11T16:04:00Z

@bors-servo try=wpt

bors-servo · 2019-11-11T16:04:01Z

@bblanke: 🔑 Insufficient privileges: not in try users

jdm · 2019-11-11T16:39:25Z

@bors-servo try=wpt

bors-servo · 2019-11-11T16:39:29Z

⌛ Trying commit ec29619 with merge a1194c4...


        Auto merge of #24524 - bblanke:consolidate-size-helpers, r=<try>

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers  Addresses issues raised in the review of PR #24518 and includes changes to 17 tests' metadata for those that now PASS. Contains fixes in PR #24518: Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes. Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24465 and fix #24536  - [X] There are tests for these changes – 17 were updated to PASS

bors-servo · 2019-11-11T17:21:29Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

jdm · 2019-11-11T17:35:31Z

@bors-servo r+
Thanks for doing this work!

bors-servo · 2019-11-11T17:35:32Z

📌 Commit ec29619 has been approved by jdm

bors-servo · 2019-11-11T17:35:37Z

⌛ Testing commit ec29619 with merge 3d2e980...


        Auto merge of #24524 - bblanke:consolidate-size-helpers, r=jdm

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers  Addresses issues raised in the review of PR #24518 and includes changes to 17 tests' metadata for those that now PASS. Contains fixes in PR #24518: Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes. Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24465 and fix #24536  - [X] There are tests for these changes – 17 were updated to PASS

bors-servo · 2019-11-11T19:26:11Z

💔 Test failed - status-taskcluster

jdm · 2019-11-11T19:27:57Z

An interesting failure:

  ▶ CRASH [expected OK] /offscreen-canvas/the-offscreen-canvas/size.large.html
  │ 
  │ 
  │ 
  │ 
  │ Stack trace for thread "CanvasThread"
  │ stack backtrace:
  │    0: backtrace::backtrace::trace
  │    1: backtrace::capture::Backtrace::new
  │    2: servo::install_crash_handler::handler
  │    3: _sigtramp
  │    4: AzCreateDrawTarget
  │    5: azure::azure_hl::DrawTarget::new
  │    6: <canvas::azure_backend::AzureBackend as canvas::canvas_data::Backend>::create_drawtarget
  │    7: canvas::canvas_data::CanvasData::recreate
  │    8: _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17hb22351f2903007feE.llvm.14423657731009663445
  │    9: __rust_maybe_catch_panic
  │   10: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   11: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  │   12: std::sys::unix::thread::Thread::new::thread_start
  │   13: _pthread_body
  │   14: _pthread_start
  └

Does ./mach test-wpt tests/wpt/web-platform-tests/offscreen-canvas/the-offscreen-canvas/size.large.html reproduce a crash for you?

jdm · 2019-11-11T19:43:52Z

Ah, I see I can reproduce it locally, and apparently on mac the drawing surface doesn't get created successfully with such a large size:

(lldb) r
Process 47583 launched: '/Users/jdm/src/servo/target/debug/servo' (x86_64)
servo was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 47583 stopped
* thread #30, name = 'CanvasThread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
    frame #0: 0x0000000107473b00 servo`::AzCreateDrawTarget(AzBackendType, AzIntSize *, AzSurfaceFormat) [inlined] mozilla::detail::RefCounted<mozilla::gfx::DrawTarget, (mozilla::detail::RefCountAtomicity)1>::AddRef(this=<unavailable>) const at RefPtr.h:111:5 [opt]
   108 	    // Note: this method must be thread safe for AtomicRefCounted.
   109 	    MOZ_ASSERT(int32_t(mRefCnt) >= 0);
   110 	#ifndef MOZ_REFCOUNTED_LEAK_CHECKING
-> 111 	    ++mRefCnt;
   112 	#else
   113 	    const char* type = static_cast<const T*>(this)->typeName();
   114 	    uint32_t size = static_cast<const T*>(this)->typeSize();
Target 0: (servo) stopped.
(lldb) up
frame #1: 0x0000000107473b00 servo`::AzCreateDrawTarget(aBackend=<unavailable>, aSize=<unavailable>, aFormat=<unavailable>) at azure-c.cpp:181 [opt]
   178 	    RefPtr<gfx::DrawTarget> target = gfx::Factory::CreateDrawTarget(backendType,
   179 	                                                                    *size,
   180 	                                                                    surfaceFormat);
-> 181 	    target->AddRef();
   182 	    return target;
   183 	}
   184

jdm · 2019-11-11T19:47:51Z

However, I can only reproduce it intermittently. Therefore, filed #24710.

@bors-servo retry

bors-servo · 2019-11-11T19:47:54Z

💣 Failed to start rebuilding: 405 Not Allowed

bors-servo · 2019-11-11T19:47:58Z

⌛ Testing commit ec29619 with merge 905f714...


        Auto merge of #24524 - bblanke:consolidate-size-helpers, r=jdm

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers  Addresses issues raised in the review of PR #24518 and includes changes to 17 tests' metadata for those that now PASS. Contains fixes in PR #24518: Updated the offscreen canvas rendering context to use the offscreen canvas' size. This involved upgrading several methods to accept u64 sizes. Additionally, the code in OffscreenCanvas::SetWidth() and OffscreenCanvas::SetHeight() was updated to send CanvasMsg::Recreate to the canvas paint thread. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24465 and fix #24536  - [X] There are tests for these changes – 17 were updated to PASS

bors-servo · 2019-11-11T20:35:23Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 905f714 to master...

bblanke · 2019-11-12T00:56:06Z

Weird; I'll investigate!

bblanke changed the title ~~Consolidate size helpers~~ Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers Oct 22, 2019

jdm self-assigned this Oct 24, 2019

jdm added the S-awaiting-review label Oct 24, 2019

jdm mentioned this pull request Oct 24, 2019

Add set_bitmap_dimensions method to CanvasState #24536

Closed

jdm requested changes Oct 24, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 24, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 28, 2019

jdm added S-needs-rebase S-needs-squash and removed S-awaiting-review labels Oct 28, 2019

highfive added the S-awaiting-review label Oct 28, 2019

jdm removed the S-awaiting-review label Oct 28, 2019

bblanke force-pushed the bblanke:consolidate-size-helpers branch from a997ceb to c4be157 Oct 28, 2019

highfive added the S-awaiting-review label Oct 28, 2019

jdm removed S-awaiting-review S-needs-squash labels Oct 28, 2019

highfive added the S-awaiting-review label Oct 28, 2019

jdm removed the S-awaiting-review label Oct 28, 2019

bblanke force-pushed the bblanke:consolidate-size-helpers branch from 127006f to 7ad224f Nov 10, 2019

bblanke force-pushed the bblanke:consolidate-size-helpers branch from 7ad224f to ec29619 Nov 10, 2019

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Nov 11, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 11, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 11, 2019

bors-servo merged commit ec29619 into servo:master Nov 11, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 11, 2019

servo / servo

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers #24524

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers #24524

bblanke commented Oct 22, 2019 •

edited by jdm

jdm commented Oct 23, 2019

bblanke commented Oct 23, 2019

jdm left a comment

jdm commented Oct 24, 2019

bors-servo commented Oct 24, 2019

bors-servo commented Oct 24, 2019

jdm commented Oct 28, 2019

bblanke commented Oct 28, 2019

jdm commented Oct 29, 2019

bblanke commented Nov 11, 2019

bblanke commented Nov 11, 2019

bors-servo commented Nov 11, 2019

jdm commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

jdm commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

jdm commented Nov 11, 2019

jdm commented Nov 11, 2019

jdm commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bblanke commented Nov 12, 2019

servo / servo

Join GitHub today

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers #24524

Make offscreen canvas rendering context use offscreen canvas' size; Consolidate size helpers #24524

Conversation

bblanke commented Oct 22, 2019 • edited by jdm

jdm commented Oct 23, 2019

bblanke commented Oct 23, 2019

jdm left a comment

jdm commented Oct 24, 2019

bors-servo commented Oct 24, 2019

bors-servo commented Oct 24, 2019

jdm commented Oct 28, 2019

bblanke commented Oct 28, 2019

jdm commented Oct 29, 2019

bblanke commented Nov 11, 2019

bblanke commented Nov 11, 2019

bors-servo commented Nov 11, 2019

jdm commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

jdm commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

jdm commented Nov 11, 2019

jdm commented Nov 11, 2019

jdm commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bors-servo commented Nov 11, 2019

bblanke commented Nov 12, 2019

bblanke commented Oct 22, 2019 •

edited by jdm