Closed Bug 1135907 Opened 5 years ago Closed 5 years ago

Clamp displayports to the maximum texture size.

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 2 obsolete files)

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).
Attached patch clamp.patch (obsolete) — Splinter Review
Attachment #8569018 - Flags: review?(bugmail.mozilla)
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.
Attachment #8569018 - Flags: review?(bugmail.mozilla) → review-
Attached patch v2 (obsolete) — Splinter Review
Keep the base rect, but only inflate each axis up to the maximum texture size.
Attachment #8569018 - Attachment is obsolete: true
Attachment #8569469 - Flags: review?(bugmail.mozilla)
(Ignore the "nsRect result" change, I fixed that locally.)
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.
Attached patch v3Splinter Review
w/ Matt's fix
Attachment #8569469 - Attachment is obsolete: true
Attachment #8569469 - Flags: review?(bugmail.mozilla)
Attachment #8569673 - Flags: review?(bugmail.mozilla)
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
Attachment #8569673 - Flags: review?(tnikkel)
Attachment #8569673 - Flags: review?(bugmail.mozilla)
Attachment #8569673 - Flags: review+
Comment on attachment 8569673 [details] [diff] [review]
v3

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8569673 - Flags: review?(tnikkel) → review+
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
Blocks: apz-linux
Assertions ended up being bogus, fixed the condition and re-pushed.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/18e46b2b3315
In what way were the assertions bogus?
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.
Keywords: leave-open
(This is particularly egregious because LayerManager::GetMaxTextureSize() returns an int32_t and you're using a uint32_t to store it.)
Attached patch follow-upSplinter Review
Attachment #8578822 - Flags: review?(bugmail.mozilla)
Comment on attachment 8578822 [details] [diff] [review]
follow-up

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

Thanks!
Attachment #8578822 - Flags: review?(bugmail.mozilla) → review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7788722&repo=mozilla-inbound
Flags: needinfo?(dvander)
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa2afb3de8f

(GetMaxTextureSize can return INT_MAX, which overflows app units)
Flags: needinfo?(dvander)
https://hg.mozilla.org/mozilla-central/rev/1aa2afb3de8f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.