Bug 102000 - [EFL][WK2] White flicker when scrolling big pages with dark background on slower hardware.
Summary: [EFL][WK2] White flicker when scrolling big pages with dark background on slo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-12 15:44 PST by Viatcheslav Ostapenko
Modified: 2012-11-16 08:20 PST (History)
10 users (show)

See Also:


Attachments
Match view background to page background. (20.64 KB, patch)
2012-11-12 16:00 PST, Viatcheslav Ostapenko
noam: review-
Details | Formatted Diff | Diff
Send color info directly to LayerTreeRenderer . (19.69 KB, patch)
2012-11-12 21:42 PST, Viatcheslav Ostapenko
noam: review-
Details | Formatted Diff | Diff
Updated patch. (21.07 KB, patch)
2012-11-13 10:48 PST, Viatcheslav Ostapenko
noam: review-
Details | Formatted Diff | Diff
Updated patch. (21.21 KB, patch)
2012-11-13 11:47 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Add drawSolidColor method to Texture Mapper (6.57 KB, patch)
2012-11-13 21:58 PST, Viatcheslav Ostapenko
noam: review-
Details | Formatted Diff | Diff
View background color patch. (14.72 KB, patch)
2012-11-13 22:00 PST, Viatcheslav Ostapenko
kenneth: review-
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Add context transformation for image buffer texture mapper. (6.72 KB, patch)
2012-11-14 08:17 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Typo fixed. Changelog fixed. Depends on patch 174162 (14.92 KB, patch)
2012-11-14 08:19 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Resubmit patch to restart EWS (14.92 KB, patch)
2012-11-15 08:42 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2012-11-12 15:44:12 PST
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.
Comment 1 Viatcheslav Ostapenko 2012-11-12 16:00:01 PST
Created attachment 173748 [details]
Match view background to page background.
Comment 2 Noam Rosenthal 2012-11-12 16:06:41 PST
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.
Comment 3 Viatcheslav Ostapenko 2012-11-12 16:28:25 PST
(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.
Comment 4 Noam Rosenthal 2012-11-12 16:30:16 PST
(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...
Comment 5 Viatcheslav Ostapenko 2012-11-12 16:35:47 PST
(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
Comment 6 Viatcheslav Ostapenko 2012-11-12 21:42:02 PST
Created attachment 173814 [details]
Send color info directly to LayerTreeRenderer .
Comment 7 Noam Rosenthal 2012-11-12 22:08:58 PST
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);
Comment 8 Viatcheslav Ostapenko 2012-11-12 23:02:47 PST
(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.
Comment 9 Noam Rosenthal 2012-11-13 06:13:16 PST
(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.
Comment 10 Viatcheslav Ostapenko 2012-11-13 08:30:07 PST
(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.
Comment 11 Noam Rosenthal 2012-11-13 09:09:45 PST
> 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.
Comment 12 Viatcheslav Ostapenko 2012-11-13 09:53:35 PST
(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?
Comment 13 Noam Rosenthal 2012-11-13 10:00:28 PST
(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.
Comment 14 Viatcheslav Ostapenko 2012-11-13 10:48:35 PST
Created attachment 173926 [details]
Updated patch.
Comment 15 Noam Rosenthal 2012-11-13 10:52:47 PST
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
Comment 16 Viatcheslav Ostapenko 2012-11-13 11:47:02 PST
Created attachment 173936 [details]
Updated patch.
Comment 17 Noam Rosenthal 2012-11-13 13:13:54 PST
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.
Comment 18 Viatcheslav Ostapenko 2012-11-13 15:47:10 PST
(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.
Comment 19 Viatcheslav Ostapenko 2012-11-13 21:58:29 PST
Created attachment 174073 [details]
Add drawSolidColor method to Texture Mapper
Comment 20 Viatcheslav Ostapenko 2012-11-13 22:00:26 PST
Created attachment 174074 [details]
View background color patch.

Depends on "Draw solid color" patch.
Comment 21 Early Warning System Bot 2012-11-13 22:09:18 PST
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
Comment 22 EFL EWS Bot 2012-11-13 22:33:49 PST
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
Comment 23 WebKit Review Bot 2012-11-14 00:52:43 PST
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
Comment 24 Kenneth Rohde Christiansen 2012-11-14 03:08:40 PST
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!
Comment 25 Noam Rosenthal 2012-11-14 06:30:35 PST
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?
Comment 26 Viatcheslav Ostapenko 2012-11-14 07:26:11 PST
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.
Comment 27 Viatcheslav Ostapenko 2012-11-14 08:17:19 PST
Created attachment 174162 [details]
Add context transformation for image buffer texture mapper.
Comment 28 Viatcheslav Ostapenko 2012-11-14 08:19:36 PST
Created attachment 174164 [details]
Typo fixed. Changelog fixed. Depends on patch 174162
Comment 29 Early Warning System Bot 2012-11-14 08:27:37 PST
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
Comment 30 EFL EWS Bot 2012-11-14 08:48:46 PST
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
Comment 31 WebKit Review Bot 2012-11-14 14:30:08 PST
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>
Comment 32 Viatcheslav Ostapenko 2012-11-14 15:00:58 PST
Comment on attachment 174164 [details]
Typo fixed. Changelog fixed. Depends on patch 174162

Patch 174162 has landed, so it should compile fine now.
Comment 33 Kenneth Rohde Christiansen 2012-11-15 07:26:11 PST
Comment on attachment 174164 [details]
Typo fixed. Changelog fixed. Depends on patch 174162

this doesnt seem o work on qt-wk2 nor efl
Comment 34 Viatcheslav Ostapenko 2012-11-15 08:42:24 PST
Created attachment 174456 [details]
Resubmit patch to restart EWS
Comment 35 Kenneth Rohde Christiansen 2012-11-15 10:02:39 PST
Comment on attachment 174456 [details]
Resubmit patch to restart EWS

Don't land before we know it passes EWS
Comment 36 WebKit Review Bot 2012-11-16 08:20:30 PST
Comment on attachment 174456 [details]
Resubmit patch to restart EWS

Clearing flags on attachment: 174456

Committed r134953: <http://trac.webkit.org/changeset/134953>
Comment 37 WebKit Review Bot 2012-11-16 08:20:37 PST
All reviewed patches have been landed.  Closing bug.