Consolidate uses of Ebr file functions. by bbowman · Pull Request #1175 · microsoft/WinObjC

bbowman · 2016-10-18T18:54:18Z

Consolidate uses of Ebr file functions to only where absolutely needed. This helps abstract any weird semantics we have to only a few locations and uses well defined APIs for foundation constructs like NSData and NSStream where possible.

…d. This helps abstract any weird semantics we have to only a few locations and uses well defined APIs for foundation constructs like NSData and NSStream where possible.

DHowett-MSFT · 2016-10-18T18:58:16Z

+    auto result = [outputStream write:(const unsigned char*)[self bytes] maxLength:numBytes];
+    [outputStream close];
+
+    if (result != numBytes) {


numBytes [](start = 18, length = 8)

Only -1 is an error. 0 < numBytes <= maxBytes is legal and should be retried to consume the rest of the buffer. #Resolved

DHowett-MSFT · 2016-10-18T18:59:14Z

+    }
+
+    return [self initWithData:tempData];
 }


I'm wary of introducing a copy here. #Pending

At the very least, NSMutableData has mutableBytes, which you can write into up to .capacity.

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

DHowett-MSFT · 2016-10-18T18:59:35Z

+
+    while ([inputStream hasBytesAvailable]) {
+        auto result = [inputStream read:byteBuffer maxLength:_countof(byteBuffer)];
+        [tempData appendBytes:byteBuffer length:result];


[tempData appendBytes:byteBuffer length:result]; [](start = 8, length = 48)

Sorry, two copies. #Pending

see my other response to JJ. Its not quite as bad as you make it out to be and any other solution is potientially more error prone. I will keep looking at other solutions though.

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

msft-Jeyaram · 2016-10-18T19:02:23Z


 /**
 @Status Caveat
 @Notes options parameter not supported


options parameter not supported [](start = 8, length = 31)

seems like we are supporting a portion of it? #Resolved

will update the comment but its still caveat.

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

msft-Jeyaram · 2016-10-18T19:02:46Z

-        TraceVerbose(TAG, L"-[NSData::writeToURL]: Only file: URLs are supported. (%hs)", [[url absoluteString] UTF8String]);
+    if (!url) {
+        if (errorp) {
+            *errorp = [NSError errorWithDomain:@"NSData" code:100 userInfo:nil];


100 [](start = 62, length = 3)

I can't recall If I checked this, but does this match the reference platform? #WontFix

copied from the file one now that file forwards to URL. Not the purpose of this pr to validate our error codes.

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

msft-Jeyaram · 2016-10-18T19:04:16Z

+        return NO;
+    }
+    auto numBytes = [self length];
+    auto result = [outputStream write:(const unsigned char*)[self bytes] maxLength:numBytes];


result = [outputStream write:(const unsigned char*)[self bytes] maxLength:numBytes]; [](start = 8, length = 85)

we might not write the whole buffer, we need to check if we have any left over and write the rest?
I recall similar code existing already. #Resolved

msft-Jeyaram · 2016-10-18T19:07:04Z

+    NSMutableData* tempData = [NSMutableData data];
+
+    while ([inputStream hasBytesAvailable]) {
+        auto result = [inputStream read:byteBuffer maxLength:_countof(byteBuffer)];


result [](start = 13, length = 6)

check for failure condition (-1)? #Pending

DHowett-MSFT · 2016-10-18T19:08:30Z

-        TraceVerbose(TAG, L"Opened %hs", pFilePath);
-    }
+    // TODO 1162: An NSInputStream should be initialized here and run loop events should be used to schedule
+    // the file operations.


I certainly hope NSURLConnection is up to the task. As well, I wonder if the reference platform short-circuits file URL handling.

It is fully possible to load a file from a file URL without spinning a run loop inbetween. #WontFix

Whoever is looking at 116 2can make those determinations.

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

msft-Jeyaram · 2016-10-18T19:17:11Z

+    NSMutableData* tempData = [NSMutableData data];
+
+    while ([inputStream hasBytesAvailable]) {
+        auto result = [inputStream read:byteBuffer maxLength:_countof(byteBuffer)];


byteBuffer [](start = 40, length = 10)

you can just get the mutableBytes of the NSMutableData and read into it directly (would help you avoid the copy)
If you need to grow the capacity, you can do it via increaseLength #Pending

increaseLength isn't really desirable here. Ideally I'd be able to read the chunks directly in and return the same data. If the length is wrong I can't return that data and need to manually reconstruct it with the right length that I keep track of. appendBytes is morally equivalent of what you are suggesting but it adjusts the capacity instead of length at the expense of the temp buffer (which is only allocated once and is stack space so its "cheap" in the grand scheme of things). Basically this is letting the NSData append impl be smart about growth etc which seems better than me trying to be smart (since I'm not very). I'll look for other ways to avoid extra copies though.

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

Agreed, in this context, append is appropriate (but just thought I would surface that).

In reply to: 83930789 [](ancestors = 83930789,83929862)

rajsesh · 2016-10-18T21:59:29Z

+                                                            getSizeFunc:getSizeFunc
+                                                            setSizeFunc:setSizeFunc];
+    [in open];
+    char header[4] = {};


char [](start = 4, length = 4)

nit: this could be uint8_t to avoid casts. It is unsigned char, so the other apis should work? #Closed

hilariously no because of the use of strncmp

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

a cast is needed somewhere so I put it here.

In reply to: 83964371 [](ancestors = 83964371,83959944)

rajsesh · 2016-10-18T23:38:37Z

-        (id)kCGImagePropertyDPIWidth : [NSNumber numberWithDouble:1000],
-        (id)kCGImagePropertyDPIHeight : [NSNumber numberWithDouble:200],
+        (id) kCGImagePropertyOrientation : encodeOrientation, (id)
+        kCGImagePropertyGIFDictionary : gifOptions, (id)


nit: plenty of silly clang format changes in this file. #Resolved

rajsesh · 2016-10-18T23:46:56Z

    return self;
 }

 - (id)startLoading {


startLoading [](start = 6, length = 12)

This could potentially break something (anyone using this externally). #WontFix

there is no functional change.

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

rajsesh · 2016-10-18T23:50:37Z

-    */
-
-    return theAnswer;
+    return;


return [](start = 4, length = 6)

nit: not needed. #WontFix

rajsesh · 2016-10-18T23:55:26Z

-}
-
-Byte* CAFDecoder::GetBytes(EbrFile* fpIn, int len) {
+Byte* CAFDecoder::GetBytes(NSInputStream* stream, int len) {


Changes to this file don't have much collateral, please be sure to validate internal apps. #Resolved

validated.

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

rajsesh · 2016-10-18T23:55:30Z

bbowman · 2016-10-19T18:42:15Z

@DHowett-MSFT @msft-Jeyaram My full build passed and it seems like CAF audio is not broken. huzzah. Any other comments before I go go?

DHowett-MSFT · 2016-10-19T18:45:14Z

I'm not 100% stoked about the foundation dep in AudioToolbox, but I understand that we already had one (IIRC) and it's fairly harmless to replace if we do decide to set up a boundary.
LGTM!

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

DHowett-MSFT · 2016-10-19T18:45:17Z

Consolidate uses of Ebr file functions to only where absolutely neede…

f28000d

…d. This helps abstract any weird semantics we have to only a few locations and uses well defined APIs for foundation constructs like NSData and NSStream where possible.

msftclas added the cla-not-required label Oct 18, 2016

DHowett-MSFT reviewed Oct 18, 2016

View reviewed changes

msft-Jeyaram reviewed Oct 18, 2016

View reviewed changes

DHowett-MSFT reviewed Oct 18, 2016

View reviewed changes

msft-Jeyaram reviewed Oct 18, 2016

View reviewed changes

rajsesh reviewed Oct 18, 2016

View reviewed changes

rajsesh added the Area-Foundation label Oct 19, 2016

bbowman added 4 commits October 18, 2016 17:06

s

288fbf3

s

8812e8d

s

65730d4

s

7c5058d

bbowman added 4 commits October 19, 2016 11:51

s

7130397

s

05b0c77

s

4e86a9e

s

d2f87e4

bbowman merged commit be78bef into microsoft:develop Oct 19, 2016

Conversation

bbowman commented Oct 18, 2016

Uh oh!

DHowett-MSFT Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh Oct 18, 2016 • edited by bbowman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DHowett-MSFT Oct 18, 2016 •

edited by bbowman

Loading

DHowett-MSFT Oct 18, 2016 •

edited by bbowman

Loading

DHowett-MSFT Oct 18, 2016 •

edited by bbowman

Loading

msft-Jeyaram Oct 18, 2016 •

edited by bbowman

Loading

msft-Jeyaram Oct 18, 2016 •

edited by bbowman

Loading

msft-Jeyaram Oct 18, 2016 •

edited by bbowman

Loading

msft-Jeyaram Oct 18, 2016 •

edited by bbowman

Loading

DHowett-MSFT Oct 18, 2016 •

edited by bbowman

Loading

msft-Jeyaram Oct 18, 2016 •

edited by bbowman

Loading

rajsesh Oct 18, 2016 •

edited

Loading

rajsesh Oct 18, 2016 •

edited by bbowman

Loading

rajsesh Oct 18, 2016 •

edited by bbowman

Loading

rajsesh Oct 18, 2016 •

edited by bbowman

Loading

rajsesh Oct 18, 2016 •

edited by bbowman

Loading