832383 - Hardware composer multirect visible regions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Diego Wilson [:diego]]

Attachment: HWC multirect visible region

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

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 703950 [details] [diff] [review]
HWC multirect visible region

I could be missing something, but where do we clear mVisibleRegions?

Comment 2

[Diego Wilson [:diego]]

(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

Comment 3

[Diego Wilson [:diego]]

Attachment: HWC multirect visible region v2

Clear visible region list on each new frame

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 5

[Diego Wilson [:diego]]

Attachment: HWC multirect visible region v3

Addressed all comments from v2 patch

Comment 6

[Diego Wilson [:diego]]

Attachment: Ignore visible region for ShadowCanvasLayer and ShadowImageLayer v1

Comment 7

[Diego Wilson [:diego]]

Attachment: Ensure color fill rect stays inside screen bounds v1

Comment 8

[Diego Wilson [:diego]]

Attachment: Don't render semitransparent color layers in HwcComposer2D v1

Comment 9

[Diego Wilson [:diego]]

Attachment: Get surface dimensions from SurfaceDecriptorGralloc v1

Comment 10

[Diego Wilson [:diego]]

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

Comment 11

[Michael Wu [:mwu]]

This bug should be fine.

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

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

Comment 13

[Diego Wilson [:diego]]

Amelie,

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

Comment 14

[Diego Wilson [:diego]]

: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

Comment 15

[Diego Wilson [:diego]]

s/HWC builds/Moz builds/

Comment 16

[Michael Wu [:mwu]]

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 //

Comment 17

[Michael Wu [:mwu]]

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 18

[Michael Wu [:mwu]]

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 19

[Michael Wu [:mwu]]

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()?

Comment 20

[Diego Wilson [:diego]]

Attachment: HWC multirect visible region v4

Addressed all nits in in v3 patch. Carry forward r=mwu

Comment 21

[Diego Wilson [:diego]]

Attachment: Ignore visible region for ShadowCanvasLayer and ShadowImageLayer v2

Comment 22

[Diego Wilson [:diego]]

Attachment: Ensure color fill rect stays inside screen bounds v2

Comment 23

[Diego Wilson [:diego]]

Attachment: Don't render semitransparent color layers in HwcComposer2D v2

Comment 24

[Diego Wilson [:diego]]

Attachment: Get surface dimensions from SurfaceDecriptorGralloc v2

Comment 25

[Diego Wilson [:diego]]

(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

Comment 26

[Diego Wilson [:diego]]

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

Comment 27

[Nick Cameron [:nrc]]

Please use 8 lines of context for your patches, it is Mozilla style and makes them easier to read, thanks!

Comment 28

[Nick Cameron [:nrc]]

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 29

[Nick Cameron [:nrc]]

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

Comment 30

[Michael Wu [:mwu]]

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

Comment 31

[Diego Wilson [:diego]]

Good point. The prob may only be with RoundOut(). Let me try out Round().

Comment 32

[Diego Wilson [:diego]]

Attachment: Patch 1 of 4 - HWC multirect visible region v5

Added 8 lines of context. Carry forward r=mwu

Comment 33

[Diego Wilson [:diego]]

Attachment: Ignore visible region for ShadowCanvasLayer and ShadowImageLayer v3

Add 8 lines of context. Address all comments from nrc to v2 patch

Comment 34

[Diego Wilson [:diego]]

Attachment: Ensure color fill rect stays inside screen bounds v3

Add 8 lines of context. Address all comments from mwu to v2 patch

Comment 35

[Michael Wu [:mwu]]

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

Comment 36

[Diego Wilson [:diego]]

Attachment: Don't render semitransparent color layers in HwcComposer2D v3

Add 8 lines of context. Carry forward r=mwu

Comment 37

[Diego Wilson [:diego]]

Attachment: Get surface dimensions from SurfaceDecriptorGralloc v3

Add 8 lines of context. Address all comments from nrc to v2 patch

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 39

[Diego Wilson [:diego]]

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.

Comment 40

[Diego Wilson [:diego]]

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.

Comment 41

[Diego Wilson [:diego]]

Attachment: Patch 2 of 4 - Ensure color fill rect stays inside screen bounds v4

Rebase patch 2. Carry forward r=mwu

Comment 42

[Diego Wilson [:diego]]

Attachment: Patch 3 of 4 - Don't render semitransparent color layers in HwcComposer2D v4

Rebase patch 3. Carry forward r=mwu

Comment 43

[Diego Wilson [:diego]]

Attachment: Patch 4 of 4 - Get surface dimensions from SurfaceDecriptorGralloc v4

Rebase patch 4. Carry forward r=bjacob.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/5feac049908f
https://hg.mozilla.org/projects/birch/rev/f52a8c803f26
https://hg.mozilla.org/projects/birch/rev/a49d9292b321
https://hg.mozilla.org/projects/birch/rev/eb146629efa6

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5feac049908f
https://hg.mozilla.org/mozilla-central/rev/f52a8c803f26
https://hg.mozilla.org/mozilla-central/rev/a49d9292b321
https://hg.mozilla.org/mozilla-central/rev/eb146629efa6

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9a40aae14db1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/359903054a15
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ca4c1a98558b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/91196d879be0

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b3d4005cf3e7
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b9d55902968a
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/995f2f3f2556
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c8bd882ee3e6