Force all loaded textures to 32bpp in GLKit #2165

msft-Jeyaram · 2017-03-06T01:39:18Z

GLKit's implementation of GLKTexture only handled an image format of 32bpp properly (other formats e.g 8bpp indexed,etc) was not handled properly (the code was never exercised).

The root cause of this was that the earlier Cairo implementation treated every image as a 32bpp. Thus when we created an image with it's proper pixel format, GLKTexture code didn't know how to handle it and thus the errors were produced.

fixes #2150

The fix converts the images to 32bpp, since GLKTexture does not support any other format properly.
Eventually, GLKTexure needs to be re-written properly.

Current Stage:

…/creation via converting the images to the correct format

rajsesh · 2017-03-06T01:43:28Z

Frameworks/GLKit/GLKTexture.mm

-+ (GLKTextureInfo*)textureWithContentsOfFile:(NSString*)fname options:(NSDictionary*)opts error:(NSError**)err {
-    CGDataProviderRef provider = CGDataProviderCreateWithFilename([fname UTF8String]);
-    CGImageRef img = _CGImageCreateFromDataProvider(provider);
+static inline CGImageRef __Create32bppPRGBAImageFromFile(NSString* fname) {


_CGImageCreateFromFileWithFormat(name, format) and export it from CG? #Resolved

I need like we are bloating CG. we already have an internal method, and we keep on building on top of it for every case. #Resolved

but i'll go ahead and make that change #Resolved

CG has 600+ apis :). What belongs there has to belong there... #Resolved

nit: if you do make this API, please perhaps call it _CGImageCreateFromFileWithWICFormat -- specifically, we need to differentiate it from the CGBitmapInfo/CGAlphaInfo APIs.

And it'll have to take a CFStringRef.

rajsesh · 2017-03-06T01:44:20Z

Frameworks/GLKit/GLKTexture.mm

+ (GLKTextureInfo*)textureWithCGImage:(CGImageRef)image options:(NSDictionary*)opts error:(NSError**)err {
+    woc::StrongCF<CGImageRef> img = { woc::MakeStrongCF(_CGImageCreateCopyWithPixelFormat(image, GUID_WICPixelFormat32bppPRGBA)) };
+
+    if (!img) {


no macro here?

DHowett-MSFT · 2017-03-06T01:57:16Z

Frameworks/GLKit/GLKTexture.mm

-+ (GLKTextureInfo*)textureWithContentsOfFile:(NSString*)fname options:(NSDictionary*)opts error:(NSError**)err {
-    CGDataProviderRef provider = CGDataProviderCreateWithFilename([fname UTF8String]);
-    CGImageRef img = _CGImageCreateFromDataProvider(provider);
+static inline CGImageRef __Create32bppPRGBAImageFromFile(NSString* fname) {


nit: if you do make this API, please perhaps call it _CGImageCreateFromFileWithWICFormat -- specifically, we need to differentiate it from the CGBitmapInfo/CGAlphaInfo APIs.

DHowett-MSFT · 2017-03-06T01:57:28Z

Frameworks/GLKit/GLKTexture.mm

-+ (GLKTextureInfo*)textureWithContentsOfFile:(NSString*)fname options:(NSDictionary*)opts error:(NSError**)err {
-    CGDataProviderRef provider = CGDataProviderCreateWithFilename([fname UTF8String]);
-    CGImageRef img = _CGImageCreateFromDataProvider(provider);
+static inline CGImageRef __Create32bppPRGBAImageFromFile(NSString* fname) {


And it'll have to take a CFStringRef.

DHowett-MSFT · 2017-03-06T01:58:04Z

Frameworks/GLKit/GLKTexture.mm

        return nil;
    }

-    CGDataProviderRelease(provider);
-    provider = CGImageGetDataProvider(img);
+    CGDataProviderRef provider = CGImageGetDataProvider(img);


Since you are fixing this code, you might as well make it use _CGDataProviderGetBytes.

DHowett-MSFT · 2017-03-06T01:58:38Z

Also nit: When you merge this, please fix the name to be GLKit, not GLKKit

DHowett-MSFT · 2017-03-06T01:59:55Z

I have renamed this pull request. Please add "Fixes #2150."

msft-Jeyaram · 2017-03-06T02:04:05Z

I think taking in char * would be fine, given that's what CGDataProviderCreateWithFilename takes not.
so no need for CFString

DHowett-MSFT · 2017-03-06T02:05:51Z

@msft-Jeyaram please do not do that. We have strong, expressive string types for a reason.

NSString is bridged directly to CFStringRef; unless you want to deal with UTF8<->UTF16 issues like #1875 and #1891 I would implore you to use CFString.

msft-Jeyaram · 2017-03-06T03:59:35Z

fixes #2150

aballway · 2017-03-06T17:17:06Z

Frameworks/CoreGraphics/CGImage.mm

+
+    RETURN_NULL_IF(!fname);
+
+    woc::StrongCF<CGDataProviderRef> provider{ woc::MakeStrongCF(CGDataProviderCreateWithFilename(fname)) };


super nit: these can be "auto provider = "

aballway · 2017-03-06T17:17:56Z

Frameworks/GLKit/GLKTexture.mm

-    NSData* data = (id)CGDataProviderCopyData(provider);
-    [data autorelease];
-    auto bytesIn = (unsigned char*)[data bytes];
+    unsigned char* bytesIn = static_cast<unsigned char*>(const_cast<void*>(_CGDataProviderGetData(CGImageGetDataProvider(img))));


super nit: a c-style cast from const void* -> char* should suffice

aballway · 2017-03-06T17:57:15Z

Frameworks/CoreGraphics/CGImage.mm

+CGImageRef _CGImageCreateFromFileWithWICFormat(CFStringRef filename, WICPixelFormatGUID format) {
+    RETURN_NULL_IF(!filename);
+
+    std::vector<char> buffer;


On second look, we'd be better off here creating a CFURL from the filename and using CGDataProviderCreateWithURL, because that's what CGDataProviderCreateWithFilename goes through, and we get the added bonus of not creating an extra CFStringRef as well as cutting down on the length of this function quite a bit.

and here was me thinking it was the other way around.

aballway

😃

GLKit GLKTexture only supports 32bpp images, enabling texture display…

88077c8

…/creation via converting the images to the correct format

msftclas added the cla-not-required label Mar 6, 2017

msft-Jeyaram requested review from DHowett-MSFT, rajsesh and MSFTFox March 6, 2017 01:40

rajsesh suggested changes Mar 6, 2017

View reviewed changes

DHowett-MSFT suggested changes Mar 6, 2017

View reviewed changes

DHowett-MSFT changed the title ~~GLKKit enabling proper texture loading~~ Force all loaded textures to 32bpp in GLKit Mar 6, 2017

add more error checking, move function into CGImage

0c6d4bc

aballway approved these changes Mar 6, 2017

View reviewed changes

aballway suggested changes Mar 6, 2017

View reviewed changes

1000 ways to create a CGDataProvider

d804e04

aballway approved these changes Mar 6, 2017

View reviewed changes

DHowett-MSFT approved these changes Mar 6, 2017

View reviewed changes

rajsesh approved these changes Mar 6, 2017

View reviewed changes

msft-Jeyaram merged commit 64fb51d into microsoft:develop Mar 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force all loaded textures to 32bpp in GLKit #2165

Force all loaded textures to 32bpp in GLKit #2165

msft-Jeyaram commented Mar 6, 2017 •

edited

Loading

rajsesh Mar 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 6, 2017 •

edited

Loading

msft-Jeyaram Mar 6, 2017 •

edited

Loading

rajsesh Mar 6, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Mar 6, 2017

DHowett-MSFT Mar 6, 2017

rajsesh Mar 6, 2017

DHowett-MSFT Mar 6, 2017

DHowett-MSFT Mar 6, 2017

DHowett-MSFT Mar 6, 2017

DHowett-MSFT commented Mar 6, 2017

DHowett-MSFT commented Mar 6, 2017

msft-Jeyaram commented Mar 6, 2017

DHowett-MSFT commented Mar 6, 2017

msft-Jeyaram commented Mar 6, 2017

aballway Mar 6, 2017

aballway Mar 6, 2017

aballway Mar 6, 2017

msft-Jeyaram Mar 6, 2017

aballway left a comment


		RETURN_NULL_IF(!fname);

		woc::StrongCF<CGDataProviderRef> provider{ woc::MakeStrongCF(CGDataProviderCreateWithFilename(fname)) };

Force all loaded textures to 32bpp in GLKit #2165

Force all loaded textures to 32bpp in GLKit #2165

Conversation

msft-Jeyaram commented Mar 6, 2017 • edited Loading

rajsesh Mar 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

msft-Jeyaram Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

rajsesh Mar 6, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 6, 2017

DHowett-MSFT commented Mar 6, 2017

msft-Jeyaram commented Mar 6, 2017

DHowett-MSFT commented Mar 6, 2017

msft-Jeyaram commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway left a comment

Choose a reason for hiding this comment

msft-Jeyaram commented Mar 6, 2017 •

edited

Loading

rajsesh Mar 6, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 6, 2017 •

edited

Loading

msft-Jeyaram Mar 6, 2017 •

edited

Loading

rajsesh Mar 6, 2017 •

edited by msft-Jeyaram

Loading