Bug 102000 – [EFL][WK2] White flicker when scrolling big pages with dark background on slower hardware.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

On slow CPU hardware webprocess is not able to render enough tiles during fast scrolling and UI shows white background behind tiles. If page has dark background this flicker is very noticeable and annoying.

[Viatcheslav Ostapenko]

Created attachment 173748 [details]
Match view background to page background.

[Noam Rosenthal]

Comment on attachment 173748 [details]
Match view background to page background.

You should clear the color inside ewk_view before painting, not add an API to LayerTreeRenderer.

[Viatcheslav Ostapenko]

(In reply to comment #2)
> (From update of attachment 173748 [details])
> You should clear the color inside ewk_view before painting, not add an API to LayerTreeRenderer.

Won't it will be useful to other ports? 
I 1st made it like you say clearing viewport before painting in EwkViewImpl, but it looked ugly with all GL ifdefs/includes and 2 branches for GL and non-GL cases.

[Noam Rosenthal]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 173748 [details] [details])
> > You should clear the color inside ewk_view before painting, not add an API to LayerTreeRenderer.
> 
> Won't it will be useful to other ports? 
> I 1st made it like you say clearing viewport before painting in EwkViewImpl, but it looked ugly with all GL ifdefs/includes and 2 branches for GL and non-GL cases.

It would be useful for other ports, but not in this way. We should pass the actual background color to LayerTreeCoordinatorProxy, not go out to the API and back in...

[Viatcheslav Ostapenko]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 173748 [details] [details] [details])
> > > You should clear the color inside ewk_view before painting, not add an API to LayerTreeRenderer.
> > 
> > Won't it will be useful to other ports? 
> > I 1st made it like you say clearing viewport before painting in EwkViewImpl, but it looked ugly with all GL ifdefs/includes and 2 branches for GL and non-GL cases.
> 
> It would be useful for other ports, but not in this way. We should pass the actual background color to LayerTreeCoordinatorProxy, not go out to the API and back in...

Ok

[Viatcheslav Ostapenko]

Created attachment 173814 [details]
Send color info directly to LayerTreeRenderer .

[Noam Rosenthal]

Comment on attachment 173814 [details]
Send color info directly to LayerTreeRenderer .

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

This will not work with semi-transparent backgrounds or with non rectangular clip, as it uses glClear and friends which completely clear the pixels in a given rectangle.
I suggest that instead of this we expose backgroundColor from DrawingAreaProxy, and let the embedded (ewk_view?) decide what and how to clear.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:96
> +    , m_backgroundColor(255, 255, 255, 255)

Color::white()

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:768
> +    m_webPage->send(Messages::LayerTreeCoordinatorProxy::SetBackgroundColor(color));

So we do this on every layout? seems wasteful. Maybe we should make sure that the color has actually changed?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2770
> +        WebCore::Color color = WebCore::Color(float(red), float(green), float(blue), float(alpha));

Color color(red, green, blue, alpha);

[Viatcheslav Ostapenko]

(In reply to comment #7)
> (From update of attachment 173814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173814&action=review
> 
> This will not work with semi-transparent backgrounds or with non rectangular clip, as it uses glClear and friends which completely clear the pixels in a given rectangle.
> I suggest that instead of this we expose backgroundColor from DrawingAreaProxy, and let the embedded (ewk_view?) decide what and how to clear.

So, every port will have its own code for this?

> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:96
> > +    , m_backgroundColor(255, 255, 255, 255)
> 
> Color::white()

Ok

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:768
> > +    m_webPage->send(Messages::LayerTreeCoordinatorProxy::SetBackgroundColor(color));
> 
> So we do this on every layout? seems wasteful. Maybe we should make sure that the color has actually changed?

Ok

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2770
> > +        WebCore::Color color = WebCore::Color(float(red), float(green), float(blue), float(alpha));
> 
> Color color(red, green, blue, alpha);

Doesn't work. red, green, blue and alpha are doubles. There is 2 constructors for Color - with 4 ints and 4 floats. Compiler cannot decide which constructor to use.

[Noam Rosenthal]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 173814 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=173814&action=review
> > 
> > This will not work with semi-transparent backgrounds or with non rectangular clip, as it uses glClear and friends which completely clear the pixels in a given rectangle.
> > I suggest that instead of this we expose backgroundColor from DrawingAreaProxy, and let the embedded (ewk_view?) decide what and how to clear.
> 
> So, every port will have its own code for this?
If you're using glClear on the entire clipRect, yes.
I don't mind having shared code here, if it does the right thing - draws a solid color rect only on the contents rect of the page, and only if the page is not transparent.

[Viatcheslav Ostapenko]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 173814 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=173814&action=review
> > > 
> > > This will not work with semi-transparent backgrounds or with non rectangular clip, as it uses glClear and friends which completely clear the pixels in a given rectangle.
> > > I suggest that instead of this we expose backgroundColor from DrawingAreaProxy, and let the embedded (ewk_view?) decide what and how to clear.
> > 
> > So, every port will have its own code for this?
> If you're using glClear on the entire clipRect, yes.
> I don't mind having shared code here, if it does the right thing - draws a solid color rect only on the contents rect of the page, and only if the page is not transparent.

No no no!
It shouldn't be used if page transparency is desired. That's why it has a switch and disabled by default. IMHO, checking for page transparency would be unnecessary overhead. Probably need to add note about this to API description and to the changelog.
I'll fix rect drawing without glClear.

[Noam Rosenthal]

> No no no!
That's very negative :)

> Probably need to add note about this to API description and to the changelog.
> I'll fix rect drawing without glClear.
Well, in this case it would also work with page transparency, wouldn't it? Which makes your first statement incorrect.

[Viatcheslav Ostapenko]

(In reply to comment #11)
> > No no no!
> That's very negative :)
> 
> > Probably need to add note about this to API description and to the changelog.
> > I'll fix rect drawing without glClear.
> Well, in this case it would also work with page transparency, wouldn't it? Which makes your first statement incorrect.

Do you mean, that if page is transparent then background color will be transparent also?
The changelog on getDocumentBackgroundColor says:
Call down into the document to get either the body's, or if there is no body, the root element's, background color.

So, let's say, the root element has non-transparent background, but has opacity set on it. Should I mix in opacity?

[Noam Rosenthal]

(In reply to comment #12)
> (In reply to comment #11)
> > > No no no!
> > That's very negative :)
> > 
> > > Probably need to add note about this to API description and to the changelog.
> > > I'll fix rect drawing without glClear.
> > Well, in this case it would also work with page transparency, wouldn't it? Which makes your first statement incorrect.
> 
> Do you mean, that if page is transparent then background color will be transparent also?
> The changelog on getDocumentBackgroundColor says:
> Call down into the document to get either the body's, or if there is no body, the root element's, background color.
> 
> So, let's say, the root element has non-transparent background, but has opacity set on it. Should I mix in opacity?

Yes, we should mix in the opacity that's passed with paintToCurrentGLContext.

[Viatcheslav Ostapenko]

Created attachment 173926 [details]
Updated patch.

[Noam Rosenthal]

Comment on attachment 173926 [details]
Updated patch.

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:432
> +    FloatRect targetRect(0, 0, data().viewport[2], data().viewport[3]);
> +    drawQuad(targetRect, TransformationMatrix(), program.get(), GraphicsContext3D::TRIANGLE_FAN, false);

Clearing the whole viewport doesn't make sense. We should expose drawSolidColor(const FloatRect&, const TransformationMatrix&, const Color&), and pass in the viewport rect from LayerTreeRenderer.

> Source/WebKit2/ChangeLog:38
> +        (WebKit::LayerTreeRenderer::enableViewBackgroundMatchToPage):

Let's change this to setDrawsBackground

[Viatcheslav Ostapenko]

Created attachment 173936 [details]
Updated patch.

[Noam Rosenthal]

Comment on attachment 173936 [details]
Updated patch.

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2776
> +    if (m_drawingArea && m_drawingArea->layerTreeHost()) {
> +        double red, green, blue, alpha;
> +        m_mainFrame->getDocumentBackgroundColor(&red, &green, &blue, &alpha);
> +        RGBA32 rgba = makeRGBA32FromFloats(red, green, blue, alpha);
> +        if (m_backgroundColor.rgb() != rgba) {
> +            m_backgroundColor.setRGB(rgba);
> +            m_drawingArea->layerTreeHost()->setBackgroundColor(m_backgroundColor);
> +        }
> +    }

This should be guarded with ACCELERATED_COMPOSITING, or alternatively it should move to a DrawingArea[Proxy] message set rather than go via coordinated graphics.
I'm ok with this in general but I'm not comfortable r+ing it because I'm not sure of the implication of calling getDocumentBackgroundColor after every layout.
I'm OK with the changes to TextureMapper, if you want to post them in a separate patch.

[Viatcheslav Ostapenko]

(In reply to comment #17)
> (From update of attachment 173936 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173936&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2776
> > +    if (m_drawingArea && m_drawingArea->layerTreeHost()) {
> > +        double red, green, blue, alpha;
> > +        m_mainFrame->getDocumentBackgroundColor(&red, &green, &blue, &alpha);
> > +        RGBA32 rgba = makeRGBA32FromFloats(red, green, blue, alpha);
> > +        if (m_backgroundColor.rgb() != rgba) {
> > +            m_backgroundColor.setRGB(rgba);
> > +            m_drawingArea->layerTreeHost()->setBackgroundColor(m_backgroundColor);
> > +        }
> > +    }
> 
> This should be guarded with ACCELERATED_COMPOSITING, or alternatively it should move to a DrawingArea[Proxy] message set rather than go via coordinated graphics.
> I'm ok with this in general but I'm not comfortable r+ing it because I'm not sure of the implication of calling getDocumentBackgroundColor after every layout.

It composed of 2 color values requested from style of body and html elements and one color (baseBackgroundColor) stored in a FrameView. Getting those values from style does invoke any search or tree traverse, but just get color values stored in style itself. Those style functions are called a lot from different places.
Also, it seems been called already on every layout for overlay scrollbars style calculations (FrameView::recalculateScrollbarOverlayStyle()) and from RenderLayerCompositor.
IMHO, it would be good to make caching of this value at least to avoid re-calculation if there was no layout (in a separate patch?). But it has to be recalculated after every layout.

> I'm OK with the changes to TextureMapper, if you want to post them in a separate patch.

Ok, I'll put it to the separate patch.

[Viatcheslav Ostapenko]

Created attachment 174073 [details]
Add drawSolidColor method to Texture Mapper

[Viatcheslav Ostapenko]

Created attachment 174074 [details]
View background color patch.

Depends on "Draw solid color" patch.

[Early Warning System Bot]

Comment on attachment 174074 [details]
View background color patch.

Attachment 174074 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14833165

[EFL EWS Bot]

Comment on attachment 174074 [details]
View background color patch.

Attachment 174074 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14837166

[WebKit Review Bot]

Comment on attachment 174074 [details]
View background color patch.

Attachment 174074 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14828359

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Kenneth Rohde Christiansen]

Comment on attachment 174074 [details]
View background color patch.

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

> Source/WebKit2/ChangeLog:11
> +        with new API function ewk_view_draws_page_brackground_set .

Please check spelling! brackground? :)

Info on why it makes sense to have it disabled by default.

Could we do something smarter even by showing a low scale version of the content instead (or between real content and background color) ? say scale 0.25, that should account for max 3 tiles for that.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:925
> +void ewk_view_draws_page_brackground_set(Evas_Object *ewkView, Eina_Bool enabled)

oh no! even here you spell it wrong!

[Noam Rosenthal]

Comment on attachment 174073 [details]
Add drawSolidColor method to Texture Mapper

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

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:130
> +    UNUSED_PARAM(matrix);

maybe context->setCTM?

[Viatcheslav Ostapenko]

Comment on attachment 174074 [details]
View background color patch.

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

>> Source/WebKit2/ChangeLog:11
>> +        with new API function ewk_view_draws_page_brackground_set .
> 
> Please check spelling! brackground? :)
> 
> Info on why it makes sense to have it disabled by default.
> 
> Could we do something smarter even by showing a low scale version of the content instead (or between real content and background color) ? say scale 0.25, that should account for max 3 tiles for that.

That would require much bigger overhead for creating and maintaining this lowres copy. Opera does something like this, but without rendering text. I don't think it looks much better. Also, webkit rendering is much faster and usually it is just small region near the border of view that flickers. In opera on the same device I was able to scroll out the whole screen and see lowres page. IMHO, doesn't look better and doesn't worth it.

[Viatcheslav Ostapenko]

Created attachment 174162 [details]
Add context transformation for image buffer texture mapper.

[Viatcheslav Ostapenko]

Created attachment 174164 [details]
Typo fixed. Changelog fixed. Depends on patch 174162

[Early Warning System Bot]

Comment on attachment 174164 [details]
Typo fixed. Changelog fixed. Depends on patch 174162

Attachment 174164 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14828488

[EFL EWS Bot]

Comment on attachment 174164 [details]
Typo fixed. Changelog fixed. Depends on patch 174162

Attachment 174164 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14823926

[WebKit Review Bot]

Comment on attachment 174162 [details]
Add context transformation for image buffer texture mapper.

Clearing flags on attachment: 174162

Committed r134671: <http://trac.webkit.org/changeset/134671>

[Viatcheslav Ostapenko]

Comment on attachment 174164 [details]
Typo fixed. Changelog fixed. Depends on patch 174162

Patch 174162 has landed, so it should compile fine now.

[Kenneth Rohde Christiansen]

Comment on attachment 174164 [details]
Typo fixed. Changelog fixed. Depends on patch 174162

this doesnt seem o work on qt-wk2 nor efl

[Viatcheslav Ostapenko]

Created attachment 174456 [details]
Resubmit patch to restart EWS

[Kenneth Rohde Christiansen]

Comment on attachment 174456 [details]
Resubmit patch to restart EWS

Don't land before we know it passes EWS

[WebKit Review Bot]

Comment on attachment 174456 [details]
Resubmit patch to restart EWS

Clearing flags on attachment: 174456

Committed r134953: <http://trac.webkit.org/changeset/134953>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.