Summary: | [Qt][Texmap] Refactor backing-store code in TextureMapper | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Noam Rosenthal <noam> | ||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | jturcotte, kenneth, menard, mrobinson, webkit.review.bot, zoltan | ||||||||||||||||
Priority: | P3 | Keywords: | Qt | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Noam Rosenthal
2012-02-09 18:03:15 PST
Created attachment 126428 [details]
Patch
This is the last huge refactor patch for now, so please bear with me.
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 (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. Created attachment 126530 [details]
Patch to address comments
Created attachment 126554 [details]
Patch
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 on attachment 126554 [details]
Patch
Removing flag due to Jocelyn's review, would address them soon :)
Comment on attachment 126554 [details] Patch Attachment 126554 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11447820 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.
Created attachment 126927 [details]
Patch
(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! Created attachment 126934 [details]
Patch
Created attachment 126957 [details]
Patch
Comment on attachment 126957 [details] Patch Clearing flags on attachment: 126957 Committed r107707: <http://trac.webkit.org/changeset/107707> All reviewed patches have been landed. Closing bug. |