Bug 150988

Summary: Adopt CGIOSurfaceContextCreateImageReference to avoid unnecessary readback
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bdakin, dino, mitz, sam, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Tim Horton 2015-11-06 17:27:15 PST
Adopt CGIOSurfaceContextCreateImageReference to avoid unnecessary readback
Comment 1 Tim Horton 2015-11-06 17:37:14 PST
Created attachment 264984 [details]
Patch
Comment 2 Build Bot 2015-11-06 18:27:45 PST
Comment on attachment 264984 [details]
Patch

Attachment 264984 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/394012

Number of test failures exceeded the failure limit.
Comment 3 Build Bot 2015-11-06 18:28:23 PST
Created attachment 264988 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Tim Horton 2015-11-09 10:28:59 PST
Created attachment 265070 [details]
Patch
Comment 5 Tim Horton 2015-11-09 15:03:45 PST
Created attachment 265103 [details]
Patch
Comment 6 Tim Horton 2015-11-09 17:20:26 PST
Created attachment 265126 [details]
Patch
Comment 7 Tim Horton 2015-11-10 12:10:29 PST
Created attachment 265214 [details]
Patch
Comment 8 Tim Horton 2015-11-13 10:20:10 PST
Simon noted that he doesn't necessarily love 'sinkInto...'. Any suggestions from the crowd?
Comment 9 Darin Adler 2015-11-30 15:54:27 PST
Comment on attachment 265214 [details]
Patch

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

Some of those WTF::move might not be needed, although most clearly are.

Looks great!

> Source/WebCore/platform/DragImage.cpp:91
> +static DragImageRef createDragImageFromSnapshot(std::unique_ptr<ImageBuffer>&& snapshot, Node* node)

Since std::unique_ptr is a move-only class, not sure that using && for the argument type has any value.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:197
> +    return BitmapImage::create(image.get()).ptr();

You shouldn’t need this ".ptr()"; can’t you initialize a Ref<Image> from a RefPtr<Image> without that?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:219
> +    RetainPtr<CGImageRef> image = sinkIntoNativeImage(WTF::move(imageBuffer));
> +    return createBitmapImageAfterScalingIfNeeded(WTF::move(image), internalSize, logicalSize, backingStoreSize, resolutionScale, scaleBehavior);

I probably would have done this without the local variable. Would also have obviated the need for one of the WTF::move as well.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:234
> +    std::unique_ptr<IOSurface> surface = IOSurface::createFromImageBuffer(WTF::move(imageBuffer));
> +    return IOSurface::sinkIntoImage(WTF::move(surface));

Would read better as a one-liner.

    return IOSurface::sinkIntoImage(IOSurface::createFromImageBuffer(WTF::move(imageBuffer)));

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:278
> +    std::unique_ptr<IOSurface> surface = IOSurface::createFromImageBuffer(WTF::move(imageBuffer));
> +    RetainPtr<CGImageRef> image = IOSurface::sinkIntoImage(WTF::move(surface));

Would read better as a one-liner:

    auto image = IOSurface::sinkIntoImage(IOSurface::createFromImageBuffer(WTF::move(imageBuffer)));

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:52
> +    static std::unique_ptr<IOSurface> createFromImageBuffer(std::unique_ptr<ImageBuffer>&&);

Since std::unique_ptr is a move-only class, not sure that using && for the argument type has any value.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:102
> +std::unique_ptr<IOSurface> IOSurface::createFromImageBuffer(std::unique_ptr<ImageBuffer>&& imageBuffer)

Since std::unique_ptr is a move-only class, not sure that using && for the argument type has any value.
Comment 10 Tim Horton 2015-12-13 21:28:36 PST
Fingers crossed. http://trac.webkit.org/changeset/194025