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 on attachment 703950 [details] [diff] [review] HWC multirect visible region I could be missing something, but where do we clear mVisibleRegions?

(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

Clear visible region list on each new frame

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.

Addressed all comments from v2 patch

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

This bug should be fine.

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

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/

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

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

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

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

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

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

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

Add 8 lines of context. Carry forward r=mwu

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

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.

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

Rebase patch 2. Carry forward r=mwu

Rebase patch 3. Carry forward r=mwu

Rebase patch 4. Carry forward r=bjacob.

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

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

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

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