Bug 43259 – Add the shadow offset as a parameter to the GraphicsContext::setShadow

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Alejandro G. Castro]

In order to draw fast shadow using tiling (bug 39582) we need to know the shadow offset defined in the html. My proposal is to add a parameter to GraphicsContext::setShadow, and store if in the GraphicsContext. Current API is: 

void GraphicsContext::setShadow(const FloatSize& size, float blur, const Color& color, ColorSpace colorSpace)

We will modify it to add this one:

void GraphicsContext::setShadow(const FloatSize& size, const FloatSize& offset, float blur, const Color& color, ColorSpace colorSpace)

We will add there the offset and the spread if required to point out the distance from the border of the shadow to the figure.

[Alejandro G. Castro]

Created attachment 63075 [details]
Proposed patch

[WebKit Review Bot]

Attachment 63075 [details] did not build on win:
Build output: http://queues.webkit.org/results/3637124

[Alejandro G. Castro]

Created attachment 63239 [details]
Proposed patch

Let's see if I found all the calls to the functions this time :).

[Alejandro G. Castro]

Created attachment 63321 [details]
Proposed patch

Added the objective-c modifications I missed.

What is “shadow offset” in this context?

[Alejandro G. Castro]

(In reply to comment #5)
> What is “shadow offset” in this context?

The distance between the border of the shadow and the border of the figure, the information is interesting to know what part of the shadow is visible.

(In reply to comment #6)
> (In reply to comment #5)
> > What is “shadow offset” in this context?
> 
> The distance between the border of the shadow and the border of the figure, the information is interesting to know what part of the shadow is visible.

Can you give an example? Can this be derived from the size and blur radius?

[Simon Fraser (smfr)]

This is very confusing. I think what is referred to as shadow "size" is already the x/y offset.

[Alejandro G. Castro]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > What is “shadow offset” in this context?
> > 
> > The distance between the border of the shadow and the border of the figure, the information is interesting to know what part of the shadow is visible.
> 
> Can you give an example? Can this be derived from the size and blur radius?

It can't be derived AFAIK, even with the size of the shadow and the blur radius we would need the position and size of the original figure to get it. And another problem is that the size it is not actually the size in some situations, for instance RenderBoxModelObject.cpp:1611

...
            // Move the fill just outside the clip, adding 1 pixel separation so that the fill does not
            // bleed in (due to antialiasing) if the context is transformed.
            IntSize extraOffset(w + max(0, shadowOffset.width()) + shadowBlur + 2 * shadowSpread + 1, 0);
            shadowOffset -= extraOffset;
            fillRect.move(extraOffset);

            context->setShadow(shadowOffset, shadowBlur, shadowColor, s->colorSpace());
...

I do not know much about mac implementation but I guess shadowSize in this case is being used to store a prepared value to render the shadow outside the figure, that way they can use the GC API: CGContextSetShadow. In this case we will have a shadowSize (shadowOffset variable in the call to setShadow) with the correct offset in the height but a complete different value in the width, actually as you can see the shadowOffset.width() is substracted from the shadowOffset. In this case we want to render the shadow just in the locations where is is showed to avoid rendering big shadows blurs that will be under the figure, in order to do that we need to know the offset to calculate where we have to draw the shadow.

[Alejandro G. Castro]

(In reply to comment #8)
> This is very confusing. I think what is referred to as shadow "size" is already the x/y offset.

Yeah, most of the time it is used like that, but in some situations it is not actually the offset from the parameter of the shadow, but a prepared offset required by some shadow rendering APIs.

[Alejandro G. Castro]

Any other feedback about this API? This could be the first step to have both parameters size and offset in the function API, with this code we could try also to add the required parameters so every port could handle the position by itself instead of trusting the calculation outside the shadow function that depends on the port. Maybe we will need more parameters in this case.

[Alejandro G. Castro]

Created attachment 64750 [details]
Proposed patch

Rebased to the current trunk.

[Ariya Hidayat]

> ....the information is interesting to know what part of the shadow is visible.

Not sure what shadow visibility do you need here? While working on https://bugs.webkit.org/show_bug.cgi?id=44091 for the Qt port, I found out that I can use the clip region to significantly reduce the shadow area needs to be filtered.

[Alejandro G. Castro]

(In reply to comment #13)
> > ....the information is interesting to know what part of the shadow is visible.
> 
> Not sure what shadow visibility do you need here?

Basically we need the part the shadow that is not under the figure that causes that shadow. For example, if you set an offset of 0px 0px in the CSS when you generate the shadow you can use tiles that just cover from the border of the figure to the end of the blur effect; but if you have 300px 300px you need to create bigger tiles to recreate the shadow.


> While working on https://bugs.webkit.org/show_bug.cgi?id=44091 for the Qt port, I found out that I can use the clip region to significantly reduce the shadow area needs to be filtered.

That's a very interesting improvement, thanks for sharing it, I was thinking about checking it after landing the tiling shadow buffer, I'll check it. Anyway if we want to create the tiling solution we need this information.

[Ariya Hidayat]

> Basically we need the part the shadow that is not under the figure that causes that shadow. 

Unless I miss something, this could be carried out in a different way using the clip region (like in #44091).

[Alejandro G. Castro]

(In reply to comment #15)
> > Basically we need the part the shadow that is not under the figure that causes that shadow. 
> 
> Unless I miss something, this could be carried out in a different way using the clip region (like in #44091).

Maybe I'm the one missing something :), I'm going to check it in detail, thanks for the comment.

[Alejandro G. Castro]

(In reply to comment #15)
> > Basically we need the part the shadow that is not under the figure that causes that shadow. 
> 
> Unless I miss something, this could be carried out in a different way using the clip region (like in #44091).

I've checked the code and maybe I'm missing something, again :), but I double checked the clipped area bounding box is the shadow rect or a piece of that rect, so we still do not have the original rect location information to know the offset of the shadow.

This patch actually also required to clean the use of the size parameter as an offset in the setShadow function anyway. I would say after the patch we should try to remove the size parameter with patches for each port moving the offset calculations required by those port shadows APIs to the port code.

[Alejandro G. Castro]

After talking to Ariya he proposed to use blur radius instead of offset to calculate the smallest buffer sid to avoid using the offset. So closing this bug. 

A cleaning patch of this API is still good idea but probably with other bug rationale. I'll try to propose when I have a moment.

Thanks for the comments again.