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

[CGD2D] Render buffer-shared G8 images as A8. #1804

Merged
merged 7 commits into from
Jan 25, 2017
Merged
Changes from 3 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
57 changes: 45 additions & 12 deletions Frameworks/CoreGraphics/CGContext.mm
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,8 @@ inline HRESULT GetTarget(ID2D1Image** pTarget) {

CGAffineTransform textMatrix{ CGAffineTransformIdentity };

using __CGContextFlushHook = void(*)(CGContextRef);
Copy link
Author

Choose a reason for hiding this comment

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

dead; drop.


private:
std::stack<__CGContextLayer> _layerStack{};
woc::unique_cf<CGMutablePathRef> _currentPath{ nullptr };
Expand Down Expand Up @@ -2743,10 +2745,29 @@ bool CGContextIsPointInPath(CGContextRef context, bool eoFill, CGFloat x, CGFloa
struct __CGBitmapContext : CoreFoundation::CppBase<__CGBitmapContext, __CGContext> {
woc::unique_cf<CGImageRef> _image;

void* _data;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know this one has been around a bit longer and CG has been using it, but I like the m_* naming convention for C++ structs/classes since it makes it more obvious what's C++ and what's objc.

Copy link
Author

Choose a reason for hiding this comment

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

@aballway what is the value in distinguishing Objective-C and C++ code? Members and instance variables are, for all intents and purposes, identical. A fully-qualified out-of-body member function refers to members just like an Objective-C method refers to ivars.

They have similar lifetime requirements, as well.

our CF stuff is closer to Objective-C due to the reference counting; it's only C++ as a measure of convenience.

Also, our coding convention states

Use the "_" prefix on all data members of Objective C and C++ classes. This is consistent with Objective C instance variables that are synthesized automatically by the compiler.


size_t _width;
size_t _height;
size_t _stride;

CGBitmapContextReleaseDataCallback _releaseDataCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make these private? nobody should be allowed to muck around with 'em

void* _releaseInfo;

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

__CGBitmapContext(ID2D1RenderTarget* renderTarget, REFWICPixelFormatGUID outputPixelFormat, void* data, size_t width, size_t height, size_t stride, CGBitmapContextReleaseDataCallback releaseDataCallback, void* releaseInfo)
: Parent(renderTarget), _outputPixelFormat(outputPixelFormat), _data(data), _width(width), _height(height), _stride(stride), _releaseDataCallback(releaseDataCallback), _releaseInfo(releaseInfo) {
}

~__CGBitmapContext() {
if (_data && _releaseDataCallback) {
_releaseDataCallback(_releaseInfo, _data);
Copy link
Contributor

Choose a reason for hiding this comment

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

will the reference platform call the release callback if data == nullptr?

Copy link
Author

Choose a reason for hiding this comment

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

No; there's no data to be released.

}
}

inline void SetImage(CGImageRef image) {
_image.reset(CGImageRetain(image));
}
Expand Down Expand Up @@ -2775,39 +2796,48 @@ CGContextRef CGBitmapContextCreate(void* data,

/**
@Status Caveat
@Notes releaseCallback and releaseInfo is ignored.
We only support formats that are 32 bits per pixel, colorspace and bitmapinfo that are ARGB.
@Notes If data is provided, it can only be in one of the few pixel formats Direct2D can render to in system memory: (P)RGBA, (P)BGRA, or Alpha8. If a buffer is
provided for a grayscale image, render operations will be carried out into an Alpha8 buffer instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Grayscale + 8bpp. Our support for grayscale colorspace for > 8bpp doesn't exist.

Luminance values will be discarded in favour of alpha values.
*/
CGContextRef CGBitmapContextCreateWithData(void* data,
size_t width,
size_t height,
size_t bitsPerComponent,
size_t bytesPerRow,
CGColorSpaceRef space,
CGColorSpaceRef colorSpace,
uint32_t bitmapInfo,
CGBitmapContextReleaseDataCallback releaseCallback,
void* releaseInfo) {
RETURN_NULL_IF(!width);
RETURN_NULL_IF(!height);
RETURN_NULL_IF(!space);
RETURN_NULL_IF(!colorSpace);

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

void* actualBackingBuffer = data;
Copy link
Contributor

Choose a reason for hiding this comment

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

seems unnecessary.

Copy link
Author

Choose a reason for hiding this comment

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

There will be a future where we want to keep track of the user's input data and destroy it with their release callback, but we don't necessarily want the CGIWICBitmap to hold a reference to it.

I'd like to leave this in for that time, but maybe rename it to something more appropriate.


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

Choose a reason for hiding this comment

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

It would be better to check outputPixelFormat == GUID_WICPixelFormat8bppGray for ultimate truthiness.

TraceWarning(TAG, L"Shared-buffer grayscale context requested at %dx%d. Since Direct2D does not support this, the context will be in A8. There may be minor color aberrations.", width, height);

// Set outputPixelFormat here as so that we don't do a copy on CGBitmapContextCGBitmapContextGetImage().
outputPixelFormat = pixelFormat = GUID_WICPixelFormat8bppAlpha;
} else {
UNIMPLEMENTED_WITH_MSG(
"CGBitmapContext does not currently support input conversion and can only render into 32bpp PRGBA buffers.");
return nullptr;
}
}
pixelFormat = GUID_WICPixelFormat32bppPRGBA;
}

// if data is null, enough memory is allocated via CGIWICBitmap
ComPtr<IWICBitmap> customBitmap = Make<CGIWICBitmap>(data, pixelFormat, height, width);
ComPtr<IWICBitmap> customBitmap = Make<CGIWICBitmap>(actualBackingBuffer, pixelFormat, height, width);
RETURN_NULL_IF(!customBitmap);

woc::unique_cf<CGImageRef> image(_CGImageCreateWithWICBitmap(customBitmap.Get()));
Expand All @@ -2818,7 +2848,10 @@ CGContextRef CGBitmapContextCreateWithData(void* data,

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

__CGBitmapContext* context = __CGBitmapContext::CreateInstance(kCFAllocatorDefault, renderTarget.Get(), outputPixelFormat, data, width, height, bytesPerRow, releaseCallback, releaseInfo);
__CGContextPrepareDefaults(context);
context->SetImage(image);
Copy link
Contributor

Choose a reason for hiding this comment

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

image [](start = 22, length = 5)

image.get()?

return context;
}

Expand Down