Bug 31258 - Win chromium is slow to draw transparent texts
: Win chromium is slow to draw transparent texts
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To:
: http://handcraftedcss.com/workshop/
:
:
:
  Show dependency treegraph
 
Reported: 2009-11-09 08:37 PST by
Modified: 2009-11-18 16:44 PST (History)


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 Review Patch | 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 Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-09 08:37:46 PST
Created an attachment (id=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 From 2009-11-09 08:47:42 PST -------
Created an attachment (id=42756) [details]
Patch v1
------- Comment #2 From 2009-11-09 08:50:27 PST -------
Created an attachment (id=42757) [details]
profile result
------- Comment #3 From 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 From 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 From 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 From 2009-11-09 11:38:14 PST -------
(From update of attachment 42756 [details])
r=me
------- Comment #7 From 2009-11-09 11:51:05 PST -------
(From update of attachment 42756 [details])
Clearing flags on attachment: 42756

Committed r50674: <http://trac.webkit.org/changeset/50674>
------- Comment #8 From 2009-11-09 11:51:11 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #9 From 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 From 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 From 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 From 2009-11-17 23:11:34 PST -------
Created an attachment (id=43407) [details]
Patch v3
------- Comment #13 From 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 From 2009-11-18 09:25:55 PST -------
(From update of attachment 43407 [details])
Looks good to me.
------- Comment #15 From 2009-11-18 09:40:59 PST -------
(From update of attachment 43407 [details])
yaay!
------- Comment #16 From 2009-11-18 16:44:27 PST -------
(From update of attachment 43407 [details])
Clearing flags on attachment: 43407

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