Bug 118383

Summary: Add initial implementation of ThreadSafeCoordinatedSurface, ThreadedCompositor, and ViewportController
Product: WebKit Reporter: Gwang Yoon Hwang <yoon>
Component: Layout and RenderingAssignee: Gwang Yoon Hwang <yoon>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cmarcelo, commit-queue, jaepark, luiz, mrobinson, noam, sergio, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 117227, 118265    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Gwang Yoon Hwang 2013-07-04 01:32:50 PDT
Threaded Compositor is a variant of Coordinated Graphics implementation.
    Basic structure of the implementaion is simliar, thus, Threaded
    Compositor reuses lots of classes from Coordinated Graphics. However,
    instead of compositing on UI Process, Threaded Compositor performs
    compositing on a separate thread of Web Process.
Comment 1 Gwang Yoon Hwang 2013-07-04 05:02:16 PDT
Created attachment 206077 [details]
Patch
Comment 2 Noam Rosenthal 2013-07-17 19:15:20 PDT
Comment on attachment 206077 [details]
Patch

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

> Source/WebCore/ChangeLog:29
> +        PageViewportController
> +        This class is responsible to handling requests from WebCore (eg. content
> +        size changes, viewport attributes changes and scroll position requests).

This is a huge change.
I would start by dealing with the compositor itself rather than having a copy of PVC in WebCore.
Comment 3 Gwang Yoon Hwang 2014-12-08 08:11:14 PST
Created attachment 242811 [details]
Patch
Comment 4 Anders Carlsson 2014-12-08 08:38:02 PST
Comment on attachment 242811 [details]
Patch

Why is this something you want to do? On both OS X and iOS, the trend is going towards doing UI side compositing.
Comment 5 Gwang Yoon Hwang 2014-12-08 09:58:48 PST
(In reply to comment #4)
> Comment on attachment 242811 [details]
> Patch
> 
> Why is this something you want to do? On both OS X and iOS, the trend is
> going towards doing UI side compositing.

There are 2 main reasons.

1. Mobile GPUs are quite unstable
 Unlike Apple's hardware, GPU drivers for embedded linux can be easily crashed unexpectedly. So we want to keep UI Process safely from OpenGL, we can recover Web Process from UI Process when we meet unexpected crashes from GPU drivers.

2. We need compositing thread even we decided to composite contents in the UI Process.
 For the responsive, we should avoid calling OpenGL calls in the main-thread of UI Process. We can provide additional thread in the UI Process side for compositing even we finally decide to composite in the UI Process using this implementation.
Comment 6 Martin Robinson 2014-12-09 03:20:36 PST
Comment on attachment 242811 [details]
Patch

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

Looks good, couple small issues.

> Source/WebKit2/ChangeLog:4
> +        [ThreadedCompositor] Implements an initial version of the Threaded Compositor.
> +        https://bugs.webkit.org/show_bug.cgi?id=118383

I updated the bug title to be more descriptive, so the patch should be updated as well.

> Source/WebKit2/ChangeLog:50
> +        (WebKit::CompositingRunLoop::CompositingRunLoop):
> +        (WebKit::CompositingRunLoop::callOnCompositingRunLoop):
> +        (WebKit::CompositingRunLoop::setUpdateTimer):
> +        (WebKit::CompositingRunLoop::stopUpdateTimer):
> +        (WebKit::CompositingRunLoop::runLoop):
> +        (WebKit::CompositingRunLoop::updateTimerFired):
> +        (WebKit::ThreadedCompositor::create):
> +        (WebKit::ThreadedCompositor::ThreadedCompositor):
> +        (WebKit::ThreadedCompositor::~ThreadedCompositor):
> +        (WebKit::ThreadedCompositor::setNeedsDisplay):
> +        (WebKit::ThreadedCompositor::setNativeSurfaceHandleForCompositing):

You can omit the blank ones and just leave the one for the file itself.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadSafeCoordinatedSurface.cpp:35
> +#if USE(TEXTURE_MAPPER)
> +#include <WebCore/TextureMapperGL.h>
> +#endif

Doesn't COORDINATED_GRAPHICS rely on texture mapper anyway? Perhaps this could just be moved to the main block without the #ifdef?

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadSafeCoordinatedSurface.cpp:86
> +#if USE(TEXTURE_MAPPER)

Same question here as above.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:67
> +    void setUpdateTimer(double interval = 0.0f)

0.0f should just be a 0 without since that converts properly.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:256
> +    m_scene->paintToCurrentGLContext(viewportTransform, 1.0, clipRect, Color::white, false, scrollPostion);

1.0 can just be 1 here.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:1
> +/*

So this should probably move to the parent directory, but just have a different name. Perhaps SimpleViewportController or DesktopViewportController, since it isn't really related to threaded compositing, but the fact that this is targeting a simpler type of screen.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:2
> + * Copyright (C) 2014 Igalia S.L.

Should this retain the copyright from the original ViewportController.h?

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:77
> +    m_allowsUserScaling = !!m_rawAttributes.userScalable;

I don't believe the double !! is necessary.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:79
> +

I don't think you need these parenthesis, right?

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:132
> +    FloatRect bounds;
> +    bounds.setWidth(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale)));
> +    bounds.setHeight(std::max(0.f, m_contentsSize.height() - floorf(m_viewportSize.height() / scale)));

This can just be FloatRect bounds(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale)), ...)

You don't need to use .f on the floating point number.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:136
> +    FloatPoint position;
> +    position.setX(clampTo(framePosition.x(), bounds.x(), bounds.width()));
> +    position.setY(clampTo(framePosition.y(), bounds.y(), bounds.height()));

This can just be FloatPoint position(clampTo(framePosition.x(), bounds.x(), bounds.width()), clampTo(framePosition.y(), bounds.y(), bounds.height()));
Comment 7 Gwang Yoon Hwang 2014-12-09 05:36:49 PST
(In reply to comment #6)

> > Source/WebKit2/ChangeLog:4
> > +        [ThreadedCompositor] Implements an initial version of the Threaded Compositor.
> > +        https://bugs.webkit.org/show_bug.cgi?id=118383
> 
> I updated the bug title to be more descriptive, so the patch should be
> updated as well.

Good suggestion.
I'll modify the patch for that.

> > Source/WebKit2/ChangeLog:50
> > +        (WebKit::CompositingRunLoop::CompositingRunLoop):
> > +        (WebKit::CompositingRunLoop::callOnCompositingRunLoop):
> > +        (WebKit::CompositingRunLoop::setUpdateTimer):
> > +        (WebKit::CompositingRunLoop::stopUpdateTimer):
> > +        (WebKit::CompositingRunLoop::runLoop):
> > +        (WebKit::CompositingRunLoop::updateTimerFired):
> > +        (WebKit::ThreadedCompositor::create):
> > +        (WebKit::ThreadedCompositor::ThreadedCompositor):
> > +        (WebKit::ThreadedCompositor::~ThreadedCompositor):
> > +        (WebKit::ThreadedCompositor::setNeedsDisplay):
> > +        (WebKit::ThreadedCompositor::setNativeSurfaceHandleForCompositing):
> 
> You can omit the blank ones and just leave the one for the file itself.

Ditto.

> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadSafeCoordinatedSurface.cpp:35
> > +#if USE(TEXTURE_MAPPER)
> > +#include <WebCore/TextureMapperGL.h>
> > +#endif
> 
> Doesn't COORDINATED_GRAPHICS rely on texture mapper anyway? Perhaps this
> could just be moved to the main block without the #ifdef?

I think it is good point also. I'll remove these from my patch set,
but also we need to clean up COORDINATED_GRAPHICS, TEXTURE_MAPPER, and TILED_BACKING_STORE stuff globally. 

Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:67
> > +    void setUpdateTimer(double interval = 0.0f)
> 
> 0.0f should just be a 0 without since that converts properly.

Good, fixed.
 
> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:256
> > +    m_scene->paintToCurrentGLContext(viewportTransform, 1.0, clipRect, Color::white, false, scrollPostion);
> 
> 1.0 can just be 1 here.

Ditto.
 
> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:1
> > +/*
> 
> So this should probably move to the parent directory, but just have a
> different name. Perhaps SimpleViewportController or
> DesktopViewportController, since it isn't really related to threaded
> compositing, but the fact that this is targeting a simpler type of screen.

I agree about it. I prefer SimpleViewportController because it can handle fixed layout also.

> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:2
> > + * Copyright (C) 2014 Igalia S.L.
> 
> Should this retain the copyright from the original ViewportController.h?

I omitted original PageViewportController's copyright. I'll update license for SimpleViewportController.
 
> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:77
> > +    m_allowsUserScaling = !!m_rawAttributes.userScalable;
> 
> I don't believe the double !! is necessary.
> 

Ah. old legacy code. I'll remove "!!" .

> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:79
> > +
> 
> I don't think you need these parenthesis, right?
> 

Yes, I don't need it.

> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:132
> > +    FloatRect bounds;
> > +    bounds.setWidth(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale)));
> > +    bounds.setHeight(std::max(0.f, m_contentsSize.height() - floorf(m_viewportSize.height() / scale)));
> 
> This can just be FloatRect bounds(std::max(0.f, m_contentsSize.width() -
> floorf(m_viewportSize.width() / scale)), ...)
> 
> You don't need to use .f on the floating point number.
> 

If we do not use .f in this place, deduction failure will be happen at std::max.
It will try std::max(int, float) 

> > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:136
> > +    FloatPoint position;
> > +    position.setX(clampTo(framePosition.x(), bounds.x(), bounds.width()));
> > +    position.setY(clampTo(framePosition.y(), bounds.y(), bounds.height()));
> 
> This can just be FloatPoint position(clampTo(framePosition.x(), bounds.x(),
> bounds.width()), clampTo(framePosition.y(), bounds.y(), bounds.height()));

I think merging these in to the single statement reduces readability. I'll make more simplified statement for these.
Comment 8 Martin Robinson 2014-12-09 05:49:57 PST
(In reply to comment #7)

> > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:136
> > > +    FloatPoint position;
> > > +    position.setX(clampTo(framePosition.x(), bounds.x(), bounds.width()));
> > > +    position.setY(clampTo(framePosition.y(), bounds.y(), bounds.height()));
> > 
> > This can just be FloatPoint position(clampTo(framePosition.x(), bounds.x(),
> > bounds.width()), clampTo(framePosition.y(), bounds.y(), bounds.height()));
> 
> I think merging these in to the single statement reduces readability. I'll
> make more simplified statement for these.

One option for improved readability is to split it over multiple lines like this:

FloatPoint position(
    clampTo(framePosition.x(), bounds.x(), bounds.width()),
    clampTo(framePosition.y(), bounds.y(), bounds.height()));

That way you avoid the slight overhead of method calls.
Comment 9 Gwang Yoon Hwang 2014-12-09 06:17:43 PST
Created attachment 242915 [details]
Patch
Comment 10 Sergio Villar Senin 2014-12-10 02:08:22 PST
(In reply to comment #7)
> > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:132
> > > +    FloatRect bounds;
> > > +    bounds.setWidth(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale)));
> > > +    bounds.setHeight(std::max(0.f, m_contentsSize.height() - floorf(m_viewportSize.height() / scale)));
> > 
> > This can just be FloatRect bounds(std::max(0.f, m_contentsSize.width() -
> > floorf(m_viewportSize.width() / scale)), ...)
> > 
> > You don't need to use .f on the floating point number.
> > 
> 
> If we do not use .f in this place, deduction failure will be happen at
> std::max.
> It will try std::max(int, float) 
> 

Not really, std::max is templated so you could do something like std::max<float>().
Comment 11 Gwang Yoon Hwang 2014-12-10 02:27:48 PST
(In reply to comment #10)
> (In reply to comment #7)
> > > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ViewportController.cpp:132
> > > > +    FloatRect bounds;
> > > > +    bounds.setWidth(std::max(0.f, m_contentsSize.width() - floorf(m_viewportSize.width() / scale)));
> > > > +    bounds.setHeight(std::max(0.f, m_contentsSize.height() - floorf(m_viewportSize.height() / scale)));
> > > 
> > > This can just be FloatRect bounds(std::max(0.f, m_contentsSize.width() -
> > > floorf(m_viewportSize.width() / scale)), ...)
> > > 
> > > You don't need to use .f on the floating point number.
> > > 
> > 
> > If we do not use .f in this place, deduction failure will be happen at
> > std::max.
> > It will try std::max(int, float) 
> > 
> 
> Not really, std::max is templated so you could do something like
> std::max<float>().

That's why I said the deduction failure.

std::max is templated somewhat like below:
template< class T > 
const T& max( const T& a, const T& b );

So the compiler cannot decide whether the T is int or float in this case.
Comment 12 Martin Robinson 2014-12-15 01:13:36 PST
Comment on attachment 242915 [details]
Patch

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

> Source/WebKit2/Shared/CoordinatedGraphics/SimpleViewportController.cpp:125
> +    return FloatSize(m_viewportSize.width() / m_pageScaleFactor, m_viewportSize.height() / m_pageScaleFactor);

Could you do FloatSize(m_viewportSize).scale(1.0 / m_pageScaleFactor); here?
Comment 13 WebKit Commit Bot 2014-12-15 02:39:11 PST
Comment on attachment 242915 [details]
Patch

Clearing flags on attachment: 242915

Committed r177276: <http://trac.webkit.org/changeset/177276>
Comment 14 WebKit Commit Bot 2014-12-15 02:39:15 PST
All reviewed patches have been landed.  Closing bug.