Bug 87695 – [Texmap] Accelerated versions of drop-shadow and blur filters

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

[Noam Rosenthal]

[Texmap] Accelerated versions of drop-shadow and blur filters

[Noam Rosenthal]

Created attachment 144449 [details]
Patch

[Early Warning System Bot]

Comment on attachment 144449 [details]
Patch

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

[Noam Rosenthal]

Created attachment 144452 [details]
Patch

[Alexandru Chiculita]

(In reply to comment #3)
> Created an attachment (id=144452) [details]
> Patch

I would rather implement the blurs using two passes: vertical and horizontal. That way you minimize the number of texture accesses.

[Noam Rosenthal]

Created attachment 144611 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 144611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144611&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:220
> +    int leftOutset, topOutset, bottomOutset, rightOutset;

Not webkit style

> Tools/qmake/mkspecs/features/unix/default_post.prf:1
> -# -------------------------------------------------------------------
> +#   -------------------------------------------------------------------

Why?

[Noam Rosenthal]

(In reply to comment #6)
> (From update of attachment 144611 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144611&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:220
> > +    int leftOutset, topOutset, bottomOutset, rightOutset;
> 
> Not webkit style
Right...
 
> > Tools/qmake/mkspecs/features/unix/default_post.prf:1
> > -# -------------------------------------------------------------------
> > +#   -------------------------------------------------------------------
> 
> Why?

Oops :)

[Alexandru Chiculita]

Comment on attachment 144611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144611&action=review

Looks good. The single thing that I don't like is the duplicated code, but I don't see a nice way to optimize it either.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:533
> +    program->prepare(filter, pass, sourceTexture.contentSize(), static_cast<const BitmapTextureGL&>(contentTexture).id());

This cast doesn't look nice, but it seems like the whole file does that.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:423
> +                    vec2 coord = v_texCoord + vec2(r * u_blurRadius, 0.);

This line is the only difference between the two passes and there's a lot of duplicated code. I'm not sure how to make this look better though, but I think you should at least add a comment explaining that (makes the later maintenance a little easier). You could get away with just one shader if you had vec2 u_blurRadius and just multiplied r with it instead, but it might have slight performance impact.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:430
> +                    vec4 dist =

I suppose that a for loop would be unrolled at compile time anyway and would make the code look better.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:452
> +                    float g =

Isn't this one just a constant at the end? I suppose the compiler would just optimize it anyway.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:591
> +                            sampleAlpha(0.) +

Ditto about the for loop.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:776
> +            radius = blur.stdDeviation().percent();

There's a better way to treat this: floatValueForLength in css/LengthFunctions.h. Also, that's what FilterEffectRenderer.cpp is using.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:812
> +PassRefPtr<StandardFilterProgram> TextureMapperShaderManager::getShaderForFilter(const FilterOperation& filter, int pass)

I think there's no need to have "pass" as a signed int. unsigned should be sufficient.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:830
> +int TextureMapperShaderManager::getPassesForFilter(const FilterOperation& operation) const

int -> unsigned ?

[Noam Rosenthal]

Created attachment 145157 [details]
Patch

[WebKit Review Bot]

Comment on attachment 145157 [details]
Patch

Attachment 145157 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12869390

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html

[WebKit Review Bot]

Created attachment 145209 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Noam Rosenthal]

(In reply to comment #10)
> (From update of attachment 145157 [details])
> Attachment 145157 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12869390
> 
> New failing tests:
> http/tests/media/media-source/video-media-source-event-attributes.html

This has to be flaky.

[Martin Robinson]

Comment on attachment 145157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145157&action=review

Maybe you could say a couple words about why the blur can be done in one pass, while the drop shadow (which is essentially a blur and a source-in composite of the shadow color) must be done in two passes. Other than that I have a few comments, which are mostly just nit-picky things.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:563
> +            m_textureMapper->drawFiltered((i || j) ? *source.get() : contentTexture, contentTexture, *filter, j);

I think you can dereference a RefPtr directly, because it has an operator overload:

(from RefPtr.h)
        T& operator*() const { return *m_ptr; }

This seems to be the only caller of ::drawFiltered, so I'm surprised that ::drawFiltered doesn't just take pointers instead of having to dereference everything here.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:222
> +    int leftOutset, topOutset, bottomOutset, rightOutset;
> +    if (m_state.filters.hasOutsets()) {
> +        m_state.filters.getOutsets(topOutset, rightOutset, bottomOutset, leftOutset);

Perhaps leftOutset, etc could be declared inside the conditional here?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:233
> +

Nit: extra newline here.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:322
> +    for (int i = 0; i < filters.size(); ++i) {
> +        switch (filters.operations().at(i)->getOperationType()) {
> +        case FilterOperation::DROP_SHADOW:
> +            return true;
> +        default:
> +            break;
> +        }
> +    }
> +
> +    return false;

Is this completely dependent on the number of passes? If so perhaps that could just check the number of passes.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:508
> +                {
> +                    // Normal distribution formula, when the mean is 0 and the standard deviation is 1.
> +                    return pow(e, -pow(v, 2.) / 2.) / (sqrt(2. * pi));
> +                }
> +
> +                lowp float sampleAlpha(float r)

I think that 'v' and 'r' deserve to be full words here.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:528
> +                lowp vec4 sourceOver(lowp vec4 front, lowp vec4 back)

Being most familiar with Cairo, the names 'source' and 'destination' seem more apt than front and back to me.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:560
> +    GLchar log[100];
> +    GLint len;
>      glGetShaderInfoLog(fragmentShader, 100, &len, log);
> +    printf("%s\n", log);

Perhaps it makes sense to use the WTF logging facilities for this? (Logging.h in platform)

[Noam Rosenthal]

(In reply to comment #13)
> (From update of attachment 145157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145157&action=review
> 
> Maybe you could say a couple words about why the blur can be done in one pass, while the drop shadow (which is essentially a blur and a source-in composite of the shadow color) must be done in two passes. Other than that I have a few comments, which are mostly just nit-picky things.

The blur is done in two passes, but uses the same shader for both. We need different shaders for the drop-shadow passes because in the second pass we also blend the content texture.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:563
> > +            m_textureMapper->drawFiltered((i || j) ? *source.get() : contentTexture, contentTexture, *filter, j);
> 
> I think you can dereference a RefPtr directly, because it has an operator overload:
> 
> (from RefPtr.h)
>         T& operator*() const { return *m_ptr; }
> 
> This seems to be the only caller of ::drawFiltered, so I'm surprised that ::drawFiltered doesn't just take pointers instead of having to dereference everything here.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:222
> > +    int leftOutset, topOutset, bottomOutset, rightOutset;
> > +    if (m_state.filters.hasOutsets()) {
> > +        m_state.filters.getOutsets(topOutset, rightOutset, bottomOutset, leftOutset);
> 
> Perhaps leftOutset, etc could be declared inside the conditional here?
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:233
> > +
> 
> Nit: extra newline here.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:322
> > +    for (int i = 0; i < filters.size(); ++i) {
> > +        switch (filters.operations().at(i)->getOperationType()) {
> > +        case FilterOperation::DROP_SHADOW:
> > +            return true;
> > +        default:
> > +            break;
> > +        }
> > +    }
> > +
> > +    return false;
> 
> Is this completely dependent on the number of passes? If so perhaps that could just check the number of passes.

No. It's very specific to the shader - blur has two passes, but doesn't need the content texture. Specifically in the drop-shadow shader we need to composite the original image on top of the blurred shadow.

I'll take care of the nitpicks :)

[Noam Rosenthal]

Created attachment 146341 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 146341 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146341&action=review

Looks pretty good, r=me if you fix the comments.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:221
> +        int leftOutset, topOutset, bottomOutset, rightOutset;

Not webkit style really

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:314
> +        // The drop-shadow filter requires the contentn texture, because it needs to composite it

spelling issue

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:505
> +

Remove newline

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:530
> +                lowp float gaussian(lowp float value)
> +                {
> +                    // Normal distribution formula, when the mean is 0 and the standard deviation is 1.
> +                    return pow(e, -pow(value, 2.) / 2.) / (sqrt(2. * pi));
> +                }

As this is defined in various places, would it be possible to share it somehow?

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:751
> +unsigned TextureMapperShaderManager::getPassesForFilter(const FilterOperation& operation) const

getPassesRequiredForFilter?

[Noam Rosenthal]

\> > Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:530
> > +                lowp float gaussian(lowp float value)
> > +                {
> > +                    // Normal distribution formula, when the mean is 0 and the standard deviation is 1.
> > +                    return pow(e, -pow(value, 2.) / 2.) / (sqrt(2. * pi));
> > +                }
> 
> As this is defined in various places, would it be possible to share it somehow?

I don't have a clean way of doing it at this point, I'll add FIXMEs.

[Noam Rosenthal]

Created attachment 146581 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 146581 [details]
Patch for landing

Rejecting attachment 146581 [details] from commit-queue.

New failing tests:
perf/object-keys.html
Full output: http://queues.webkit.org/results/12907999

[WebKit Review Bot]

Created attachment 146610 [details]
Archive of layout-test-results from ec2-cq-01

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Noam Rosenthal]

Comment on attachment 146581 [details]
Patch for landing

Flaky bot.

[WebKit Review Bot]

Comment on attachment 146581 [details]
Patch for landing

Clearing flags on attachment: 146581

Committed r119849: <http://trac.webkit.org/changeset/119849>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.