Closed Bug 1150944 Opened 5 years ago Closed 4 years ago

turn on SkiaGL for canvas on OSX

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed
firefox44 --- fixed
relnote-firefox --- 42+

People

(Reporter: bugs, Assigned: mattwoodrow, NeedInfo)

References

(Depends on 2 open bugs, Blocks 1 open bug)

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
: 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.
Whiteboard: gfx-noted
Duplicate of this bug: 1165076
Assignee: nobody → matt.woodrow
Attachment #8639998 - Flags: review?(mstange)
Attachment #8640001 - Flags: review?(mstange)
Attachment #8639998 - Flags: review?(mstange) → review+
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+
Attachment #8640000 - Flags: review?(ehsan) → review+
Attachment #8640076 - Flags: review?(mstange)
Attachment #8640076 - Flags: review?(mstange) → review+
Attachment #8640104 - Flags: review?(jmuizelaar)
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-
(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).
Attachment #8640104 - Flags: review?(jmuizelaar) → review+
Blocks: 1163729
Attachment #8640537 - Flags: review?(jgilbert) → review+
Rebased to current inbound.
Attachment #8640706 - Flags: review+
Depends on: 1191885
Is this relnote-worthy? Are there some obvious performance metrics that show we are better off with the new backend, noticable by end users?
(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)
"Hardware acceleration for 2D canvas content (Mac OS X only)" added to the release notes
(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.
I don't think this has ever actually been turned on, due to bug 1206076.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Attachment #8664327 - Attachment is obsolete: true
Attachment #8664327 - Flags: review?(gwright)
Attachment #8664490 - Flags: review?(jmuizelaar)
Attachment #8664491 - Flags: review?(jmuizelaar) → review+
Attachment #8664490 - Flags: review?(jmuizelaar) → review+
Attachment #8664487 - Flags: review?(gwright) → review+
Blocks: 1206763
AzureSkiaAccelerated seems to be still disabled in today's nightly. Is that the expected behavior?
Depends on: 1233416
Depends on: 1244569
See Also: → 1248525
See Also: → 1248398
Depends on: 1254456
You need to log in before you can comment on or make changes to this bug.