Bug 45599 - [Cairo] Generalize ContextShadow from the Qt port and use it for shadow code
Summary: [Cairo] Generalize ContextShadow from the Qt port and use it for shadow code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords:
: 43962 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-09-11 13:04 PDT by Martin Robinson
Modified: 2010-09-23 08:51 PDT (History)
4 users (show)

See Also:


Attachments
Preliminary patch for this issue (83.13 KB, patch)
2010-09-14 17:14 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Generalize ContextShadow and add a (disabled) Cairo implementation (47.33 KB, patch)
2010-09-15 20:03 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch with Alex's suggestions (46.50 KB, patch)
2010-09-16 09:26 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch with Dirk's suggestions (43.09 KB, patch)
2010-09-22 11:27 PDT, Martin Robinson
ariya.hidayat: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2010-09-11 13:04:55 PDT
I believe we should fork Qt's ContextShadow implementation and use it for all Cairo shadowing code. Reasons for this:

1. It will better abstract shadowing logic. My initial implementation of this has already  simplified the shadowing code in places like FontCairo.cpp. It will also make it simpler to fix shadow bugs and add full-shadow support in places where it is broken: (see https://bugs.webkit.org/show_bug.cgi?id=45597).

2. Reduce the number of full-surface copies. The SVG filter code requires one surface copy for the input and makes another copy for the output. That's probably one more than we need.

3. It can be alpha-only. webkit-box-shadow, text shadows and canvas shadows are alpha only. The blurring algorithm should be more efficient if it's operating on only one channel.

4. We can support shadows without depending on SVG filters. This is a minor point, but it's a nice thing to have for embedders who want a small library.

Why fork:

The Qt version of ContextShadow is written with fast transposition in mind (Cairo doesn't support this) and four channel images. There is very little extra logic outside of this (calculating the appropriate transformations for the temporary surface, for instance). I'm confidant unforking is possible though, especially if some other port wishes to use this abstraction.

I should have a first-round patch for this early next week.
Comment 1 Ariya Hidayat 2010-09-11 14:44:41 PDT
Can you wait until I land the patch for https://bugs.webkit.org/show_bug.cgi?id=44222 first? The blur code is rewritten to be smaller and faster there and you certainly don't want to redo the generalization.
Comment 2 Martin Robinson 2010-09-11 18:54:20 PDT
Hrm. If you land a generalized version at some time in the future, the cost of switching the implementation should be almost zero. In the Cairo version I've kept the interface to ShadowContext exactly the same as your version. The blur algorithm that I'm using is currently a very naive sliding window type, because the most important optimization for us was to properly handle clipping. This means that when you land your code, there will be a large incentive for us to share it and I don't mind doing that work.

Additionally, we'd also like to expose our tiling optimizations on it as a higher level method. I think having a fork (even if for a short amount of time), will allow us to do this without getting in your way.
Comment 3 Martin Robinson 2010-09-14 17:11:13 PDT
Arriya informed me that his soon-to-be-posted changes to the blur algorithm do not assume fast transposition. I think instead of forking we should probably just generalize ContextShadow. I've posted prelimary patch which does this. I'll prepare it for real once Arriya lands his improvements (an advance version of which this patch uses).
Comment 4 Martin Robinson 2010-09-14 17:14:41 PDT
Created attachment 67620 [details]
Preliminary patch for this issue
Comment 5 Martin Robinson 2010-09-15 20:03:02 PDT
Created attachment 67766 [details]
Generalize ContextShadow and add a (disabled) Cairo implementation
Comment 6 WebKit Review Bot 2010-09-15 20:05:41 PDT
Attachment 67766 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/cairo/CairoUtilities.h:29:  cairo_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/ContextShadow.h:37:  cairo_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/ContextShadow.h:38:  cairo_surface_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Martin Robinson 2010-09-15 20:06:07 PDT
Ariya has landed his awesome improvements to the blurring algorithm, so I've posted the first patch for this issue. I've decided to split this into two patches.

1. The first will generalize ContextShadow and add an unused Cairo implementation.
2. The second will activate it for all shadow code and update the baselines.

This should make the first patch easier to review by those unfamiliar with the Cairo code.
Comment 8 Martin Robinson 2010-09-15 20:06:50 PDT
Here is a bug tracking the issue with check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=45867
Comment 9 Martin Robinson 2010-09-16 09:26:48 PDT
Created attachment 67804 [details]
Patch with Alex's suggestions
Comment 10 WebKit Review Bot 2010-09-16 09:29:04 PDT
Attachment 67804 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/cairo/CairoUtilities.h:29:  cairo_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/ContextShadow.h:38:  cairo_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/ContextShadow.h:39:  cairo_surface_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Alejandro G. Castro 2010-09-16 09:39:32 PDT
*** Bug 43962 has been marked as a duplicate of this bug. ***
Comment 12 Dirk Schulze 2010-09-22 08:44:13 PDT
Comment on attachment 67804 [details]
Patch with Alex's suggestions

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

Ok, great patch. I would like to see Ariya or someother guy of the Qt folks to look at the qt changes. Otherwise looks good, please fix the style issues. Did not look at the style bot results. r- for the issues mentioned above. If no Qt guy is available, I look at the qt changes again.

> WebCore/platform/graphics/ContextShadow.cpp:33
> +#include "MathExtras.h"
> +#include "Timer.h"

shouldn'T it be <wtf/MathExtras.h>? not sure about timer. Why do you need timer?

> WebCore/platform/graphics/ContextShadow.cpp:57
> -    if (!color.isValid() || !color.alpha()) {
> +    if (!m_color.isValid()) {

Why not chechink for alpha?

> WebCore/platform/graphics/ContextShadow.cpp:76
> +    m_offset = FloatSize(0, 0);

Just FloatSize()

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:49
> +// ContextShadow needs a scratch image as the buffer for the blur filter.
> +// Instead of creating and destroying the buffer for every operation,
> +// we create a buffer which will be automatically purged via a timer.
> +class PurgeScratchBufferTimer : public TimerBase {
> +private:

Ah here you need the timer, can the include get removed on th platform independent file?
Comment 13 Martin Robinson 2010-09-22 11:27:09 PDT
Created attachment 68401 [details]
Patch with Dirk's suggestions
Comment 14 Martin Robinson 2010-09-22 11:29:35 PDT
(In reply to comment #12)

Thanks for the review!

> > WebCore/platform/graphics/ContextShadow.cpp:33
> > +#include "MathExtras.h"
> > +#include "Timer.h"
> shouldn'T it be <wtf/MathExtras.h>? not sure about timer. Why do you need timer?

Fixed! Also removed the extra Timer.h.

> > WebCore/platform/graphics/ContextShadow.cpp:57
> > -    if (!color.isValid() || !color.alpha()) {
> > +    if (!m_color.isValid()) {
> Why not chechink for alpha?

I'm not sure where that went. I added it back.

> > WebCore/platform/graphics/ContextShadow.cpp:76
> > +    m_offset = FloatSize(0, 0);
> Just FloatSize()

Fixed!
Comment 15 Ariya Hidayat 2010-09-22 22:26:17 PDT
Comment on attachment 68401 [details]
Patch with Dirk's suggestions

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

Nice stuff :)

> WebCore/platform/graphics/ContextShadow.h:113
> +    void calculateMinimalLayerRect(const FloatRect& layerArea, const IntRect& clipRect);

I guess 'calculateLayerBoundingRect' is less complicated. 'Minimal' there does not add anything.


Otherwise, LGTM.
Comment 16 Martin Robinson 2010-09-23 08:45:17 PDT
(In reply to comment #15)

> I guess 'calculateLayerBoundingRect' is less complicated. 'Minimal' there does not add anything.

Thanks for the review! I'll change this before landing.
Comment 17 Martin Robinson 2010-09-23 08:51:58 PDT
Committed r68145: <http://trac.webkit.org/changeset/68145>