810334 - Harware composer should honor layer clip rect

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

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

I would recommend that we disable hwc by default in configurations for downstream partners until we can resolve these bugs.

Comment 2

[Diego Wilson [:diego]]

FYI I tried patching HwcComposer2D and it doesn't solve any of the known bugs. I think there's some other Layer magic that's missing here

Comment 3

[Diego Wilson [:diego]]

Bug 822985 is actually caused by bug 808339. Added as dependency there.

Comment 4

[Diego Wilson [:diego]]

Bug 823366 is actually caused by bug 808339. Added as dependency there.

Comment 5

[Diego Wilson [:diego]]

Attachment: Honor clip rect

Comment 6

[Diego Wilson [:diego]]

FYI this indeed solves bug 822611

Comment 7

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

Comment on attachment 701952 [details] [diff] [review]
Honor clip rect

Vlad, are you comfortable reviewing this?  I'd like to increase the bus factor on this code ;).

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 701952 [details] [diff] [review]
Honor clip rect

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

One real issue/question, and a bunch of style nits.

::: widget/gonk/HwcComposer2D.cpp
@@ +213,5 @@
> +                  nsIntRect aParentClip, nsIntRect* aRenderClip) {
> +
> +    *aRenderClip = aParentClip;
> +
> +    if(!aLayerClip) {

nit: space before (, here and below

@@ +227,5 @@
> +    gfxRect r(clip);
> +    gfxRect trClip = aTransform.TransformBounds(r);
> +    trClip.Round();
> +    if (!gfxUtils::GfxRectToIntRect(trClip, &clip)) {
> +        return false;

Hmm, is this right?  If GfxRectToIntRect returns false, that means that the rectangle couldn't be represented exactly as an int rect -- that doesn't mean we should skip rendering it, no?

@@ +269,5 @@
>      }
>  
> +    nsIntRect clip;
> +    if (!CalculateClipRect(aParentTransform * aGLWorldTransform, aLayer->GetEffectiveClipRect(),
> +                aClip, &clip)) {

nit: odd indentation (maybe align args, stick { on a line all its own because of multi-line clause?)

@@ +347,5 @@
>      hwc_layer_t& hwcLayer = mList->hwLayers[current];
>  
> +    if(!PrepareLayerRects(visibleRect, transform * aGLWorldTransform,
> +                clip, bufferRect, &(hwcLayer.sourceCrop),
> +                &(hwcLayer.displayFrame))) {

nit: same nits as above.. space before (, better alignment of args (align with other args to function)

@@ +402,3 @@
>  
> +    if (!PrepareLayerList(aRoot, mScreenRect,
> +                gfxMatrix(), aGLWorldTransform)) {

nit: etc.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Minusing this since no reason for why this should block was provided.

Feel free to renominate and provide a description of what won't work if this isn't fixed if you still think this should make v1.0.0.0

Comment 10

[Diego Wilson [:diego]]

(In reply to Jonas Sicking (:sicking) from comment #9)
> Minusing this since no reason for why this should block was provided.
> 
> Feel free to renominate and provide a description of what won't work if this
> isn't fixed if you still think this should make v1.0.0.0

Please see bug 828876 for justification

Comment 11

[Diego Wilson [:diego]]

Attachment: Honor clip rect v2

Addressed all vlad's comments

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, I still have questions regarding the GfxRectToIntRect call.  It's transforming an IntRect (clip) by 'aParentTransform * aGLWorldTransform', both of which are generic gfxMatrices.  I don't think there's any guarantee that the resulting rect will be int-aligned, and if it isn't, just ignoring the GfxRectToIntRect result could lead to wrong rendering.

Instead, why not just inline CalculateClipRect code into its only call site?  That way if the RectToIntRect call returns false, you can just return false from the function and skip using hwcomposer correctly.

Comment 13

[Diego Wilson [:diego]]

Maybe I can just do what the GPU path does? I believe it just doesn't draw anything (skip the layer)

https://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#714

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 703382 [details] [diff] [review]
Honor clip rect  v2

That's.. very surprising, and makes me want to go try to create a testcase for that...  But in that case, sure, let's do it.  Can you just add a note to that effect and state why we're skipping the layer (basically what your original patch does, I guess! :)

Comment 15

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

Comment on attachment 703382 [details] [diff] [review]
Honor clip rect  v2

If we don't land this in b2g18, it's going to result in a forked gecko.  I don't care to bicker about tef+ but a forked gecko is a nightmare scenario for development.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc3b3ad2875
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1dd38fa003d6

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/bfc3b3ad2875

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0

Comment 19

[amiller]

Does not make sense to create a regression issue.

Comment 20

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 21

[cbarker]

Diego Wilson if you can Verify this bug out, it would be much appreciated.

Comment 22

[Diego Wilson [:diego]]

Verified. The artifacts in the gallery progress bar when HwcComposer2D is enabled (bug 822611) are gone now.

Comment 23

[Shally Li（leave from Mozilla）]

Per comment 22,I clear "Verifyme".