-
Notifications
You must be signed in to change notification settings - Fork 808
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
Changes from 13 commits
7f203a1
e0a1531
32eb393
a1f32dd
f8cce71
830d03c
b769558
2647ae3
bc63c8e
bdc4c1f
d25a309
04f358c
c1dfbce
3880bfd
51b0a38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
}; | ||
|
||
/** | ||
|
@@ -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; | ||
} | ||
pixelFormat = GUID_WICPixelFormat32bppPRGBA; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super nit: just return this #WontFix There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
/** | ||
|
@@ -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."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
#import <ImageIO/CGImageDestination.h> | ||
#import "CGImageDestinationInternal.h" | ||
#import <StubReturn.h> | ||
#import <CGImageInternal.h> | ||
#include <windows.h> | ||
#include <string> | ||
|
||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
FAILED ? #WontFix There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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: | ||
|
@@ -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)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
#include <COMIncludes.h> | ||
#import <DWrite.h> | ||
#import <D2d1.h> | ||
#import <wincodec.h> | ||
#include <COMIncludes_End.h> | ||
|
||
#import "CoreGraphicsInternal.h" | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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 }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Migrating comments from other PR- There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this in your other PR? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super nit: make operator== for WICRect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can width == 0? can sourceDataStride < fullRect.Width? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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