Summary: | Coordinated Graphics: Remove a texture if an direct composited image is off the viewport. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | noam, webkit.review.bot, zeno | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 101023 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Dongseong Hwang
2012-11-15 17:12:40 PST
Created attachment 174565 [details]
Patch
I tested snowstack http://www.satine.org/research/webkit/snowleopard/snowstack.html Previously, we create directly composited image textures endlessly. Now, we create less than 60 image textures at the peak, and then we keep 9 textures after 3 seconds. Created attachment 174598 [details]
Patch
rebased to upstream. Comment on attachment 174598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174598&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeCoordinatorProxy.h:72 > + void makeImageBackingInvisible(CoordinatedImageBackingID); Let's rename this to clearImageBackingContents > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:NaN > void CoordinatedImageBacking::update() Is this called during an animation? > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:91 > + bool changedToVisible = setInvisibleIfNeeded(); This line doesn't make sense from the caller site. Some of the names need refactoring. Created attachment 174902 [details]
Patch
(In reply to comment #5) Thanks for review! > (From update of attachment 174598 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174598&action=review > Let's rename this to clearImageBackingContents Done. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:NaN > > void CoordinatedImageBacking::update() > > Is this called during an animation? Yes, it called during an animation. CoordinatedImageBacking::update() is called every LayerTreeCoordinator::flushPendingLayerChanges(). We cannot guarantee every frame of UI Process animations calls this function, because UI Process can progress between sending RenderNextFrame message and receiving next DidRenderFrame message. However, I think CoordinatedImageBacking::update() is called often enough. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:91 > > + bool changedToVisible = setInvisibleIfNeeded(); > > This line doesn't make sense from the caller site. Some of the names need refactoring. I renamed changeVisibilityIfNeeded(bool& changedToVisible). How about it? In addition, I add some code to reduce redundant texture updates. For example, when javascript creates a directly composited img element while another directly composited img exists, the previous patch updates the texture redundantly. This patch handles this case also. Comment on attachment 174902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174902&action=review Good progress. See comments. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:503 > + backingStore->removeTile(1 /* id */); How about adding a removeAllTiles / clear function ? > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:107 > + // currentFrameUpdatedAlready() can decode an image redundantly, so we check at the last. > + // When more than two layers refer the same image, markDirty() can be called even though we already updated. > + // For example, javascript can create a directly composited img element while another directly composited img exists. > + if (currentFrameUpdatedAlready()) { > + m_isDirty = false; > + return; > + } > + } The comments are more confusing. It would be cleaner to replace this hole phrase with if (m_nativeImagePtr == m_image->nativeImageForCurrentFrame()) m_isDirty = false return > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:143 > +void CoordinatedImageBacking::changeVisibilityIfNeeded(bool& changedToVisible) updateVisibilityIfNeeded > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.h:50 > + class LayerClient { How about class Host ? Created attachment 175047 [details]
Patch
(In reply to comment #8) > (From update of attachment 174902 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174902&action=review > > Good progress. See comments. Thank you for review! > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:503 > > + backingStore->removeTile(1 /* id */); > > How about adding a removeAllTiles / clear function ? Add removeAllTiles because we can not really clear at this time. > > The comments are more confusing. > It would be cleaner to replace this hole phrase with > if (m_nativeImagePtr == m_image->nativeImageForCurrentFrame()) > m_isDirty = false > return Done. > updateVisibilityIfNeeded Done. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.h:50 > > + class LayerClient { > > How about class Host ? Good suggestion! Done. Comment on attachment 175047 [details]
Patch
I believe this is ready!
Comment on attachment 175047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175047&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:189 > + // CoordinatedImageBacking::LayerClient Oops, here is nits. I'll update Created attachment 175049 [details]
Patch
Created attachment 175052 [details]
Patch
Comment on attachment 175052 [details] Patch Rebase to upstream because this conflicts Bug 102664 (In reply to comment #15) > (From update of attachment 175052 [details]) > Rebase to upstream because this conflicts Bug 102664 (When a patch is already review, leave the r flag blank, no need to set it as r+) (In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 175052 [details] [details]) > > Rebase to upstream because this conflicts Bug 102664 > > (When a patch is already review, leave the r flag blank, no need to set it as r+) Yes. Thanks for explanation! Comment on attachment 175052 [details] Patch Clearing flags on attachment: 175052 Committed r135207: <http://trac.webkit.org/changeset/135207> All reviewed patches have been landed. Closing bug. |