Bug 187684 - IOSurfaces should update the display mask when the window is moved to another screen.
Summary: IOSurfaces should update the display mask when the window is moved to another...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-14 20:58 PDT by Per Arne Vollan
Modified: 2018-07-18 13:19 PDT (History)
11 users (show)

See Also:


Attachments
Patch (20.13 KB, patch)
2018-07-14 21:35 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.00 KB, patch)
2018-07-15 10:23 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.87 KB, patch)
2018-07-15 11:27 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.40 KB, patch)
2018-07-15 12:18 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.57 KB, patch)
2018-07-15 14:46 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.56 KB, patch)
2018-07-15 14:53 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.51 KB, patch)
2018-07-15 14:59 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.51 KB, patch)
2018-07-15 15:23 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.57 KB, patch)
2018-07-15 20:30 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (22.27 KB, patch)
2018-07-17 09:53 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.22 KB, patch)
2018-07-18 11:36 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.26 KB, patch)
2018-07-18 11:46 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.30 KB, patch)
2018-07-18 13:17 PDT, Per Arne Vollan
pvollan: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-07-14 20:58:37 PDT
The system API function CGIOSurfaceContextSetDisplayMask should be used to update the display mask.
Comment 1 Per Arne Vollan 2018-07-14 21:35:57 PDT
Created attachment 345052 [details]
Patch
Comment 2 Per Arne Vollan 2018-07-15 10:23:10 PDT
Created attachment 345061 [details]
Patch
Comment 3 Per Arne Vollan 2018-07-15 11:27:27 PDT
Created attachment 345062 [details]
Patch
Comment 4 Per Arne Vollan 2018-07-15 12:18:04 PDT
Created attachment 345063 [details]
Patch
Comment 5 Per Arne Vollan 2018-07-15 14:46:21 PDT
Created attachment 345066 [details]
Patch
Comment 6 Per Arne Vollan 2018-07-15 14:53:14 PDT
Created attachment 345067 [details]
Patch
Comment 7 Per Arne Vollan 2018-07-15 14:59:22 PDT
Created attachment 345068 [details]
Patch
Comment 8 Per Arne Vollan 2018-07-15 15:23:12 PDT
Created attachment 345069 [details]
Patch
Comment 9 Per Arne Vollan 2018-07-15 20:30:57 PDT
Created attachment 345077 [details]
Patch
Comment 10 Simon Fraser (smfr) 2018-07-16 08:47:18 PDT
Comment on attachment 345077 [details]
Patch

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

> Source/WebCore/page/WindowObserver.h:31
> +class WindowObserver {

I think this needs a different name. "Window" has multiple meanings in WebCore already (DOMWindow, the JS "window" object etc). Maybe this should be HostWindowObserver or something.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:120
> +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> +    , public WindowObserver
> +#endif

Don't do this. Just always inherit.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1329
> +    void windowScreenDidChange(uint32_t) override;

Can the uint32_t be a PlatformDisplayID?

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:415
> +    // find virtual screen

Comment should be full sentence. This one doesn't add anything useful.

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:47
> +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> +    : public WindowObserver
> +#endif

Yuck.

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:180
> +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> +    WeakPtr<HostWindow> m_hostWindow;
> +#endif

I really don't think we want ever IOSurface to have a weak ptr to hostWIndow. This layering feels all wrong.
Comment 11 Dean Jackson 2018-07-16 11:21:42 PDT
Comment on attachment 345077 [details]
Patch

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

>> Source/WebCore/page/WindowObserver.h:31
>> +class WindowObserver {
> 
> I think this needs a different name. "Window" has multiple meanings in WebCore already (DOMWindow, the JS "window" object etc). Maybe this should be HostWindowObserver or something.

Agreed.

> Source/WebCore/page/WindowObserver.h:34
> +    virtual void windowScreenDidChange(uint32_t) { };

Shouldn't this be just screenDidChange? It's probably also worth naming the parameter.

> Source/WebCore/platform/HostWindow.h:82
> +    virtual void registerWindowObserver(WindowObserver&) = 0;
> +    virtual void unregisterWindowObserver(WindowObserver&) = 0;

registerObserver() ?

> Source/WebCore/platform/graphics/ImageBuffer.h:69
> -    WEBCORE_EXPORT static std::unique_ptr<ImageBuffer> create(const FloatSize&, RenderingMode, float resolutionScale = 1, ColorSpace = ColorSpaceSRGB, const HostWindow* = nullptr);
> +    WEBCORE_EXPORT static std::unique_ptr<ImageBuffer> create(const FloatSize&, RenderingMode, float resolutionScale = 1, ColorSpace = ColorSpaceSRGB, HostWindow* = nullptr);

Why is this necessary?

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:475
> +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> +void IOSurface::windowScreenDidChange(unsigned displayID)
> +{
> +    ASSERT(m_cgContext);
> +    if (!m_cgContext)
> +        return;
> +    CGIOSurfaceContextSetDisplayMask(m_cgContext.get(), displayMaskForDisplay(displayID));
> +}
> +#endif

It seems it would be better to have a single observer and have it talk to all the IOSurface objects, rather than an observer for each IOSurface.
Comment 12 Per Arne Vollan 2018-07-16 12:38:09 PDT
(In reply to Dean Jackson from comment #11)
> Comment on attachment 345077 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345077&action=review
> 
> >> Source/WebCore/page/WindowObserver.h:31
> >> +class WindowObserver {
> > 
> > I think this needs a different name. "Window" has multiple meanings in WebCore already (DOMWindow, the JS "window" object etc). Maybe this should be HostWindowObserver or something.
> 
> Agreed.
> 
> > Source/WebCore/page/WindowObserver.h:34
> > +    virtual void windowScreenDidChange(uint32_t) { };
> 
> Shouldn't this be just screenDidChange? It's probably also worth naming the
> parameter.
> 
> > Source/WebCore/platform/HostWindow.h:82
> > +    virtual void registerWindowObserver(WindowObserver&) = 0;
> > +    virtual void unregisterWindowObserver(WindowObserver&) = 0;
> 
> registerObserver() ?
> 
> > Source/WebCore/platform/graphics/ImageBuffer.h:69
> > -    WEBCORE_EXPORT static std::unique_ptr<ImageBuffer> create(const FloatSize&, RenderingMode, float resolutionScale = 1, ColorSpace = ColorSpaceSRGB, const HostWindow* = nullptr);
> > +    WEBCORE_EXPORT static std::unique_ptr<ImageBuffer> create(const FloatSize&, RenderingMode, float resolutionScale = 1, ColorSpace = ColorSpaceSRGB, HostWindow* = nullptr);
> 
> Why is this necessary?
> 
> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:475
> > +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> > +void IOSurface::windowScreenDidChange(unsigned displayID)
> > +{
> > +    ASSERT(m_cgContext);
> > +    if (!m_cgContext)
> > +        return;
> > +    CGIOSurfaceContextSetDisplayMask(m_cgContext.get(), displayMaskForDisplay(displayID));
> > +}
> > +#endif
> 
> It seems it would be better to have a single observer and have it talk to
> all the IOSurface objects, rather than an observer for each IOSurface.

Thanks for reviewing, all! I will update the patch.
Comment 13 Per Arne Vollan 2018-07-17 09:53:56 PDT
Created attachment 345160 [details]
Patch
Comment 14 Dean Jackson 2018-07-17 14:06:40 PDT
Comment on attachment 345160 [details]
Patch

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

> Source/WebCore/page/HostWindowObserver.h:30
> +class PopupOpeningObserver {

Why is this file called HostWindowObserver and the class called PopupOpeningObserver?
Why does the patch think there are two versions of this file?

> Source/WebCore/platform/graphics/GraphicsContext3D.h:117
> +class GraphicsContext3D : public RefCounted<GraphicsContext3D>, public HostWindowObserver {

See note below about layering.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1325
> +    void screenDidChange(uint32_t) override;

Please make a type for the parameter, e.g. PlatformDisplayType or PlatformScreenType.

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:420
> +    auto pixelFormat = adoptNS([[NSOpenGLPixelFormat alloc] initWithCGLPixelFormatObj:pixelFormatObj]);
> +    
> +    // Set virtual screen based on matching display mask.
> +    GLint numberOfVirtualScreen = [pixelFormat numberOfVirtualScreens];
> +    for (GLint virtualScreen = 0; virtualScreen < numberOfVirtualScreen; virtualScreen++) {
> +        GLint mask = 0;
> +        [pixelFormat getValues:&mask forAttribute:NSOpenGLPFAScreenMask forVirtualScreen:virtualScreen];
> +        if (mask & displayMaskOpenGL) {

Is there a CGL equivalent for getting the virtual screens and screen mask? We already have the pixelFormatObj - why create another one?

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:505
> +    m_hostWindow = makeWeakPtr(hostWindow);
> +    if (m_hostWindow)
> +        m_hostWindow->registerObserver(*this);

HostWindowObserver is in /page/ and this class is in /platform/. We shouldn't layer things this way, having platform reference page. You'll need something in the middle to use as a protocol.

I also don't think it's right that this class have a weakPtr. It should never need to check whether an observer exists.

Also, we have a GraphicsContext3D manager class/object that it supposed to keep track of the contexts. Maybe the logic could be there?
Comment 15 Per Arne Vollan 2018-07-18 11:36:57 PDT
Created attachment 345265 [details]
Patch
Comment 16 Tim Horton 2018-07-18 11:42:09 PDT
Comment on attachment 345265 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/IOSurfaceManager.cpp:62
> +    m_hostWindowMap.set(&surface, hostWindow);

Do things ever get removed from here?
Comment 17 Per Arne Vollan 2018-07-18 11:44:45 PDT
(In reply to Tim Horton from comment #16)
> Comment on attachment 345265 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345265&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/IOSurfaceManager.cpp:62
> > +    m_hostWindowMap.set(&surface, hostWindow);
> 
> Do things ever get removed from here?

Good point! I'll remove from the map when the surface is removed.
Comment 18 Per Arne Vollan 2018-07-18 11:46:54 PDT
Created attachment 345266 [details]
Patch
Comment 19 Simon Fraser (smfr) 2018-07-18 12:01:58 PDT
Comment on attachment 345266 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/IOSurfaceManager.cpp:39
> +        if (m_hostWindowMap.contains(surface) && m_hostWindowMap.get(surface) == hostWindow)

Double hash lookup.
Comment 20 Simon Fraser (smfr) 2018-07-18 12:11:32 PDT
This doesn't feel like the right way to fix this problem. What if an IOSurface is being presented on both displays at the same time? What happens if the display mask changes in the middle of rendering? Maybe we should only set the display mask when getting the CGContextRef for an IOSurface?
Comment 21 Per Arne Vollan 2018-07-18 13:10:36 PDT
(In reply to Simon Fraser (smfr) from comment #20)
> This doesn't feel like the right way to fix this problem. What if an
> IOSurface is being presented on both displays at the same time? What happens
> if the display mask changes in the middle of rendering? Maybe we should only
> set the display mask when getting the CGContextRef for an IOSurface?

These are good points. We are currently setting the display mask when getting the CGContextRef. The intention of this patch was to switch the GPU used by an IO surface, when the window is moved to another screen. Perhaps this is not needed, and we can stay on the same GPU?


Thanks for reviewing, all!
Comment 22 Per Arne Vollan 2018-07-18 13:17:01 PDT
Created attachment 345275 [details]
Patch
Comment 23 Per Arne Vollan 2018-07-18 13:19:14 PDT
(In reply to Per Arne Vollan from comment #22)
> Created attachment 345275 [details]
> Patch

This patch should fix the double hash lookup. We should discuss if this patch is really needed. Perhaps we can stay on the same GPU, even if we move the window to a new screen.