Closed Bug 832383 Opened 7 years ago Closed 7 years ago

Hardware composer multirect visible regions

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
1.0.1 IOT1 (10may)
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: diego, Assigned: diego)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(4 files, 16 obsolete files)

7.36 KB, patch
diego
: review+
Details | Diff | Splinter Review
2.22 KB, patch
diego
: review+
Details | Diff | Splinter Review
2.25 KB, patch
diego
: review+
Details | Diff | Splinter Review
2.39 KB, patch
diego
: review+
Details | Diff | Splinter Review
Attached patch HWC multirect visible region (obsolete) — Splinter Review
Currently HwcComposer2D falls back to GPU when it encounters a layer with visible region with more that one rectangle. This results in the key use case of camera preview falling back to GPU
Attachment #703950 - Flags: review?(jones.chris.g)
Comment on attachment 703950 [details] [diff] [review]
HWC multirect visible region

I could be missing something, but where do we clear mVisibleRegions?
Attachment #703950 - Flags: review?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Comment on attachment 703950 [details] [diff] [review]
> HWC multirect visible region
> 
> I could be missing something, but where do we clear mVisibleRegions?

My bad. I had it in but at some point I accidentally removed it. New patch coming up
Attached patch HWC multirect visible region v2 (obsolete) — Splinter Review
Clear visible region list on each new frame
Attachment #703950 - Attachment is obsolete: true
Attachment #705606 - Flags: review?(jones.chris.g)
Comment on attachment 705606 [details] [diff] [review]
HWC multirect visible region v2

>diff --git a/widget/gonk/HwcComposer2D.cpp b/widget/gonk/HwcComposer2D.cpp

>+static bool
>+PrepareVisibleRegion(const nsIntRegion& aVisible, const gfxMatrix& aTransform,
>+                  nsIntRect aClip, nsIntRect aBufferRect,
>+                  std::vector<hwc_rect_t>* aVisibleRegionScreen) {

(Nit: RectVector*)

>+
>+    nsIntRegionRectIterator iter(aVisible);
>+    bool isVisible = false;
>+    while (const nsIntRect* visibleRect = iter.Next()) {

Nit: the names are a bit verbose for me here.  How about |rect| for
the iterator here ...

>+        hwc_rect_t visibleRectScreen;
>+
>+        gfxRect gfxVisibleRect(*visibleRect);

|screenRect| for this guy ...

>+        gfxRect gfxVisibleRectScreen = aTransform.TransformBounds(gfxVisibleRect);

and reuse screenRect for this guy.

>+        visibleRectScreen.right  = gfxVisibleRectScreen.x + gfxVisibleRectScreen.width;

screenRect.XMost()

>+        visibleRectScreen.bottom = gfxVisibleRectScreen.y + gfxVisibleRectScreen.height;

screenRect.YMost()

>+    if(!isVisible) {

Nit: space after |if ()|

>+        if (visibleRegion.GetNumRects() > 1) {
>+            std::vector<hwc_rect_t> visibleRects;
>+            mVisibleRegions.push_back(visibleRects);

Nit: let's do

  mVisibleRegions.push_back(RectVector());
  RectVector* visibleRects = &mVisibleRegions.back();

and then replace the uses of |mVisibleRegions.back()| with
|visibleRects|.

>+            region.rects = &(mVisibleRegions.back()[0]);

(There's a C++11/gcc .data() API that's much nicer here, but probably
better to stay away.)

>diff --git a/widget/gonk/HwcComposer2D.h b/widget/gonk/HwcComposer2D.h

>+    std::list<std::vector<hwc_rect_t>>   mVisibleRegions;

This needs a comment describing what it does.

Nit: add a |typedef std::vector<hwc_rect_t> RectVector| so we can
refer to that more concisely elsewhere.

clear()'ing a std::list will release all the internal nodes and
destroy what they point to.  So we'll end up doing dynamic memory
allocation on each TryRender() here.  I'm not /too/ worried about
that, but if it shows up on profiles we can use something like

 typedef nsAutoTArray<hwc_rect_t, 8/*xxx tune*/> RectVector;
 nsAutoTArray<RectArray, 16> mVisibleRegions;

That will preallocate space for 16 regions (layers) with 8 rects per
region.  As long as we stay within those limits, we'll never malloc.

Looks good, but would like to see one more version.
Attachment #705606 - Flags: review?(jones.chris.g)
Blocks: 865112
blocking-b2g: --- → tef+
Attached patch HWC multirect visible region v3 (obsolete) — Splinter Review
Addressed all comments from v2 patch
Attachment #742767 - Flags: review?(mwu)
Attachment #705606 - Attachment is obsolete: true
:mwu,

I added 4 more smaller stability patches. 3 were long pending from CAF and another addresses bug 865112. I figured it may be easier to deal with them together, but I can split them off into different bugs if you prefer.
Flags: needinfo?(mwu)
This bug should be fine.
Flags: needinfo?(mwu)
Lukas, not sure if you tef+ this one by accident? my understanding is that hardware composer is not turned on on tef devices so it seems interesting that this bug it tef+. Thanks
Flags: needinfo?(lsblakk)
Amelie,

The patch in comment 9 is the one that fixes the stretch issue.
:jcheng,

All 1.0.1 partners enable HWC by default. I believe the plan is to enable HWC by default in HWC builds ASAP. See bug 828876
s/HWC builds/Moz builds/
Blocks: 862952
Target Milestone: --- → 1.0.1 IOT1 (10may)
Comment on attachment 742767 [details] [diff] [review]
HWC multirect visible region v3

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

Looks fine to me - I just have a bunch of nits. I would like to do the storage of the hwc_rect list better but we can worry about that later.

::: widget/gonk/HwcComposer2D.cpp
@@ +210,5 @@
> + *         false if the layer can be skipped
> + */
> +static bool
> +PrepareVisibleRegion(const nsIntRegion& aVisible, const gfxMatrix& aTransform,
> +                  nsIntRect aClip, nsIntRect aBufferRect,

nit: align this with const on the line before

@@ +219,5 @@
> +    while (const nsIntRect* visibleRect = rect.Next()) {
> +        hwc_rect_t visibleRectScreen;
> +
> +        gfxRect screenRect(*visibleRect);
> +        screenRect.IntersectRect(screenRect, aBufferRect);

This seems slightly awkward to me. Maybe pass gfxRect(*visibleRect) to IntersectRect so it's easier to see what we're intersecting?

@@ +223,5 @@
> +        screenRect.IntersectRect(screenRect, aBufferRect);
> +        screenRect = aTransform.TransformBounds(screenRect);
> +        screenRect.IntersectRect(screenRect, aClip);
> +        screenRect.RoundIn();
> +        if(screenRect.IsEmpty()) {

nit: space after if

@@ +233,5 @@
> +        visibleRectScreen.bottom = screenRect.YMost();
> +        aVisibleRegionScreen->push_back(visibleRectScreen);
> +        isVisible = true;
> +    }
> +    if (!isVisible) {

return isVisible;

@@ +439,5 @@
> +            }
> +            region.numRects = visibleRects->size();
> +            region.rects = &((*visibleRects)[0]);
> +        }
> +        else {

nit: else on the same line as }

@@ +469,4 @@
>          mList->numHwLayers = 0;
>      }
>  
> +    //XXX: The clear() below means all rect vectors will be have to be

nit: space after //
Attachment #742767 - Flags: review?(mwu) → review+
Comment on attachment 742770 [details] [diff] [review]
Don't render semitransparent color layers in HwcComposer2D v1

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

::: widget/gonk/HwcComposer2D.cpp
@@ -299,5 @@
>      }
>  
>      float opacity = aLayer->GetEffectiveOpacity();
> -    if (opacity <= 0) {
> -        LOGD("Layer is fully transparent so skip rendering");

? Not sure I understand your comment about why we're skipping fully transparent layers.

@@ +451,5 @@
>      } else {
>          hwcLayer.flags |= HWC_COLOR_FILL;
>          ColorLayer* colorLayer = static_cast<ColorLayer*>(layerGL->GetLayer());
> +        uint32_t color = colorLayer->GetColor().Packed();
> +        if (((color >> 24) & 0xff) < 255) {

colorLayer->GetColor().a ?
Comment on attachment 742768 [details] [diff] [review]
Ignore visible region for ShadowCanvasLayer and ShadowImageLayer v1

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

Could you get someone more familiar with Layers to review this?
Comment on attachment 742769 [details] [diff] [review]
Ensure color fill rect stays inside screen bounds v1

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

Do you know why we don't use Round()?
Attached patch HWC multirect visible region v4 (obsolete) — Splinter Review
Addressed all nits in in v3 patch. Carry forward r=mwu
Attachment #742767 - Attachment is obsolete: true
Attachment #743370 - Flags: review+
Attachment #742768 - Attachment is obsolete: true
Attachment #742769 - Attachment is obsolete: true
Attachment #742770 - Attachment is obsolete: true
Attachment #742771 - Attachment is obsolete: true
(In reply to Michael Wu [:mwu] from comment #17)
> ? Not sure I understand your comment about why we're skipping fully
> transparent layers.

LayersOGL don't ignore transparent layers. Couldn't completely figure out how exactly they're treated. Since it didn't affect the key HWC use cases I err on the safe side and fall back to GPU when there's transparent layers.
 
> colorLayer->GetColor().a ?

Addressed in v2
(In reply to Michael Wu [:mwu] from comment #19)
> Do you know why we don't use Round()?

Round() and RoundOut() can sometimes end one pixel outside the screen after scaling and rotating. HWC does not accept coordinates outside the screen.
Attachment #743375 - Flags: review+
Please use 8 lines of context for your patches, it is Mozilla style and makes them easier to read, thanks!
Comment on attachment 743373 [details] [diff] [review]
Ignore visible region for ShadowCanvasLayer and ShadowImageLayer v2

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

::: widget/gonk/HwcComposer2D.cpp
@@ +370,5 @@
>  
>      sp<GraphicBuffer> buffer = fillColor ? nullptr : GrallocBufferActor::GetFrom(*state.mSurface);
>  
> +    nsIntRect visibleRect;
> +    if (aLayer->Name() == "ShadowCanvasLayer" || aLayer->Name() == "ShadowImageLayer") {

Please don't rely on Name() like this - they are only meant to be used in debugging output etc., they might change at any time (and in this case will change fairly soon). Better to use type methods if these exist or add some API.
Comment on attachment 743376 [details] [diff] [review]
Get surface dimensions from SurfaceDecriptorGralloc v2

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

::: widget/gonk/HwcComposer2D.cpp
@@ +389,3 @@
>          } else {
>              bufferRect = nsIntRect(visibleRect.x, visibleRect.y,
> +                int(surfaceSize.width), int(surfaceSize.height));

no need for the casts, they are ints already
(In reply to Diego Wilson [:diego] from comment #26)
> (In reply to Michael Wu [:mwu] from comment #19)
> > Do you know why we don't use Round()?
> 
> Round() and RoundOut() can sometimes end one pixel outside the screen after
> scaling and rotating. HWC does not accept coordinates outside the screen.

Hmm that seems odd that enough errors accumulate that Round() doesn't work. Would cropping the final result to the screen/buffer bounds work? I'm a little paranoid about a side getting rounded inwards by mistake and not rendering a line when necessary.

Also, is it easy to reproduce this issue?
Good point. The prob may only be with RoundOut(). Let me try out Round().
Added 8 lines of context. Carry forward r=mwu
Attachment #743370 - Attachment is obsolete: true
Attachment #744395 - Flags: review+
Add 8 lines of context. Address all comments from nrc to v2 patch
Attachment #743373 - Attachment is obsolete: true
Add 8 lines of context. Address all comments from mwu to v2 patch
Attachment #743374 - Attachment is obsolete: true
Attachment #744399 - Attachment description: Ensure color fill rect stays inside screen bounds v2 → Ensure color fill rect stays inside screen bounds v3
Comment on attachment 744399 [details] [diff] [review]
Ensure color fill rect stays inside screen bounds v3

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

Looks good
Attachment #744399 - Flags: review+
Add 8 lines of context. Carry forward r=mwu
Attachment #744402 - Flags: review+
Attachment #743375 - Attachment is obsolete: true
Add 8 lines of context. Address all comments from nrc to v2 patch
Attachment #743376 - Attachment is obsolete: true
Comment on attachment 744405 [details] [diff] [review]
Get surface dimensions from SurfaceDecriptorGralloc v3

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

Self-assigning for review according to IRC conversation.
Attachment #744405 - Flags: review?(bjacob)
Attachment #744405 - Flags: review?(bjacob) → review+
Removing lsblakk needinfo?

jcheng,

I think comment 14 may have answered your question in comment 12. If not, please feel free to re-ping lsblakk.
Flags: needinfo?(lsblakk)
Attachment #744395 - Attachment description: HWC multirect visible region v5 → Patch 1 - HWC multirect visible region v5
Comment on attachment 744398 [details] [diff] [review]
Ignore visible region for ShadowCanvasLayer and ShadowImageLayer v3

The "Ignore visible region..." patch is not applicable to the trunk anymore. I even tested hwc on gecko v1-train. I can't reproduce the bug this patch was supposed to fix.

Obsoleting.
Attachment #744398 - Attachment is obsolete: true
Attachment #744395 - Attachment description: Patch 1 - HWC multirect visible region v5 → Patch 1 of 4 - HWC multirect visible region v5
Rebase patch 2. Carry forward r=mwu
Attachment #744399 - Attachment is obsolete: true
Attachment #745573 - Flags: review+
Rebase patch 3. Carry forward r=mwu
Attachment #745574 - Flags: review+
Attachment #744402 - Attachment is obsolete: true
Rebase patch 4. Carry forward r=bjacob.
Attachment #744405 - Attachment is obsolete: true
Attachment #745576 - Flags: review+
(In reply to Diego Wilson [:diego] from comment #42)
> Created attachment 745574 [details] [diff] [review]
> Patch 3 of 4 - Don't render semitransparent color layers in HwcComposer2D v4
> 
> Rebase patch 3. Carry forward r=mwu

ColorLayers are created as an optimization. If they're actually a *de*optimization in some cases, we could have layout detect those cases and fall back to the default behavior, which is to render the color to a ThebesLayer buffer and use that. That's a bit more complex, but if you see any cases in the wild where this ColorLayer behavior is causing us to fall off the HWC path, we can make the fix.
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.