WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
86425
[chromium] Expose layer tiling size options to WebSettings
https://bugs.webkit.org/show_bug.cgi?id=86425
Summary
[chromium] Expose layer tiling size options to WebSettings
Vangelis Kokkevis
Reported
2012-05-14 18:55:23 PDT
Currently the default tile dimensions and maximum un-tiled layer size are hardcoded in TiledLayerChromium. This makes it hard to experiment with different tiling strategies. Exposing these values via WebSettings will allow us to pass the desired tiling parameters via command-line flags.
Attachments
Patch
(14.21 KB, patch)
2012-05-15 12:40 PDT
,
Vangelis Kokkevis
no flags
Details
Formatted Diff
Diff
Patch
(13.99 KB, patch)
2012-05-17 09:53 PDT
,
Vangelis Kokkevis
no flags
Details
Formatted Diff
Diff
Patch
(14.00 KB, patch)
2012-05-17 12:04 PDT
,
Vangelis Kokkevis
no flags
Details
Formatted Diff
Diff
Patch
(14.37 KB, patch)
2012-05-17 16:49 PDT
,
Vangelis Kokkevis
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Vangelis Kokkevis
Comment 1
2012-05-15 12:40:06 PDT
Created
attachment 142029
[details]
Patch
WebKit Review Bot
Comment 2
2012-05-15 12:44:42 PDT
Please wait for approval from
abarth@webkit.org
,
dglazkov@chromium.org
,
fishd@chromium.org
,
jamesr@chromium.org
or
tkent@chromium.org
before submitting, as this patch contains changes to the Chromium public API. See also
https://trac.webkit.org/wiki/ChromiumWebKitAPI
.
Dana Jansens
Comment 3
2012-05-15 12:49:37 PDT
Comment on
attachment 142029
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=142029&action=review
I guess we may have to think and experiment a bit with occlusion tracking and different tile sizes. The 160 seemed to work well with 256x256 tiles. If tiles are 100x100, then tracking 100x100 areas would probably yield some benefits (or maybe not!). Anyhow, something for us to consider.
> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:118 > + const CCSettings settings = layerTreeHost()->settings();
reference?
James Robinson
Comment 4
2012-05-15 12:58:45 PDT
Comment on
attachment 142029
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=142029&action=review
> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:47 > +// When tiling is enabled, use tiles of this size. The LTH > +// settings can override these values.
we shouldn't need these values at all since we always grab them off the CCSettings. Can you just delete them?
> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:116 > + ASSERT(layerTreeHost());
do we always have a valid layerTreeHost in tests?
> Source/WebKit/chromium/public/WebSettings.h:151 > + virtual void setDefaultTileWidth(int) = 0; > + virtual void setDefaultTileHeight(int) = 0; > + virtual void setMaxUntiledLayerWidth(int) = 0; > + virtual void setMaxUntiledLayerHeight(int) = 0;
rather than width/height pairs can the tile parameters be WebRects (and IntRects in the WebCore code)?
> Source/WebKit/chromium/src/WebSettingsImpl.h:151 > + virtual void setDefaultTileWidth(int); > + virtual void setDefaultTileHeight(int); > + virtual void setMaxUntiledLayerWidth(int); > + virtual void setMaxUntiledLayerHeight(int);
can you put these with the rest of the virtuals (above showFPSCounter/etc)?
Vangelis Kokkevis
Comment 5
2012-05-15 15:01:26 PDT
(In reply to
comment #4
)
> (From update of
attachment 142029
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=142029&action=review
> > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:47 > > +// When tiling is enabled, use tiles of this size. The LTH > > +// settings can override these values. > > we shouldn't need these values at all since we always grab them off the CCSettings. Can you just delete them?
They are used in the constructor to create the initial CCLayerTilingData. In theory should be able to function even if updateTileSizeAndTilingOption() isn't called so we probably want to initialize it with something reasonable.
> > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:116 > > + ASSERT(layerTreeHost()); > > do we always have a valid layerTreeHost in tests?
It's used a bit further down in this function without any checks so I assume yes. I just put in the assert just in case.
> > > Source/WebKit/chromium/public/WebSettings.h:151 > > + virtual void setDefaultTileWidth(int) = 0; > > + virtual void setDefaultTileHeight(int) = 0; > > + virtual void setMaxUntiledLayerWidth(int) = 0; > > + virtual void setMaxUntiledLayerHeight(int) = 0; > > rather than width/height pairs can the tile parameters be WebRects (and IntRects in the WebCore code)?
Will do.
> > > Source/WebKit/chromium/src/WebSettingsImpl.h:151 > > + virtual void setDefaultTileWidth(int); > > + virtual void setDefaultTileHeight(int); > > + virtual void setMaxUntiledLayerWidth(int); > > + virtual void setMaxUntiledLayerHeight(int); > > can you put these with the rest of the virtuals (above showFPSCounter/etc)?
Will do.
Vangelis Kokkevis
Comment 6
2012-05-17 09:53:08 PDT
Created
attachment 142492
[details]
Patch
Vangelis Kokkevis
Comment 7
2012-05-17 09:53:57 PDT
(In reply to
comment #6
)
> Created an attachment (id=142492) [details] > Patch
Addressed review comments. Please take another look!
James Robinson
Comment 8
2012-05-17 11:16:25 PDT
Comment on
attachment 142492
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=142492&action=review
> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:129 > + const bool anyDimensionLarge = contentBounds().width() > settings.maxUntiledLayerSize.width() || contentBounds().height() > settings.maxUntiledLayerSize.height(); > + const bool anyDimensionOneTile = (contentBounds().width() <= settings.defaultTileSize.width() || contentBounds().height() <= settings.defaultTileSize.height()) > + && (contentBounds().width() * contentBounds().height()) <= (settings.maxUntiledLayerSize.width() * settings.maxUntiledLayerSize.height());
these are all quite a mouthful. any way to simplify these (maybe store some repeated invocations like contentBounds().width() in locals)? the area check appears new, what's it for?
> Source/WebKit/chromium/public/WebSettings.h:150 > + virtual void setDefaultTileSize(const WebSize&) = 0; > + virtual void setMaxUntiledLayerSize(const WebSize&) = 0;
I think we should pass WebSize by value in the WK API, it's just a struct with 2 ints. const ref is overkill
Vangelis Kokkevis
Comment 9
2012-05-17 12:03:17 PDT
(In reply to
comment #8
)
> (From update of
attachment 142492
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=142492&action=review
> > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:129 > > + const bool anyDimensionLarge = contentBounds().width() > settings.maxUntiledLayerSize.width() || contentBounds().height() > settings.maxUntiledLayerSize.height(); > > + const bool anyDimensionOneTile = (contentBounds().width() <= settings.defaultTileSize.width() || contentBounds().height() <= settings.defaultTileSize.height()) > > + && (contentBounds().width() * contentBounds().height()) <= (settings.maxUntiledLayerSize.width() * settings.maxUntiledLayerSize.height()); > > these are all quite a mouthful. any way to simplify these (maybe store some repeated invocations like contentBounds().width() in locals)? > > the area check appears new, what's it for?
The heuristic was originally in place to deal with skinny long layers such as scrollbars to avoid creating lots of tiles for them. However, if you allow for arbitrary tiling sizes, skinny becomes somewhat less well defined. Case in point: Imagine you set your defaultTileSize to (1024, 256) in order to make wide but short tiles that could be more optimal for vertical scrolling. If your window size drops to 1000 pixels wide then with the old logic you would end up creating a single tile of (1000, pageHeight) pixels up to pageHeight == maxTextureSize for the root layer. The resulting texture could end up being quite large. With this added logic, we only trigger the single-tile special case if the resulting overall tile area will be smaller than the area of the largest layer we're comfortable not tiling. We actually had a similar problem in:
http://code.google.com/p/chromium/issues/detail?id=97241#c7
so I figured it's a good fix regardless. New patch addressing the rest of the feedback is on its way. Thanks.
> > > Source/WebKit/chromium/public/WebSettings.h:150 > > + virtual void setDefaultTileSize(const WebSize&) = 0; > > + virtual void setMaxUntiledLayerSize(const WebSize&) = 0; > > I think we should pass WebSize by value in the WK API, it's just a struct with 2 ints. const ref is overkill
Vangelis Kokkevis
Comment 10
2012-05-17 12:04:39 PDT
Created
attachment 142522
[details]
Patch
Adrienne Walker
Comment 11
2012-05-17 15:05:09 PDT
Comment on
attachment 142522
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=142522&action=review
> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:49 > +// When tiling is enabled, use tiles of this size, unless the LTH > +// settings provide different values. > static int defaultTileWidth = 256; > static int defaultTileHeight = 256;
It still seems a little weird to have these values here, as if they'll ever be used. I'd almost rather just pass IntSize() as the initial tile size so that there can be no mistake about where the tile size is coming from.
Vangelis Kokkevis
Comment 12
2012-05-17 16:49:31 PDT
Created
attachment 142587
[details]
Patch
Vangelis Kokkevis
Comment 13
2012-05-17 16:50:31 PDT
(In reply to
comment #11
)
> (From update of
attachment 142522
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=142522&action=review
> > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:49 > > +// When tiling is enabled, use tiles of this size, unless the LTH > > +// settings provide different values. > > static int defaultTileWidth = 256; > > static int defaultTileHeight = 256; > > It still seems a little weird to have these values here, as if they'll ever be used. I'd almost rather just pass IntSize() as the initial tile size so that there can be no mistake about where the tile size is coming from.
Makes sense. I did what you suggested in the latest patch!
Adrienne Walker
Comment 14
2012-05-17 17:24:11 PDT
Comment on
attachment 142587
[details]
Patch Thanks! R=me.
WebKit Review Bot
Comment 15
2012-05-17 18:19:59 PDT
Comment on
attachment 142587
[details]
Patch Clearing flags on attachment: 142587 Committed
r117521
: <
http://trac.webkit.org/changeset/117521
>
WebKit Review Bot
Comment 16
2012-05-17 18:20:04 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug