WebGL - implement powerPreference

RESOLVED FIXED in Firefox 63

Status

()

P2
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: daoshengmu, Assigned: daoshengmu)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete, feature})

unspecified
Future
dev-doc-complete, feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 63+, firefox63+ fixed)

Details

(Whiteboard: [gfx-noted], URL)

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

2 years ago
Add powerPreference support under Context creation parameters to provides a hint to the user agent indicating what configuration of GPU is suitable for this WebGL context.

See https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.2.1
(Assignee)

Updated

2 years ago
Summary: WebGL powerPreference support → WebGL - implement powerPreference
Keywords: feature
Priority: -- → P3
Whiteboard: [gfx-noted]
(Assignee)

Updated

2 years ago
Blocks: 1207170
(Assignee)

Updated

a year ago
See Also: → bug 1339215
Let's aim for ff60.
Assignee: nobody → dmu
Priority: P3 → P2
Target Milestone: --- → Future
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
According to Virtual display [1], it mentions OSX will create several virtual displays for all available GPU cards and a software renderer. Therefore, in the case of MBP 15" 2015, it has three virtual display (0: integrated gpu, 1: AMD gpu, 3: software). We just need to change the virtual display id, then we can make it switch to different OpenGL context from different gpu cards.


[1] https://developer.apple.com/library/content/documentation/GraphicsImaging/Conceptual/OpenGL-MacProgGuide/opengl_pg_concepts/opengl_pg_concepts.html
(Assignee)

Comment 6

a year ago
(Assignee)

Comment 7

a year ago
(In reply to Daosheng Mu[:daoshengmu] from comment #6)
> Created attachment 8941344 [details]
> The test case for WebGL power preference

I run the test case in Safari and Chromium, but it has no effect when changing to other power preference modes. I suppose the WebGL renderer would be changed to the integrated GPU instead of the discrete GPU when my laptop's battery is less than 35%.
(Assignee)

Comment 8

a year ago
update for recreating canvas element and gl context when changing the powerPreference mode.
Attachment #8941344 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
Comment on attachment 8940664 [details]
Bug 1349799 - Part 3: WebGL PowerPreference support on OSX;

Jeff, please give me some feedback about the powerpreference implementation on OSX. I try the test case from attachment 8941348 [details] in Safari and Chromium, but both of them don't work. The discussion from Apple [1] is very close what I did currently. It is "low-power" by default, but removing NSOpenGLPFAAllowOfflineRenderers when request for "high-performance".

[1] https://www.khronos.org/webgl/public-mailing-list/public_webgl/1703/msg00023.php
Attachment #8940664 - Flags: feedback?(jgilbert)

Comment 13

a year ago
mozreview-review
Comment on attachment 8940664 [details]
Bug 1349799 - Part 3: WebGL PowerPreference support on OSX;

https://reviewboard.mozilla.org/r/210914/#review217702

::: gfx/gl/GLContextProviderCGL.mm:322
(Diff revision 2)
> +        memcpy(attribs, kAttribs_offscreen_coreProfile, sizeof(kAttribs_offscreen_coreProfile));
> +
> +        if (powerPreference == PowerPreferenceFlags::HIGH_PERFORMANCE) {
> +            // At high performance mode, we don't want to switch to
> +            // offline renderer, (NSOpenGLPFAAllowOfflineRenderers).
> +            attribs[3] = (NSOpenGLPixelFormatAttribute)0;

Use a different array for this, or dynamically generate using a vector.

Generally we want to branch to add something, not to remove something.

::: gfx/gl/GLContextProviderCGL.mm:326
(Diff revision 2)
> +            // offline renderer, (NSOpenGLPFAAllowOfflineRenderers).
> +            attribs[3] = (NSOpenGLPixelFormatAttribute)0;
> +        }
> +#if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_5
> +        // NSOpenGLPFAAllowOfflineRenderers is not supported on
> +        // OS X 10.4 or earlier.

We don't support these old versions, so drop this ifdef.

::: gfx/gl/GLContextProviderCGL.mm:413
(Diff revision 2)
>          triedToCreateContext = true;
>  
>          MOZ_RELEASE_ASSERT(!gGlobalContext);
>          nsCString discardFailureId;
>          RefPtr<GLContext> temp = CreateHeadless(CreateContextFlags::NONE,
> +                                                PowerPreferenceFlags::DEFAULT,

Roll this flag into CreateContextFlags as HIGH_POWER or something.

Comment 14

a year ago
mozreview-review
Comment on attachment 8939767 [details]
Bug 1349799 - Part 1: Add WebGL PowerPreference attribute;

https://reviewboard.mozilla.org/r/210074/#review217704

::: dom/canvas/WebGLContext.cpp:772
(Diff revision 3)
>  
>      //////
>  
>      if (tryNativeGL) {
>          if (useEGL)
> -            return CreateAndInitGLWith(CreateGLWithEGL, baseCaps, flags, out_failReasons);
> +            return CreateAndInitGLWith(CreateGLWithEGL, baseCaps, flags, mOptions.powerPreference,

CreateAndInitGLWith isn't static, so it has access to mOptions.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
I would like to ask for reviewing the current work for Mac platform. Then, I will keep working on powerPreference support on other platforms.
(Assignee)

Comment 19

a year ago
For Nvidia Optimus and AMD PowerXpress on Windows, we can refer this discussion. But, I am not sure if it would affect the context that is created from other processes.
(Assignee)

Comment 20

a year ago
(In reply to Daosheng Mu[:daoshengmu] from comment #19)
> For Nvidia Optimus and AMD PowerXpress on Windows, we can refer this
> discussion. But, I am not sure if it would affect the context that is
> created from other processes.

Adding the discussion link, https://stackoverflow.com/questions/6036292/select-a-graphic-device-in-windows-opengl/27881472#27881472.

Comment 21

a year ago
mozreview-review
Comment on attachment 8939767 [details]
Bug 1349799 - Part 1: Add WebGL PowerPreference attribute;

https://reviewboard.mozilla.org/r/210074/#review219090

::: dom/canvas/WebGLContext.cpp:594
(Diff revision 4)
>  
>      MOZ_RELEASE_ASSERT(!gl, "GFX: Already have a context.");
>      RefPtr<gl::GLContext> potentialGL;
>      while (!fallbackCaps.empty()) {
>          const gl::SurfaceCaps& caps = fallbackCaps.front();
> -        potentialGL = fnCreateGL(caps, flags, this, out_failReasons);
> +        potentialGL = fnCreateGL(caps, flags, this, mOptions.powerPreference,

Why isn't the power preference passed via `flags`?
Also this looks like it should be in a later patch in this queue.
Attachment #8939767 - Flags: review?(jgilbert) → review+

Comment 22

a year ago
mozreview-review
Comment on attachment 8940664 [details]
Bug 1349799 - Part 3: WebGL PowerPreference support on OSX;

https://reviewboard.mozilla.org/r/210914/#review219122
Attachment #8940664 - Flags: review?(jgilbert) → review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8941365 [details]
Bug 1349799 - Part 2: Adding high power context creation flag;

https://reviewboard.mozilla.org/r/211682/#review219124

::: dom/canvas/WebGLContext.cpp:97
(Diff revision 2)
>  using namespace mozilla::dom;
>  using namespace mozilla::gfx;
>  using namespace mozilla::gl;
>  using namespace mozilla::layers;
>  
> +#define POWER_PREFERENCE_EVENT(event, power, flags) ((power == dom::WebGLPowerPreference::High_performance) && \

This is way too much logic for a macro. Just use a function, stored to a clearly named intermediate variable.

::: dom/canvas/WebGLContext.cpp:101
(Diff revision 2)
>  
> +#define POWER_PREFERENCE_EVENT(event, power, flags) ((power == dom::WebGLPowerPreference::High_performance) && \
> +    (((event) & \
> +    (CanvasEventType::WEBGL_CONTEXT_LOST|CanvasEventType::WEBGL_CONTEXT_RESTORED)) == \
> +    (CanvasEventType::WEBGL_CONTEXT_LOST|CanvasEventType::WEBGL_CONTEXT_RESTORED)) ? \
> +    flags | gl::CreateContextFlags::HIGH_POWER : flags)

The default for now should be HIGH_POWER. We will eventually add a heuristic for interpreting "default" as low-power sometimes, but for now, "default" should mean high-power.

::: dom/html/HTMLCanvasElement.cpp:631
(Diff revision 2)
> +                                    EventListener* aCallback,
> +                                    const AddEventListenerOptionsOrBoolean& aOptions,
> +                                    const Nullable<bool>& aWantsUntrusted,
> +                                    ErrorResult& aRv)
> +{
> +  if (aType == NS_LITERAL_STRING("webglcontextlost")) {

This doesn't handle if RemoveEventListener is called afterwards. Ask someone from DOM how this should be done. :dholbert may be able to get you started.

Let's handle this in a follow-up bug, and just ignore the requirement of events for now.
Attachment #8941365 - Flags: review?(jgilbert) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Depends on: 1431013
(Assignee)

Comment 27

a year ago
mozreview-review-reply
Comment on attachment 8939767 [details]
Bug 1349799 - Part 1: Add WebGL PowerPreference attribute;

https://reviewboard.mozilla.org/r/210074/#review219090

> Why isn't the power preference passed via `flags`?
> Also this looks like it should be in a later patch in this queue.

Agree. I have moved this fix to the patch 2.
(Assignee)

Comment 28

a year ago
mozreview-review-reply
Comment on attachment 8941365 [details]
Bug 1349799 - Part 2: Adding high power context creation flag;

https://reviewboard.mozilla.org/r/211682/#review219124

> This doesn't handle if RemoveEventListener is called afterwards. Ask someone from DOM how this should be done. :dholbert may be able to get you started.
> 
> Let's handle this in a follow-up bug, and just ignore the requirement of events for now.

I file a follow-up Bug 1431013 to handle these events.
(Assignee)

Comment 29

a year ago
(In reply to Daosheng Mu[:daoshengmu] from comment #20)
> (In reply to Daosheng Mu[:daoshengmu] from comment #19)
> > For Nvidia Optimus and AMD PowerXpress on Windows, we can refer this
> > discussion. But, I am not sure if it would affect the context that is
> > created from other processes.
> 
> Adding the discussion link,
> https://stackoverflow.com/questions/6036292/select-a-graphic-device-in-
> windows-opengl/27881472#27881472.

I have tried to add "NvOptimusEnablement" at GLContextProviderEGL.cpp and GLContextProviderWGL.cpp in Windows, but it still get the context from the integrated GPU. I suppose it should allow us to create the context from the discrete GPU.
(In reply to Daosheng Mu[:daoshengmu] from comment #29)
> (In reply to Daosheng Mu[:daoshengmu] from comment #20)
> > (In reply to Daosheng Mu[:daoshengmu] from comment #19)
> > > For Nvidia Optimus and AMD PowerXpress on Windows, we can refer this
> > > discussion. But, I am not sure if it would affect the context that is
> > > created from other processes.
> > 
> > Adding the discussion link,
> > https://stackoverflow.com/questions/6036292/select-a-graphic-device-in-
> > windows-opengl/27881472#27881472.
> 
> I have tried to add "NvOptimusEnablement" at GLContextProviderEGL.cpp and
> GLContextProviderWGL.cpp in Windows, but it still get the context from the
> integrated GPU. I suppose it should allow us to create the context from the
> discrete GPU.

Let's leave non-mac implementations to follow-up bugs.
Attachment #8939767 - Flags: review?(kyle)
Comment on attachment 8939767 [details]
Bug 1349799 - Part 1: Add WebGL PowerPreference attribute;

https://reviewboard.mozilla.org/r/210074/#review258648
Attachment #8939767 - Flags: review?(kyle) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

9 months ago
mozreview-review
Comment on attachment 8987227 [details]
Bug 1349799 - Add PodEqual(T&,T&). -

https://reviewboard.mozilla.org/r/252490/#review258984

::: mfbt/PodOperations.h:194
(Diff revision 1)
>  PodEqual(const T (&one)[N], const T (&two)[N])
>  {
>    return PodEqual(one, two, N);
>  }
>  
> +template<typename T>

Add something like

    /** Compare the raw bytes of two objects of the same type. */

::: mfbt/PodOperations.h:196
(Diff revision 1)
>    return PodEqual(one, two, N);
>  }
>  
> +template<typename T>
> +static MOZ_ALWAYS_INLINE bool
> +PodEqual(const T& a, const T& b) {

{ on its own line.
Attachment #8987227 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8987226 [details]
Bug 1349799 - Add WebGLPowerPreference webidl. -

https://reviewboard.mozilla.org/r/252488/#review258986
Attachment #8987226 - Flags: review?(kyle) → review+
Attachment #8939767 - Attachment is obsolete: true
Attachment #8940664 - Attachment is obsolete: true
Attachment #8941365 - Attachment is obsolete: true
Attachment #8941365 - Flags: review?(jgilbert)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8987229 - Attachment is obsolete: true

Comment 42

9 months ago
mozreview-review
Comment on attachment 8987228 [details]
Bug 1349799 - Implement WebGLPowerPreference and gl::CreateContextFlags::HIGH_POWER. -

https://reviewboard.mozilla.org/r/252492/#review259232

::: dom/canvas/WebGLContext.cpp:384
(Diff revision 3)
>                 newOpts.alpha ? 1 : 0,
>                 newOpts.premultipliedAlpha ? 1 : 0,
>                 newOpts.preserveDrawingBuffer ? 1 : 0);
>  #endif
>  
> -    if (mOptionsFrozen && newOpts != mOptions) {
> +    if (mOptionsFrozen && !PodEqual(newOpts, mOptions)) {

I see this being used in a few places but I don't quite understand why it would be correct, given that the compiler is free to insert gaps between fields (with uninitialized content) when aligning them.

::: dom/canvas/WebGLContext.cpp:668
(Diff revision 3)
>      }
>  
> +    switch (mOptions.powerPreference) {
> +    case dom::WebGLPowerPreference::Default:
> +        // Eventually add a heuristic, but for now default to high-performance.
> +        // We can even make it dynamic by holding on to a ForceDiscreteGPUHelperCGL iff

nit: "iff" typo

::: gfx/gl/GLContextProviderCGL.mm:295
(Diff revision 3)
> -    if (!(flags & CreateContextFlags::REQUIRE_COMPAT_PROFILE)) {
> -        context = CreateWithFormat(kAttribs_offscreen_coreProfile);
> +    std::vector<NSOpenGLPixelFormatAttribute> attribs;
> +
> +    if (flags & CreateContextFlags::ALLOW_OFFLINE_RENDERER ||
> +        !(flags & CreateContextFlags::HIGH_POWER))
> +    {
> +        attribs.push_back(NSOpenGLPFAAllowOfflineRenderers);

what happens when HIGH_POWER is set? we aren't allowing offline rendering?
(In reply to Dzmitry Malyshau [:kvark] from comment #42)
> > -    if (mOptionsFrozen && newOpts != mOptions) {
> > +    if (mOptionsFrozen && !PodEqual(newOpts, mOptions)) {
> 
> I see this being used in a few places but I don't quite understand why it
> would be correct, given that the compiler is free to insert gaps between
> fields (with uninitialized content) when aligning them.

Oh bah, that's totally right.  Do dee do dee do...

Comment 44

9 months ago
mozreview-review
Comment on attachment 8987227 [details]
Bug 1349799 - Add PodEqual(T&,T&). -

https://reviewboard.mozilla.org/r/252490/#review259244

La la la
Attachment #8987227 - Flags: review+ → review-
Attachment #8987227 - Attachment is obsolete: true

Comment 45

9 months ago
mozreview-review-reply
Comment on attachment 8987228 [details]
Bug 1349799 - Implement WebGLPowerPreference and gl::CreateContextFlags::HIGH_POWER. -

https://reviewboard.mozilla.org/r/252492/#review259232

> I see this being used in a few places but I don't quite understand why it would be correct, given that the compiler is free to insert gaps between fields (with uninitialized content) when aligning them.

Yep, removed.

> nit: "iff" typo

It's correct: iff as in if-and-only-if.

> what happens when HIGH_POWER is set? we aren't allowing offline rendering?

I added a comment.
Comment hidden (mozreview-request)

Comment 47

9 months ago
mozreview-review
Comment on attachment 8987228 [details]
Bug 1349799 - Implement WebGLPowerPreference and gl::CreateContextFlags::HIGH_POWER. -

https://reviewboard.mozilla.org/r/252492/#review259460
Attachment #8987228 - Flags: review?(kvark) → review+

Comment 48

9 months ago
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c26fe9f3466f
Add WebGLPowerPreference webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee86c06ef4cf
Implement WebGLPowerPreference and gl::CreateContextFlags::HIGH_POWER. - r=kvark

Comment 50

9 months ago
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc4c0859e7f
Add WebGLPowerPreference webidl. - r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/923a690eed4f
Implement WebGLPowerPreference and gl::CreateContextFlags::HIGH_POWER. - r=kvark

Comment 51

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8fc4c0859e7f
https://hg.mozilla.org/mozilla-central/rev/923a690eed4f
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
It's likely worth relnoting this:
MacOS: WebGL power preferences allow non-performance-critical applications and applets to request the low-power GPU instead of the high-power GPU in multi-GPU systems.
relnote-firefox: --- → ?
Flags: needinfo?(jgilbert)
Added to nightly 63 release notes
tracking-firefox63: --- → +
relnote-firefox: ? → 63+
Thanks!
See Also: → bug 1523728
You need to log in before you can comment on or make changes to this bug.