WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
105377
[Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel calls
https://bugs.webkit.org/show_bug.cgi?id=105377
Summary
[Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel...
Noam Rosenthal
Reported
2012-12-18 18:42:38 PST
[Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel calls
Attachments
Patch
(38.54 KB, patch)
2012-12-18 21:41 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Patch
(38.63 KB, patch)
2012-12-19 06:20 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Patch
(38.63 KB, patch)
2012-12-19 06:22 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Patch
(38.52 KB, patch)
2012-12-27 14:58 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Patch
(38.53 KB, patch)
2012-12-27 15:18 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Patch
(40.05 KB, patch)
2012-12-28 01:50 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Patch for landing
(40.37 KB, patch)
2012-12-28 18:19 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Patch for landing
(40.39 KB, patch)
2012-12-28 18:20 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Patch for landing
(40.38 KB, patch)
2012-12-28 18:24 PST
,
Noam Rosenthal
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Noam Rosenthal
Comment 1
2012-12-18 21:41:07 PST
Created
attachment 180089
[details]
Patch
Kenneth Rohde Christiansen
Comment 2
2012-12-19 00:58:51 PST
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?
Noam Rosenthal
Comment 3
2012-12-19 05:59:49 PST
(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.
Noam Rosenthal
Comment 4
2012-12-19 06:20:25 PST
Created
attachment 180154
[details]
Patch
Noam Rosenthal
Comment 5
2012-12-19 06:22:30 PST
Created
attachment 180155
[details]
Patch
Noam Rosenthal
Comment 6
2012-12-27 14:58:13 PST
Created
attachment 180824
[details]
Patch
WebKit Review Bot
Comment 7
2012-12-27 15:01:45 PST
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.
Noam Rosenthal
Comment 8
2012-12-27 15:02:55 PST
(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.
EFL EWS Bot
Comment 9
2012-12-27 15:09:46 PST
Comment on
attachment 180824
[details]
Patch
Attachment 180824
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/15545586
Early Warning System Bot
Comment 10
2012-12-27 15:14:29 PST
Comment on
attachment 180824
[details]
Patch
Attachment 180824
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/15545589
Early Warning System Bot
Comment 11
2012-12-27 15:17:02 PST
Comment on
attachment 180824
[details]
Patch
Attachment 180824
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/15543633
Noam Rosenthal
Comment 12
2012-12-27 15:18:00 PST
Created
attachment 180825
[details]
Patch
WebKit Review Bot
Comment 13
2012-12-27 15:20:36 PST
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.
Kenneth Rohde Christiansen
Comment 14
2012-12-28 01:31:17 PST
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?
Noam Rosenthal
Comment 15
2012-12-28 01:34:22 PST
(In reply to
comment #14
)
> toDeviceSpace?
It's really toViewportSpace, does that sound ok?
Noam Rosenthal
Comment 16
2012-12-28 01:44:10 PST
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
Noam Rosenthal
Comment 17
2012-12-28 01:50:51 PST
Created
attachment 180857
[details]
Patch
WebKit Review Bot
Comment 18
2012-12-28 01:52:53 PST
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.
Noam Rosenthal
Comment 19
2012-12-28 02:00:58 PST
Thanks Kenneth, Martin has promised to look at this tomorrow but if he doesn't I'll land.
Martin Robinson
Comment 20
2012-12-28 11:32:30 PST
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.
Noam Rosenthal
Comment 21
2012-12-28 18:19:32 PST
Created
attachment 180904
[details]
Patch for landing
Noam Rosenthal
Comment 22
2012-12-28 18:20:20 PST
Created
attachment 180905
[details]
Patch for landing
WebKit Review Bot
Comment 23
2012-12-28 18:21:51 PST
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.
Noam Rosenthal
Comment 24
2012-12-28 18:24:35 PST
Created
attachment 180906
[details]
Patch for landing
WebKit Review Bot
Comment 25
2012-12-28 19:06:46 PST
Comment on
attachment 180906
[details]
Patch for landing Clearing flags on attachment: 180906 Committed
r138555
: <
http://trac.webkit.org/changeset/138555
>
WebKit Review Bot
Comment 26
2012-12-28 19:06:52 PST
All reviewed patches have been landed. Closing bug.
Kangil Han
Comment 27
2012-12-28 21:13:33 PST
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()
Noam Rosenthal
Comment 28
2012-12-28 21:19:02 PST
(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.
Kangil Han
Comment 29
2012-12-28 21:24:00 PST
(In reply to
comment #28
)
> I'll get it fixed - please don't roll out.
ok. :)
Noam Rosenthal
Comment 30
2012-12-28 21:24:47 PST
(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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug