Bug 105377 - [Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel calls
Summary: [Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords:
Depends on: 104815
Blocks: 105809
  Show dependency treegraph
 
Reported: 2012-12-18 18:42 PST by Noam Rosenthal
Modified: 2012-12-28 21:24 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-12-18 18:42:38 PST
[Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel calls
Comment 1 Noam Rosenthal 2012-12-18 21:41:07 PST
Created attachment 180089 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 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?
Comment 3 Noam Rosenthal 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.
Comment 4 Noam Rosenthal 2012-12-19 06:20:25 PST
Created attachment 180154 [details]
Patch
Comment 5 Noam Rosenthal 2012-12-19 06:22:30 PST
Created attachment 180155 [details]
Patch
Comment 6 Noam Rosenthal 2012-12-27 14:58:13 PST
Created attachment 180824 [details]
Patch
Comment 7 WebKit Review Bot 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.
Comment 8 Noam Rosenthal 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.
Comment 9 EFL EWS Bot 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
Comment 10 Early Warning System Bot 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
Comment 11 Early Warning System Bot 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
Comment 12 Noam Rosenthal 2012-12-27 15:18:00 PST
Created attachment 180825 [details]
Patch
Comment 13 WebKit Review Bot 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.
Comment 14 Kenneth Rohde Christiansen 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?
Comment 15 Noam Rosenthal 2012-12-28 01:34:22 PST
(In reply to comment #14)
> toDeviceSpace?
It's really toViewportSpace, does that sound ok?
Comment 16 Noam Rosenthal 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
Comment 17 Noam Rosenthal 2012-12-28 01:50:51 PST
Created attachment 180857 [details]
Patch
Comment 18 WebKit Review Bot 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.
Comment 19 Noam Rosenthal 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.
Comment 20 Martin Robinson 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.
Comment 21 Noam Rosenthal 2012-12-28 18:19:32 PST
Created attachment 180904 [details]
Patch for landing
Comment 22 Noam Rosenthal 2012-12-28 18:20:20 PST
Created attachment 180905 [details]
Patch for landing
Comment 23 WebKit Review Bot 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.
Comment 24 Noam Rosenthal 2012-12-28 18:24:35 PST
Created attachment 180906 [details]
Patch for landing
Comment 25 WebKit Review Bot 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>
Comment 26 WebKit Review Bot 2012-12-28 19:06:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Kangil Han 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()
Comment 28 Noam Rosenthal 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.
Comment 29 Kangil Han 2012-12-28 21:24:00 PST
(In reply to comment #28)
> I'll get it fixed - please don't roll out.

ok. :)
Comment 30 Noam Rosenthal 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