Bug 31258 - Win chromium is slow to draw transparent texts
Summary: Win chromium is slow to draw transparent texts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://handcraftedcss.com/workshop/
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-09 08:37 PST by Shinichiro Hamaji
Modified: 2009-11-18 16:44 PST (History)
3 users (show)

See Also:


Attachments
reduction (2.85 KB, text/html)
2009-11-09 08:37 PST, Shinichiro Hamaji
no flags Details
Patch v1 (2.52 KB, patch)
2009-11-09 08:47 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
profile result (335.55 KB, image/png)
2009-11-09 08:50 PST, Shinichiro Hamaji
no flags Details
Patch v3 (3.15 KB, patch)
2009-11-17 23:11 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinichiro Hamaji 2009-11-09 08:37:46 PST
Created attachment 42755 [details]
reduction

Scrolling in the posted URL is very slow with Windows Chromium. This doesn't happen in other environments.

sunandt found that fixed background and rgba text causes this issue, and I noticed this issue happens with rgba text. With the attached HTML which doesn't have background, page up/down is still slow. Scrolling with mouse or arrow keys are OK because we can optimize scrolling with blit for this case (as background prevents this optimization, the original URL was slow even for mouse and arrow keys).

It seems TransparencyAwareFontPainter in FontChromiumWin.cpp is the reason of this slowness. It seems the graphics context isn't clipped properly when GDI is used.

I'll post a patch for this, but I'm not sure if the fix is good as I don't have enough knowledges about this code. Any suggestions will be appreciated.

Chromium side bug: http://code.google.com/p/chromium/issues/detail?id=18537
Comment 1 Shinichiro Hamaji 2009-11-09 08:47:42 PST
Created attachment 42756 [details]
Patch v1
Comment 2 Shinichiro Hamaji 2009-11-09 08:50:27 PST
Created attachment 42757 [details]
profile result
Comment 3 Brett Wilson (Google) 2009-11-09 09:40:55 PST
I see a "save" in initializeForGDI, and a restore in the destructor. What happens when m_useGDI is false? It looks like we'll "restore" with no saved state which will cause problems.
Comment 4 Shinichiro Hamaji 2009-11-09 09:58:15 PST
(In reply to comment #3)
> I see a "save" in initializeForGDI, and a restore in the destructor. What
> happens when m_useGDI is false? It looks like we'll "restore" with no saved
> state which will cause problems.

Thanks for the quick review. I think when m_useGDI is false, initializeForGDI() won't be called.

FYI, the following code block exists above the first code chunk.

void TransparencyAwareFontPainter::init()
{
    if (m_useGDI)
        initializeForGDI();
}
Comment 5 Brett Wilson (Google) 2009-11-09 11:32:27 PST
My concern was the destructor which looked like it was doing the restore every time. But that has an early return for !m_useGDI, so I think this case is OK.

This change LGTM, but I'm not a WebKit reviewer.
Comment 6 Dimitri Glazkov (Google) 2009-11-09 11:38:14 PST
Comment on attachment 42756 [details]
Patch v1

r=me
Comment 7 WebKit Commit Bot 2009-11-09 11:51:05 PST
Comment on attachment 42756 [details]
Patch v1

Clearing flags on attachment: 42756

Committed r50674: <http://trac.webkit.org/changeset/50674>
Comment 8 WebKit Commit Bot 2009-11-09 11:51:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Shinichiro Hamaji 2009-11-17 01:40:56 PST
Reopening this bug as this is still slow. Though creating a big transparency layer is faster than compositing a big transparency layer, it seems we need to avoid creation of a big transparency layer as well.
Comment 10 Dimitri Glazkov (Google) 2009-11-17 08:29:09 PST
brettw, can you look at the second patch on bug 31289? Hamaji-san posted it there by mistake (it's late in Tokyo :), but if you likey, I can land and not wait for another spin of the globe.
Comment 11 Brett Wilson (Google) 2009-11-17 09:28:05 PST
Skia should create layers sized to the current clip. So I think this can be more simply fixed by just moving the current beginTransparencyLayer call to below where the clips are installed. Let me know if this doesn't work.
Comment 12 Shinichiro Hamaji 2009-11-17 23:11:34 PST
Created attachment 43407 [details]
Patch v3
Comment 13 Shinichiro Hamaji 2009-11-17 23:14:43 PST
(In reply to comment #11)
> Skia should create layers sized to the current clip. So I think this can be
> more simply fixed by just moving the current beginTransparencyLayer call to
> below where the clips are installed. Let me know if this doesn't work.

It worked. Thanks for the comment!

And Dimitri, thank you very much for finding I posted my patch on the wrong bug!
Comment 14 Brett Wilson (Google) 2009-11-18 09:25:55 PST
Comment on attachment 43407 [details]
Patch v3

Looks good to me.
Comment 15 Dimitri Glazkov (Google) 2009-11-18 09:40:59 PST
Comment on attachment 43407 [details]
Patch v3

yaay!
Comment 16 WebKit Commit Bot 2009-11-18 16:44:27 PST
Comment on attachment 43407 [details]
Patch v3

Clearing flags on attachment: 43407

Committed r51155: <http://trac.webkit.org/changeset/51155>
Comment 17 WebKit Commit Bot 2009-11-18 16:44:34 PST
All reviewed patches have been landed.  Closing bug.