Closed Bug 810334 Opened 9 years ago Closed 8 years ago

Harware composer should honor layer clip rect

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: diego, Assigned: diego)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Group: qualcomm-confidential
I would recommend that we disable hwc by default in configurations for downstream partners until we can resolve these bugs.
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
Bug 822985 is actually caused by bug 808339. Added as dependency there.
No longer blocks: 822985
Bug 823366 is actually caused by bug 808339. Added as dependency there.
Attached patch Honor clip rect (obsolete) — Splinter Review
Attachment #701952 - Flags: review?(jones.chris.g)
Assignee: nobody → dwilson
Status: NEW → ASSIGNED
blocking-b2g: --- → tef?
FYI this indeed solves bug 822611
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 ;).
Attachment #701952 - Flags: review?(jones.chris.g) → review?(vladimir)
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.
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
blocking-b2g: tef? → -
(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
blocking-b2g: - → tef?
Addressed all vlad's comments
Attachment #701952 - Attachment is obsolete: true
Attachment #701952 - Flags: review?(vladimir)
Attachment #703382 - Flags: review?(vladimir)
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.
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 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! :)
Attachment #703382 - Flags: review?(vladimir) → review+
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.
Attachment #703382 - Flags: approval-mozilla-b2g18+
blocking-b2g: tef? → tef+
Target Milestone: --- → B2G C4 (2jan on)
Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0
Does not make sense to create a regression issue.
Flags: in-moztrap-
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Diego Wilson if you can Verify this bug out, it would be much appreciated.
Flags: needinfo?(dwilson)
Keywords: verifyme
Verified. The artifacts in the gallery progress bar when HwcComposer2D is enabled (bug 822611) are gone now.
Status: RESOLVED → VERIFIED
Flags: needinfo?(dwilson)
Per comment 22,I clear "Verifyme".
You need to log in before you can comment on or make changes to this bug.