Bug 174457 – [GTK][WPE] Implement antialiased rounded rectangle clipping in TextureMapper

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

As commented in bug 174157, image elements that are drawn in their own RenderLayer can be directly composited without needing to render them to a backing store. This is an optimization that saves rendering time and also memory (we use exactly the size of the image instead of the wasting tiles).

One of these cases that are optimized is when the image has a border-radius property but doesn't have a visible border. This should be directly composited and the rounded rectangle should be clipped during the composition. But this rounded rectangle clipping is not supported in the TextureMapper yet. In bug 174157 a workaround was added so this case was not directly composited. While this works, it's not optimum.

The optimum fix is to implement rounded rectangle clipping in the TextureMapper and then directly composite the images with border-radius, while clipping appropriately.

As a reminder for me: we need to also support antialiasing during the clipping. I don't see how this can be performed with the stencil, so it discards the approach I was making for the clipping. I think the clipping needs to be implemented in the shader, checking for each pixel whether it's not inside the clipping area or not. So pixels inside the rectagle are draw, pixels outside arent', and pixels in the border are maybe blended for antialiasing purposes.

[Miguel Gomez]

Created attachment 411876 [details]
Patch

[Fujii Hironori]

Comment on attachment 411876 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        the defined rounded rectangle, and paints or skips it as needed.

The problem of this approach is that it can't be stacked. Right?
What will happen there are nested rounded clipped layers?

In addition to it, can this approach clip child layers?

> Source/WebCore/ChangeLog:14
> +        Covered by existent tests.

This is a behavior change. Why don't this change update TestExpectations?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:954
> +    // This is implemented by telling the fragment shader to check whether each pixel is inside the rounded rectangle

Is it a good idea to update scissorBox in this case to narrow the target region?

    FloatQuad quad = modelViewMatrix.projectQuad(targetRect.rect());
    IntRect rect = quad.enclosingBoundingBox();
    clipStack().intersect(rect);

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:965
> +        return;

I think it'd be good to fall back to rect clipping in this case.
beginClip(modelViewMatrix, targetRect.rect());

[Fujii Hironori]

Does this change need to revert r219445?

[Fujii Hironori]

Comment on attachment 411876 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/ClipStack.h:35
> +        State(const IntRect& scissors = IntRect(), int stencil = 1, const FloatRoundedRect& roundedRect = FloatRoundedRect(), const TransformationMatrix& transform = TransformationMatrix())

You added arguments to ctor, but not used. Why do you want to add this arugments?

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:429
> +            vec2 topLeftCentre = bounds.xy + topLeftRadii;

You are using 'Centre' and 'center'. Do you prefer US English? Do you want to spell 'topLeftCenter' here?

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:456
> +            color *= roundedRectCoverage(fragCoord.xy);

I'm wondering why all components of 'color' is scaled. Why not only color.a?
color.a *= roundedRectCoverage(fragCoord.xy);
I know applyAlphaBlur does same above.

[Fujii Hironori]

Comment on attachment 411876 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:444
> +            return 1.0 - dot(vec4(isOut), cornerCoverages);

You are calculating cornerCoverages and isOut for all four corners. Is this effective? What about an idea calculating only one corner?
if (p.x < topLeftCentre.x && p.y < topLeftCentre.y) {
    return ellipsisCoverage(p, topLeftCentre, topLeftRadii);
} else if (...) {
...
} else {
    return 1;
}

[Carlos Garcia Campos]

Comment on attachment 411876 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        Covered by existent tests.
> 
> This is a behavior change. Why don't this change update TestExpectations?

I expected this to fix several backdrop filter tests at least.

[Miguel Gomez]

(In reply to Fujii Hironori from comment #2)
> Comment on attachment 411876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411876&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        the defined rounded rectangle, and paints or skips it as needed.
> 
> The problem of this approach is that it can't be stacked. Right?
> What will happen there are nested rounded clipped layers?

As it's done it can't be stacked, only a single clip can be performed. It can be modified to stack several clips, but it's expensive as each pixels needs to check whether it's inside all of the rectangles passed.

> In addition to it, can this approach clip child layers?

Yes. Every TextureMapper draw that happens between beginClip and endClip that uses a shader will be clipped.

> > Source/WebCore/ChangeLog:14
> > +        Covered by existent tests.
> 
> This is a behavior change. Why don't this change update TestExpectations?

Because this doesn't really fix any test. There are other issues to fix besides adding this implementation.

> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:954
> > +    // This is implemented by telling the fragment shader to check whether each pixel is inside the rounded rectangle
> 
> Is it a good idea to update scissorBox in this case to narrow the target
> region?
> 
>     FloatQuad quad = modelViewMatrix.projectQuad(targetRect.rect());
>     IntRect rect = quad.enclosingBoundingBox();
>     clipStack().intersect(rect);

Indeed! That's a good idea that will reduce the amount of pixels.

> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:965
> > +        return;
> 
> I think it'd be good to fall back to rect clipping in this case.
> beginClip(modelViewMatrix, targetRect.rect());

Ok.

[Miguel Gomez]

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 411876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411876&action=review
> 
> > Source/WebCore/platform/graphics/texmap/ClipStack.h:35
> > +        State(const IntRect& scissors = IntRect(), int stencil = 1, const FloatRoundedRect& roundedRect = FloatRoundedRect(), const TransformationMatrix& transform = TransformationMatrix())
> 
> You added arguments to ctor, but not used. Why do you want to add this
> arugments?

Just to use the same structure than the previous parameters. They could be removed.

> > Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:429
> > +            vec2 topLeftCentre = bounds.xy + topLeftRadii;
> 
> You are using 'Centre' and 'center'. Do you prefer US English? Do you want
> to spell 'topLeftCenter' here?

Ooops, my fault! It should be center everywhere.

> > Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:456
> > +            color *= roundedRectCoverage(fragCoord.xy);
> 
> I'm wondering why all components of 'color' is scaled. Why not only color.a?
> color.a *= roundedRectCoverage(fragCoord.xy);
> I know applyAlphaBlur does same above.

Yes, you're right, I forgot to update that.

[Miguel Gomez]

(In reply to Fujii Hironori from comment #5)
> Comment on attachment 411876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411876&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:444
> > +            return 1.0 - dot(vec4(isOut), cornerCoverages);
> 
> You are calculating cornerCoverages and isOut for all four corners. Is this
> effective? What about an idea calculating only one corner?
> if (p.x < topLeftCentre.x && p.y < topLeftCentre.y) {
>     return ellipsisCoverage(p, topLeftCentre, topLeftRadii);
> } else if (...) {
> ...
> } else {
>     return 1;
> }

Yes, you're right.

[Noam Rosenthal]

Comment on attachment 411876 [details]
Patch

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

>>> Source/WebCore/ChangeLog:12
>>> +        the defined rounded rectangle, and paints or skips it as needed.
>> 
>> The problem of this approach is that it can't be stacked. Right?
>> What will happen there are nested rounded clipped layers?
>> 
>> In addition to it, can this approach clip child layers?
> 
> As it's done it can't be stacked, only a single clip can be performed. It can be modified to stack several clips, but it's expensive as each pixels needs to check whether it's inside all of the rectangles passed.

The original plan with this was to fall back to an intermediate surface when rounded-rect clipping needs to occur for a stack of elements, and then clip that surface with the rounded-rectangle. probably achievable today.

[Miguel Gomez]

(In reply to Carlos Garcia Campos from comment #6)
> Comment on attachment 411876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411876&action=review
> 
> >> Source/WebCore/ChangeLog:14
> >> +        Covered by existent tests.
> > 
> > This is a behavior change. Why don't this change update TestExpectations?
> 
> I expected this to fix several backdrop filter tests at least.

It doesn't. There must be some piece missing besides this. I'll look into it when this is landed.

[Miguel Gomez]

(In reply to Fujii Hironori from comment #3)
> Does this change need to revert r219445?

The idea is to remove that change, yes, but there's something to be done besides removing that code.

Just after that change there's this call
return m_graphicsLayer->shouldDirectlyCompositeImage(image);

My idea is to use that call to the CoordinatedGraphicsLayer to check whether the layer transform is invertible, and return false if its not, so cairo does the job in that case. I need to be sure that the transformation matrix is already stored in the CoordinatedGraphicsLayer at that point though.

[Miguel Gomez]

 
> The original plan with this was to fall back to an intermediate surface when
> rounded-rect clipping needs to occur for a stack of elements, and then clip
> that surface with the rounded-rectangle. probably achievable today.

This approach can clip a stack of elements. The clipping layer will call beginClip and start drawing its children, will will all be clipped. Then the clip will be deactivated with endClip.

What this approach can't do is perform several rounded rect clips at the same time. It will only perform the last one set. I thought this would be enough but I'll add that feature as well.

[Noam Rosenthal]

(In reply to Miguel Gomez from comment #13)
>  
> > The original plan with this was to fall back to an intermediate surface when
> > rounded-rect clipping needs to occur for a stack of elements, and then clip
> > that surface with the rounded-rectangle. probably achievable today.
> 
> This approach can clip a stack of elements. The clipping layer will call
> beginClip and start drawing its children, will will all be clipped. Then the
> clip will be deactivated with endClip.
> 
> What this approach can't do is perform several rounded rect clips at the
> same time. It will only perform the last one set. I thought this would be
> enough but I'll add that feature as well.

Gotcha. It's up to you whether that is enough at this point.

[Carlos Garcia Campos]

Comment on attachment 411876 [details]
Patch

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

>>>>> Source/WebCore/ChangeLog:14
>>>>> +        Covered by existent tests.
>>>> 
>>>> This is a behavior change. Why don't this change update TestExpectations?
>>> 
>>> I expected this to fix several backdrop filter tests at least.
>> 
>> Because this doesn't really fix any test. There are other issues to fix besides adding this implementation.
> 
> It doesn't. There must be some piece missing besides this. I'll look into it when this is landed.

Yes, we are still passing the rect, I think I have the changes needed in a local branch.

[Miguel Gomez]

> I'm wondering why all components of 'color' is scaled. Why not only color.a?
> color.a *= roundedRectCoverage(fragCoord.xy);
> I know applyAlphaBlur does same above.

In a lot of documentation that I've checked all the components are multiplied for antialiasing purposes, and not only the alpha, that's why I did it that way.

But your question is a valid one! I've been playing with both options and I see that the result is visually better when multiplying all the components and not just the alpha. Seems that the color darkening that's applied helps with the antialiasing effect.

[Noam Rosenthal]

(In reply to Miguel Gomez from comment #16)
> > I'm wondering why all components of 'color' is scaled. Why not only color.a?
> > color.a *= roundedRectCoverage(fragCoord.xy);
> > I know applyAlphaBlur does same above.
> 
> In a lot of documentation that I've checked all the components are
> multiplied for antialiasing purposes, and not only the alpha, that's why I
> did it that way.
You can setup the GL blending (glBlendFunc) as premultiplied or not-premultiplied. It's better to keep it consistent.

> 
> But your question is a valid one! I've been playing with both options and I
> see that the result is visually better when multiplying all the components
> and not just the alpha. Seems that the color darkening that's applied helps
> with the antialiasing effect.
The effect actually creates something along the lines of a = pow(a, 2), which is sort of an easing function of alpha, I suggest to make explicit :)

[Carlos Garcia Campos]

Comment on attachment 411876 [details]
Patch

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

>>> Source/WebCore/platform/graphics/texmap/ClipStack.h:35
>>> +        State(const IntRect& scissors = IntRect(), int stencil = 1, const FloatRoundedRect& roundedRect = FloatRoundedRect(), const TransformationMatrix& transform = TransformationMatrix())
>> 
>> You added arguments to ctor, but not used. Why do you want to add this arugments?
> 
> Just to use the same structure than the previous parameters. They could be removed.

I think it's easier to use only FloatRoundedRect everywhere since we can have a FloatRoundedRect with 0 radii.

> Source/WebCore/platform/graphics/texmap/ClipStack.h:42
>          IntRect scissorBox;

We could rename this to just rect

> Source/WebCore/platform/graphics/texmap/ClipStack.h:65
> +    const FloatRoundedRect& roundedRect() { return clipState.roundedRect; }
> +    bool isRoundedRectClipEnabled() { return !clipState.roundedRect.isEmpty(); }

These should be const. If we just return the rect the caller can simply do rect().isRounded()

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:80
> +    virtual void beginClip(const TransformationMatrix&, const FloatRoundedRect&) = 0;
>      virtual void beginClip(const TransformationMatrix&, const FloatRect&) = 0;

Same here, just modify existing beingClip to receive a FloatRoundedRect

>>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:965
>>> +        return;
>> 
>> I think it'd be good to fall back to rect clipping in this case.
>> beginClip(modelViewMatrix, targetRect.rect());
> 
> Ok.

Should we also check it's not empty? or FloatRoundedRect::isRenderable?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:969
> +    clipStack().push();
> +    clipStack().setRoundedRect(targetRect);
> +    clipStack().setRoundedRectInverseTransform(modelViewMatrix.inverse().value());

So, this function could be like beginScissorClip(), beginRoundedClip, for example. Then the only beginClip would try first with RoundedClip, then Scissor clip and finally stencil

[Fujii Hironori]

Comment on attachment 411876 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/texmap/ClipStack.h:35
>>>> +        State(const IntRect& scissors = IntRect(), int stencil = 1, const FloatRoundedRect& roundedRect = FloatRoundedRect(), const TransformationMatrix& transform = TransformationMatrix())
>>> 
>>> You added arguments to ctor, but not used. Why do you want to add this arugments?
>> 
>> Just to use the same structure than the previous parameters. They could be removed.
> 
> I think it's easier to use only FloatRoundedRect everywhere since we can have a FloatRoundedRect with 0 radii.

scissorBox and roundedRect live in different coordinate system. scissorBox is in the viewport coordinate system. roundedRect is in the transformed coordinate system. They can't be merged.

[Fujii Hironori]

Comment on attachment 411876 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:12
>>>> +        the defined rounded rectangle, and paints or skips it as needed.
>>> 
>>> The problem of this approach is that it can't be stacked. Right?
>>> What will happen there are nested rounded clipped layers?
>>> 
>>> In addition to it, can this approach clip child layers?
>> 
>> As it's done it can't be stacked, only a single clip can be performed. It can be modified to stack several clips, but it's expensive as each pixels needs to check whether it's inside all of the rectangles passed.
> 
> The original plan with this was to fall back to an intermediate surface when rounded-rect clipping needs to occur for a stack of elements, and then clip that surface with the rounded-rectangle. probably achievable today.

You don't need to add roundedRect to ClipStack::State if it can't be stacked at the moment. You can add m_roundedClipRect to TextureMapperGL. This is just an idea.

[Fujii Hironori]

Comment on attachment 411876 [details]
Patch

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

>>>>> Source/WebCore/ChangeLog:12
>>>>> +        the defined rounded rectangle, and paints or skips it as needed.
>>>> 
>>>> The problem of this approach is that it can't be stacked. Right?
>>>> What will happen there are nested rounded clipped layers?
>>>> 
>>>> In addition to it, can this approach clip child layers?
>>> 
>>> As it's done it can't be stacked, only a single clip can be performed. It can be modified to stack several clips, but it's expensive as each pixels needs to check whether it's inside all of the rectangles passed.
>> 
>> The original plan with this was to fall back to an intermediate surface when rounded-rect clipping needs to occur for a stack of elements, and then clip that surface with the rounded-rectangle. probably achievable today.
> 
> You don't need to add roundedRect to ClipStack::State if it can't be stacked at the moment. You can add m_roundedClipRect to TextureMapperGL. This is just an idea.

I was wrong. ClipStack::State should have roundedRect for the nested rounded clipped case even if it is not supported.

[Miguel Gomez]

Created attachment 412071 [details]
Patch

[Miguel Gomez]

(In reply to Miguel Gomez from comment #22)
> Created attachment 412071 [details]
> Patch

Second version, this time allowing the simultaneous clipping of up to 10 rounded rectangles. This limitation comes from GLSL, where there are restrictions when using undefined length arrays. So I'm using arrays big enough to hold the values to perform up to 10 clips.

As Carlos suggested, I've modified TextureMapper::beginClip() to receive a FloatRoundedRect and that function will apply the appropriate clip depending on the passed rectangle.

I've also modified the way the rectangles and matrices are stored in ClipStack. There are 2 arrays to hold the rectangles and matrices components, and what's stored in the state is the number of rectangles we have stored. When adding a new rectangle and matrix, their components are stored in the matrices instead of storing the FloatRoundedRect and TransformationMatrix instances. This allows us to pass the array pointers directly to the shader without needing any conversion.

[Noam Rosenthal]

Comment on attachment 412071 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Covered by existent tests.

I don't believe that there are currently existing tests covering composited clipping of a deep rounded-rect stack

[Carlos Garcia Campos]

Comment on attachment 412071 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/ClipStack.cpp:86
> +    clipState.roundedRectIndex++;

Should we mark the state as dirty?

> Source/WebCore/platform/graphics/texmap/ClipStack.h:35
> +static const int s_roundedRectMaxClipsAllowed = 10;

unsigned

> Source/WebCore/platform/graphics/texmap/ClipStack.h:41
> +static const int s_roundedRectComponentsPerRect = 12;
> +static const int s_roundedRectComponentsArraySize = s_roundedRectMaxClipsAllowed * s_roundedRectComponentsPerRect;

Ditto.

> Source/WebCore/platform/graphics/texmap/ClipStack.h:47
> +static const int s_roundedRectInverseTransformComponentsPerRect = 16;
> +static const int s_roundedRectInverseTransformComponentsArraySize = s_roundedRectMaxClipsAllowed * s_roundedRectInverseTransformComponentsPerRect;

Ditto.

> Source/WebCore/platform/graphics/texmap/ClipStack.h:56
> -        State(const IntRect& scissors = IntRect(), int stencil = 1)
> +        State(const IntRect& scissors = IntRect(), int stencil = 1, int roundedRect = 0)
>              : scissorBox(scissors)
>              , stencilIndex(stencil)
> +            , roundedRectIndex(roundedRect)
>          { }

I also find confusing the constructor parameters that are never really used, because I think the State is only constructed as a member of ClipStack, so always with no parameters. I would simply this struct as:

struct State {
    IntRect scissorBox;
    int stencilIndex { 1 };
    Optional<int> roundedRectIndex;
};

> Source/WebCore/platform/graphics/texmap/ClipStack.h:60
> +        int roundedRectIndex;

Optional<int>?

> Source/WebCore/platform/graphics/texmap/ClipStack.h:97
> +    std::array<float, s_roundedRectComponentsArraySize> m_roundedRectComponents = { };
> +    std::array<float, s_roundedRectInverseTransformComponentsArraySize> m_roundedRectInverseTransformComponents = { };

Why not WTF::Vector? I don't think we need the empty initialization.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:232
> -        options.textureMapper.beginClip(clipTransform, layerRect());
> +        options.textureMapper.beginClip(clipTransform, FloatRoundedRect(layerRect(), FloatRoundedRect::Radii()));

No need to pass en empty radii, that's the default FloatRoundedRect(layerRect())

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:251
> -        options.textureMapper.beginClip(clipTransform, layerRect());
> +        options.textureMapper.beginClip(clipTransform, FloatRoundedRect(layerRect(), FloatRoundedRect::Radii()));

Ditto.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:407
> -        options.textureMapper.beginClip(TransformationMatrix(), rect);
> +        options.textureMapper.beginClip(TransformationMatrix(), FloatRoundedRect(rect, FloatRoundedRect::Radii()));

Ditto.

> Source/WebKit/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:74
> -    m_textureMapper->beginClip(TransformationMatrix(), clipRect);
> +    m_textureMapper->beginClip(TransformationMatrix(), FloatRoundedRect(clipRect, FloatRoundedRect::Radii()));

Ditto.

[Carlos Garcia Campos]

Comment on attachment 412071 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        Covered by existent tests.
> 
> I don't believe that there are currently existing tests covering composited clipping of a deep rounded-rect stack

Well, in this case it actually means it doesn't regress existing tests, but I agree it would be better to not say anything about tests.

[Noam Rosenthal]

Comment on attachment 412071 [details]
Patch

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

>>> Source/WebCore/ChangeLog:15
>>> +        Covered by existent tests.
>> 
>> I don't believe that there are currently existing tests covering composited clipping of a deep rounded-rect stack
> 
> Well, in this case it actually means it doesn't regress existing tests, but I agree it would be better to not say anything about tests.

If the feature is going to be working after this patch, you should add tests that didn't work before and work now (e.g. nested composited rounded rects).
If the feature is still missing something in order to be supported, I suggest to combine to a single patch that actually makes the feature work.
Not regressing old tests is important but not sufficient...

[Miguel Gomez]

(In reply to Carlos Garcia Campos from comment #25)
> Comment on attachment 412071 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412071&action=review
> 
> > Source/WebCore/platform/graphics/texmap/ClipStack.cpp:86
> > +    clipState.roundedRectIndex++;
> 
> Should we mark the state as dirty?

It's not really needed. The dirty state means that something needs to be done when applyIfNeeded is called. But there's nothing to do in this case. Unlike the scissor or stencil cases, where we need to do something in order to apply the clip, we don't need to do anything with rounded rectangles.

> > Source/WebCore/platform/graphics/texmap/ClipStack.h:35
> > +static const int s_roundedRectMaxClipsAllowed = 10;
> 
> unsigned

ok
 
> > Source/WebCore/platform/graphics/texmap/ClipStack.h:41
> > +static const int s_roundedRectComponentsPerRect = 12;
> > +static const int s_roundedRectComponentsArraySize = s_roundedRectMaxClipsAllowed * s_roundedRectComponentsPerRect;
> 
> Ditto.

ok

> > Source/WebCore/platform/graphics/texmap/ClipStack.h:47
> > +static const int s_roundedRectInverseTransformComponentsPerRect = 16;
> > +static const int s_roundedRectInverseTransformComponentsArraySize = s_roundedRectMaxClipsAllowed * s_roundedRectInverseTransformComponentsPerRect;
> 
> Ditto.

ok

> > Source/WebCore/platform/graphics/texmap/ClipStack.h:56
> > -        State(const IntRect& scissors = IntRect(), int stencil = 1)
> > +        State(const IntRect& scissors = IntRect(), int stencil = 1, int roundedRect = 0)
> >              : scissorBox(scissors)
> >              , stencilIndex(stencil)
> > +            , roundedRectIndex(roundedRect)
> >          { }
> 
> I also find confusing the constructor parameters that are never really used,
> because I think the State is only constructed as a member of ClipStack, so
> always with no parameters. I would simply this struct as:
> 
> struct State {
>     IntRect scissorBox;
>     int stencilIndex { 1 };
>     Optional<int> roundedRectIndex;
> };

I removed stencilIndex and roundedRectIndex from the constructor, but scissorBox is actually being used, so I left it.

> > Source/WebCore/platform/graphics/texmap/ClipStack.h:60
> > +        int roundedRectIndex;
> 
> Optional<int>?

As I commented to you, roundedRectIndex contains the number of rounded rects that are stored, and it's used to index the component arrays. For this 0 is a valid
value, so I don't think using Optional is useful.

> > Source/WebCore/platform/graphics/texmap/ClipStack.h:97
> > +    std::array<float, s_roundedRectComponentsArraySize> m_roundedRectComponents = { };
> > +    std::array<float, s_roundedRectInverseTransformComponentsArraySize> m_roundedRectInverseTransformComponents = { };
> 
> Why not WTF::Vector? I don't think we need the empty initialization.

Removed the initialization of the array.
Why not a Vector? mmmm I don't see any advantage using a Vector in this case. I don't need almost any of the methods it provides, other than accessing its internal buffer. All I need is an array.

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:232
> > -        options.textureMapper.beginClip(clipTransform, layerRect());
> > +        options.textureMapper.beginClip(clipTransform, FloatRoundedRect(layerRect(), FloatRoundedRect::Radii()));
> 
> No need to pass en empty radii, that's the default
> FloatRoundedRect(layerRect())

ok

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:251
> > -        options.textureMapper.beginClip(clipTransform, layerRect());
> > +        options.textureMapper.beginClip(clipTransform, FloatRoundedRect(layerRect(), FloatRoundedRect::Radii()));
> 
> Ditto.

ok

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:407
> > -        options.textureMapper.beginClip(TransformationMatrix(), rect);
> > +        options.textureMapper.beginClip(TransformationMatrix(), FloatRoundedRect(rect, FloatRoundedRect::Radii()));
> 
> Ditto.

ok

> > Source/WebKit/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:74
> > -    m_textureMapper->beginClip(TransformationMatrix(), clipRect);
> > +    m_textureMapper->beginClip(TransformationMatrix(), FloatRoundedRect(clipRect, FloatRoundedRect::Radii()));
> 
> Ditto.

ok

Also, I found that there's a test that's passing now, so I'll unskip it.

[Miguel Gomez]

Created attachment 412093 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 412093 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Covered by existent tests.

I still think it's better to remove this. Are you going to add a test for the nested rounded clips in a follow up patch?

> Source/WebCore/platform/graphics/texmap/ClipStack.h:52
> -        State(const IntRect& scissors = IntRect(), int stencil = 1)
> +        State(const IntRect& scissors = IntRect())

Where is it used? If we really need to kep the IntRect parameter we should add the explicit keyword.

[Fujii Hironori]

Comment on attachment 412093 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/ClipStack.cpp:75
> +    m_roundedRectComponents[basePosition + 3] = roundedRect.rect().height();

You have a matrix. You can remove x, y, width and height.
You can remove x and y by translating the matrix.
You can remove width and height by scaling the coorinate system to fit the rect to 1x1 dimension.
Too complicated? Just an idea.

> Source/WebCore/platform/graphics/texmap/ClipStack.h:35
> +static const unsigned s_roundedRectMaxClipsAllowed = 10;

s_roundedRectMaxClipsAllowed sounds a bit redundant to me. How about s_roundedRectMaxClips or s_maxRoundedRectClips?

> Source/WebCore/platform/graphics/texmap/ClipStack.h:58
> +        unsigned roundedRectIndex { 0 };

roundedRectIndex isn't an index. It should be roundedRectCount or numberOfRoundedRect.

[Fujii Hironori]

Note that Bug 217737 is going to add a new GraphicsLayer::supportsRoundedClip method.

[Miguel Gomez]

Created attachment 412166 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 412166 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/ClipStack.h:52
> -        State(const IntRect& scissors = IntRect(), int stencil = 1)
> +        State(const IntRect& scissors = IntRect())

explicit

[Miguel Gomez]

Created attachment 412167 [details]
Patch

[Miguel Gomez]

Created attachment 412168 [details]
Patch

[EWS]

Committed r268923: <https://trac.webkit.org/changeset/268923>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412168 [details].

[Fujii Hironori]

Comment on attachment 412168 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:469
> +            int nRects = min(ROUNDED_RECT_MAX_RECTS, u_roundedRectNumber);

min(genIType) isn't supported for GLSL ES < 3.0
Filed : Bug 218164 – [WinCairo] REGRESSION(r268923): Nothing is drawn in AC mode

[Fujii Hironori]

*** Bug 115615 has been marked as a duplicate of this bug. ***