IOSurfaces should update the display mask when the window is moved to another screen.

• Component: WebCore Misc.
• Status: NEW ---

[Per Arne Vollan]

The system API function CGIOSurfaceContextSetDisplayMask should be used to update the display mask.

[Per Arne Vollan]

Created attachment 345052 [details]
Patch

[Per Arne Vollan]

Created attachment 345061 [details]
Patch

[Per Arne Vollan]

Created attachment 345062 [details]
Patch

[Per Arne Vollan]

Created attachment 345063 [details]
Patch

[Per Arne Vollan]

Created attachment 345066 [details]
Patch

[Per Arne Vollan]

Created attachment 345067 [details]
Patch

[Per Arne Vollan]

Created attachment 345068 [details]
Patch

[Per Arne Vollan]

Created attachment 345069 [details]
Patch

[Per Arne Vollan]

Created attachment 345077 [details]
Patch

[Simon Fraser (smfr)]

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.

[Dean Jackson]

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.

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 345160 [details]
Patch

[Dean Jackson]

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?

[Per Arne Vollan]

Created attachment 345265 [details]
Patch

[Tim Horton]

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?

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 345266 [details]
Patch

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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?

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 345275 [details]
Patch

[Per Arne Vollan]

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

[Simon Fraser (smfr)]

Comment on attachment 345275 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=187684

Radar?

> Source/WebCore/ChangeLog:10
> +        Create a new IOSurfaceManager which will keep a list of all IO surfaces. When the screen of a host window has changed,
> +        update all IO surfaces with the new display mask. The IO surface manager keeps a mapping between IO surface context
> +        and host window in order to know which IO surfaces should be updated.

What about GPU Process?