Closed Bug 1378355 Opened 7 years ago Closed 7 years ago

nsLayoutUtils::GetMaxDisplayPortSize should take into account the gfx.max-texture-size pref

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

Bug 1378355 - Remove unused function.
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Bug 1378355 - Extract gfxPlatform wrappers for MaxAllocSize and MaxTextureSize.
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Bug 1378355 - Update GetMaxDisplayPortSize to respect the gfx.max-texture-size pref.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1378355 - Move the safe-maximum displayport fallback code into the GetMaxDisplayPortSize function.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details 
See bug 1336060 comment 22 for backstory.

The nsLayoutUtils::GetMaxDisplayPortSize function tells layout what the upper bound on the displayport size is, by using the max texture size that the layer manager can handle. However, the compositor further limits the texture by the gfx.max-texture-size pref value. This means that if this pref is set, we might end up creating a displayport that's larger than the real allowed maximum which can result in badness. If GetMaxDisplayPortSize respects the pref things are less bad.
Assignee: nobody → bugmail
Comment on attachment 8883606 [details]
Bug 1378355 - Update GetMaxDisplayPortSize to respect the gfx.max-texture-size pref.

https://reviewboard.mozilla.org/r/154542/#review159690

::: layout/base/nsLayoutUtils.cpp:961
(Diff revision 1)
>  
>    int32_t maxSizeInDevPixels = lm->GetMaxTextureSize();
>    if (maxSizeInDevPixels < 0 || maxSizeInDevPixels == INT_MAX) {
>      return nscoord_MAX;
>    }
> +  // If the gfx.max-texture-size pref is set, further restrict the displayport

Shouldn't we take into account gfxPlatform::MaxTextureSize() before the return just before this code?
Comment on attachment 8883606 [details]
Bug 1378355 - Update GetMaxDisplayPortSize to respect the gfx.max-texture-size pref.

https://reviewboard.mozilla.org/r/154542/#review159694

::: layout/base/nsLayoutUtils.cpp:961
(Diff revision 1)
>  
>    int32_t maxSizeInDevPixels = lm->GetMaxTextureSize();
>    if (maxSizeInDevPixels < 0 || maxSizeInDevPixels == INT_MAX) {
>      return nscoord_MAX;
>    }
> +  // If the gfx.max-texture-size pref is set, further restrict the displayport

Do you mean before the "maxSizeInDevPixels < 0 || maxSizeInDevPixels == INT_MAX" check? I don't see why - that check is basically making sure that the value is not garbage. If we do std::min before this point we might be using a garbage value as one of the inputs.

One possibility is to replace all the "return nscoord_MAX" lines with "return gfxPlatform::MaxTextureSize()" but it doesn't sound like that's what you meant...
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment on attachment 8883606 [details]
Bug 1378355 - Update GetMaxDisplayPortSize to respect the gfx.max-texture-size pref.

https://reviewboard.mozilla.org/r/154542/#review159696

::: layout/base/nsLayoutUtils.cpp:961
(Diff revision 1)
>  
>    int32_t maxSizeInDevPixels = lm->GetMaxTextureSize();
>    if (maxSizeInDevPixels < 0 || maxSizeInDevPixels == INT_MAX) {
>      return nscoord_MAX;
>    }
> +  // If the gfx.max-texture-size pref is set, further restrict the displayport

Yeah, returning gfxPlatform::MaxTextureSize() if the layer manager returns garbage seems like the right thing to do here, does it not?
Forgot to click publish like two days ago...sorry.
Comment on attachment 8883604 [details]
Bug 1378355 - Remove unused function.

https://reviewboard.mozilla.org/r/154538/#review160602
Attachment #8883604 - Flags: review?(bas) → review+
Comment on attachment 8883605 [details]
Bug 1378355 - Extract gfxPlatform wrappers for MaxAllocSize and MaxTextureSize.

https://reviewboard.mozilla.org/r/154540/#review160604
Attachment #8883605 - Flags: review?(bas) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #7)
> Yeah, returning gfxPlatform::MaxTextureSize() if the layer manager returns
> garbage seems like the right thing to do here, does it not?

I don't think so. MaxTextureSize() by default is set to a pretty large value (32767) and is only reduced if the user manually reduces it. However, currently, if the layer manager returns garbage we use a different default value (8192) [1] which is a much more appropriate texture size for most platforms (or as the comment says, "lowest maximum texture size"). I think we want continue doing that and not suddenly start assuming the platform supports the "highest maximum texture size" (which is what I would consider the gfxPlatform::MaxTextureSize()).

[1] http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/layout/base/nsLayoutUtils.cpp#1092
That being said, I'd be ok with pushing the 8192 fallback value into the function, and returning min(8192, MaxTextureSize()) in all the places that we currently return nscoord_MAX. I'll update the patch to do that, it should address both our concerns, I think.
It ended up being a little more ugly than I expected because we need a presContext to convert between app units and dev pixels. Suggestions for improvement welcome.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25cb083ad582aa1ee5d484713e3e65a39f4f649d
Comment on attachment 8884841 [details]
Bug 1378355 - Move the safe-maximum displayport fallback code into the GetMaxDisplayPortSize function.

https://reviewboard.mozilla.org/r/155722/#review160974
Attachment #8884841 - Flags: review?(tnikkel) → review+
Comment on attachment 8883606 [details]
Bug 1378355 - Update GetMaxDisplayPortSize to respect the gfx.max-texture-size pref.

https://reviewboard.mozilla.org/r/154542/#review160976
Attachment #8883606 - Flags: review?(tnikkel) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/872c6d6ff7d0
Remove unused function. r=bas
https://hg.mozilla.org/integration/autoland/rev/513729849663
Extract gfxPlatform wrappers for MaxAllocSize and MaxTextureSize. r=bas
https://hg.mozilla.org/integration/autoland/rev/2d7d525f5496
Move the safe-maximum displayport fallback code into the GetMaxDisplayPortSize function. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/23dbfb2e451f
Update GetMaxDisplayPortSize to respect the gfx.max-texture-size pref. r=tnikkel
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: