Bug 149814 – Compress snapshots on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Beth Dakin]

Compress snapshots on iOS

[Radar WebKit Bug Importer]

<rdar://problem/22976230>

[Beth Dakin]

Created attachment 262453 [details]
Patch

[WebKit Commit Bot]

Attachment 262453 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.mm:273:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:40:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:48:  The parameter name "pixelFormat" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:108:  The parameter name "allocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:109:  The parameter name "accelerator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:114:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:115:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:116:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:119:  The parameter name "accelerator" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 9 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 262453 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:39
> +enum class PixelFormatForColor {

I don't think we need the "ForColor" here, and I think maybe this should be inside WebCore::IOSurface? Maybe WebCore::IOSurface::Format?

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:42
> +    YUVCompressed

I don't think we need "compressed" here. I think this should be more specific, too: YUV422, or something like that.

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:91
> +    WEBCORE_EXPORT void convertToFormat(PixelFormatForColor, std::function<void(std::unique_ptr<WebCore::IOSurface>)>);

This should probably be a class method that takes a move-only WebCore::IOSurface, so that it's clear that it consumes the surface. Otherwise someone could accidentally keep using the now dead surface. Something like this:

WEBCORE_EXPORT static void convertToFormat(std::unique_ptr<WebCore::IOSurface>&& inSurface, PixelFormatForColor, std::function<void(std::unique_ptr<WebCore::IOSurface>)>);

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:264
> +    static IOSurfaceAcceleratorRef accelerator;
> +    if (!accelerator) {
> +        IOSurfaceAcceleratorCreate(nullptr, nullptr, &accelerator);
> +
> +        auto runLoopSource = IOSurfaceAcceleratorGetRunLoopSource(accelerator);
> +        CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode);
> +    }

We should check with the appropriate folks that it's OK to keep one of these alive forever. I'm not sure.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:266
> +    auto destinationSurface = IOSurface::create(m_size, m_colorSpace, pixelFormat);

We should bail (call the callback synchronously with the current surface) if the pixel formats are the same.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:274
> +

no newline here

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:282
> +    IOSurfaceAcceleratorTransformSurface(accelerator, m_surface.get(), destinationIOSurfaceRef, nullptr, nullptr, &completion, nullptr, nullptr);

Please retrieve the IOReturn from this and from IOSurfaceAcceleratorCreate and ASSERT_UNUSED(ret, ret == kIOReturnSuccess); like we do in other places.

[Beth Dakin]

Created attachment 262458 [details]
Patch

[Beth Dakin]

(In reply to comment #4)
> Comment on attachment 262453 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262453&action=review
> 
> > Source/WebCore/platform/graphics/cocoa/IOSurface.h:39
> > +enum class PixelFormatForColor {
> 
> I don't think we need the "ForColor" here, and I think maybe this should be
> inside WebCore::IOSurface? Maybe WebCore::IOSurface::Format?
> 

Fixed! I went with your suggestion.

> > Source/WebCore/platform/graphics/cocoa/IOSurface.h:42
> > +    YUVCompressed
> 
> I don't think we need "compressed" here. I think this should be more
> specific, too: YUV422, or something like that.
> 

Fixed! I again went with your suggestion.

> > Source/WebCore/platform/graphics/cocoa/IOSurface.h:91
> > +    WEBCORE_EXPORT void convertToFormat(PixelFormatForColor, std::function<void(std::unique_ptr<WebCore::IOSurface>)>);
> 
> This should probably be a class method that takes a move-only
> WebCore::IOSurface, so that it's clear that it consumes the surface.
> Otherwise someone could accidentally keep using the now dead surface.
> Something like this:
> 
> WEBCORE_EXPORT static void
> convertToFormat(std::unique_ptr<WebCore::IOSurface>&& inSurface,
> PixelFormatForColor,
> std::function<void(std::unique_ptr<WebCore::IOSurface>)>);
> 

Fixed! Should I move it to the top of the file? It doesn't really matter, but I feel like that is where class methods go.

> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:264
> > +    static IOSurfaceAcceleratorRef accelerator;
> > +    if (!accelerator) {
> > +        IOSurfaceAcceleratorCreate(nullptr, nullptr, &accelerator);
> > +
> > +        auto runLoopSource = IOSurfaceAcceleratorGetRunLoopSource(accelerator);
> > +        CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode);
> > +    }
> 
> We should check with the appropriate folks that it's OK to keep one of these
> alive forever. I'm not sure.
> 

Okay…Will follow up on this.

> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:266
> > +    auto destinationSurface = IOSurface::create(m_size, m_colorSpace, pixelFormat);
> 
> We should bail (call the callback synchronously with the current surface) if
> the pixel formats are the same.
> 
> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:274
> > +
> 
> no newline here
> 

Fixed!

> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:282
> > +    IOSurfaceAcceleratorTransformSurface(accelerator, m_surface.get(), destinationIOSurfaceRef, nullptr, nullptr, &completion, nullptr, nullptr);
> 
> Please retrieve the IOReturn from this and from IOSurfaceAcceleratorCreate
> and ASSERT_UNUSED(ret, ret == kIOReturnSuccess); like we do in other places.


Fixed!

Thanks Tim!

[Simon Fraser (smfr)]

Comment on attachment 262458 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Though the default is still RGBA, it is now possible to create and IOSurface 

"and"

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:259
> +        auto runLoopSource = IOSurfaceAcceleratorGetRunLoopSource(accelerator);
> +        CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode);

Should we consider running this on another thread?

[WebKit Commit Bot]

Attachment 262458 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.mm:252:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.mm:273:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:42:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:44:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:108:  The parameter name "allocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:109:  The parameter name "accelerator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:119:  The parameter name "accelerator" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

(In reply to comment #7)
> Comment on attachment 262458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262458&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Though the default is still RGBA, it is now possible to create and IOSurface 
> 
> "and"
> 

Fixed!

> > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:259
> > +        auto runLoopSource = IOSurfaceAcceleratorGetRunLoopSource(accelerator);
> > +        CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode);
> 
> Should we consider running this on another thread?

I'll chat with Anders about this.

Thanks!

http://trac.webkit.org/changeset/190574

[Tim Horton]

Comment on attachment 262458 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:259
>>> +        CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode);
>> 
>> Should we consider running this on another thread?
> 
> I'll chat with Anders about this.
> 
> Thanks!
> 
> http://trac.webkit.org/changeset/190574

I think the important thing here is to know whether it uses the run loop that we install its source on for anything *other* than calling the callback. If not, then there's no need to move it to another thread, but if it does other work on that runloop we might want to.