Bug 235659 – [WP] Avoid calling IOSurfaceAlignProperty

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Avoid calling IOSurfaceAlignProperty in the WebContent process, since it requires IOKit access.

[Per Arne Vollan]

Created attachment 450048 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 450048 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:121
> +    WEBCORE_EXPORT static size_t bytesPerRowAlignment();
> +    WEBCORE_EXPORT static void setBytesPerRowAlignment(size_t);

This doesn't protect against bytesPerRowAlignment() being called before setBytesPerRowAlignment(). Maybe bytesPerRowAlignment() should return an optional.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:282
> +        surfaceBytesPerRowAlignment().store(IOSurfaceGetPropertyAlignment(kIOSurfaceBytesPerRow));

This is surprising, since calling bytesPerRowAlignment() in the WP before setBytesPerRowAlignment() will trigger the IOSurface call.

[Per Arne Vollan]

Created attachment 450062 [details]
Patch

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 450048 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450048&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/IOSurface.h:121
> > +    WEBCORE_EXPORT static size_t bytesPerRowAlignment();
> > +    WEBCORE_EXPORT static void setBytesPerRowAlignment(size_t);
> 
> This doesn't protect against bytesPerRowAlignment() being called before
> setBytesPerRowAlignment(). Maybe bytesPerRowAlignment() should return an
> optional.
> 
> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:282
> > +        surfaceBytesPerRowAlignment().store(IOSurfaceGetPropertyAlignment(kIOSurfaceBytesPerRow));
> 
> This is surprising, since calling bytesPerRowAlignment() in the WP before
> setBytesPerRowAlignment() will trigger the IOSurface call.

That is a good point. I have uploaded a new patch to address this.

Thanks for reviewing!

[Simon Fraser (smfr)]

Comment on attachment 450062 [details]
Patch

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

> Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:97
> +    return (bytesPerRow + alignmentMask) & ~alignmentMask;

Pretty sure we have a macro for this.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:461
>      if (!parameters.maximumIOSurfaceSize.isEmpty())
>          WebCore::IOSurface::setMaximumSize(parameters.maximumIOSurfaceSize);
>  
> +    WebCore::IOSurface::setBytesPerRowAlignment(parameters.bytesPerRowIOSurfaceAlignment);

We should probably do both of these for the GPU Process too, right? Also maybe we need to revisit bug 235526 now that I see we already send over maximumIOSurfaceSize.

[Per Arne Vollan]

Created attachment 450080 [details]
Patch

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 450062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450062&action=review
> 
> > Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:97
> > +    return (bytesPerRow + alignmentMask) & ~alignmentMask;
> 
> Pretty sure we have a macro for this.
> 

I searched for this, but I was not able to find it.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:461
> >      if (!parameters.maximumIOSurfaceSize.isEmpty())
> >          WebCore::IOSurface::setMaximumSize(parameters.maximumIOSurfaceSize);
> >  
> > +    WebCore::IOSurface::setBytesPerRowAlignment(parameters.bytesPerRowIOSurfaceAlignment);
> 
> We should probably do both of these for the GPU Process too, right? Also
> maybe we need to revisit bug 235526 now that I see we already send over
> maximumIOSurfaceSize.

I revised the original patch to catch (with an assert) when the value is being queried before being set.

I will revisit bug 235526 in a separate bug, if you're OK with that. I think we can avoid the launch time regression by sending the value in the WebPage creation parameters.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 450062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450062&action=review
> 
> > Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:97
> > +    return (bytesPerRow + alignmentMask) & ~alignmentMask;
> 
> Pretty sure we have a macro for this.
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:461
> >      if (!parameters.maximumIOSurfaceSize.isEmpty())
> >          WebCore::IOSurface::setMaximumSize(parameters.maximumIOSurfaceSize);
> >  
> > +    WebCore::IOSurface::setBytesPerRowAlignment(parameters.bytesPerRowIOSurfaceAlignment);
> 
> We should probably do both of these for the GPU Process too, right? Also
> maybe we need to revisit bug 235526 now that I see we already send over
> maximumIOSurfaceSize.

For the GPU process, I don't think it is strictly needed to do this, since the IOSurface calls will succeed there.

[Per Arne Vollan]

Comment on attachment 450080 [details]
Patch

Thanks for reviewing!

[Per Arne Vollan]

<rdar://88024079>

[EWS]

Committed r288662 (246470@main): <https://commits.webkit.org/246470@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450080 [details].