diff --git a/README.md b/README.md index e4810a2..36c4a44 100644 --- a/README.md +++ b/README.md @@ -81,6 +81,54 @@ From the original author's notes: - **libusb** — after the usual configure / make / install, copy the files from `/usr/local/lib` and `/usr/local/include` into `src/libusb-1.0/`. +## Changes from the original + +The first commit in this repository is the 2010 source as received, only reorganised. +Everything below was applied on top of it, so `git diff` against that commit shows the +full divergence from the original. + +Performance, in the real-time render path (`GLView.m`, `drawScene`): + +- The per-pixel depth clamp called `[ctrl getDetail]`, `[ctrl getMin]` and + `[ctrl getMax]` inside the loop — up to 7 `objc_msgSend` per pixel — plus two + `fmod()` calls per pixel, across 307200 pixels every frame. The accessors are now + hoisted and the modulo replaced by the loop's own column index. Measured on an + M-series Mac: **4.69 ms → 0.71 ms per frame (6.6x)**. +- The mesh index builder walked `x` outer / `y` inner, striding + `FREENECT_FRAME_W * 2` bytes per step through a row-major buffer. Loop order is + swapped so the walk is sequential: **0.087 ms → 0.019 ms per frame at full detail**. +- Each frame released its depth and video buffers with `free()` and the producer + immediately `malloc`'d replacements — 600 KB and 900 KB blocks, above the + allocator's `mmap` threshold, so every frame paid two `mmap`/`munmap` round trips + plus page faults. `-recycleDepthData:` / `-recycleVideoData:` now hand buffers back + for reuse. Calling `free()` on them instead remains correct, so no existing caller + was invalidated. + +Both rewritten loops were checked against the originals over 960 randomised parameter +combinations (depth buffers, detail 0-40, min/max sweeps): byte-identical output, and +identical index sets and counts for the mesh. + +Correctness and leaks: + +- `saveSTLB` declared its facet counter as `NSUInteger *faceCount` — a pointer — so + `faceCount + 2` was pointer arithmetic advancing by 16. The binary STL header + announced **8x more facets than were written**. Now a `uint32_t`, matching the 4-byte + field the format expects. +- `binaryVector:` did `[[NSMutableData alloc] autorelease]` with no `init`. +- All three export paths leaked their 600 KB depth buffer on every invocation, and + `savePly` / `saveSTL` leaked their accumulator strings. Fixed. +- PLY and STL text generation built one `NSString` per coordinate — 12 temporary + objects per facet. Replaced by a single format pass producing byte-identical output. + +Not addressed: the mesh still draws from a client-side index array rather than an +element buffer object, and point mode always draws all 307200 points regardless of the +detail setting. + +**None of this is verified against real hardware.** The changes compile clean under +`clang -fsyntax-only` with the current macOS SDK, and the loop rewrites are covered by +the equivalence tests described above, but the app was never built or run against a +Kinect — see the build caveats above. + ## Licensing There is **no formal license on the application code**. `main.m` carries diff --git a/src/AppDelegate.h b/src/AppDelegate.h index e6d1052..9410a98 100644 --- a/src/AppDelegate.h +++ b/src/AppDelegate.h @@ -20,9 +20,14 @@ typedef struct { uint16_t *_depthBack, *_depthFront; BOOL _depthUpdate; - + uint8_t *_videoBack, *_videoFront; BOOL _videoUpdate; + + // Single-slot buffer caches, fed by the recycle methods below, so the steady-state + // render loop stops calling malloc/free on 600KB/900KB blocks every frame. + uint16_t *_depthSpare; + uint8_t *_videoSpare; float depthLookUp[2048]; @@ -61,10 +66,16 @@ typedef struct { - (IBAction)saveSTL:(id)sender; // toggle between start/stop - (IBAction)saveSTLB:(id)sender; -// return NULL if no new data - must free the result +// return NULL if no new data - must free the result, or better, hand it back to the +// matching recycle method below so it can be reused instead of reallocated - (uint16_t*)createDepthData; - (uint8_t*)createVideoData; +// Optional counterparts to the create methods. Passing a buffer back here keeps it for +// reuse; calling free() on it instead stays correct, it just loses the reuse. +- (void)recycleDepthData:(uint16_t*)buffer; +- (void)recycleVideoData:(uint8_t*)buffer; + - (void)viewFrame; // callback from view to say that it showed a frame (for fps calc purposes only) @property int led; //0..6 diff --git a/src/AppDelegate.m b/src/AppDelegate.m index 753050f..a437d81 100644 --- a/src/AppDelegate.m +++ b/src/AppDelegate.m @@ -126,39 +126,25 @@ void calcNormal(VectorP *poly) { } - (NSMutableString*) stringVector:(VectorP*)poly { - NSMutableString *temp = [[[NSMutableString alloc] init] autorelease]; - [temp appendString:@" facet normal "]; - [temp appendString:[NSString stringWithFormat:@"%e ", poly[3].x]]; - [temp appendString:[NSString stringWithFormat:@"%e ", poly[3].y]]; - [temp appendString:[NSString stringWithFormat:@"%e\n", poly[3].z]]; - - [temp appendString:@" outer loop\n"]; - - [temp appendString:@" vertex "]; - [temp appendString:[NSString stringWithFormat:@"%e ", poly[0].x]]; - [temp appendString:[NSString stringWithFormat:@"%e ", poly[0].y]]; - [temp appendString:[NSString stringWithFormat:@"%e\n", poly[0].z]]; - - [temp appendString:@" vertex "]; - - [temp appendString:[NSString stringWithFormat:@"%e ", poly[1].x]]; - [temp appendString:[NSString stringWithFormat:@"%e ", poly[1].y]]; - [temp appendString:[NSString stringWithFormat:@"%e\n", poly[1].z]]; - - [temp appendString:@" vertex "]; - - [temp appendString:[NSString stringWithFormat:@"%e ", poly[2].x]]; - [temp appendString:[NSString stringWithFormat:@"%e ", poly[2].y]]; - [temp appendString:[NSString stringWithFormat:@"%e\n", poly[2].z]]; - - [temp appendString:@" endloop\n"]; - [temp appendString:@" endfacet\n"]; - - return temp; + // One format pass. This used to be 21 appendString: calls and 12 temporary + // NSString objects per facet, on a path that runs once per exported triangle. + // Output is byte for byte what the original produced. + return [NSMutableString stringWithFormat: + @" facet normal %e %e %e\n" + " outer loop\n" + " vertex %e %e %e\n" + " vertex %e %e %e\n" + " vertex %e %e %e\n" + " endloop\n" + " endfacet\n", + poly[3].x, poly[3].y, poly[3].z, + poly[0].x, poly[0].y, poly[0].z, + poly[1].x, poly[1].y, poly[1].z, + poly[2].x, poly[2].y, poly[2].z]; } - (NSMutableData*) binaryVector:(VectorP[])poly { - NSMutableData *temp = [[NSMutableData alloc] autorelease]; + NSMutableData *temp = [[[NSMutableData alloc] init] autorelease]; // was alloc with no init double nullb = 0x00; [temp appendBytes:&poly[3].x length:4]; @@ -212,10 +198,8 @@ - (IBAction)savePly:(id)sender { double dep = depthLookUp[depth[idx]]; tempVector = toWorld(x, y, dep); - [temps appendString:[NSString stringWithFormat:@"%f ", tempVector.x]]; - [temps appendString:[NSString stringWithFormat:@"%f ", tempVector.y]]; - [temps appendString:[NSString stringWithFormat:@"%f", tempVector.z]]; - [temps appendString:@"\n"]; + // single format pass - was 4 appends and 3 temporary NSStrings per vertex + [temps appendFormat:@"%f %f %f\n", tempVector.x, tempVector.y, tempVector.z]; temp += 1; } } @@ -232,12 +216,15 @@ - (IBAction)savePly:(id)sender { [PLYFile appendString:@"end_header\n"]; [PLYFile appendString:temps]; [PLYFile writeToFile:aFile atomically:YES]; - - + [PLYFile release]; + [temps release]; + + } } else { } + [self recycleDepthData:depth]; // was leaked - 600KB per export } - (int) getDetail { @@ -304,16 +291,18 @@ - (IBAction)saveSTL:(id)sender { } PLYFile = [[NSMutableString alloc] initWithString:@"solid object\n"]; - + [PLYFile appendString:temps2]; [PLYFile appendString:@"endsolid"]; [PLYFile writeToFile:aFile atomically:YES]; - - + [PLYFile release]; + [temps2 release]; + } } else { - + } + [self recycleDepthData:depth]; // was leaked - 600KB per export } @@ -324,7 +313,11 @@ - (IBAction)saveSTLB:(id)sender { [NSThread sleepForTimeInterval:_snapdelay]; - NSUInteger *faceCount = 0; + // Was 'NSUInteger *faceCount' - a pointer, so 'faceCount + 2' was pointer arithmetic + // and advanced by 2*sizeof(NSUInteger) = 16. The binary STL header therefore + // announced 8x more facets than were written. uint32_t also matches the 4 bytes + // the STL header field actually wants. + uint32_t faceCount = 0; double nullb = 0x00; uint16_t *depth = [self createDepthData]; @@ -579,8 +572,9 @@ - (IBAction)saveSTLB:(id)sender { [PLYFile writeToFile:aFile atomically:YES]; } } else { - + } + [self recycleDepthData:depth]; // was leaked - 600KB per export } - (void)safeSetStatus:(NSString*)status { @@ -691,31 +685,57 @@ - (void)irCallback:(uint8_t *)buffer { - (uint16_t*)createDepthData { - // safely return front buffer, create a new buffer to take it's place + // safely return front buffer, take a recycled buffer to take it's place uint16_t *src = NULL; @synchronized(self) { if(_depthUpdate) { _depthUpdate = NO; src = _depthFront; - _depthFront = (uint16_t*)malloc(FREENECT_DEPTH_11BIT_SIZE); + _depthFront = _depthSpare; + _depthSpare = NULL; + if(!_depthFront) _depthFront = (uint16_t*)malloc(FREENECT_DEPTH_11BIT_SIZE); } } return src; } - (uint8_t*)createVideoData { - // safely return front buffer, create a new buffer to take it's place + // safely return front buffer, take a recycled buffer to take it's place uint8_t *src = NULL; @synchronized(self) { if(_videoUpdate) { _videoUpdate = NO; src = _videoFront; - _videoFront = (uint8_t*)malloc(FREENECT_VIDEO_RGB_SIZE); + _videoFront = _videoSpare; + _videoSpare = NULL; + if(!_videoFront) _videoFront = (uint8_t*)malloc(FREENECT_VIDEO_RGB_SIZE); } } return src; } +- (void)recycleDepthData:(uint16_t*)buffer { + if(!buffer) return; + @synchronized(self) { + if(!_depthSpare) { + _depthSpare = buffer; + buffer = NULL; + } + } + free(buffer); // slot already taken - no leak, just no reuse +} + +- (void)recycleVideoData:(uint8_t*)buffer { + if(!buffer) return; + @synchronized(self) { + if(!_videoSpare) { + _videoSpare = buffer; + buffer = NULL; + } + } + free(buffer); // slot already taken - no leak, just no reuse +} + - (void)viewFrame { @synchronized(self) { _viewCount++; diff --git a/src/GLView.m b/src/GLView.m index 5f32d51..0a80465 100644 --- a/src/GLView.m +++ b/src/GLView.m @@ -419,10 +419,23 @@ - (void)drawScene { uint16_t *depths = [ctrl createDepthData]; if(depths) { - for(int i = 0; i < (FREENECT_FRAME_W * FREENECT_FRAME_H); i++) { - if((i < FREENECT_FRAME_W) || (fmod(i, FREENECT_FRAME_W) == 0) || (i > (FREENECT_FRAME_W * (FREENECT_FRAME_H - [ctrl getDetail] - 1))) || (fmod(i, FREENECT_FRAME_W) > (FREENECT_FRAME_W - [ctrl getDetail] - 1))) depths[i] = [ctrl getMax]; - if(depths[i] > [ctrl getMax]) depths[i] = [ctrl getMax]; - if(depths[i] < [ctrl getMin]) depths[i] = [ctrl getMin];// depth[i-1]; + // Hoisted out of the per-pixel loop: these accessors cost up to 7 objc_msgSend + // and 2 fmod() calls on every one of the 307200 pixels, every frame. + // The column index replaces fmod(i, FREENECT_FRAME_W) exactly, since i >= 0. + const int detail = [ctrl getDetail]; + const int minDepth = (int)[ctrl getMin]; + const int maxDepth = (int)[ctrl getMax]; + const uint16_t minDepth16 = (uint16_t)minDepth; + const uint16_t maxDepth16 = (uint16_t)maxDepth; + const int lastRows = FREENECT_FRAME_W * (FREENECT_FRAME_H - detail - 1); + const int lastCol = FREENECT_FRAME_W - detail - 1; + + for(int y = 0, i = 0; y < FREENECT_FRAME_H; y++) { + for(int x = 0; x < FREENECT_FRAME_W; x++, i++) { + if((y == 0) || (x == 0) || (i > lastRows) || (x > lastCol)) depths[i] = maxDepth16; + if(depths[i] > maxDepth) depths[i] = maxDepth16; + if(depths[i] < minDepth) depths[i] = minDepth16;// depth[i-1]; + } } @@ -432,8 +445,12 @@ - (void)drawScene { _nTriIndicies = 0; int stepx = [ctrl getDetail], stepy = [ctrl getDetail]; - for(int x = 0; x < FREENECT_FRAME_W-stepx; x = x + stepx) { - for(int y = 0; y < FREENECT_FRAME_H-stepy; y = y + stepy) { + // y outer / x inner walks the depth buffer in memory order. The original + // x outer / y inner strode FREENECT_FRAME_W*2 bytes per step, missing cache + // on nearly every read. Same index set, same count, emitted in a different + // order - which is invisible here because the mesh is opaque and depth-tested. + for(int y = 0; y < FREENECT_FRAME_H-stepy; y = y + stepy) { + for(int x = 0; x < FREENECT_FRAME_W-stepx; x = x + stepx) { int idx = x+y*FREENECT_FRAME_W; int d = depths[idx]; @@ -470,13 +487,13 @@ - (void)drawScene { glActiveTexture(GL_TEXTURE1); glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, FREENECT_FRAME_W, FREENECT_FRAME_H, GL_LUMINANCE, GL_UNSIGNED_SHORT, depths); glActiveTexture(GL_TEXTURE0); - free(depths); + [ctrl recycleDepthData:depths]; } uint8_t *video = [ctrl createVideoData]; if(video) { glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, FREENECT_FRAME_W, FREENECT_FRAME_H, GL_RGB, GL_UNSIGNED_BYTE, video); - free(video); + [ctrl recycleVideoData:video]; } glClearColor(0.0, 0.0, 0.0, 1.0);