Closed Bug 828531 Opened 7 years ago Closed 7 years ago

Change CSS pixels values from preferences breaks translated elements

Categories

(Firefox OS Graveyard :: Hardware, defect)

x86
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:leo+, b2g18 fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed

People

(Reporter: basiclines, Assigned: nrc)

References

Details

Attachments

(1 file, 4 obsolete files)

Install on the device latest Gaia code with custom-prefs.js and this values: 
pref("layout.css.devPixelsPerPx", "1.6875");

When Gecko tries to translate some element vertically (via CSS) there is a flickering (the element goes up for a second) as it seems to does not take in count the new CSS pixels value
(In reply to Ismael from comment #0)
> When Gecko tries to translate some element vertically (via CSS) there is a
> flickering (the element goes up for a second) as it seems to does not take
> in count the new CSS pixels value

Need more information about the testcase. Does this involve transitions/animations?
Yes this happens when applying transitions/animations
Nick, can you reproduce this on an OMTA desktop build? If so, please fix it :-).
Assignee: jones.chris.g → ncameron
Basically, try OMTA testcases with full-zoom enabled so the ratio of CSS pixels to device pixels is not 1-1.
Yes, I can reproduce this. Now for the hard part...
I can repro this without throttling, but not without OMTA.
Attached patch fix (obsolete) — Splinter Review
Attachment #712097 - Flags: review?(roc)
Comment on attachment 712097 [details] [diff] [review]
fix

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

Glad you tracked this down!

::: gfx/layers/ipc/PLayers.ipdl
@@ +144,3 @@
>    gfxPoint3D mozOrigin;
>    gfxPoint3D perspectiveOrigin;
>    nsRect bounds;

Can you please add some comments documenting all these?

::: layout/base/nsDisplayList.cpp
@@ +393,5 @@
>        }
>      }
>      nsPoint origin = aItem->ToReferenceFrame();
>  
> +    int32_t auPerCSSPixel = frame->PresContext()->AppUnitsPerDevPixel();

"auPerCSSPixel" doesn't seem like the right name here!
Attached patch fix (obsolete) — Splinter Review
with comments and a name change
Attachment #712097 - Attachment is obsolete: true
Attachment #712097 - Flags: review?(roc)
Attachment #712283 - Flags: review?(roc)
Comment on attachment 712283 [details] [diff] [review]
fix

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +723,5 @@
>                                                       data.perspectiveOrigin(),
>                                                       data.perspective());
>    gfx3DMatrix transform =
>      nsDisplayTransform::GetResultingTransformMatrix(props, data.origin(),
>                                                      nsDeviceContext::AppUnitsPerCSSPixel(),

shouldn't this use appUnitsPerDevPixel as well? I'm pretty sure it should!

::: gfx/layers/ipc/PLayers.ipdl
@@ +143,3 @@
>    nsPoint origin;
> +  // origin in device pixels
> +  gfxPoint3D scaledOrigin;

Why is this a gfxPoint3D? You only need x/y right?

I think we really just want to pass appUnitsPerDevPixel here anyway.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Comment on attachment 712283 [details] [diff] [review]
> fix
> 
> Review of attachment 712283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +723,5 @@
> >                                                       data.perspectiveOrigin(),
> >                                                       data.perspective());
> >    gfx3DMatrix transform =
> >      nsDisplayTransform::GetResultingTransformMatrix(props, data.origin(),
> >                                                      nsDeviceContext::AppUnitsPerCSSPixel(),
> 
> shouldn't this use appUnitsPerDevPixel as well? I'm pretty sure it should!
> 
I thought so too, but that animates incorrectly - when we use perDevPixel here it overcompensates the scale.

> ::: gfx/layers/ipc/PLayers.ipdl
> @@ +143,3 @@
> >    nsPoint origin;
> > +  // origin in device pixels
> > +  gfxPoint3D scaledOrigin;
> 
> Why is this a gfxPoint3D? You only need x/y right?
> 
Because it gets used as a gfx3DPoint in CompositorParent, and I thought it was easier to do it this way than to convert it later...

> I think we really just want to pass appUnitsPerDevPixel here anyway.

...but this is better in any case.
Attached patch fix (obsolete) — Splinter Review
Attachment #712283 - Attachment is obsolete: true
Attachment #712283 - Flags: review?(roc)
Attachment #712321 - Flags: review?(roc)
(In reply to Nick Cameron [:nrc] from comment #11)
> I thought so too, but that animates incorrectly - when we use perDevPixel
> here it overcompensates the scale.

That doesn't make sense to me. Does it make sense to you?

I suspect that this should be appunits-per-dev-pixel and that changing it to be appunits-per-dev-pixel has exposed another bug.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> (In reply to Nick Cameron [:nrc] from comment #11)
> > I thought so too, but that animates incorrectly - when we use perDevPixel
> > here it overcompensates the scale.
> 
> That doesn't make sense to me. Does it make sense to you?

It does not make sense to me.

> 
> I suspect that this should be appunits-per-dev-pixel and that changing it to
> be appunits-per-dev-pixel has exposed another bug.

I will attempt to find it...
Attached patch patch (obsolete) — Splinter Review
Attachment #712321 - Attachment is obsolete: true
Attachment #712321 - Flags: review?(roc)
Attachment #712819 - Flags: review?(roc)
Comment on attachment 712819 [details] [diff] [review]
patch

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

Very good!

::: gfx/layers/Layers.h
@@ +861,5 @@
>    gfxPoint GetFixedPositionAnchor() { return mAnchor; }
>    Layer* GetMaskLayer() { return mMaskLayer; }
>  
> +  // Note that all animation data is in either css pixels or app units
> +  // and must be converted to device pixels.

// Note that all lengths in animation data are either in CSS pixels or app units
 // and must be converted to device pixels by the compositor.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +723,5 @@
> +  gfxPoint3D mozOrigin = data.mozOrigin();
> +  mozOrigin.x = mozOrigin.x * float(nsDeviceContext::AppUnitsPerCSSPixel())
> +                            / data.appUnitsPerDevPixel();
> +  mozOrigin.y = mozOrigin.y * float(nsDeviceContext::AppUnitsPerCSSPixel())
> +                            / data.appUnitsPerDevPixel();

Factor out AppUnitsPerCSSPixel/data.appUnitsPerDevPixel into a double and use it repeatedly here.
Attachment #712819 - Flags: review?(roc) → review+
carrying r=roc
Attachment #712819 - Attachment is obsolete: true
Attachment #713064 - Flags: review+
blocking-b2g: --- → tef?
tef? because we need this for OMTA (and thus smooth animations) to work for any resolution other than the default unscaled one.
I'm not sure which release the hidpi devices are tracking, but whichever that is, this needs to block it.
https://hg.mozilla.org/mozilla-central/rev/300ea51c6353
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Bumping to leo? since this is not a tef/shira blocker.
blocking-b2g: tef? → leo?
Peak is hidpi.
blocking-b2g: leo? → leo+
is this going to land in b2g-18 or just in master one?
Thx!
blocking-b2g: leo+ → ---
Did you mean to clear blocking-b2g?
The patch is very safe and can probably land wherever it's needed.

However, I hope we're not planning to add all kinds of high-DPI support to the B2G18 branch. There might be new features needed like high-DPI canvas that wouldn't be suitable for the 18 branch.
(In reply to Ismael from comment #24)
> is this going to land in b2g-18 or just in master one?
> Thx!

Yes, this should land on b2g-18, if that is the right branch for leo+ patches. I was waiting for someone to confirm that. Also, can I have my leo+ back please?
blocking-b2g: --- → leo?
Flags: in-moztrap-
Duplicate of this bug: 822455
You need to log in before you can comment on or make changes to this bug.