Bug 150988 - Adopt CGIOSurfaceContextCreateImageReference to avoid unnecessary readback
Summary: Adopt CGIOSurfaceContextCreateImageReference to avoid unnecessary readback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-06 17:27 PST by Tim Horton
Modified: 2015-12-13 21:28 PST (History)
6 users (show)

See Also:


Attachments
Patch (30.32 KB, patch)
2015-11-06 17:37 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (1.13 MB, application/zip)
2015-11-06 18:28 PST, Build Bot
no flags Details
Patch (30.51 KB, patch)
2015-11-09 10:28 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (30.61 KB, patch)
2015-11-09 15:03 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (30.04 KB, patch)
2015-11-09 17:20 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (30.18 KB, patch)
2015-11-10 12:10 PST, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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