Implementation of CGImage using WIC#1185
Conversation
|
|
||
| return [obj autorelease]; | ||
| static inline CGImageRef getImage(UIImage *self) { | ||
| // TODO: This can be removed given deferredLoading is no longer used. |
There was a problem hiding this comment.
TODO: This can be removed given deferredLoading is no longer used. [](start = 4, length = 67)
in WIC we trust #ByDesign
| EbrFile *fpIn; | ||
| BYTE in[8] = {0}; | ||
|
|
||
| fpIn = EbrFopen(pathStr, "rb"); |
There was a problem hiding this comment.
brain's changes would take care of the EBR usage here #ByDesign
| _img = NULL; | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
these are going away ASAP #Closed
| _CGImageLoadBMP; | ||
| _CGImageLoadTIFF; | ||
| _CGImageLoadPNG; | ||
| _CGImageLoadJPEG; |
There was a problem hiding this comment.
i'm taking these out, these don't need to be exported. #Resolved
| } | ||
|
|
||
| #include "LoggingNative.h" | ||
| #define RETURN_RESULT_IF_FAILED(input, result) \ |
There was a problem hiding this comment.
Why are we redefining these? You should use wil\result.h instead. #Resolved
There was a problem hiding this comment.
this is not present in result.h.
i'll move it there so it can be used by others.
In reply to: 84145558 [](ancestors = 84145558)
| virtual void UnlockWritableBitmapTexture(DisplayTexture* tex) = 0; | ||
| virtual void RetainDisplayTexture(DisplayTexture* tex) = 0; | ||
| virtual void ReleaseDisplayTexture(DisplayTexture* tex) = 0; | ||
| virtual void *LockWritableBitmapTexture(DisplayTexture *tex, int *stride) = 0; |
There was a problem hiding this comment.
clang format gone awry? #Resolved
There was a problem hiding this comment.
| size_t bitsPerComponent, size_t bitsPerPixel, | ||
| size_t bytesPerRow, CGDataProviderRef provider, | ||
| const CGFloat *decode, bool shouldInterpolate) { | ||
| RETURN_NULL_IF(((bitsPerComponent == 8) && (bitsPerPixel == 32))); |
There was a problem hiding this comment.
add a comment why #Resolved
|
|
||
| return (CGImageRef)newImage; | ||
| } | ||
| unsigned char *data = (unsigned char *)[dataProvider bytes]; |
There was a problem hiding this comment.
nit: keep const and c++ cast #ByDesign
There was a problem hiding this comment.
| return ret; | ||
| } | ||
| CGColorRenderingIntent CGImageGetRenderingIntent(CGImageRef image) { | ||
| if (!image) { |
There was a problem hiding this comment.
nit: make ternary statement #WontFix
|
|
||
| return ret; | ||
| bool CGImageGetShouldInterpolate(CGImageRef image) { | ||
| RETURN_FALSE_IF(!image); |
| DWORD ret = 0; | ||
| NSData *sourceData = (__bridge NSData *)source; | ||
| CGImageRef imageRef = | ||
| _CGImageLoadJPEG((void *)[sourceData bytes], [sourceData length]); |
There was a problem hiding this comment.
should this be made with a copy of the data? #ByDesign
There was a problem hiding this comment.
given data could be huge, i'd rather not introduce copies.
In reply to: 84143048 [](ancestors = 84143048)
There was a problem hiding this comment.
is the lifetime of the CGImage supposed to be independent of the data it's created with or are the two tied on the reference platform? #ByDesign
There was a problem hiding this comment.
lifetime of the image has to be independent from the data (where the data can come from a file, etc)
In reply to: 84159980 [](ancestors = 84159980)
| } | ||
|
|
||
| /** | ||
| @Status Stub |
There was a problem hiding this comment.
should be caveat #Closed
| } | ||
|
|
||
| inline size_t Height() { | ||
| size_t width, height; |
There was a problem hiding this comment.
can we just get height here? #ByDesign
There was a problem hiding this comment.
There was a problem hiding this comment.
instead of passing in &width pass in nullptr? #ByDesign
There was a problem hiding this comment.
There was a problem hiding this comment.
| * specific prior written permission. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND | ||
| * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" |
There was a problem hiding this comment.
this needs to be fixed #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
The format of this file was changed in error, and it needs to be not changed.
It clutters up the diff, and makes it harder to follow.
In reply to: 84149027 [](ancestors = 84149027,84147544)
There was a problem hiding this comment.
There was a problem hiding this comment.
When you reformat with fixed clang format it may not return this to normal, so it may need to be done manually #Resolved
There was a problem hiding this comment.
You marked this resolved but it isn't... #Resolved
There was a problem hiding this comment.
| UIImageOrientationRightMirrored, // vertical flip | ||
| UIImageOrientationUp, | ||
| UIImageOrientationDown, // 180 deg rotation | ||
| UIImageOrientationLeft, // 90 deg CCW |
There was a problem hiding this comment.
nit: full words #WontFix
There was a problem hiding this comment.
| // THE SOFTWARE. | ||
| // | ||
| //****************************************************************************** | ||
| #import <TestFramework.h> |
There was a problem hiding this comment.
don't we already have these files? #ByDesign
There was a problem hiding this comment.
There was a problem hiding this comment.
Should we share those instead of replicating util code? #Closed
There was a problem hiding this comment.
I'd rather not as we will be adding CoreGraphics related helpers here and we don't necessarliy need it in foundation.
In reply to: 84152959 [](ancestors = 84152959)
There was a problem hiding this comment.
Make a coregraphics specific helper file then?
In reply to: 84154588 [](ancestors = 84154588,84152959)
| typedef struct { | ||
| CGColorSpaceModel colorSpaceModel; | ||
| CGBitmapInfo bitmapInfo; | ||
| unsigned int bitsPerComponent; |
There was a problem hiding this comment.
unsigned in [](start = 2, length = 11)
nit: BYTE is probably sufficient. #Resolved
| <ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\CoreGraphics\CGPDFString.mm" /> | ||
| <ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\CoreGraphics\CGShading.mm" /> | ||
| <ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\CoreGraphics\D2DWrapper.mm" /> | ||
| <ClangCompile Include="$(MSBuildThisFileDirectory)..\..\..\Frameworks\CoreGraphics\D2DUtility.mm" /> |
There was a problem hiding this comment.
D2DUtility [](start = 87, length = 10)
(This file doesn't need to be renamed.) #Resolved
There was a problem hiding this comment.
Given it had WIC methods, I thought it might be sane., but Wrapper seems more generic.
In reply to: 84149288 [](ancestors = 84149288)
| } __CGImagePixelProperties; | ||
|
|
||
| struct __CGImageImpl { | ||
| CFRuntimeBase _base; |
There was a problem hiding this comment.
Definitely don't put this inside the __Impl class for CppBase. CppBase has one already. #Resolved
There was a problem hiding this comment.
| } | ||
| virtual void ConstructIfNeeded() { | ||
| struct __CGImage : CoreFoundation::CppBase<__CGImage, __CGImageImpl> { | ||
|
|
There was a problem hiding this comment.
This structure should eventually move into CGImage.mm and be completely private. #Resolved
There was a problem hiding this comment.
Yep, that's the plan when we shed off the old code we are carrying.
In reply to: 84149494 [](ancestors = 84149494)
|
|
||
| struct __CGImageImpl { | ||
| CFRuntimeBase _base; | ||
| Microsoft::WRL::ComPtr<IWICBitmapSource> bitmapImageSource{nullptr}; |
There was a problem hiding this comment.
nullptr [](start = 61, length = 7)
no need to do this. #Resolved
| } _ImageInfo; | ||
|
|
||
| static const int number_of_jpeg_files = 4; | ||
| static const _ImageInfo imagesJPEG[number_of_jpeg_files] = { |
There was a problem hiding this comment.
number_of_jpeg_files [](start = 35, length = 20)
no need for this, you can use _countof on a statically-initialized array. #Resolved
| CFRuntimeBase _base; | ||
| Microsoft::WRL::ComPtr<IWICBitmapSource> bitmapImageSource{nullptr}; | ||
| CGBitmapInfo bitmapInfo; | ||
| bool isMask; |
There was a problem hiding this comment.
isMask [](start = 7, length = 6)
The scalar types, you do need to initialize, this probably warrants a constructor. #Resolved
|
|
||
| inline size_t Height() { | ||
| size_t width, height; | ||
| if (FAILED(_impl.bitmapImageSource->GetSize(&width, &height))) { |
There was a problem hiding this comment.
bitmapImageSource [](start = 21, length = 17)
For the sake of abstraction, consider moving these to be utility methods in impl and just forwarding them here. #Resolved
| return height; | ||
| } | ||
|
|
||
| inline size_t Width() { |
There was a problem hiding this comment.
Width(), Height(), etc, should be const methods. #Resolved
| @property (readonly, nonatomic) UIImageOrientation imageOrientation; | ||
| @property (readonly, nonatomic) UIImageResizingMode resizingMode STUB_PROPERTY; | ||
| @property(nonatomic, readonly) CGFloat scale; | ||
| @property(nonatomic, readonly) CGImageRef CGImage; |
There was a problem hiding this comment.
I think running clang-format (once fixed) will clear these all up. #Resolved
There was a problem hiding this comment.
| @@ -0,0 +1,45 @@ | |||
| //****************************************************************************** | |||
| // | |||
| // Copyright (c) 2016 Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
please use new header :). #Resolved
|
|
||
| inline CGBitmapInfo BitmapInfo() { return _impl.bitmapInfo; } | ||
|
|
||
| // Setters |
There was a problem hiding this comment.
Setters [](start = 5, length = 7)
removed #Resolved
| EbrFile *fpIn; | ||
| BYTE in[8] = {0}; | ||
|
|
||
| fpIn = EbrFopen(pathStr, "rb"); |
There was a problem hiding this comment.
could you merge rebase? Brian's changes are now in CGD2D #Resolved
| TraceVerbose(TAG, L"%02x ", in[i]); | ||
| if ((i + 1) % 16 == 0) | ||
| TraceVerbose(TAG, L""); | ||
| } |
There was a problem hiding this comment.
I dont think any of this should be needed. This method should just wrap the _CGImageGetImageFromData. #Resolved
| BYTE* srcIn = | ||
| ((BYTE*)ref->Backing()->LockImageData()) + startY * ref->Backing()->BytesPerRow() + startX * ref->Backing()->BytesPerPixel(); | ||
| BYTE* destOut = (BYTE*)newImage->Backing()->LockImageData(); | ||
| // TODO: take care of the stride (mapping of the guids) |
There was a problem hiding this comment.
Issue tracking this TODO? #Resolved
There was a problem hiding this comment.
| srcIn += ref->Backing()->BytesPerRow(); | ||
| destOut += newImage->Backing()->BytesPerRow(); | ||
| } | ||
| imageRef->SetBitmapInfo(bitmapInfo) |
There was a problem hiding this comment.
This chaining is a neat trick, but should be avoided in favor of clarity. There is nothing wrong with multiple imageRef-> calls here. #ByDesign
There was a problem hiding this comment.
It seems much clearer given these.
We are already have inline functions for getters.
In reply to: 84331527 [](ancestors = 84331527)
| */ | ||
| CGImageRef CGImageCreateWithMask(CGImageRef image, CGImageRef mask) { | ||
| // Given how masks are applied during rendering via D2D, we will hold onto the | ||
| // mask then apply it at the appropriate time. |
There was a problem hiding this comment.
Comment doesn't match implementation. If this is a future planned work, please open an issue to track this. #Resolved
| CGImageRef _CGImageGetImageFromData(void *data, int length) { | ||
| unsigned char *bytes = static_cast<unsigned char *>(data); | ||
|
|
||
| // TODO: (The below code is not really needed.) |
There was a problem hiding this comment.
TODO: (The below code is not really needed.) [](start = 5, length = 44)
Wic should detect the image types, so lets get rid of this code given your TODO. #Resolved
This change is