Description
Dongseong Hwang
2012-12-27 02:41:16 PST
Created attachment 180783 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
Created attachment 180784 [details]
WIP Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Not for review: I need to test more. I need feedback, especially how to deal with [QT_]WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS Created attachment 180785 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Comment on attachment 180783 [details] Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] View in context: https://bugs.webkit.org/attachment.cgi?id=180783&action=review I wanted to do this for ages but was lazy :) However I have some comments. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:121 > + TextureMapperPaintParameters parameters; > + parameters.modelViewMatrix = transform; > + parameters.opacity = options.opacity; > + parameters.mask = options.mask.get(); Parameters and options are colliding here. We should move TextureMapperPaintOptions to be defined in TextureMapperLayer.cpp, and have an internal member of TextureMapperPaintParameters. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:32 > +class TextureMapperPaintParameters { This should be in TextureMapper.h, maybe later we'd want to change the signature of TextureMapper functions to use it. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:34 > + TransformationMatrix modelViewMatrix; transform instead of modelViewMatrix. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:36 > + BitmapTexture* mask; RefPtr > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:38 > + TextureMapperPaintParameters() Any reason why targetRect is left out? Comment on attachment 180785 [details] Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2] View in context: https://bugs.webkit.org/attachment.cgi?id=180785&action=review This is actually a big change to how we use repaint counters, which would make them slow and unusable since we draw text and upload it for every paint even if there is no update. > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:402 > + updateDebugBorderAndRepaintCount(); This is wrong :) repaint counter should be updated when the backing store is actually updated, not when the layer is flushed. (In reply to comment #6) > (From update of attachment 180785 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180785&action=review > > This is actually a big change to how we use repaint counters, which would make them slow and unusable since we draw text and upload it for every paint even if there is no update. Very valid concern. Currently CoordinatedBackingStore draws repaint count in this slow way. I apply this way to all platform layers. I think we can refactor later. And IMHO it would be not bad to draw debug visual slowly. My later improvement idea is to prepare repaint count texture in TextureMapperLayer::flushCompositingStateForThisLayerOnly() if (changeMask & RepaintCountChange) { m_state.repaintCount = graphicsLayer->repaintCount(); prepare texture!! } > > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:402 > > + updateDebugBorderAndRepaintCount(); > > This is wrong :) > repaint counter should be updated when the backing store is actually updated, not when the layer is flushed. Yes, so I checked several conditions in updateDebugBorderAndRepaintCount(). // only WK1 increases repaint count at this point. if (!m_hasOwnBackingStore) return; // If the layer has a backing store and dirty rect, we increases repaint count. bool needsToRepaint = shouldHaveBackingStore() && (m_needsDisplay || !m_needsDisplayRect.isEmpty()) (In reply to comment #5) > (From update of attachment 180783 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180783&action=review > > I wanted to do this for ages but was lazy :) > However I have some comments. While you improve overall performance of AC like background color, I can contribute like this readability refactoring :) > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:121 > > + TextureMapperPaintParameters parameters; > > + parameters.modelViewMatrix = transform; > > + parameters.opacity = options.opacity; > > + parameters.mask = options.mask.get(); > > Parameters and options are colliding here. > We should move TextureMapperPaintOptions to be defined in TextureMapperLayer.cpp, and have an internal member of TextureMapperPaintParameters. It is something like what I want to hear from you. > > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:38 > > + TextureMapperPaintParameters() > > Any reason why targetRect is left out? For example, CoordinatedBackingStore needs to make new TextureMapperPaintParameters per tile if CoordinatedBackingStore stores targetRect. However, we can store targetRect in TextureMapperPaintParameters if we want :) And there is no reason TextureMapper is left out. Do you think we should store all parameters including TextureMapper and targetRect in TextureMapperPaintParameters? btw, could you review Bug 105760 and Bug 105781 firstly. This bug depends on both physically. Created attachment 180852 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
Comment on attachment 180852 [details] Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] View in context: https://bugs.webkit.org/attachment.cgi?id=180852&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:33 > + TextureMapperPaintParameters parameters; I don't store targetRect in TextureMapperPaintParameters because it is weird for TextureMapperPaintOptions to have targetRect as well as the tile related reason as I mentioned earlier. btw, I encounter new problem. I will add new member into TextureMapperPaintParameters: bool showDebugBorders; Color debugBorderColor; float debugBorderWidth; bool showRepaintCounter; int repaintCount; But it is weird for TextureMapperPaintOptions to have those new members. IMO, TextureMapperPaintOptions has duplicated members although it collides TextureMapperPaintParameters because both class's purpose is different. How do you think? Created attachment 180856 [details]
Not for Review(WIP) : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Comment on attachment 180856 [details] Not for Review(WIP) : [TexMap] Refactor code related to debug border and repaint count. [2/2] View in context: https://bugs.webkit.org/attachment.cgi?id=180856&action=review To clarify my problem, I submitted this WIP second patch. > Source/WebCore/platform/graphics/texmap/TextureMapper.h:66 > + int repaintCount; 5 members is not needed by TextureMapperPaintOptions although TextureMapperPaintParameters is a member of TextureMapperPaintOptions. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:117 > + parameters.repaintCount = m_state.repaintCount; This ugly code is still needed. Comment on attachment 180852 [details] Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] View in context: https://bugs.webkit.org/attachment.cgi?id=180852&action=review I'm doing some work on mask code, which would make it so that we actually remove the "mask" parameter from textureMapperPlatformLayer. I'm thinking overloads of TextureMapperLayer should *always* support only transform and opacity, and for everything else they should provide a BitmapTexture and let TextureMapperLayer handle it. >> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:33 >> + TextureMapperPaintParameters parameters; > > I don't store targetRect in TextureMapperPaintParameters because it is weird for TextureMapperPaintOptions to have targetRect as well as the tile related reason as I mentioned earlier. > > btw, I encounter new problem. I will add new member into TextureMapperPaintParameters: > bool showDebugBorders; > Color debugBorderColor; > float debugBorderWidth; > bool showRepaintCounter; > int repaintCount; > > But it is weird for TextureMapperPaintOptions to have those new members. > > IMO, TextureMapperPaintOptions has duplicated members although it collides TextureMapperPaintParameters because both class's purpose is different. > How do you think? I think it's ok to have duplicated members. I meant they collide in a way where it's hard for a person reading the code to understand which is which. Regarding the repaint counters - it should not be part of the paint parameters. It should be part of the "updateContents" process. Comment on attachment 180852 [details] Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] View in context: https://bugs.webkit.org/attachment.cgi?id=180852&action=review >>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:33 >>> + TextureMapperPaintParameters parameters; >> >> I don't store targetRect in TextureMapperPaintParameters because it is weird for TextureMapperPaintOptions to have targetRect as well as the tile related reason as I mentioned earlier. >> >> btw, I encounter new problem. I will add new member into TextureMapperPaintParameters: >> bool showDebugBorders; >> Color debugBorderColor; >> float debugBorderWidth; >> bool showRepaintCounter; >> int repaintCount; >> >> But it is weird for TextureMapperPaintOptions to have those new members. >> >> IMO, TextureMapperPaintOptions has duplicated members although it collides TextureMapperPaintParameters because both class's purpose is different. >> How do you think? > > I think it's ok to have duplicated members. I meant they collide in a way where it's hard for a person reading the code to understand which is which. > Regarding the repaint counters - it should not be part of the paint parameters. It should be part of the "updateContents" process. How about renaming from TextureMapperPaintParameters to TextureMapperPlatformLayerPaintOptions. IMO it is natural for repaintCount to belong to the paint parameters because we count repaintCount based on GraphicsLayer, not backing store. So TextureMapperLayer holds repaintCount and pass to backing store at painting time. And then TextureMapperLayer draws repaintCount using TextureMapperGL. As I mentioned earlier, we can prepare repaintCount texture at TextureMapperLayer::flushCompositingLayer process. I think it is reasonable for backing store to count repaintCount at updateContents process but we don't have a single point which applies repaintCount logic to all platform layers. And it needs another huge refactoring. If needed, we can make it in further refactoring. Created attachment 181151 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
Comment on attachment 181151 [details] Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] View in context: https://bugs.webkit.org/attachment.cgi?id=181151&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:122 > + parameters.mask = options.mask.get(); TextureMapperPaintOptions has duplicated members to TextureMapperPlatformLayerPaintParameters because both classes have different purpose as we discussed. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:34 > +class TextureMapperPlatformLayerPaintParameters { Rename TextureMapperPlatformLayerPaintParameters to distinguish with TextureMapperPaintOptions. Replace it in TextureMapperPlatformLayer.h because TextureMapperPlatformLayerPaintParameters is part of TextureMapperPlatformLayer API. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40 > + BitmapTexture* mask; Use BitmapTexture* instead of RefPtr<BitmapTexture> because I don't want "#include <BitmapTexture.h>" I think after noamr refactors mask, he will remove the mask member. Created attachment 182266 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
Created attachment 182291 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Created attachment 182317 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]
ping. IMO patch [1/2] has no problem. could you review the patch. And I need more feedback for patch [2/2]. Comment on attachment 182266 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
In second thought, I think this is not a good direction.
By making it easy to add parameters to the paint functions, we make things easy to silently regress in subclasses of TextureMapperPlatformLayer if we add more parameters.
I prefer to even remove the mask parameters, and have that paint function always be paintToTextureMapper(tm, rect, transform, opacity), giving the reset of the control to the subclass, and add a TextureMapper::setMask/setDebugBorder etc.
Created attachment 182652 [details]
Patch : [Qt][EFL][GTK][TexMap] Refactor code related to debug border and repaint count.
(In reply to comment #21) > (From update of attachment 182266 [details]) > In second thought, I think this is not a good direction. > By making it easy to add parameters to the paint functions, we make things easy to silently regress in subclasses of TextureMapperPlatformLayer if we add more parameters. > > I prefer to even remove the mask parameters, and have that paint function always be paintToTextureMapper(tm, rect, transform, opacity), giving the reset of the control to the subclass, and add a TextureMapper::setMask/setDebugBorder etc. I agree. I felt some weirdness when making. It seems to be because of your explanation. I gave up drawing debug visual via piggy-backing paintToTextureMapper(). Now I explicitly calls drawDebugVisuals() from TextureMapperLayer. I made this patch as prerequisite of actor model, because debug visuals make GraphicsLayerTextureMapper and TextureMapperLayer code complex currently. IMO this patch also helps you make actor model implementation. Comment on attachment 182652 [details] Patch : [Qt][EFL][GTK][TexMap] Refactor code related to debug border and repaint count. View in context: https://bugs.webkit.org/attachment.cgi?id=182652&action=review > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:149 > +void TextureMapper::drawDebugVisuals(const FloatRect& targetRect, const TransformationMatrix& transform, bool showDebugBorders, const Color& borderColor, float borderWidth, bool showRepaintCounter, int repaintCount) I will optimize this method later (in follow-up bug) via reusing repaint count texture. I will pass BitmapImage for repaintCount instead of repaintCount. The BitmapImage will be created in TextureMapperLayer::flushCompositingStateForThisLayerOnly() void TextureMapperLayer::flushCompositingStateForThisLayerOnly(GraphicsLayerTextureMapper* graphicsLayer) { .... if (changeMask & RepaintCountChange) m_state.repaintCount = graphicsLayer->repaintCount(); <-- HERE .... } Created attachment 182677 [details]
Patch
Comment on attachment 182677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182677&action=review > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:369 > + if (isShowingRepaintCounter()) { > + incrementRepaintCount(); > + m_changeMask |= TextureMapperLayer::RepaintCountChange; > + } This change seems a bit different; Maybe it should come in a different patch? > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:427 > + // When this has its own backing store (i.e. Qt WK1), update the repaint count before calling TextureMapperLayer::flushCompositingStateForThisLayerOnly(). (i.e. Qt WK1) -> you mean e.g.; GTK also uses ownBackingStore. > Source/WebCore/platform/graphics/texmap/TextureMapper.h:136 > + void drawDebugVisuals(const FloatRect&, const TransformationMatrix&, bool showDebugBorders, const Color&, float borderWidth, bool showRepaintCounter, int repaintCount); bool showDebugBorders seems unnecessary; use a color with no alpha or borderWidth of zero. Same with repaintCount; if it's 0 there's no point in painting it. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:149 > + if (m_state.showDebugBorders || m_state.showRepaintCounter) > + m_backingStore->drawDebugVisuals(options.textureMapper, layerRect(), transform, m_state.showDebugBorders, m_state.debugBorderColor, m_state.debugBorderWidth, m_state.showRepaintCounter, m_state.repaintCount); > } > > if (m_contentsLayer) { > ASSERT(!layerRect().isEmpty()); > m_contentsLayer->paintToTextureMapper(options.textureMapper, m_state.contentsRect, transform, opacity, mask.get()); > + if (m_state.showDebugBorders || m_state.showRepaintCounter) > + m_contentsLayer->drawDebugVisuals(options.textureMapper, m_state.contentsRect, transform, m_state.showDebugBorders, m_state.debugBorderColor, m_state.debugBorderWidth, m_state.showRepaintCounter, m_state.repaintCount); The API here seems a bit messy; Any particular reason to have drawDebugVisuals as one function with booleans instead of two functions? > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.cpp:38 > +void TextureMapperPlatformLayer::drawDebugVisuals(TextureMapper* textureMapper, const FloatRect& targetRect, const TransformationMatrix& transform, bool showDebugBorders, const Color& borderColor, float borderWidth, bool showRepaintCounter, int repaintCount) > +{ > + textureMapper->drawDebugVisuals(targetRect, transform, showDebugBorders, borderColor, borderWidth, showRepaintCounter, repaintCount); > +} This can be done in the header file. > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:39 > + virtual void drawDebugVisuals(TextureMapper*, const FloatRect&, const TransformationMatrix&, bool showDebugBorders, const Color&, float borderWidth, bool showRepaintCounter, int repaintCount); You really need it as a virtual function only for directly composited images; Otherwise you can use the layer transform normally. I think all this virtualization should be removed; We should draw the repaint counters only for the main backing store of the layer. Wouldn't [TexMap][WK2] be enough tags for this bug? is it REALLY necessary to mention all the ports using texmap? :) Comment on attachment 182677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182677&action=review >> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:369 >> + } > > This change seems a bit different; Maybe it should come in a different patch? you're right. follow-up patch will contain it. >> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:427 >> + // When this has its own backing store (i.e. Qt WK1), update the repaint count before calling TextureMapperLayer::flushCompositingStateForThisLayerOnly(). > > (i.e. Qt WK1) -> you mean e.g.; GTK also uses ownBackingStore. you're right. I was confused i.e. with e.g. >> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:149 >> + m_contentsLayer->drawDebugVisuals(options.textureMapper, m_state.contentsRect, transform, m_state.showDebugBorders, m_state.debugBorderColor, m_state.debugBorderWidth, m_state.showRepaintCounter, m_state.repaintCount); > > The API here seems a bit messy; Any particular reason to have drawDebugVisuals as one function with booleans instead of two functions? I wanted to handle it using only one kind of function. TextureMapper::drawDebugVisuals() and TextureMapperPlatformLayer::drawDebugVisuals(). I agree this is a bit messy. I separate like TextureMapper::drawDebugBorder() and TextureMapperPlatformLayer::drawDebugBorder(). TextureMapper::drawRepaintCounter() and TextureMapperPlatformLayer::drawRepaintCounter(). >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.cpp:38 >> +} > > This can be done in the header file. I think so. But I encountered build fail due to incomplete declaration of TextureMapper. I'll try again. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:39 >> + virtual void drawDebugVisuals(TextureMapper*, const FloatRect&, const TransformationMatrix&, bool showDebugBorders, const Color&, float borderWidth, bool showRepaintCounter, int repaintCount); > > You really need it as a virtual function only for directly composited images; Otherwise you can use the layer transform normally. > I think all this virtualization should be removed; We should draw the repaint counters only for the main backing store of the layer. I need it as a virtual function only for tiled platformLayer: CoordinatedBackingStore and TextureMapperTiledBackingStore. I agree that only the main backing store of the layer should have repaint counter, but debug border is helpful for canvas, video, directed image and background color. I need to rethink repaint count, and then I'll submit a patch again. Created attachment 182890 [details]
Patch
Comment on attachment 182890 [details] Patch Attachment 182890 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15902210 Comment on attachment 182890 [details] Patch Attachment 182890 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15905181 Comment on attachment 182890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182890&action=review > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:420 > + incrementRepaintCount(); increase repaint count for a backing store. see CoordinatedGraphicsLayer. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:145 > + m_backingStore->drawRepaintCounter(options.textureMapper, m_state.repaintCount, m_state.debugBorderColor, layerRect(), transform); We draw the repaint count for only backing store. It means we draw only border for background color, webgl, composited image and video. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:-386 > - graphicsLayer->updateDebugIndicators(); This patch removes this line. After this patch, remained dependency on GraphicsLayer is only calling getter of GraphicsLayer. It is why I want to land this patch before actor model. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:738 > + m_coordinator->setRepaintCount(id(), incrementRepaintCount()); increase repaint count for a backing store. Currently we keep repaint count in GraphicsLayer, not each tile. Final goal would make Tile manage repaint count by itself, but it needs huge refactoring. So I need to postpone refactoring of this as well as optimization. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2338 > +#if PLATFORM(QT) It is a bit messy. Is it ok? Created attachment 182903 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API (In reply to comment #33) > Created an attachment (id=182903) [details] > Patch Build fix Don't change WebPage in WK2 not to be WK2 patch. Set preferences in each port file. Comment on attachment 182903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182903&action=review Getting there :) > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:147 > + TransformationMatrix adjustedTransform = transform; > + adjustedTransform.multiply(TransformationMatrix::rectToRect(rect(), targetRect)); This needs to move to its own helper function. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:143 > + // Draw the repaint count of only a backing store. // Only draw repaint count for the main backing store. > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:162 > + FloatRect rectOnContents(FloatPoint::zero(), m_size); > + TransformationMatrix adjustedTransform = transform; > + adjustedTransform.multiply(TransformationMatrix::rectToRect(rectOnContents, targetRect)); This needs to move to a helper function; Probably in TextureMapperBackingStore. > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:245 > +void CoordinatedLayerTreeHostProxy::setRepaintCount(CoordinatedLayerID id, int value) setLayerRepaintCount Created attachment 182926 [details]
Patch
(In reply to comment #36) > (From update of attachment 182903 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182903&action=review > > Getting there :) Wow! Thank you for your review :) > This needs to move to its own helper function. > // Only draw repaint count for the main backing store. > This needs to move to a helper function; Probably in TextureMapperBackingStore. > setLayerRepaintCount all done. Comment on attachment 182926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182926&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:36 > +TransformationMatrix TextureMapperBackingStore::rectToRect(const TransformationMatrix& transform, const FloatRect& from, const FloatRect& to) I was thinking of calling it adjustedTransformForRect(const FloatRect& targetRect), not pass the "from" rect since it's a member anyway. And then at the calling site do something like adjustedTransform = transform * adjustedTransformForRect(targetRect). Created attachment 182933 [details]
Patch
(In reply to comment #39) > (From update of attachment 182926 [details]) > I was thinking of calling it adjustedTransformForRect(const FloatRect& targetRect), not pass the "from" rect since it's a member anyway. > And then at the calling site do something like adjustedTransform = transform * adjustedTransformForRect(targetRect). aha, good idea. done. Comment on attachment 182933 [details]
Patch
Can you separate the change to several patches? It's pretty big; Seems like at least the settings part is rather isolated.
Created attachment 183313 [details]
Patch
(In reply to comment #42) > (From update of attachment 182933 [details]) > Can you separate the change to several patches? It's pretty big; Seems like at least the settings part is rather isolated. I extract two parts. 1. settings in Bug 107198 2. changes of CoordinatedTile in Bug 107196 I'm looking forward review :) Comment on attachment 183313 [details]
Patch
This looks fine to me; Please ask a WebKit2 owner for review.
Comment on attachment 183313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183313&action=review That's a lot of code for a debugging helper! :) Here are my nitpicks: > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:407 > + if (m_hasOwnBackingStore) > + updateDebugBorderAndRepaintCount(); > m_layer->flushCompositingStateForThisLayerOnly(this); > - updateBackingStore(); > + if (m_hasOwnBackingStore) > + updateBackingStore(); > didFlushCompositingState(); Why did you move the test for m_hasOwnBackingStore out of updateBackingStore() and replace it by an assertion? This makes the code more fragile and is not explained in the ChangeLog. > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:430 > + // The default values for GraphicsLayer debug borders are a little > + // hard to see (some less than one pixel wide), so we double their size here. > + m_debugBorderColor = color; > + m_debugBorderWidth = width * 2; This is odd. The minimum width of GraphicsLayer::getDebugBorderInfo() is 2. Why do you say some are less than one pixel wide? What you should do instead of doubling the width here, is override GraphicsLayer::getDebugBorderInfo(). > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:315 > +static inline RGBA32 convertABGRToARGB(RGBA32 pixel) > { > + return ((pixel << 16) & 0xff0000) | ((pixel >> 16) & 0xff) | (pixel & 0xff00ff00); > +} This looks like type abuse. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:319 > + static const int pointSize = 8; Why is this static? > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:336 > - painter.fillRect(sourceRect, Qt::blue); // Since we won't swap R+B for speed, this will paint red. > + painter.fillRect(sourceRect, convertABGRToARGB(color.rgb())); // Since we won't swap R+B for speed, this will paint the given color. I don't get any of this. There is Color->QColor implicit conversion. Why use the RGBA32 value? The comment is not helping. It also looks like the defaultFormat thingy is premultiplied. While the input color is not. Why? > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40 > + virtual void drawBorder(TextureMapper* textureMapper, const Color& color, float borderWidth, const FloatRect& targetRect, const TransformationMatrix& transform) > + { > + textureMapper->drawBorder(color, borderWidth, targetRect, transform); > + } Virtual functions should not be implemented in headers. (In reply to comment #46) > (From update of attachment 183313 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183313&action=review > > That's a lot of code for a debugging helper! :) Thank you for review!! > > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:407 > > + if (m_hasOwnBackingStore) > > + updateDebugBorderAndRepaintCount(); > > m_layer->flushCompositingStateForThisLayerOnly(this); > > - updateBackingStore(); > > + if (m_hasOwnBackingStore) > > + updateBackingStore(); > > didFlushCompositingState(); > > Why did you move the test for m_hasOwnBackingStore out of updateBackingStore() and replace it by an assertion? > This makes the code more fragile and is not explained in the ChangeLog. m_hasOwnBackingStore is WK1 legacy and GraphicsLayerTextureMapper::flushCompositingStateForThisLayerOnly() is important function, so I want to highlight updateDebugBorderAndRepaintCount() and updateBackingStore() are only used in WK1 legacy. There is no behavior change. I'll explain in the ChangeLog. > > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:430 > > + // The default values for GraphicsLayer debug borders are a little > > + // hard to see (some less than one pixel wide), so we double their size here. > > + m_debugBorderColor = color; > > + m_debugBorderWidth = width * 2; > > This is odd. The minimum width of GraphicsLayer::getDebugBorderInfo() is 2. Why do you say some are less than one pixel wide? > What you should do instead of doubling the width here, is override GraphicsLayer::getDebugBorderInfo(). I agree on you. It is existing code, not my code. But I correct this: remove comment as well as "*2" > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:315 > > +static inline RGBA32 convertABGRToARGB(RGBA32 pixel) > > { > > + return ((pixel << 16) & 0xff0000) | ((pixel >> 16) & 0xff) | (pixel & 0xff00ff00); > > +} > > This looks like type abuse. Do you have good idea? > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:319 > > + static const int pointSize = 8; > > Why is this static? static is not necessary. I think it is a bit faster than allocating stack in every time. I'll remove static. > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:336 > > - painter.fillRect(sourceRect, Qt::blue); // Since we won't swap R+B for speed, this will paint red. > > + painter.fillRect(sourceRect, convertABGRToARGB(color.rgb())); // Since we won't swap R+B for speed, this will paint the given color. > > I don't get any of this. > There is Color->QColor implicit conversion. Why use the RGBA32 value? > > The comment is not helping. This code is hack to upload buffer to texture without swizzling. the buffer format of qpainter is ARGB but texture format of opengl es is ABGR. So we consciously draw blue color into qpainter if we want red color. After we upload texture, we can see red color. > It also looks like the defaultFormat thingy is premultiplied. While the input color is not. Why? Should Input color be converted to premultiplied when defaultFormat is premultiplied? I did not know that. I'll change. > > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40 > > + virtual void drawBorder(TextureMapper* textureMapper, const Color& color, float borderWidth, const FloatRect& targetRect, const TransformationMatrix& transform) > > + { > > + textureMapper->drawBorder(color, borderWidth, targetRect, transform); > > + } > > Virtual functions should not be implemented in headers. Yes, I know. inline virtual does not make sense. but today compiler well handles it as just virtual. And we don't have TextureMapperPlatformLayer.cpp and we don't need to make TextureMapperPlatformLayer.cpp as noam mentioned in #c26 Created attachment 183594 [details]
Patch
Comment on attachment 183594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183594&action=review > Source/WebCore/ChangeLog:33 > + and updateBackingStore() are only used in WK1. Add comment. > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:430 > +} Remove "*2" > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:313 > +static inline unsigned convertARGBToABGR(unsigned pixel) replace RGBA32 to unsigned rename from convertABGRToARGB to convertARGBToABGR > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:338 > + painter.fillRect(sourceRect, convertARGBToABGR(premultipliedARGBFromColor(color))); // Since we won't swap R+B when uploading a texture, paint with the swapped R+B color. convert to premultiplied rephrase comment > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40 > + } Remain this code not to make TextureMapperPlatformLayer.cpp Since this patch is about a refactory, IMO it would be nice to unify the envvar name (ie. unprefix QT_ from WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS), what do you guys think? (In reply to comment #50) > Since this patch is about a refactory, IMO it would be nice to unify the envvar name (ie. unprefix QT_ from WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS), what do you guys think? I agree on you. We can unprefix QT_ in Bug 107198. Benjamin: ping Could you review this bug? Thank you in advance. Comment on attachment 183594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183594&action=review I think this is an overkill for repaint counting. I don't want to add extra IPC messages for every repaint just for a debug feature. We already know of a repaint in the UI process, when UpdateTile is called, and we should use that information. I think we should review the WK1 part separately (it looks quite OK to me), and then in WK2 call GraphicsLayerTextureMapper->incrementRepaintCount() when we get a createTile/updateTile message. >> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40 >> + } > > Remain this code not to make TextureMapperPlatformLayer.cpp I think it's OK if the default implementation did nothing. The tiled backing store itself should deal with drawing the border. (In reply to comment #53) > (From update of attachment 183594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183594&action=review > > I think this is an overkill for repaint counting. I don't want to add extra IPC messages for every repaint just for a debug feature. We already know of a repaint in the UI process, when UpdateTile is called, and we should use that information. Ok, I agree. > I think we should review the WK1 part separately (it looks quite OK to me), and then in WK2 call GraphicsLayerTextureMapper->incrementRepaintCount() when we get a createTile/updateTile message. Yes, right direction. Many bugs wait this bug. I'll separate soon! > I think it's OK if the default implementation did nothing. The tiled backing store itself should deal with drawing the border. I don't think it is good idea for tiled backing store to draw the border. I'm sure it is one of solutions. But I feel that "Compositor" has responsibility to draw border. Currently, TBS does not care of border. I'm wonder if it is necessary for TBS to deal with border. I want only compositor to deal with border. In addition, it is not efficient for TBS to draw borders. For example, if users toggle "showDebugBorder" via web inspector, we must update whole tiles. On the other hand, it is cheap for TexMap to draw border. (In reply to comment #53) > (From update of attachment 183594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183594&action=review > > I think this is an overkill for repaint counting. I don't want to add extra IPC messages for every repaint just for a debug feature. We already know of a repaint in the UI process, when UpdateTile is called, and we should use that information. > I think we should review the WK1 part separately (it looks quite OK to me), and then in WK2 call GraphicsLayerTextureMapper->incrementRepaintCount() when we get a createTile/updateTile message. After rethinking, it is not good for WK2 to use GraphicsLayerTextureMapper->incrementRepaintCount(). It conflicts against removing GraphicsLayerTextureMapper from WK2. In the short term, I need to make new overkill message to use [Coordinated]GraphicsLayer::incrementRepaintCount(). In the long term, we will get rid of code related to dealing with repaint count in GraphicsLayer. As we know, each tile should deal with each repaint count because each tile has different repaint count. At that time, we will remove the overkill message. Created attachment 184656 [details]
Patch
Comment on attachment 184656 [details] Patch This patch can be compiled and run well without Bug 107910 (separated WK2 part). View in context: https://bugs.webkit.org/attachment.cgi?id=184656&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:350 > + painter.fillRect(sourceRect, convertARGBToABGR(color.rgb())); // Since we won't swap R+B when uploading a texture, paint with the swapped R+B color. It is correct to send unmultiplied color. Comment on attachment 184656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184656&action=review Almost there. > Source/WebCore/ChangeLog:17 > + 2. We use the same color and width to Mac port because we get both info using both info -> that info > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:406 > + if (m_hasOwnBackingStore) > + updateDebugBorderAndRepaintCount(); I think you should always call updateDebugBorderAndRepaintCountIfNeeded(), and return early if it doesn't have own backing store. > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:409 > + if (m_hasOwnBackingStore) > + updateBackingStore(); Ditto (updateBackingStoreIfNeeded) > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:98 > + Remove line > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:45 > + ASSERT_NOT_REACHED(); I don't think it's necessary to have an ASSERT like this to a debug feature. >> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:350 >> + painter.fillRect(sourceRect, convertARGBToABGR(color.rgb())); // Since we won't swap R+B when uploading a texture, paint with the swapped R+B color. > > It is correct to send unmultiplied color. I would prefer to call Color::createUnchecked(color.blue(), color.green(), color.red()) Comment on attachment 184656 [details]
Patch
See comments
Created attachment 184700 [details]
Patch
(In reply to comment #58) > (From update of attachment 184656 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184656&action=review Thank you for review! all Done! Thank you! Could you cq+? I don't have a committer authority yet. Comment on attachment 184700 [details] Patch Clearing flags on attachment: 184700 Committed r140821: <http://trac.webkit.org/changeset/140821> All reviewed patches have been landed. Closing bug. After the patch committed in r140821 it seems the Qt5-WK1 and Qt5-WK2 layout test results are different with a (usingTiledLayer 1) http://build.webkit.org/results/Qt%20Linux%20Release/r140821%20%2856761%29/results.html Can you help me find out what can cause this difference, please? (In reply to comment #65) > After the patch committed in r140821 it seems the Qt5-WK1 and Qt5-WK2 layout test results are different with a > > (usingTiledLayer 1) > > http://build.webkit.org/results/Qt%20Linux%20Release/r140821%20%2856761%29/results.html > > Can you help me find out what can cause this difference, please? Thanks for reporting. I missed it. In this patch, I set GraphicsLayer::m_usingTiledLayer to 1 when the layer has tiled backing. So Qt1 layout test results were changed. But Bug 107910 will apply the same thing to WK2, so now WK1 and WK2 results are different. Are you ok if I update test results after Bug 107910 landing? Reopening to attach new patch. Created attachment 185105 [details]
Patch : It is a follow-up patch not to change layout test results.
Comment on attachment 185105 [details]
Patch : It is a follow-up patch not to change layout test results.
After re-surveying, only mac uses usingTiledLayer, so default expected results don't expect (usingTiledLayer 1).
So we (EFL GTK QT) don't set usingTiledLayer to true.
Comment on attachment 185105 [details]
Patch : It is a follow-up patch not to change layout test results.
Since we use tiled layers for everything, I think we should simply rebase our tests.
(In reply to comment #70) > (From update of attachment 185105 [details]) > Since we use tiled layers for everything, I think we should simply rebase our tests. btw why do we set m_usingTiledLayer to true in the first place? Comment on attachment 185105 [details] Patch : It is a follow-up patch not to change layout test results. View in context: https://bugs.webkit.org/attachment.cgi?id=185105&action=review > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:420 > + m_usingTiledLayer = false; (In reply to comment #71) > (In reply to comment #70) > > (From update of attachment 185105 [details] [details]) > > Since we use tiled layers for everything, I think we should simply rebase our tests. Yes, we can just rebase, but I'm afraid that it would be burden for gardening Qt and EFL port. As I mentioned, default expected txt does not expect (usingTiledLayer 1). For example, LayoutTest/compositing/tiling/rotated-tiled-preserve3d-clamped-expected.txt does not include (usingTiledLayer 1). If we use m_usingTiledLayer, we must manage LayoutTest/platform/qt/compositing/tiling/rotated-tiled-preserve3d-clamped-expected.txt like LayoutTests/platform/mac/compositing/tiling/rotated-tiled-preserve3d-clamped-expected.txt I think other maintainers don't like it. > btw why do we set m_usingTiledLayer to true in the first place? updateDebugIndicators() return different color for a tiled backing. I want to match the color of texture mapper to safari. I agree that my solution is a bit weird. I want to listen your opinion. :)
> > btw why do we set m_usingTiledLayer to true in the first place?
>
> updateDebugIndicators() return different color for a tiled backing. I want to match the color of texture mapper to safari.
>
> I agree that my solution is a bit weird. I want to listen your opinion. :)
Remove the useTiling flag for now. We can mess with the debug colors to match Safari in a different patch.
(In reply to comment #73) > Remove the useTiling flag for now. We can mess with the debug colors to match Safari in a different patch. ok. so simple and succinct solution :) Created attachment 185156 [details]
Patch : It is a follow-up patch not to change layout test results.
Comment on attachment 185156 [details] Patch : It is a follow-up patch not to change layout test results. View in context: https://bugs.webkit.org/attachment.cgi?id=185156&action=review > Source/WebCore/ChangeLog:10 > + The previous patch set GraphicsLayer::m_usingTiledLayer to true when using a set -> sets > Source/WebCore/ChangeLog:18 > + No new tests. Covered by existing tests. Debug feature, not covered in tests. Created attachment 185163 [details]
Patch for landing : It is a follow-up patch not to change layout test results.
Comment on attachment 185163 [details] Patch for landing : It is a follow-up patch not to change layout test results. Clearing flags on attachment: 185163 Committed r141067: <http://trac.webkit.org/changeset/141067> All reviewed patches have been landed. Closing bug. |