Avoid calling IOSurfaceAlignProperty in the WebContent process, since it requires IOKit access.
Created attachment 450048 [details] Patch
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.
Created attachment 450062 [details] Patch
(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!
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.
Created attachment 450080 [details] Patch
(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!
(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.
Comment on attachment 450080 [details] Patch Thanks for reviewing!
<rdar://88024079>
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].