Daosheng Mu[:daoshengmu] Assignee
	Description • 7 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

	Kelsey Gilbert [:jgilbert]
	Comment 1 • 6 years ago

Let's aim for ff60.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 5 • 6 years 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

	Daosheng Mu[:daoshengmu] Assignee
	Comment 7 • 6 years 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%.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 8 • 6 years ago

update for recreating canvas element and gl context when changing the powerPreference mode.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 12 • 6 years 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

	Kelsey Gilbert [:jgilbert]
	Comment 13 • 6 years 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.

	Kelsey Gilbert [:jgilbert]
	Comment 14 • 6 years 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.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 18 • 6 years 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.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 19 • 6 years 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.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 20 • 6 years 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.

	Kelsey Gilbert [:jgilbert]
	Comment 21 • 6 years 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.

	Kelsey Gilbert [:jgilbert]
	Comment 22 • 6 years ago
mozreview-review

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

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

	Kelsey Gilbert [:jgilbert]
	Comment 23 • 6 years 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.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 27 • 6 years 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.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 28 • 6 years 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.

	Daosheng Mu[:daoshengmu] Assignee
	Comment 29 • 6 years 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.

	Kelsey Gilbert [:jgilbert]
	Comment 30 • 6 years ago

(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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 31 • 6 years ago
mozreview-review

Comment on attachment 8939767 [details]
Bug 1349799 - Part 1: Add WebGL PowerPreference attribute;

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

	Jeff Walden [:Waldo]
	Comment 35 • 6 years 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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 36 • 6 years ago
mozreview-review

Comment on attachment 8987226 [details]
Bug 1349799 - Add WebGLPowerPreference webidl. -

https://reviewboard.mozilla.org/r/252488/#review258986

	Kelsey Gilbert [:jgilbert]
	Comment 37 • 6 years ago
mozreview-review

Comment on attachment 8987227 [details]
Bug 1349799 - Add PodEqual(T&,T&). -

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

	Dzmitry Malyshau [:kvark]
	Comment 42 • 6 years 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?

	Jeff Walden [:Waldo]
	Comment 43 • 6 years ago

(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...

	Jeff Walden [:Waldo]
	Comment 44 • 6 years ago
mozreview-review

Comment on attachment 8987227 [details]
Bug 1349799 - Add PodEqual(T&,T&). -

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

La la la

	Kelsey Gilbert [:jgilbert]
	Comment 45 • 6 years 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.

	Dzmitry Malyshau [:kvark]
	Comment 47 • 6 years ago
mozreview-review

Comment on attachment 8987228 [details]
Bug 1349799 - Implement WebGLPowerPreference and gl::CreateContextFlags::HIGH_POWER. -

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

	Pulsebot
	Comment 48 • 6 years 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

	Cosmin Sabou [:CosminS]
	Comment 49 • 6 years ago

Backed out for build bustages on WebGLContext.cpp.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=ee86c06ef4cf80b7ff7c7491fff7312930480cf9&tochange=2293404aad33465a91a13f9f49483907f9ed423a&selectedJob=184827958

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=184827958&repo=mozilla-inbound&lineNumber=33102

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/2293404aad33465a91a13f9f49483907f9ed423a

	Pulsebot
	Comment 50 • 6 years 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

	Dorel Luca [:dluca]
	Comment 51 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8fc4c0859e7f
https://hg.mozilla.org/mozilla-central/rev/923a690eed4f

	Kelsey Gilbert [:jgilbert]
	Comment 52 • 6 years ago

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.

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 53 • 6 years ago

Added to nightly 63 release notes

	Florian Scholz (Open Web Docs)
	Comment 54 • 6 years ago

Also announced on Firefox 63 for developers:
https://developer.mozilla.org/en-US/Firefox/Releases/63#Canvas_and_WebGL

Updated reference pages:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/getContext
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/getContextAttributes

Compat data update:
https://github.com/mdn/browser-compat-data/pull/2404

	Kelsey Gilbert [:jgilbert]
	Comment 55 • 6 years ago

Thanks!