Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ImageIO for WIC & do a lot fewer image conversions in CGImage/CGContext. #1529

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions Frameworks/CoreGraphics/CGContext.mm
Original file line number Diff line number Diff line change
Expand Up @@ -2145,12 +2145,19 @@ void CGContextDrawGlyphRun(CGContextRef context, const DWRITE_GLYPH_RUN* glyphRu
struct __CGBitmapContext : CoreFoundation::CppBase<__CGBitmapContext, __CGContext> {
woc::unique_cf<CGImageRef> _image;

__CGBitmapContext(ID2D1RenderTarget* renderTarget) : Parent(renderTarget) {
__CGBitmapContext(ID2D1RenderTarget* renderTarget, REFWICPixelFormatGUID outputPixelFormat) : Parent(renderTarget), _outputPixelFormat(outputPixelFormat) {
}

inline void SetImage(CGImageRef image) {
_image.reset(CGImageRetain(image));
}

inline REFWICPixelFormatGUID GetOutputPixelFormat() const {
return _outputPixelFormat;
}

private:
WICPixelFormatGUID _outputPixelFormat;
};

/**
Expand Down Expand Up @@ -2187,11 +2194,15 @@ CGContextRef CGBitmapContextCreateWithData(void* data,

// bitsperpixel = ((bytesPerRow/width) * 8bits/byte)
size_t bitsPerPixel = ((bytesPerRow / width) << 3);
REFGUID pixelFormat = _CGImageGetWICPixelFormatFromImageProperties(bitsPerComponent, bitsPerPixel, space, bitmapInfo);
REFWICPixelFormatGUID outputPixelFormat = _CGImageGetWICPixelFormatFromImageProperties(bitsPerComponent, bitsPerPixel, space, bitmapInfo);
WICPixelFormatGUID pixelFormat = outputPixelFormat;

if (!_CGIsValidRenderTargetPixelFormat(pixelFormat)) {
UNIMPLEMENTED_WITH_MSG("CGBitmapContext does not currently support conversion and can only render into 32bpp PRGBA buffers.");
return nullptr;
if (data) {
UNIMPLEMENTED_WITH_MSG("CGBitmapContext does not currently support input conversion and can only render into 32bpp PRGBA buffers.");
return nullptr;
}
Copy link
Contributor

@aballway aballway Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: add line after } #WontFix

pixelFormat = GUID_WICPixelFormat32bppPRGBA;
Copy link
Contributor

@msft-Jeyaram msft-Jeyaram Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. I think at one point we decided not to do this. It makes sense and is needed :) #Closed

}

// if data is null, enough memory is allocated via CGIWICBitmap
Expand All @@ -2206,7 +2217,8 @@ CGContextRef CGBitmapContextCreateWithData(void* data,

ComPtr<ID2D1RenderTarget> renderTarget;
RETURN_NULL_IF_FAILED(factory->CreateWicBitmapRenderTarget(customBitmap.Get(), D2D1::RenderTargetProperties(), &renderTarget));
return _CGBitmapContextCreateWithRenderTarget(renderTarget.Get(), image.get());
CGContextRef context = _CGBitmapContextCreateWithRenderTarget(renderTarget.Get(), image.get(), outputPixelFormat);
Copy link
Contributor

@aballway aballway Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: just return this #WontFix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be additional things here acting on context.


In reply to: 92289253 [](ancestors = 92289253)

return context;
}

/**
Expand Down Expand Up @@ -2288,7 +2300,14 @@ size_t CGBitmapContextGetBytesPerRow(CGContextRef context) {
*/
CGImageRef CGBitmapContextCreateImage(CGContextRef context) {
NOISY_RETURN_IF_NULL(context, nullptr);
return CGImageCreateCopy(CGBitmapContextGetImage(context));
if (CFGetTypeID(context) != __CGBitmapContext::GetTypeID()) {
TraceError(TAG, L"Image requested from non-bitmap CGContext.");
Copy link
Contributor

@aballway aballway Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: print name of what type it actually is for easier debugging #WontFix

Copy link
Author

@DHowett-MSFT DHowett-MSFT Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's too much work (digging around in CF to map the type ID) #WontFix

return nullptr;
}

// This copy is a no-op if the output format requested matches the backing image format.
__CGBitmapContext* bitmapContext = (__CGBitmapContext*)context;
return _CGImageCreateCopyWithPixelFormat(bitmapContext->_image.get(), bitmapContext->GetOutputPixelFormat());
}

CGImageRef CGBitmapContextGetImage(CGContextRef context) {
Expand All @@ -2300,9 +2319,9 @@ CGImageRef CGBitmapContextGetImage(CGContextRef context) {
return ((__CGBitmapContext*)context)->_image.get();
}

CGContextRef _CGBitmapContextCreateWithRenderTarget(ID2D1RenderTarget* renderTarget, CGImageRef img) {
CGContextRef _CGBitmapContextCreateWithRenderTarget(ID2D1RenderTarget* renderTarget, CGImageRef img, WICPixelFormatGUID outputPixelFormat) {
RETURN_NULL_IF(!renderTarget);
__CGBitmapContext* context = __CGBitmapContext::CreateInstance(kCFAllocatorDefault, renderTarget);
__CGBitmapContext* context = __CGBitmapContext::CreateInstance(kCFAllocatorDefault, renderTarget, outputPixelFormat);
__CGContextPrepareDefaults(context);
context->SetImage(img);
return context;
Expand Down
36 changes: 14 additions & 22 deletions Frameworks/ImageIO/CGImageDestination.mm
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#import <ImageIO/CGImageDestination.h>
#import "CGImageDestinationInternal.h"
#import <StubReturn.h>
#import <CGImageInternal.h>
#include <windows.h>
#include <string>

Expand Down Expand Up @@ -979,6 +980,14 @@ void CGImageDestinationAddImage(CGImageDestinationRef idst, CGImageRef image, CF
}
}

// Turn image into a WIC Bitmap
ComPtr<IWICBitmap> inputImage;
status = _CGImageGetWICImageSource(image, &inputImage);
if (!SUCCEEDED(status)) {
Copy link
Contributor

@msft-Jeyaram msft-Jeyaram Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!SUCCEEDED [](start = 8, length = 10)

FAILED ? #WontFix

Copy link
Author

@DHowett-MSFT DHowett-MSFT Dec 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be too much churn if I were to adjust the CC in this file. #WontFix

NSTraceInfo(TAG, @"_CGImageGetWICImageSource failed with status=%x\n", status);
return;
}

IWICMetadataQueryWriter* propertyWriter = imageFrameMetadataWriter.Get();

// Set the pixel format based on file format and write necessary metadata
Expand All @@ -998,7 +1007,11 @@ void CGImageDestinationAddImage(CGImageDestinationRef idst, CGImageRef image, CF
writeGIFProperties(propertyWriter, properties, imageWidth, imageHeight);
break;
case typePNG:
formatGUID = GUID_WICPixelFormat32bppRGBA;
status = inputImage->GetPixelFormat(&formatGUID);
if (!SUCCEEDED(status)) {
Copy link
Contributor

@msft-Jeyaram msft-Jeyaram Dec 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!SUCCEEDED [](start = 16, length = 10)

same here? #WontFix

NSTraceInfo(TAG, @"Get Image Source Pixel Format failed with status=%x\n", status);
return;
}
writePNGProperties(propertyWriter, properties, imageWidth, imageHeight);
break;
case typeBMP:
Expand All @@ -1015,27 +1028,6 @@ void CGImageDestinationAddImage(CGImageDestinationRef idst, CGImageRef image, CF
return;
}

CGDataProviderRef provider = CGImageGetDataProvider(image);
NSData* imageByteData = (id)CGDataProviderCopyData(provider);

// Turn image into a WIC Bitmap
ComPtr<IWICBitmap> inputImage;

// All our input coming in from CGImagesource is in 32bppRGBA
ComPtr<IWICImagingFactory> imageFactory = imageDestination.idFactory;
status = imageFactory->CreateBitmapFromMemory(imageWidth,
imageHeight,
GUID_WICPixelFormat32bppRGBA,
imageWidth * 4,
imageHeight * imageWidth * 4,
(unsigned char*)[imageByteData bytes],
&inputImage);
[imageByteData release];
if (!SUCCEEDED(status)) {
NSTraceInfo(TAG, @"CreateBitmapFromMemory failed with status=%x\n", status);
return;
}

ComPtr<IWICBitmapSource> inputBitmapSource;
status = WICConvertBitmapSource(formatGUID, inputImage.Get(), &inputBitmapSource);
if (!SUCCEEDED(status)) {
Expand Down
79 changes: 8 additions & 71 deletions Frameworks/ImageIO/CGImageSource.mm
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
#include <wrl/client.h>
#include "COMIncludes_End.h"

#import <CGImageInternal.h>

using namespace Microsoft::WRL;

static const wchar_t* TAG = L"CGImageSource";
Expand Down Expand Up @@ -1684,48 +1686,10 @@ CGImageRef CGImageSourceCreateImageAtIndex(CGImageSourceRef isrc, size_t index,
ComPtr<IWICBitmapFrameDecode> imageFrame;
RETURN_NULL_IF_FAILED(imageDecoder->GetFrame(index, &imageFrame));

unsigned int frameWidth = 0;
unsigned int frameHeight = 0;
RETURN_NULL_IF_FAILED(imageFrame->GetSize(&frameWidth, &frameHeight));

ComPtr<IWICFormatConverter> imageFormatConverter;
RETURN_NULL_IF_FAILED(imageFactory->CreateFormatConverter(&imageFormatConverter));

if (options && CFDictionaryContainsKey(options, kCGImageSourceShouldAllowFloat)) {
UNIMPLEMENTED_WITH_MSG("kCGImageSourceShouldAllowFloat is not supported in current implementation.");
}
ComPtr<IWICBitmap> imageBitmap;
RETURN_NULL_IF_FAILED(imageFactory->CreateBitmapFromSource(imageFrame.Get(), WICBitmapCacheOnDemand, &imageBitmap));

RETURN_NULL_IF_FAILED(imageFormatConverter->Initialize(
imageFrame.Get(), GUID_WICPixelFormat32bppRGBA, WICBitmapDitherTypeNone, nullptr, 0.f, WICBitmapPaletteTypeCustom));

const unsigned int frameSize = frameWidth * frameHeight * 4;
unsigned char* frameByteArray = static_cast<unsigned char*>(IwMalloc(frameSize));
if (!frameByteArray) {
NSTraceInfo(TAG, @"CGImageSourceCreateImageAtIndex cannot allocate memory");
return nullptr;
}

auto cleanup = wil::ScopeExit([&]() { IwFree(frameByteArray); });
RETURN_NULL_IF_FAILED(imageFormatConverter->CopyPixels(0, frameWidth * 4, frameSize, frameByteArray));
cleanup.Dismiss();

NSData* frameData = [NSData dataWithBytesNoCopy:frameByteArray length:frameSize freeWhenDone:YES];
CGDataProviderRef frameDataProvider = CGDataProviderCreateWithCFData((CFDataRef)frameData);
CGColorSpaceRef colorspaceRgb = CGColorSpaceCreateDeviceRGB();
CGImageRef imageRef = CGImageCreate(frameWidth,
frameHeight,
8,
32,
frameWidth * 4,
colorspaceRgb,
kCGImageAlphaLast,
frameDataProvider,
nullptr,
true,
kCGRenderingIntentDefault);
CGDataProviderRelease(frameDataProvider);
CGColorSpaceRelease(colorspaceRgb);
return imageRef;
return _CGImageCreateWithWICBitmap(imageBitmap.Get());
}

/**
Expand Down Expand Up @@ -1825,37 +1789,10 @@ CGImageRef CGImageSourceCreateThumbnailAtIndex(CGImageSourceRef isrc, size_t ind
UNIMPLEMENTED_WITH_MSG("kCGImageSourceShouldAllowFloat is not supported in current implementation.");
}

RETURN_NULL_IF_FAILED(imageFormatConverter->Initialize(
imageScaler.Get(), GUID_WICPixelFormat32bppRGBA, WICBitmapDitherTypeNone, nullptr, 0.f, WICBitmapPaletteTypeCustom));

const unsigned int thumbnailSize = thumbnailWidth * thumbnailHeight * 4;
unsigned char* thumbnailByteArray = static_cast<unsigned char*>(IwMalloc(thumbnailSize));
if (!thumbnailByteArray) {
NSTraceInfo(TAG, @"CGImageSourceCreateThumbnailAtIndex cannot allocate memory");
return nullptr;
}
ComPtr<IWICBitmap> imageBitmap;
RETURN_NULL_IF_FAILED(imageFactory->CreateBitmapFromSource(imageScaler.Get(), WICBitmapCacheOnDemand, &imageBitmap));

auto cleanup = wil::ScopeExit([&]() { IwFree(thumbnailByteArray); });
RETURN_NULL_IF_FAILED(imageFormatConverter->CopyPixels(0, thumbnailWidth * 4, thumbnailSize, thumbnailByteArray));
cleanup.Dismiss();

NSData* thumbnailData = [NSData dataWithBytesNoCopy:thumbnailByteArray length:thumbnailSize freeWhenDone:YES];
CGDataProviderRef thumbnailDataProvider = CGDataProviderCreateWithCFData((CFDataRef)thumbnailData);
CGColorSpaceRef colorspaceRgb = CGColorSpaceCreateDeviceRGB();
CGImageRef imageRef = CGImageCreate(thumbnailWidth,
thumbnailHeight,
8,
32,
thumbnailWidth * 4,
colorspaceRgb,
kCGImageAlphaLast,
thumbnailDataProvider,
nullptr,
true,
kCGRenderingIntentDefault);
CGDataProviderRelease(thumbnailDataProvider);
CGColorSpaceRelease(colorspaceRgb);
return imageRef;
return _CGImageCreateWithWICBitmap(imageBitmap.Get());
}

/**
Expand Down
2 changes: 1 addition & 1 deletion Frameworks/QuartzCore/CALayer.mm
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ CGContextRef CreateLayerContentsBitmapContext32(int width, int height, float sca
RETURN_NULL_IF_FAILED(factory->CreateWicBitmapRenderTarget(customWICBtmap.Get(), D2D1::RenderTargetProperties(), &renderTarget));
renderTarget->SetDpi(c_windowsDPI * scale, c_windowsDPI * scale);

return _CGBitmapContextCreateWithRenderTarget(renderTarget.Get(), image.get());
return _CGBitmapContextCreateWithRenderTarget(renderTarget.Get(), image.get(), GUID_WICPixelFormat32bppPBGRA);
}

return nullptr;
Expand Down
3 changes: 2 additions & 1 deletion Frameworks/include/CGContextInternal.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <COMIncludes.h>
#import <DWrite.h>
#import <D2d1.h>
#import <wincodec.h>
#include <COMIncludes_End.h>

#import "CoreGraphicsInternal.h"
Expand All @@ -40,6 +41,6 @@ COREGRAPHICS_EXPORT bool CGContextIsPointInPath(CGContextRef c, bool eoFill, flo
COREGRAPHICS_EXPORT void CGContextDrawGlyphRun(CGContextRef ctx, const DWRITE_GLYPH_RUN* glyphRun);

// Bitmap Context Internal
COREGRAPHICS_EXPORT CGContextRef _CGBitmapContextCreateWithRenderTarget(ID2D1RenderTarget* renderTarget, CGImageRef img = nullptr);
COREGRAPHICS_EXPORT CGContextRef _CGBitmapContextCreateWithRenderTarget(ID2D1RenderTarget* renderTarget, CGImageRef img, WICPixelFormatGUID outputPixelFormat);
Copy link
Contributor

@rajsesh rajsesh Dec 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: bitmapPixelFormat might be more accurate name (i suppose outputPixelFormat means the pixel format output by the bitmap render target. Okay with current name though. #WontFix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

output captures it better: it's not the input format in the user-provided data :)


In reply to: 92748140 [](ancestors = 92748140)

COREGRAPHICS_EXPORT CGContextRef _CGBitmapContextCreateWithFormat(int width, int height, __CGSurfaceFormat fmt);
COREGRAPHICS_EXPORT CGImageRef CGBitmapContextGetImage(CGContextRef ctx);
34 changes: 28 additions & 6 deletions Frameworks/include/CGIWICBitmap.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ class CGIWICBitmap : public Microsoft::WRL::RuntimeClass<Microsoft::WRL::Runtime
}

// IWICBitmap interface
// TODO #1124: Today we do not support locking a region of the WIC bitmap for rendering. We only support locking the complete bitmap.
// TODO #1379: Today we do not support locking of a region smaller than the entire bitmap.
// This will suffice CoreText requirement but needs to be revisted for CoreGraphics usage in future.

HRESULT STDMETHODCALLTYPE Lock(_In_ const WICRect* region, _In_ DWORD flags, _COM_Outptr_ IWICBitmapLock** outLock) {
Expand Down Expand Up @@ -187,17 +187,39 @@ class CGIWICBitmap : public Microsoft::WRL::RuntimeClass<Microsoft::WRL::Runtime
_Out_writes_all_(cbBufferSize) BYTE* buffer) {
RETURN_HR_IF_NULL(E_POINTER, buffer);

const WICRect fullRect = { 0, 0, m_width, m_height };
Copy link
Contributor

@aballway aballway Dec 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrating comments from other PR-
nit: m_width and m_height are UINT, but WICRect expects INT :(
#ByDesign

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine?


In reply to: 92295863 [](ancestors = 92295863)

Microsoft::WRL::ComPtr<IWICBitmapLock> lock;
RETURN_IF_FAILED(Lock(copyRect, 0, &lock));
// TODO #1379: Support sub-regional locking.
RETURN_IF_FAILED(Lock(&fullRect, 0, &lock));

if (!copyRect) {
copyRect = &fullRect;
}

RETURN_HR_IF(E_INVALIDARG, copyRect->Width == 0 || copyRect->Height == 0 || copyRect->X < 0 || copyRect->Y < 0);

UINT sourceDataStride;
UINT sourceDataSize;
BYTE* sourceData;
RETURN_IF_FAILED(lock->GetStride(&sourceDataStride));
RETURN_IF_FAILED(lock->GetDataPointer(&sourceDataSize, &sourceData));

RETURN_HR_IF(E_INVALIDARG, sourceDataSize > bufferSize);

// TODO #1379 - should support regional copying.
RETURN_HR_IF(E_UNEXPECTED, memcpy_s(buffer, bufferSize, sourceData, sourceDataSize) != 0);
if (copyRect->X == 0 && copyRect->Y == 0 && copyRect->Width == m_width && copyRect->Height == m_height) {
Copy link
Contributor

@aballway aballway Dec 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this in your other PR? #Closed

Copy link
Contributor

@aballway aballway Dec 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: make operator== for WICRect
#Closed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no.


In reply to: 92295888 [](ancestors = 92295888)

RETURN_HR_IF(E_INVALIDARG, sourceDataSize > bufferSize);
RETURN_HR_IF(E_UNEXPECTED, memcpy_s(buffer, bufferSize, sourceData, sourceDataSize) != 0);
} else {
// Once we support sub-regional locking we can fix this stride copier.
RETURN_HR_IF(E_NOTIMPL, sourceDataStride < fullRect.Width);
size_t bytesPerPixel = sourceDataStride / fullRect.Width;
Copy link
Contributor

@aballway aballway Dec 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can width == 0? can sourceDataStride < fullRect.Width? #Closed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can. That'd be a <8bpp image, and we should bomb out for that ;P


In reply to: 92295930 [](ancestors = 92295930)

ptrdiff_t xOffBytes = copyRect->X * bytesPerPixel;
for (size_t i = 0, j = copyRect->Y; i < copyRect->Height; ++i, ++j) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of ++j here, xOffBytes += SourceDataStride? Also sourceOffset = stride, and stride += stride instead of i? multiplications will always get you on perf.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajsesh-msft Done with the latest revision!

RETURN_HR_IF(E_UNEXPECTED,
memcpy_s(buffer + (stride * i),
bufferSize - (stride * i),
sourceData + xOffBytes + (sourceDataStride * j),
stride));
}
}
return S_OK;
}

Expand Down
Loading