Bug 31258 – Win chromium is slow to draw transparent texts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Shinichiro Hamaji]

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

[Shinichiro Hamaji]

Created attachment 42756 [details]
Patch v1

[Shinichiro Hamaji]

Created attachment 42757 [details]
profile result

[Brett Wilson (Google)]

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.

[Shinichiro Hamaji]

(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();
}

[Brett Wilson (Google)]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 42756 [details]
Patch v1

r=me

[WebKit Commit Bot]

Comment on attachment 42756 [details]
Patch v1

Clearing flags on attachment: 42756

Committed r50674: <http://trac.webkit.org/changeset/50674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shinichiro Hamaji]

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.

[Dimitri Glazkov (Google)]

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.

[Brett Wilson (Google)]

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.

[Shinichiro Hamaji]

Created attachment 43407 [details]
Patch v3

[Shinichiro Hamaji]

(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!

[Brett Wilson (Google)]

Comment on attachment 43407 [details]
Patch v3

Looks good to me.

[Dimitri Glazkov (Google)]

Comment on attachment 43407 [details]
Patch v3

yaay!

[WebKit Commit Bot]

Comment on attachment 43407 [details]
Patch v3

Clearing flags on attachment: 43407

Committed r51155: <http://trac.webkit.org/changeset/51155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.