Bug 78305 - [Qt][Texmap] Refactor backing-store code in TextureMapper
Summary: [Qt][Texmap] Refactor backing-store code in TextureMapper
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2012-02-09 18:03 PST by Noam Rosenthal
Modified: 2012-02-14 04:53 PST (History)
6 users (show)

See Also:


Attachments
Patch (76.04 KB, patch)
2012-02-09 18:46 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch to address comments (76.31 KB, patch)
2012-02-10 10:09 PST, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch (56.38 KB, patch)
2012-02-10 12:28 PST, Noam Rosenthal
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (78.01 KB, patch)
2012-02-11 09:26 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (72.70 KB, patch)
2012-02-14 01:10 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (72.82 KB, patch)
2012-02-14 01:36 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (72.83 KB, patch)
2012-02-14 04:31 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-02-09 18:03:15 PST
TextureMapperNode and LayerTreeHostProxy currenty includes a lot of code for dealing with backing-store. That code is hard to maintain and is messy. 
This is an attempt at making it a lot cleaner, and making the way backing-stores in TextureMapper be transparent.
Comment 1 Noam Rosenthal 2012-02-09 18:46:02 PST
Created attachment 126428 [details]
Patch

This is the last huge refactor patch for now, so please bear with me.
Comment 2 Kenneth Rohde Christiansen 2012-02-10 00:59:41 PST
Comment on attachment 126428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126428&action=review

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.h:49
> +    virtual int maxTextureDimension() { return 2000; }

Would it make sense to make it possible to use different dimentions horizontally, vertically?

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:304
> +    m_compositedImage = TextureMapperCompositedImage::get(m_image.get(), textureMapper);
> +    m_contentsLayer = m_media ? m_media : m_compositedImage.get();

Would it make sense to check for m_media first?

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:109
> +    virtual int maxTextureDimension() { return 10000; }

Why such a crazy big default? if you want to avoid tiling why not use MAX_INT or so.

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:31
> +void TextureMapperTile::updateContents(TextureMapper* textureMapper, Image* image, const IntRect& dirtyRect)
> +{
> +    IntRect targetRect = enclosingIntRect(m_rect);

Please notice that webkit just switched to floats for layouts. Maybe we soon need to change to using LayoutRects instead

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:44
> +        m_texture->reset(targetRect.size(), false);

specify what the false means

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:59
> +void TextureMapperBackingStore::computeTilesIfNeeded(const FloatSize& size, int tileSize)

It seems more like a createTilesIfNeeded()

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:80
> +        bool found = false;

bool existsAlready = false?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:82
> +PassRefPtr<TextureMapperBackingStore> TextureMapperNode::getBackingStore()

why not just backingStore() ?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:99
> +    FloatRect dirtyRect = m_state.needsDisplay ? layerRect() : m_state.needsDisplayRect;

is this correct?

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:49
> +#if PLATFORM(QT)
> +    QImage qtImage = m_backBuffer->createQImage();
> +    pixels = qtImage.constBits();
> +#endif

You intent this file to move out of /qt/ ?

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:50
> +    texture->updateContents(pixels, IntRect(IntPoint(), m_backBuffer->size()));

why not use ::zero() here
Comment 3 Noam Rosenthal 2012-02-10 09:14:27 PST
(In reply to comment #2)
> (From update of attachment 126428 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126428&action=review
> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.h:49
> > +    virtual int maxTextureDimension() { return 2000; }
> 
> Would it make sense to make it possible to use different dimentions horizontally, vertically?
It might look nice, but what is the use for that?

> 
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:304
> > +    m_compositedImage = TextureMapperCompositedImage::get(m_image.get(), textureMapper);
> > +    m_contentsLayer = m_media ? m_media : m_compositedImage.get();
> 
> Would it make sense to check for m_media first?
You mean before testing for m_compositedImage? that wouldn't work, because we need to reset the composited image if m_image is 0.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapper.h:109
> > +    virtual int maxTextureDimension() { return 10000; }
> 
> Why such a crazy big default? if you want to avoid tiling why not use MAX_INT or so.
For super-huge images I still want a bit of tiling.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:31
> > +void TextureMapperTile::updateContents(TextureMapper* textureMapper, Image* image, const IntRect& dirtyRect)
> > +{
> > +    IntRect targetRect = enclosingIntRect(m_rect);
> 
> Please notice that webkit just switched to floats for layouts. Maybe we soon need to change to using LayoutRects instead
Right.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:44
> > +        m_texture->reset(targetRect.size(), false);
> 
> specify what the false means
Sure, had a boolean trap there... I'll add a comment and make it into an enum in a different patch.

> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:82
> > +PassRefPtr<TextureMapperBackingStore> TextureMapperNode::getBackingStore()
> 
> why not just backingStore() ?
Because it might create the backing-store; I like using the word get when the function can have side-effects.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:99
> > +    FloatRect dirtyRect = m_state.needsDisplay ? layerRect() : m_state.needsDisplayRect;
> 
> is this correct?
Yes. needsDisplay means we have to update the entire layerRect.

> 
> > Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:49
> > +#if PLATFORM(QT)
> > +    QImage qtImage = m_backBuffer->createQImage();
> > +    pixels = qtImage.constBits();
> > +#endif
> 
> You intent this file to move out of /qt/ ?
Yes, most of this is not Qt specific though we're the only ones to do this cross-process AC so far.
Comment 4 Noam Rosenthal 2012-02-10 10:09:10 PST
Created attachment 126530 [details]
Patch to address comments
Comment 5 Noam Rosenthal 2012-02-10 12:28:14 PST
Created attachment 126554 [details]
Patch
Comment 6 Jocelyn Turcotte 2012-02-10 12:50:30 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=126530&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:59
> +void TextureMapperBackingStore::createOrDestroyTilesIfNeeded(const FloatSize& size, const IntSize& tileSize)

A few comments in that method could help.

> Source/WebCore/platform/graphics/texmap/TextureMapperCompositedImage.cpp:31
> +static HashMap<int64_t, RefPtr<TextureMapperCompositedImage> > s_imageBackingStores;

The map itself wouldn't be cleared, isn't that a bit dirty?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:103
> +    m_backingStore->updateContents(textureMapper, layer, IntRect(dirtyRect));

Tiles swapBuffers are being done as an obscure side effect of m_backingStore->updateContents inside updateBackingStore inside syncCompositingState. This is an important step and I think it would be nice if it was more explicit, directly called from LayerTreeHostProxy on the LayerBackingStore after the layer state sync.

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:32
> +void LayerBackingStoreTile::swapBuffers(WebCore::TextureMapper* textureMapper)

Maybe have something in the name that buffers swapping only happens if needed.

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:76
> +    clear();

I've spent 15 minutes trying to understand the code until I saw this line, please add a comment :)
It seems unnecessary to do all this synchronization work each time something gets updated, couldn't LayerBackingStoreTile inherit from TextureMapperTile?

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:79
> +    HashMap<int, LayerBackingStoreTile>::iterator end = m_tiles.end();
> +    Vector<TextureMapperTile>& tilesToPaint = getTiles();
> +    for (HashMap<int, LayerBackingStoreTile>::iterator it = m_tiles.begin(); it != end; ++it) {

This part is confusing, getTiles(), LayerBackingStore::m_tiles and TextureMapperBackingStore::m_tiles don't have much in their name to tell their purposes appart.
Comment 7 Noam Rosenthal 2012-02-10 13:00:29 PST
Comment on attachment 126554 [details]
Patch

Removing flag due to Jocelyn's review, would address them soon :)
Comment 8 Early Warning System Bot 2012-02-10 13:30:19 PST
Comment on attachment 126554 [details]
Patch

Attachment 126554 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11447820
Comment 9 Noam Rosenthal 2012-02-11 09:26:36 PST
Created attachment 126638 [details]
Patch

I've addressed Jocelyn's comments in this patch; Since the change is rather significant, I think it requires an additional review.
Jocelyn, what do you think of the changes? I tried to make this a lot more straightforward.
Comment 10 Noam Rosenthal 2012-02-14 01:10:25 PST
Created attachment 126927 [details]
Patch
Comment 11 Jocelyn Turcotte 2012-02-14 01:25:17 PST
(In reply to comment #9)
> I've addressed Jocelyn's comments in this patch; Since the change is rather significant, I think it requires an additional review.
> Jocelyn, what do you think of the changes? I tried to make this a lot more straightforward.

Looks good to me!
Comment 12 Noam Rosenthal 2012-02-14 01:36:18 PST
Created attachment 126934 [details]
Patch
Comment 13 Noam Rosenthal 2012-02-14 04:31:13 PST
Created attachment 126957 [details]
Patch
Comment 14 WebKit Review Bot 2012-02-14 04:53:50 PST
Comment on attachment 126957 [details]
Patch

Clearing flags on attachment: 126957

Committed r107707: <http://trac.webkit.org/changeset/107707>
Comment 15 WebKit Review Bot 2012-02-14 04:53:56 PST
All reviewed patches have been landed.  Closing bug.