[Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel calls
Created attachment 180089 [details] Patch
Comment on attachment 180089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180089&action=review > Source/WebCore/ChangeLog:19 > + It is very difficult to test performance gain; However, the following is apparent: > + * The matrix multiplications and inversions done before in the quad inflation code were > + always visible in instruments/valgrind and they are now gone. > + * We now perform a single smoothstep() command per pixel instead of 8 clamp(dot()) commands. > + * It should now be possible to antialias individual edges. > + sounds good, why is this not up for review?
(In reply to comment #2) > (From update of attachment 180089 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180089&action=review > > > Source/WebCore/ChangeLog:19 > > + It is very difficult to test performance gain; However, the following is apparent: > > + * The matrix multiplications and inversions done before in the quad inflation code were > > + always visible in instruments/valgrind and they are now gone. > > + * We now perform a single smoothstep() command per pixel instead of 8 clamp(dot()) commands. > > + * It should now be possible to antialias individual edges. > > + > > sounds good, why is this not up for review? I wanted to chat with Martin Robinson on IRC about this first. But comments are welcome at any point.
Created attachment 180154 [details] Patch
Created attachment 180155 [details] Patch
Created attachment 180824 [details] Patch
Attachment 180824 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:125: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #7) > Attachment 180824 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:125: Code inside a namespace should not be indented. [whitespace/indent] [4] > Total errors found: 1 in 5 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. This false positive was already reported.
Comment on attachment 180824 [details] Patch Attachment 180824 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15545586
Comment on attachment 180824 [details] Patch Attachment 180824 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15545589
Comment on attachment 180824 [details] Patch Attachment 180824 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15543633
Created attachment 180825 [details] Patch
Attachment 180825 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:125: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 180825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180825&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:143 > RefPtr<SharedGLData> m_sharedGLData; > RefPtr<BitmapTexture> currentSurface; > + HashMap<const void*, Platform3DObject> vbos; some have m_ some dont, confusing > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-425 > - m_context3D->useProgram(program->programID()); > - Should you describe why this is not needed anymore > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:449 > +#define SIDE_TRIANGLE(x1, y1, x2, y2) x1, y1, x1, y1, x2, y2, x2, y2, Center, Center, (x1 + x2) / 2, (y1 + y2) / 2 SIDE_TRIANGLE_DATA ? > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:476 > + TransformationMatrix matrix = > + TransformationMatrix(modelViewMatrix) > + .multiply(TransformationMatrix::rectToRect(FloatRect(0, 0, 1, 1), rect)); why not keep as two lines? >> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:125 >> + uniform mat4 u_modelViewMatrix; > > Code inside a namespace should not be indented. [whitespace/indent] [4] Seems you should report this bug > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:136 > + vec4 toScreenSpace(vec2 pos) { return vec4(pos, 0., 1.) * u_modelViewMatrix; } toDeviceSpace? > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:140 > + void applyAntialias(inout vec2 position) ApplyAntialiasing ? > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:359 > + shaderBuilder.append(\ > + (options & TextureMapperShaderManager::Applier) \ > + ? ENABLE_APPLIER(Applier) \ > + : DISABLE_APPLIER(Applier)) why not just one line?
(In reply to comment #14) > toDeviceSpace? It's really toViewportSpace, does that sound ok?
Comment on attachment 180825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180825&action=review >> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-425 >> - > > Should you describe why this is not needed anymore Will add a function comment in the Changelog. >>> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:125 >>> + uniform mat4 u_modelViewMatrix; >> >> Code inside a namespace should not be indented. [whitespace/indent] [4] > > Seems you should report this bug Already have... Getting tired of putting a reference to it everytime it appears
Created attachment 180857 [details] Patch
Attachment 180857 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:125: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks Kenneth, Martin has promised to look at this tomorrow but if he doesn't I'll land.
Comment on attachment 180857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180857&action=review Looks good to me, with a couple small suggestions. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:448 > + const GC3Dfloat Left = 0; > + const GC3Dfloat Top = 0; > + const GC3Dfloat Right = 1; > + const GC3Dfloat Bottom = 1; > + const GC3Dfloat Center = 0.5; Style nit: Left should be left and so on. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:449 > +#define SIDE_TRIANGLE_DATA(x1, y1, x2, y2) x1, y1, x1, y1, x2, y2, x2, y2, Center, Center, (x1 + x2) / 2, (y1 + y2) / 2 I think a comment here explaining that each point is followed by a control point would make this a lot clearer. > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:168 > + position = center + (position - center) * inflationRatio; So center + (position - center) here ensures that the center point is never expanded? Nice.
Created attachment 180904 [details] Patch for landing
Created attachment 180905 [details] Patch for landing
Attachment 180905 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:125: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 180906 [details] Patch for landing
Comment on attachment 180906 [details] Patch for landing Clearing flags on attachment: 180906 Committed r138555: <http://trac.webkit.org/changeset/138555>
All reviewed patches have been landed. Closing bug.
After this patch, below assertion failure has been reproduced in EFL WK2. ASSERTION FAILED: GLCompileSuccess == GL_TRUE WebKit/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp(479) : void WebCore::GraphicsContext3D::compileShader(Platform3DObject) 1 0x7f955c1d2c45 WebCore::GraphicsContext3D::compileShader(unsigned int) 2 0x7f955c1e5748 WebCore::TextureMapperShaderProgram::TextureMapperShaderProgram(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&) 3 0x7f955c1e6667 WebCore::TextureMapperShaderProgram::create(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&) 4 0x7f955c1e6537 WebCore::TextureMapperShaderManager::getShaderProgram(unsigned int) 5 0x7f955c1dd597 WebCore::TextureMapperGL::drawTexture(unsigned int, int, WebCore::IntSize const&, WebCore::FloatRect const&, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture const*, unsigned int) 6 0x7f955c1dd443 WebCore::TextureMapperGL::drawTexture(WebCore::BitmapTexture const&, WebCore::FloatRect const&, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture const*, unsigned int) 7 0x7f955b7542f0 WebCore::TextureMapperTile::paint(WebCore::TextureMapper*, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture*, unsigned int) 8 0x7f955f7ff40a WebKit::CoordinatedBackingStore::paintTilesToTextureMapper(WTF::Vector<WebCore::TextureMapperTile*, 0ul>&, WebCore::TextureMapper*, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture*, WebCore::FloatRect const&) 9 0x7f955f7ff998 WebKit::CoordinatedBackingStore::paintToTextureMapper(WebCore::TextureMapper*, WebCore::FloatRect const&, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture*) 10 0x7f955b757b36 WebCore::TextureMapperLayer::paintSelf(WebCore::TextureMapperPaintOptions const&) 11 0x7f955b757d37 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&) 12 0x7f955b758679 WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&) 13 0x7f955b7587ab WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&) 14 0x7f955b757ea3 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&) 15 0x7f955b758679 WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&) 16 0x7f955b7587ab WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&) 17 0x7f955b757ea3 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&) 18 0x7f955b758679 WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&) 19 0x7f955b7587ab WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&) 20 0x7f955b7576cd WebCore::TextureMapperLayer::paint() 21 0x7f955f80dc45 WebKit::LayerTreeRenderer::paintToCurrentGLContext(WebCore::TransformationMatrix const&, float, WebCore::FloatRect const&, unsigned int) 22 0x7f955f9565c9 EwkViewImpl::displayTimerFired(WebCore::Timer<EwkViewImpl>*) 23 0x7f955f95dc4c WebCore::Timer<EwkViewImpl>::fired() 24 0x7f955b6a8732 WebCore::ThreadTimers::sharedTimerFiredInternal() 25 0x7f955b6a8653 WebCore::ThreadTimers::sharedTimerFired()
(In reply to comment #27) > After this patch, below assertion failure has been reproduced in EFL WK2. > > ASSERTION FAILED: GLCompileSuccess == GL_TRUE > WebKit/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp(479) : void WebCore::GraphicsContext3D::compileShader(Platform3DObject) > 1 0x7f955c1d2c45 WebCore::GraphicsContext3D::compileShader(unsigned int) > 2 0x7f955c1e5748 WebCore::TextureMapperShaderProgram::TextureMapperShaderProgram(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&) > 3 0x7f955c1e6667 WebCore::TextureMapperShaderProgram::create(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&) > 4 0x7f955c1e6537 WebCore::TextureMapperShaderManager::getShaderProgram(unsigned int) > 5 0x7f955c1dd597 WebCore::TextureMapperGL::drawTexture(unsigned int, int, WebCore::IntSize const&, WebCore::FloatRect const&, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture const*, unsigned int) > 6 0x7f955c1dd443 WebCore::TextureMapperGL::drawTexture(WebCore::BitmapTexture const&, WebCore::FloatRect const&, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture const*, unsigned int) > 7 0x7f955b7542f0 WebCore::TextureMapperTile::paint(WebCore::TextureMapper*, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture*, unsigned int) > 8 0x7f955f7ff40a WebKit::CoordinatedBackingStore::paintTilesToTextureMapper(WTF::Vector<WebCore::TextureMapperTile*, 0ul>&, WebCore::TextureMapper*, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture*, WebCore::FloatRect const&) > 9 0x7f955f7ff998 WebKit::CoordinatedBackingStore::paintToTextureMapper(WebCore::TextureMapper*, WebCore::FloatRect const&, WebCore::TransformationMatrix const&, float, WebCore::BitmapTexture*) > 10 0x7f955b757b36 WebCore::TextureMapperLayer::paintSelf(WebCore::TextureMapperPaintOptions const&) > 11 0x7f955b757d37 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&) > 12 0x7f955b758679 WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&) > 13 0x7f955b7587ab WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&) > 14 0x7f955b757ea3 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&) > 15 0x7f955b758679 WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&) > 16 0x7f955b7587ab WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&) > 17 0x7f955b757ea3 WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions const&) > 18 0x7f955b758679 WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica(WebCore::TextureMapperPaintOptions const&) > 19 0x7f955b7587ab WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions const&) > 20 0x7f955b7576cd WebCore::TextureMapperLayer::paint() > 21 0x7f955f80dc45 WebKit::LayerTreeRenderer::paintToCurrentGLContext(WebCore::TransformationMatrix const&, float, WebCore::FloatRect const&, unsigned int) > 22 0x7f955f9565c9 EwkViewImpl::displayTimerFired(WebCore::Timer<EwkViewImpl>*) > 23 0x7f955f95dc4c WebCore::Timer<EwkViewImpl>::fired() > 24 0x7f955b6a8732 WebCore::ThreadTimers::sharedTimerFiredInternal() > 25 0x7f955b6a8653 WebCore::ThreadTimers::sharedTimerFired() I'll get it fixed - please don't roll out.
(In reply to comment #28) > I'll get it fixed - please don't roll out. ok. :)
(In reply to comment #29) > (In reply to comment #28) > > I'll get it fixed - please don't roll out. > > ok. :) Created bug https://bugs.webkit.org/show_bug.cgi?id=105848