Bug 105377 – [Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel calls

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Noam Rosenthal]

[Texmap] Edge antialiasing in TextureMapper could be done with less per-pixel calls

[Noam Rosenthal]

Created attachment 180089 [details]
Patch

[Kenneth Rohde Christiansen]

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]

(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]

Created attachment 180154 [details]
Patch

[Noam Rosenthal]

Created attachment 180155 [details]
Patch

[Noam Rosenthal]

Created attachment 180824 [details]
Patch

[WebKit Review Bot]

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]

(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 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 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 on attachment 180824 [details]
Patch

Attachment 180824 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15543633

[Noam Rosenthal]

Created attachment 180825 [details]
Patch

[WebKit Review Bot]

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 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]

(In reply to comment #14)
> toDeviceSpace?
It's really toViewportSpace, does that sound ok?

[Noam Rosenthal]

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]

Created attachment 180857 [details]
Patch

[WebKit Review Bot]

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]

Thanks Kenneth, Martin has promised to look at this tomorrow but if he doesn't I'll land.

[Martin Robinson]

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]

Created attachment 180904 [details]
Patch for landing

[Noam Rosenthal]

Created attachment 180905 [details]
Patch for landing

[WebKit Review Bot]

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]

Created attachment 180906 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180906 [details]
Patch for landing

Clearing flags on attachment: 180906

Committed r138555: <http://trac.webkit.org/changeset/138555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kangil Han]

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]

(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]

(In reply to comment #28)
> I'll get it fixed - please don't roll out.

ok. :)

[Noam Rosenthal]

(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