Bug 235526 – [iOS] Avoid calling IOSurfaceGetPropertyMaximum

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

There should be no need to call IOSurfaceGetPropertyMaximum on iOS, since the return value will exceed the hardcoded maximum.

[Per Arne Vollan]

Created attachment 449826 [details]
Patch

[Per Arne Vollan]

Created attachment 449828 [details]
Patch

[Per Arne Vollan]

Created attachment 449830 [details]
Patch

[Per Arne Vollan]

Thanks for reviewing!

[Per Arne Vollan]

<rdar://85128431>

[Darin Adler]

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

[Per Arne Vollan]

Created attachment 449843 [details]
Patch

[Per Arne Vollan]

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

[Darin Adler]

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.

[Per Arne Vollan]

Created attachment 449870 [details]
Patch

[Per Arne Vollan]

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

[EWS]

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

[Simon Fraser (smfr)]

Why did we need this after https://trac.webkit.org/changeset/270392/webkit? We should send this from the UI Process already.

[Per Arne Vollan]

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

[Simon Fraser (smfr)]

Bug 219484 has no mention of a revert or perf regression.

[Per Arne Vollan]

(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();

[Simon Fraser (smfr)]

...which blames to bug 221346 so let's relate the bugs.