Bug 44091 – [Qt] Reduce the size of the shadow buffer to the clip region

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ariya Hidayat]

http://trac.webkit.org/changeset/65425 (from https://bugs.webkit.org/show_bug.cgi?id=44025) adds blur support to the shadow. The buffer image for the shadow is however slightly larger than the rectangle that casts the shadow. This becomes a problem if the rectangle is e.g. a very big <div> in a web page. Technically the shadow buffer needs to be as large as the viewport or the current clipping region only.

[Ariya Hidayat]

Created attachment 64551 [details]
Patch

[Dirk Schulze]

I wounder if the clipping should be applied before adjusting the shadow rect. Doesn't your shadow have some blurring on the sides of the screen, if the shadowed rect is bigger than the current viewport?

[Ariya Hidayat]

(In reply to comment #2)
> I wounder if the clipping should be applied before adjusting the shadow rect. 

In that case, we might miss the case where the clip is adjacent to the shadow rect, and the shadow has a large blur radius.

But I see your point. I guess when the clip region can not fully contain the enlarged shadow rect (thus, they are intersecting or reverse contained), we need to expand the resulting rect once again.

I'll update the patch to reflect this situation.

[Ariya Hidayat]

Created attachment 64556 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 64556 [details]
Patch

Is there any performance difference with this?WebCore/platform/graphics/qt/ContextShadow.cpp:212
 +  
why a new line here?

[Ariya Hidayat]

(In reply to comment #5)
> (From update of attachment 64556 [details])
> Is there any performance difference with this?

Definitely. Some sites, e.g. http://sueddeutsche.de/ has superlarge div, several thousand pixels height, with drop shadow and blur. Without this patch, we keep using QImage as large as that while a much smaller one will do the job just fine.

> WebCore/platform/graphics/qt/ContextShadow.cpp:212
>  +  
> why a new line here?

Not necessary.

[Kenneth Rohde Christiansen]

Can this be slower in some cases as well? or is that neglectable?

[Ariya Hidayat]

Created attachment 64564 [details]
Patch

[Dirk Schulze]

Not sure how this is called. But doesn't Qt store the current drawing of the page? So that the page doesn't need to draw on scrolling? That would mean, you just need to calculate the shadow once, and not over and over again?

[Ariya Hidayat]

(In reply to comment #7)
> Can this be slower in some cases as well? or is that neglectable?

The shadow blur area is guaranteed to be the same or smaller with this patch, vs without this patch.

The only possible slowdown is the clipRegion().boundingRect() calculation. But this is really nothing (=negligible) compared to the blur operations itself (the most expensive part!), thus the extra cost is really worth the benefit.

[Ariya Hidayat]

(In reply to comment #9)
> Not sure how this is called. But doesn't Qt store the current drawing of the page? So that the page doesn't need to draw on scrolling? That would mean, you just need to calculate the shadow once, and not over and over again?

Do you mean the tiled backing store? It's just an optimization to reduce the painting operations, but the shadow (as with other primitives) still needs be drawn once.

[Dirk Schulze]

(In reply to comment #11)
> Do you mean the tiled backing store? It's just an optimization to reduce the painting operations, but the shadow (as with other primitives) still needs be drawn once.

Yes I mean tiled backing store. But what do you mean with "still needs be drawn once.", this wouldn't affect scrolling right? Means you just have a longer calculation of the effect once? (I'm just curious)

[Ariya Hidayat]

> Yes I mean tiled backing store. But what do you mean with "still needs be drawn once.", this wouldn't affect scrolling right? Means you just have a longer calculation of the effect once? (I'm just curious)

It's just caching the page onto a set of pixmaps (=tiles) with the coverage slightly larger than the viewport. When you scroll a bit and the tiles still cover the new viewport entirely, only bitmap blit is necessary. When you scroll farther and the newly exposed area in the viewport are not covered by any of the tiles, then new tiles are generated (this is where the page is drawn, to the tiles).

[Ariya Hidayat]

Comment on attachment 64564 [details]
Patch

Clearing flags on attachment: 64564

Committed r65488: <http://trac.webkit.org/changeset/65488>

[Ariya Hidayat]

All reviewed patches have been landed.  Closing bug.