1057088 - Add a way to sync query compositor properties like overfill and HWC status

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

In the last 2 b2g release we've regressed work we've done in getting HWC to run in particular app and have regressed the overfill.

We should expose a way to query if we're running with HWC and the overfill. This will let app set their own overfill budget and if they want to promise HWC (like camera, webgl games).

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Smaug can you take a look adding an API to window for testing. Gaia doesn't run with chrome privilege AFAIK so making this chrome only isn't useful. This means exposing the property to web content which is not ideal. Do you have any suggestions? Is this acceptable?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8477000 [details] [diff] [review]
patch

This is not acceptable. We don't add more moz-prefixed stuff if just possible.

What kind of code needs mozRequestCompositorProperty ?
We could expose that method only in case of some permission or
app type
[CheckPermission] or [AvailableIn]
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#typemapping

But even then mozRequestCompositorProperty sounds too implementation specific API.
Also, the method shouldn't probably take a string, but a webidl enum.

Is this perhaps something which could be exposed using navigator.getFeature?
http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp?rev=0611006cc095&force=1&mark=1499-1499#1498
or does the API really have to be sync?

Comment 3

[Benoit Girard (:BenWa)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 8477000 [details] [diff] [review]
> patch
> 
> This is not acceptable. We don't add more moz-prefixed stuff if just
> possible.
> 
> What kind of code needs mozRequestCompositorProperty ?
> We could expose that method only in case of some permission or
> app type
> [CheckPermission] or [AvailableIn]
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#typemapping

Actually I'm told that we can access ChromeOnly tests from UI-Tests. I think we should use that then.

Given that do we want chrome only property to have a moz prefix or not?

> 
> But even then mozRequestCompositorProperty sounds too implementation
> specific API.

It's incredibly implementation specific. That's why I want to shield it and have it only usable for regression testing.

> Also, the method shouldn't probably take a string, but a webidl enum.
> 
> Is this perhaps something which could be exposed using navigator.getFeature?
> http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.
> cpp?rev=0611006cc095&force=1&mark=1499-1499#1498
> or does the API really have to be sync?

We can still implement a sync api with an async contract if we query the value and store it, but I wouldn't want it accessible to things that have access to navigator.getFeature.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

If you're going to have [ChromeOnly] property, then any name is fine assuming it is not something
which might be used in production code. The name does after all end up to the chrome global
scope.
Though, if this is just for testing, why not DOMWindowUtils?

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch

Hows this? I put a simple test in gecko, but the most useful will go in gaia:

https://tbpl.mozilla.org/?tree=Try&rev=9f1fa3685dd2

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8481025 [details] [diff] [review]
patch

>+nsDOMWindowUtils::RequestCompositorProperty(const nsAString& property,
aProperty

>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
>@@ -1669,16 +1669,22 @@ interface nsIDOMWindowUtils : nsISupport
update uuid
>+float
>+ClientLayerManager::RequestProperty(const nsAString& property)
aProperty



>@@ -307,16 +309,20 @@ private:
> 
>   bool mInTransaction;
>   bool mIsCompositorReady;
>   bool mDebugOverlayWantsNextFrame;
> 
>   RefPtr<CompositingRenderTarget> mTwoPassTmpTarget;
>   RefPtr<TextRenderer> mTextRenderer;
>   bool mGeometryChanged;
>+
>+  // Testing property. If hardware composer is supported, this will return
>+  // true if the last frame was deemed 'too complicated' to be rendered.
>+  bool mLastFrameMissedHWC;
Please initialize this to false in ctor.



>+LayerTransactionParent::RecvRequestProperty(const nsString& aProperty, float* aValue)
>+{
>+  if (aProperty.Equals(NS_LITERAL_STRING("overdraw"))) {
>+    *aValue = layer_manager()->GetCompositor()->GetFillRatio();
>+  } else if (aProperty.Equals(NS_LITERAL_STRING("missed_hwc"))) {
>+    *aValue = layer_manager()->LastFrameMissedHWC() ? 1 : 0;
>+  } else {
>+    *aValue = -1;
>+  }
The possible values need to be documented somewhere.
nsIDOMWindowUtils.idl/requestCompositorProperty would be pretty reasonable place. 



(it would be good if someone at least a tiny bit familiar with gfx code would look at this too.)

Comment 7

[Benoit Girard (:BenWa)]

Thanks for the review. I will get a graphics review. I just wanted to get approval on how to expose this testing property to tests.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8482941 [details] [diff] [review]
patch v2

Review of attachment 8482941 [details] [diff] [review]:
-----------------------------------------------------------------

If we want to get the overdraw numbers this way, can we also delete the old window.mozRequestOverfill code paths?

Comment 10

[Benoit Girard (:BenWa)]

Sure, does nothing else use it ATM? My patch wont support getting a histogram like your patch did.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8482941 [details] [diff] [review]
patch v2

Review of attachment 8482941 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/mochitest/test_overdraw.html
@@ +8,5 @@
> +<body>
> +<script type="application/javascript">
> +var domWindowUtils = SpecialPowers.getDOMWindowUtils(window);
> +
> +var overdraw = parseFloat(domWindowUtils.requestCompositorProperty("overdraw"));

Why do you call parseFloat?

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch v3 (rebased)

Comment 13

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=aafe76c4ee82

Comment 14

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/036bd137c290

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/036bd137c290

Comment 16

[Benoit Girard (:BenWa)]

Bug 1064136 disables the test introduce here. I think it's racy. Need time to investigate.