Closed
Bug 1150944
Opened 9 years ago
Closed 9 years ago
turn on SkiaGL for canvas on OSX
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: bugs, Assigned: mattwoodrow, NeedInfo)
References
(Depends on 2 open bugs)
Details
(Whiteboard: gfx-noted)
Attachments
(10 files, 3 obsolete files)
8.17 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
3.10 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
4.06 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
22.71 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
25.54 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
gw280
:
review+
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #932958 +++ Bug 932958 is now about skia. This bug is about getting skiagl up and running for canvas.
Updated•9 years ago
|
Whiteboard: gfx-noted
Assignee | ||
Comment 2•9 years ago
|
||
Assignee: nobody → matt.woodrow
Attachment #8639998 -
Flags: review?(mstange)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8640000 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8640001 -
Flags: review?(mstange)
Updated•9 years ago
|
Attachment #8639998 -
Flags: review?(mstange) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8640001 [details] [diff] [review] Enable skia-gl for OSX canvas Review of attachment 8640001 [details] [diff] [review]: ----------------------------------------------------------------- Have you checked the discrete GPU situation?
Attachment #8640001 -
Flags: review?(mstange) → review+
Updated•9 years ago
|
Attachment #8640000 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8640076 -
Flags: review?(mstange)
Updated•9 years ago
|
Attachment #8640076 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8640103 -
Flags: review?(jgilbert)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8640104 -
Flags: review?(jmuizelaar)
Comment 9•9 years ago
|
||
Comment on attachment 8640103 [details] [diff] [review] Add a flags parameter to GLContextProvider functions instead of a bool Review of attachment 8640103 [details] [diff] [review]: ----------------------------------------------------------------- Don't go halfway. Either keep the bool or make a new enum class. I personally don't think having an enum class for every bool is useful. If `true` or `false` isn't clear enough at the callsite, pull them out into a named local that says what it's for. ::: dom/canvas/WebGLContext.cpp @@ +550,5 @@ > return nullptr; > } > > + nsRefPtr<GLContext> gl = > + gl::GLContextProvider::CreateHeadless(requireCompatProfile ? GLContextProvider::REQUIRE_COMPAT_PROFILE : 0); Don't inject a large expression in another expression if it means the outer expression now needs to be wrapped. Convert the bool we use locally to the target flag, then pass the flag explicitly. ::: dom/media/platforms/android/AndroidDecoderModule.cpp @@ +185,5 @@ > if (mGLContext) { > return true; > } > > + mGLContext = GLContextProvider::CreateHeadless(0); `0` is definitely worse than `false`.
Attachment #8640103 -
Flags: review?(jgilbert) → review-
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #9) > Comment on attachment 8640103 [details] [diff] [review] > Add a flags parameter to GLContextProvider functions instead of a bool > > Review of attachment 8640103 [details] [diff] [review]: > ----------------------------------------------------------------- > > Don't go halfway. Either keep the bool or make a new enum class. > I personally don't think having an enum class for every bool is useful. If > `true` or `false` isn't clear enough at the callsite, pull them out into a > named local that says what it's for. Sorry, I guess this wasn't clear. The next patch in the series adds a new flag, and I wanted to avoid taking two bool parameters (because that's just awful).
Updated•9 years ago
|
Attachment #8640104 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8640103 -
Attachment is obsolete: true
Attachment #8640537 -
Flags: review?(jgilbert)
Updated•9 years ago
|
Attachment #8640537 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8640706 -
Attachment is obsolete: true
Attachment #8640708 -
Flags: review+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7681b1567e https://hg.mozilla.org/integration/mozilla-inbound/rev/2f29ac2e31cd https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae813666ed5 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c510537d20b https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a6160242e5
Backed out (along with bug 1034370) for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12268534&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/d45d53f73998
Flags: needinfo?(matt.woodrow)
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/223bbff210ab https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8184f0b35a https://hg.mozilla.org/integration/mozilla-inbound/rev/1b6d4b77f3a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdf3da13458 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4b34d70807e https://hg.mozilla.org/integration/mozilla-inbound/rev/6444888e596c
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/223bbff210ab https://hg.mozilla.org/mozilla-central/rev/ba8184f0b35a https://hg.mozilla.org/mozilla-central/rev/1b6d4b77f3a0 https://hg.mozilla.org/mozilla-central/rev/4bdf3da13458 https://hg.mozilla.org/mozilla-central/rev/d4b34d70807e https://hg.mozilla.org/mozilla-central/rev/6444888e596c https://hg.mozilla.org/mozilla-central/rev/06156197d11d https://hg.mozilla.org/mozilla-central/rev/eabf1cf9b676
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 20•9 years ago
|
||
Is this relnote-worthy? Are there some obvious performance metrics that show we are better off with the new backend, noticable by end users?
Comment 21•9 years ago
|
||
(In reply to Florian Bender from comment #20) > Is this relnote-worthy? Are there some obvious performance metrics that show > we are better off with the new backend, noticable by end users? IMO yes - for 2d canvas-based games it's something like 2x-3x fps on OS X.
Release Note Request (optional, but appreciated) [Why is this notable]: Improved performance with 2D canvas on OS X. [Suggested wording]: OS X by default uses hardware acceleration for 2D canvas content. [Links (documentation, blog post, etc)]:
relnote-firefox:
--- → ?
Flags: needinfo?(matt.woodrow)
Comment 23•9 years ago
|
||
"Hardware acceleration for 2D canvas content (Mac OS X only)" added to the release notes
Comment 24•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #23) > "Hardware acceleration for 2D canvas content (Mac OS X only)" added to the > release notes To be super-clear: 2d canvas was already accelerated on Windows ( maybe Linux?? ), this change is bringing OS X into parity.
Comment 25•9 years ago
|
||
I don't think this has ever actually been turned on, due to bug 1206076.
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 26•9 years ago
|
||
Use the correct origin for SkiaGL textures. The only callers of this function (CanvasRenderingContext2D::DrawImage) passes a texture that was created with a TopLeft origin. We could swap this caller instead, but all of Moz2D currently assumes a TopLeft origin, so I think it makes sense to keep doing the same. Chiajung, can you see if drawing video to canvas is currently broken (upside-down) on b2g? I believe the work to add the TopLeft/BottomLeft enum broke this, at least for PLANAR_YCBCR images.
Flags: needinfo?(ffantasy1999)
Attachment #8664327 -
Flags: review?(gwright)
Assignee | ||
Updated•9 years ago
|
Attachment #8664327 -
Attachment is obsolete: true
Attachment #8664327 -
Flags: review?(gwright)
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8664487 -
Flags: review?(gwright)
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8664490 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 29•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8fba5c26146
Attachment #8664491 -
Flags: review?(jmuizelaar)
Updated•9 years ago
|
Attachment #8664491 -
Flags: review?(jmuizelaar) → review+
Updated•9 years ago
|
Attachment #8664490 -
Flags: review?(jmuizelaar) → review+
Updated•9 years ago
|
Attachment #8664487 -
Flags: review?(gwright) → review+
Comment 30•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c95806def45 https://hg.mozilla.org/integration/mozilla-inbound/rev/36059cdb634d https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed72fdd6327
Comment 31•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c95806def45 https://hg.mozilla.org/mozilla-central/rev/36059cdb634d https://hg.mozilla.org/mozilla-central/rev/5ed72fdd6327
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Comment 32•9 years ago
|
||
AzureSkiaAccelerated seems to be still disabled in today's nightly. Is that the expected behavior?
You need to log in
before you can comment on or make changes to this bug.
Description
•