Bug 161017 – Implement preferLowPowerToHighPerformance for WebGL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Implement preferLowPowerToHighPerformance for WebGL

[Dean Jackson]

<rdar://problem/26819135>

[Dean Jackson]

Created attachment 286489 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 286489 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286489&action=review

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:64
>      dictionary.tryGetProperty("preserveDrawingBuffer", graphicsAttrs.preserveDrawingBuffer);

I thought there was a way to do all this stuff inside the bindings. We should migrate to that.

> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:66
> +    // FIXME: Get this property when we support the feature.

bug number?

I'm not sure why it's valuable to put this line of code here. It isn't helpful when we're reading code, and it isn't helpful when we're writing code. I would argue a bug should be made instead of these comments.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:97
> +        attribs.append(kCGLPFAAllowOfflineRenderers);

I think we should add a quote and a link to https://developer.apple.com/library/mac/technotes/tn2229/_index.html about how "integrated" actually means "offline"

How about "Any Macintosh system configuration that includes more GPUs than displays will have both online and offline GPUs. ... In these configurations you may wish to take advantage of the hardware that is not connected to a display... For CGL add kCGLPFAAllowOfflineRenderers."

> ManualTests/webgl-preferLowPowerToHighPerformance.html:13
> +    result.textContent = "UNMASKED_VENDOR_WEBGL is " + gl.getParameter(ext.UNMASKED_VENDOR_WEBGL) + "\n";

Please say something like "Test Passed"

[Dean Jackson]

Comment on attachment 286489 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286489&action=review

>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:64
>>      dictionary.tryGetProperty("preserveDrawingBuffer", graphicsAttrs.preserveDrawingBuffer);
> 
> I thought there was a way to do all this stuff inside the bindings. We should migrate to that.

I don't think so. WebGLContextAttributes is a class, not a dictionary.

But in general, it would be nice if there was bindings code that did this for us.

>> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:66
>> +    // FIXME: Get this property when we support the feature.
> 
> bug number?
> 
> I'm not sure why it's valuable to put this line of code here. It isn't helpful when we're reading code, and it isn't helpful when we're writing code. I would argue a bug should be made instead of these comments.

I don't know if we'll ever implement this.

I'll just remove these lines.

>> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:97
>> +        attribs.append(kCGLPFAAllowOfflineRenderers);
> 
> I think we should add a quote and a link to https://developer.apple.com/library/mac/technotes/tn2229/_index.html about how "integrated" actually means "offline"
> 
> How about "Any Macintosh system configuration that includes more GPUs than displays will have both online and offline GPUs. ... In these configurations you may wish to take advantage of the hardware that is not connected to a display... For CGL add kCGLPFAAllowOfflineRenderers."

I don't know about this. We'd just be replicating what already in our documentation, which people can find if they search for kCGLPFAAllowOfflineRenderers

>> ManualTests/webgl-preferLowPowerToHighPerformance.html:13
>> +    result.textContent = "UNMASKED_VENDOR_WEBGL is " + gl.getParameter(ext.UNMASKED_VENDOR_WEBGL) + "\n";
> 
> Please say something like "Test Passed"

Well, except you can't be sure it passed :)

You need to know what hardware is available, which is why it is a manual test. You might get different GPUs, but how could this test know which is the "lower power" one?

[Myles C. Maxfield]

Comment on attachment 286489 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286489&action=review

>>> ManualTests/webgl-preferLowPowerToHighPerformance.html:13
>>> +    result.textContent = "UNMASKED_VENDOR_WEBGL is " + gl.getParameter(ext.UNMASKED_VENDOR_WEBGL) + "\n";
>> 
>> Please say something like "Test Passed"
> 
> Well, except you can't be sure it passed :)
> 
> You need to know what hardware is available, which is why it is a manual test. You might get different GPUs, but how could this test know which is the "lower power" one?

I think considering the test a success if they are different is sufficient. This, coupled with your note about how the test is only valid on particular hardware, should be fine.

[Dean Jackson]

Committed r204664: <http://trac.webkit.org/changeset/204664>

[Myles C. Maxfield]

Note: now that it's possible to use WebGL without triggering the discrete GPU, other applications may cause us to switch to the discrete GPU, which causes the capabilities exposed to WebGL to change. This means that we should fix https://bugs.webkit.org/show_bug.cgi?id=161026