[Webkit-unassigned] [Bug 149814] Compress snapshots on iOS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=149814

--- Comment #4 from Tim Horton <thorton at apple.com> ---
Comment on attachment 262453
  --> https://bugs.webkit.org/attachment.cgi?id=262453
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151005/062363fe/attachment.html>