Bug 78305 – [Qt][Texmap] Refactor backing-store code in TextureMapper

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 126428 [details]
Patch

This is the last huge refactor patch for now, so please bear with me.

[Kenneth Rohde Christiansen]

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

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Created attachment 126530 [details]
Patch to address comments

[Noam Rosenthal]

Created attachment 126554 [details]
Patch

[Jocelyn Turcotte]

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.

[Noam Rosenthal]

Comment on attachment 126554 [details]
Patch

Removing flag due to Jocelyn's review, would address them soon :)

[Early Warning System Bot]

Comment on attachment 126554 [details]
Patch

Attachment 126554 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11447820

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 126927 [details]
Patch

[Jocelyn Turcotte]

(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!

[Noam Rosenthal]

Created attachment 126934 [details]
Patch

[Noam Rosenthal]

Created attachment 126957 [details]
Patch

[WebKit Review Bot]

Comment on attachment 126957 [details]
Patch

Clearing flags on attachment: 126957

Committed r107707: <http://trac.webkit.org/changeset/107707>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.