1135907 - Clamp displayports to the maximum texture size.

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Since we don't want APZ to have a dependency on tiling, we should make sure that huge displayports either get clamped to the maximum texture size or get disabled (which we used to do until bug 1009961).

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: clamp.patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8569018 [details] [diff] [review]
clamp.patch

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

See comment below.

::: layout/base/nsLayoutUtils.cpp
@@ +981,5 @@
> +  nscoord width = nscoord(double(aDisplayPort->width) * resolution.width);
> +  nscoord height = nscoord(double(aDisplayPort->height) * resolution.height);
> +
> +  aDisplayPort->width = std::min(width, maxSizeInAppUnits);
> +  aDisplayPort->height = std::min(height, maxSizeInAppUnits);

I think we need to do something smarter than just clamping the width/height. The problem here is that if that say we have a base rect of (x=100,y=100,w=100,h=100) and margins (left=100,top=100,right=0,bottom=0). Then once we combine the margins on the base rect we end up with a displayport of (x=0,y=0,w=200,h=200). Now let's assume the max texture size is 100x100 and we do this clamping - we're going to end up with clamped displayport of (x=0,y=0,w=100,h=100).

Note that the base rect is roughly equivalent to the region actually visible and the margins are a hint as to what direction the APZ is expecting to move - so we should always paint the base rect fully but in this case we are not. It could lead to scenarios where we end up with perma-checkerboarding.

I think you need to move this clamping code to be more tightly incorporated into where we expand the base rect with the margins. There are also cases that we probably want to warn/assert for, such as if the base rect is larger than the texture size. In the case where we're using a provided displayport (i.e. GetDisplayPortFromRectData rather than GetDisplayPortFromMarginsData) we should also assert if the provided displayport is larger than the texture size.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Keep the base rect, but only inflate each axis up to the maximum texture size.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(Ignore the "nsRect result" change, I fixed that locally.)

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8569469 [details] [diff] [review]
v2

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

::: layout/base/nsLayoutUtils.cpp
@@ +833,5 @@
> +
> +  nsIFrame* frame = aContent->GetPrimaryFrame();
> +  if (!frame) {
> +    return nscoord_MAX;
> +  }

I think you want to include a call to nsLayoutUtils::GetDisplayRootFrame before grabbing the widget.

I'm not sure when we could have a widget between the current frame and the root, but I believe it's possible.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v3

w/ Matt's fix

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8569673 [details] [diff] [review]
v3

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

This looks fine to me with comments below addressed. However I'd feel better if tn reviewed it also.

::: layout/base/nsLayoutUtils.cpp
@@ +844,5 @@
> +  LayerManager* lm = widget->GetLayerManager();
> +  if (!lm) {
> +    return nscoord_MAX;
> +  }
> +  nsPresContext* presContext = frame->PresContext();

I think you want to be getting the presContext from the frame before you run it through GetDisplayRootFrame. The presContext for the display-root might be different from this presContext, and the two might have different DevPixelsToAppUnits conversions. I might be wrong though, not really sure.

@@ +1050,5 @@
> +
> +  if (!gfxPrefs::LayersTilesEnabled()) {
> +    // Either we should have gotten a valid rect directly from the displayport
> +    // base, or we should have computed a valid rect from the margins.
> +    NS_ASSERTION(result.width < GetMaxDisplayPortSize(aContent),

<= instead of <. Ditto on height

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8569673 [details] [diff] [review]
v3

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> I think you want to be getting the presContext from the frame before you run
> it through GetDisplayRootFrame. The presContext for the display-root might
> be different from this presContext, and the two might have different
> DevPixelsToAppUnits conversions. I might be wrong though, not really sure.

You're right, good catch.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4916ccff311d

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7175243&repo=mozilla-inbound

Comment 11

[Botond Ballo [:botond]]

Note that a Linux build with APZ enabled and e10s disabled hits this assertion without the fix in this bug: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp?rev=43eb116a884d#990

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Assertions ended up being bogus, fixed the condition and re-pushed.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/18e46b2b3315

Comment 13

[Kartikaya Gupta (email:[e-mail])]

In what way were the assertions bogus?

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Oh, I see, GetMaxTextureSize was returning -1 in some cases. I'd rather you fixed GetMaxDisplayPortSize to catch that -1 return and return nscoord_MAX to have a more normalized return value. Please land a follow-up for it.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(This is particularly egregious because LayerManager::GetMaxTextureSize() returns an int32_t and you're using a uint32_t to store it.)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18e46b2b3315

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: follow-up

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8578822 [details] [diff] [review]
follow-up

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

Thanks!

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb73c395a78d

Comment 20

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7788722&repo=mozilla-inbound

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa2afb3de8f

(GetMaxTextureSize can return INT_MAX, which overflows app units)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1aa2afb3de8f