There should be no need to call IOSurfaceGetPropertyMaximum on iOS, since the return value will exceed the hardcoded maximum.
Created attachment 449826 [details] Patch
Created attachment 449828 [details] Patch
Created attachment 449830 [details] Patch
Thanks for reviewing!
<rdar://85128431>
Comment on attachment 449826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449826&action=review > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:235 > + const int maxSurfaceDimension = 8 * 1024; constexpr > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:242 > const int maxSurfaceDimensionLowerBound = 1024; constexpr > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:244 > // IOSurface::maximumSize() can return { INT_MAX, INT_MAX } when hardware acceleration is unavailable. I don’t understand how this comment relates to the code below, which does not return INT_MAX. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:245 > const int maxSurfaceDimension = 32 * 1024; constexpr
Created attachment 449843 [details] Patch
(In reply to Darin Adler from comment #6) > Comment on attachment 449826 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449826&action=review > > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:235 > > + const int maxSurfaceDimension = 8 * 1024; > > constexpr > > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:242 > > const int maxSurfaceDimensionLowerBound = 1024; > > constexpr > > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:244 > > // IOSurface::maximumSize() can return { INT_MAX, INT_MAX } when hardware acceleration is unavailable. > > I don’t understand how this comment relates to the code below, which does > not return INT_MAX. > Removed what seems to be an obsolete comment. > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:245 > > const int maxSurfaceDimension = 32 * 1024; > > constexpr Fixed! Thanks for reviewing!
Comment on attachment 449843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449843&action=review > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:231 > +static IntSize maxSurfaceDimensionCA() Should be static constexpr IntSize. Unless the IntSize constructor is not constexpr, which it should be, but at that point it’s obviously too much for just this patch. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:-242 > - // IOSurface::maximumSize() can return { INT_MAX, INT_MAX } when hardware acceleration is unavailable. Oh, wait, I figured out what this might have meant: If hardware acceleration is unavailable, IOSurface::maximumSize() can return "infinity", but we still want to impose a maximum in that case.
Created attachment 449870 [details] Patch
(In reply to Darin Adler from comment #9) > Comment on attachment 449843 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449843&action=review > > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:231 > > +static IntSize maxSurfaceDimensionCA() > > Should be static constexpr IntSize. Unless the IntSize constructor is not > constexpr, which it should be, but at that point it’s obviously too much for > just this patch. > Fixed. > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:-242 > > - // IOSurface::maximumSize() can return { INT_MAX, INT_MAX } when hardware acceleration is unavailable. > > Oh, wait, I figured out what this might have meant: > > If hardware acceleration is unavailable, IOSurface::maximumSize() can return > "infinity", but we still want to impose a maximum in that case. I put the comment back in. Thanks for reviewing!
Committed r288489 (246363@main): <https://commits.webkit.org/246363@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449870 [details].
Why did we need this after https://trac.webkit.org/changeset/270392/webkit? We should send this from the UI Process already.
(In reply to Simon Fraser (smfr) from comment #13) > Why did we need this after https://trac.webkit.org/changeset/270392/webkit? > We should send this from the UI Process already. This will avoid the perf regression from https://trac.webkit.org/changeset/270392/webkit, I believe.
Bug 219484 has no mention of a revert or perf regression.
(In reply to Simon Fraser (smfr) from comment #15) > Bug 219484 has no mention of a revert or perf regression. Yes, although I believe it was a launch time regression. Looking at WebProcessPoolCocoa.mm, I see: // However, querying this is a launch time regression, so limit this to only the necessary case. if (m_defaultPageGroup->preferences().useGPUProcessForDOMRenderingEnabled()) parameters.maximumIOSurfaceSize = WebCore::IOSurface::maximumSize();
...which blames to bug 221346 so let's relate the bugs.