Closed Bug 1057088 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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).
Attached patch patch (obsolete) — Splinter Review
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?
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8477000 - Flags: review?(bugs)
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?
Attachment #8477000 - Flags: review?(bugs) → review-
(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.
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
Attached patch patch (obsolete) — Splinter Review
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
Attachment #8477000 - Attachment is obsolete: true
Attachment #8481025 - Flags: review?(bugs)
Blocks: 1060528
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.)
Attachment #8481025 - Flags: review?(bugs) → review+
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.
Attachment #8481025 - Flags: review?(jmuizelaar)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8481025 - Attachment is obsolete: true
Attachment #8481025 - Flags: review?(jmuizelaar)
Attachment #8482941 - Flags: review?(jmuizelaar)
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?
Sure, does nothing else use it ATM? My patch wont support getting a histogram like your patch did.
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?
Attachment #8482941 - Flags: review?(jmuizelaar) → review+
Attachment #8482941 - Attachment is obsolete: true
Attachment #8485113 - Flags: review+
Blocks: 1063673
https://hg.mozilla.org/mozilla-central/rev/036bd137c290
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Bug 1064136 disables the test introduce here. I think it's racy. Need time to investigate.
You need to log in before you can comment on or make changes to this bug.