2
\$\begingroup\$

I have created a function in C on OSX in Xcode which captures the screen, adds the mouse pointer to the image, rescales the image to fit a certain width X height requirement, and puts the rescaled image in a "bounding box", and finally converts the image from RGBA to RGB.

Is there a kind and experienced OSX developer out there who could review the code and tell me what I should change to make it execute faster?

But most importantly, I'm interested in hearing if I overlooked something and calling the function is generating memory leaks.

The code is here:

VIEW

DOWNLOAD

void screenCapture(void)
{
   int rc;
   NSRect e;
   int H;
   int W;
   CGRect rect;
   CGImageRef image;
   float screenshotWidth;
   float screenshotHeight;
   float requestedWidth;
   float requestedHeight;
   NSPoint mouseLoc;
   NSImage *overlay;
   NSPoint offset;
   int x;
   int y;
   int w;
   int h;
   int org_x;
   int org_y;
   size_t height;
   size_t width;
   size_t bytesPerRow;
   unsigned int * imgData;
   CGRect bgBoundingBox;
   CGContextRef context;
   CGImageRef imageWithMousePointer;
   CGImageRef imageWithMousePointerRescaled;
   CGImageRef imageWithMousePointerRescaledInBox;
   CFDataRef rawData;

   float wscale;
   float hscale;
   float scale;
   float newWidth;
   float newHeight;
   float rescaledWidth;
   float rescaledHeight;
   float origX;
   float origY;
   size_t boundingBoxWidth;
   size_t boundingBoxHeight;

   float imageWidth;
   float imageHeight;

   UInt8* buf;
   unsigned long byteLen;
   unsigned long bmpIdx;

   void*          bitmapData;
   int             bitmapByteCount;
   int             bitmapBytesPerRow;
   int n_width;
   int n_height;
   CGColorSpaceRef colorspace;

   rc = 1;
   e = [[NSScreen mainScreen] frame];
   H = (int)e.size.height;
   W = (int)e.size.width;
   screenshotWidth = (float) W;
   screenshotHeight = (float) H;
   rect.size.height = H;
   rect.size.width = W;
   rect.origin.x = 0;
   rect.origin.y = 0;

   requestedWidth = 1280.0f;
   requestedHeight = 720.0f;

   /***************************** Screenshot *************************************/  
   image = CGWindowListCreateImage(rect, kCGWindowListOptionIncludingWindow|kCGWindowListOptionOnScreenBelowWindow, 0, kCGWindowImageDefault);


   /***************************** Render cursor *************************************/  
   mouseLoc   = [NSEvent mouseLocation];
   overlay    = [[[NSCursor arrowCursor] image] copy];
   offset     = [[NSCursor arrowCursor] hotSpot];
   x = (int)mouseLoc.x;
   y = (int)mouseLoc.y;
   w = (int)[overlay size].width;
   h = (int)[overlay size].height;
   org_x = (x - w/2) - offset.x;
   org_y = (y - h/2) - offset.y;
   height = CGImageGetHeight(image);
   width =  CGImageGetWidth(image);
   bytesPerRow = CGImageGetBytesPerRow(image);
   imgData = (unsigned int*)malloc(height*bytesPerRow);
   bgBoundingBox = CGRectMake (0, 0, width,height);
   context =  CGBitmapContextCreate(imgData, width, height, 8,  bytesPerRow, CGImageGetColorSpace(image), CGImageGetBitmapInfo(image));
   CGContextDrawImage(context,bgBoundingBox,image);
   CGContextDrawImage(context,CGRectMake(0, 0, width,height),image);
   CGContextDrawImage(context,CGRectMake(org_x, org_y, w,h),[overlay CGImageForProposedRect: NULL context: NULL hints: NULL] );
   imageWithMousePointer = CGBitmapContextCreateImage(context);
   CGContextRelease(context);
   free(imgData);


   /***************************** Rescale *************************************/  
   wscale = requestedWidth / screenshotWidth;
   hscale = requestedHeight / screenshotHeight;
   scale = hscale;
   if (wscale < hscale) scale = wscale;
   newWidth = ((float) CGImageGetWidth(imageWithMousePointer)) * scale;
   newHeight = ((float) CGImageGetHeight(imageWithMousePointer)) * scale;
   n_width = CGImageGetWidth(imageWithMousePointer) * scale;
   n_height = CGImageGetHeight(imageWithMousePointer) * scale;
   bitmapBytesPerRow   = (n_width * 4);
   bitmapByteCount     = (bitmapBytesPerRow * n_height);
   bitmapData = malloc( bitmapByteCount );
   colorspace = CGImageGetColorSpace(imageWithMousePointer);
   context = CGBitmapContextCreate (bitmapData,n_width,n_height,8,bitmapBytesPerRow, colorspace,kCGImageAlphaNoneSkipFirst);
   //CGColorSpaceRelease(colorspace);
   CGContextDrawImage(context, CGRectMake(0,0,n_width, n_height), imageWithMousePointer);
   imageWithMousePointerRescaled = CGBitmapContextCreateImage(context);
   CGContextRelease(context);
   free(bitmapData);
   origX = (requestedWidth - newWidth) / 2.0f;
   origY = (requestedHeight - newHeight) / 2.0f;

   /************** Insert in box ********************************/
   boundingBoxWidth = (size_t) requestedWidth;
   boundingBoxHeight = (size_t) requestedHeight;
   bytesPerRow = boundingBoxWidth * (8 + 8 + 8 + 8) / 8;
   imgData = (unsigned int*)malloc(boundingBoxHeight*bytesPerRow);
   imageWidth = (float) CGImageGetWidth(imageWithMousePointerRescaled);
   imageHeight = (float) CGImageGetHeight(imageWithMousePointerRescaled);
   memset(imgData, 0, boundingBoxHeight*bytesPerRow);
   context =  CGBitmapContextCreate(imgData, boundingBoxWidth, boundingBoxHeight, 8, bytesPerRow, CGImageGetColorSpace(imageWithMousePointerRescaled), CGImageGetBitmapInfo(imageWithMousePointerRescaled));
   CGContextDrawImage(context, CGRectMake(origX,origY,imageWidth, imageHeight), imageWithMousePointerRescaled);
   imageWithMousePointerRescaledInBox = CGBitmapContextCreateImage(context);
   CGContextRelease(context);    
   free(imgData);

   /*********** Convert from RGBA to RGB **********/
   rawData = CGDataProviderCopyData(CGImageGetDataProvider(imageWithMousePointerRescaledInBox));
   buf = (UInt8*) CFDataGetBytePtr(rawData);

   for (int x = 0; x < 1280; x++)
   {
      for (int v = 0; v < 720; v++)
      {
         int pixelStartIndex = (x + (v * 1280)) * 4;
         int newPixelStartIndex = (x + (v * 1280)) * 3;
         UInt8 R  = buf[pixelStartIndex + 1];
         UInt8 G = buf[pixelStartIndex + 2];
         UInt8 B = buf[pixelStartIndex + 3];
         bmpimg[newPixelStartIndex] = R;
         bmpimg[newPixelStartIndex + 1] = G;
         bmpimg[newPixelStartIndex + 2] = B;
      }
   }

}
Improve this question
\$\endgroup\$
7
  • \$\begingroup\$ Please include the code that you would like us to review in your question (see here). \$\endgroup\$
    – Gareth Rees
    Oct 9, 2013 at 12:34
  • \$\begingroup\$ Here you go.... \$\endgroup\$
    – user1884325
    Oct 9, 2013 at 15:01
  • \$\begingroup\$ Does OSX really use floats for width/height/etc... ? \$\endgroup\$
    – technosaurus
    Oct 9, 2013 at 19:02
  • \$\begingroup\$ Yeah, the coordinate system is all floats. It allows for handling the difference between retina and non-retina displays better. It follows that view frames are all in floats. \$\endgroup\$
    – boztalay
    Jan 7, 2014 at 7:44
  • \$\begingroup\$ @technosaurus They're not floats at all. They're CGFloats. On a 32-bit system, they're floats. On a 64-bit system, they're doubles. \$\endgroup\$
    – nhgrif
    Jun 17, 2014 at 0:33

1 Answer 1

Reset to default
2
\$\begingroup\$

Please make everyone (including yourself) a favor :

  • Declare your variable in the smallest possible scope. Also define them as your declare them if you can (and you usually can).

  • Split your code into smaller entities - smaller functions, smaller structures, etc.

  • Enable every single warning your compiler supports. Here you have loads of unused variables. Keeping them is just a way to make things even harder to read.

  • Avoid magic numbers.

Here's what I have after declaring and defining variables in the same time :

void screenCapture(void)
{
   // unused ? int rc = 1;
   // unused ? float rescaledWidth;
   // unused ? float rescaledHeight;
   // unused ? unsigned long byteLen;
   // unused ? unsigned long bmpIdx;

   NSRect e = [[NSScreen mainScreen] frame];
   int H = (int)e.size.height;
   int W = (int)e.size.width;
   float screenshotWidth = (float) W;
   float screenshotHeight = (float) H;
   CGRect rect;
   rect.size.height = H;
   rect.size.width = W;
   rect.origin.x = 0;
   rect.origin.y = 0;

   float requestedWidth = 1280.0f;
   float requestedHeight = 720.0f;

   /***************************** Screenshot *************************************/  
   CGImageRef image = CGWindowListCreateImage(rect, kCGWindowListOptionIncludingWindow|kCGWindowListOptionOnScreenBelowWindow, 0, kCGWindowImageDefault);


   /***************************** Render cursor *************************************/  
   NSPoint mouseLoc = [NSEvent mouseLocation];
   NSImage *overlay    = [[[NSCursor arrowCursor] image] copy];
   NSPoint offset     = [[NSCursor arrowCursor] hotSpot];
   int x = (int)mouseLoc.x;
   int y = (int)mouseLoc.y;
   int w = (int)[overlay size].width;
   int h = (int)[overlay size].height;
   int org_x = (x - w/2) - offset.x;
   int org_y = (y - h/2) - offset.y;
   size_t height = CGImageGetHeight(image);
   size_t width =  CGImageGetWidth(image);
   size_t bytesPerRow = CGImageGetBytesPerRow(image);
   unsigned int * imgData = (unsigned int*)malloc(height*bytesPerRow);
   GCRect bgBoundingBox = CGRectMake (0, 0, width,height);
   CFDataRef context =  CGBitmapContextCreate(imgData, width, height, 8,  bytesPerRow, CGImageGetColorSpace(image), CGImageGetBitmapInfo(image));
   CGContextDrawImage(context,bgBoundingBox,image);
   CGContextDrawImage(context,CGRectMake(0, 0, width,height),image);
   CGContextDrawImage(context,CGRectMake(org_x, org_y, w,h),[overlay CGImageForProposedRect: NULL context: NULL hints: NULL] );
   CGImageRef imageWithMousePointer = CGBitmapContextCreateImage(context);
   CGContextRelease(context);
   free(imgData);


   /***************************** Rescale *************************************/  
   float wscale = requestedWidth / screenshotWidth;
   float hscale = requestedHeight / screenshotHeight;
   float scale = hscale;
   if (wscale < hscale) scale = wscale;
   float newWidth = ((float) CGImageGetWidth(imageWithMousePointer)) * scale;
   float newHeight = ((float) CGImageGetHeight(imageWithMousePointer)) * scale;
   int n_width = CGImageGetWidth(imageWithMousePointer) * scale;
   int n_height = CGImageGetHeight(imageWithMousePointer) * scale;
   int bitmapBytesPerRow   = (n_width * 4);
   int bitmapByteCount     = (bitmapBytesPerRow * n_height);
   void* bitmapData = malloc( bitmapByteCount );
   CGColorSpaceRef colorspace = CGImageGetColorSpace(imageWithMousePointer);
   context = CGBitmapContextCreate (bitmapData,n_width,n_height,8,bitmapBytesPerRow, colorspace,kCGImageAlphaNoneSkipFirst);
   //CGColorSpaceRelease(colorspace);
   CGContextDrawImage(context, CGRectMake(0,0,n_width, n_height), imageWithMousePointer);
   CGImageRef imageWithMousePointerRescaled = CGBitmapContextCreateImage(context);
   CGContextRelease(context);
   free(bitmapData);
   float origX = (requestedWidth - newWidth) / 2.0f;
   float origY = (requestedHeight - newHeight) / 2.0f;

   /************** Insert in box ********************************/
   size_t boundingBoxWidth = (size_t) requestedWidth;
   size_t boundingBoxHeight = (size_t) requestedHeight;
   bytesPerRow = boundingBoxWidth * (8 + 8 + 8 + 8) / 8;
   imgData = (unsigned int*)malloc(boundingBoxHeight*bytesPerRow);
   float imageWidth = (float) CGImageGetWidth(imageWithMousePointerRescaled);
   float imageHeight = (float) CGImageGetHeight(imageWithMousePointerRescaled);
   memset(imgData, 0, boundingBoxHeight*bytesPerRow);
   context =  CGBitmapContextCreate(imgData, boundingBoxWidth, boundingBoxHeight, 8, bytesPerRow, CGImageGetColorSpace(imageWithMousePointerRescaled), CGImageGetBitmapInfo(imageWithMousePointerRescaled));
   CGContextDrawImage(context, CGRectMake(origX,origY,imageWidth, imageHeight), imageWithMousePointerRescaled);
   CGImageRef imageWithMousePointerRescaledInBox = CGBitmapContextCreateImage(context);
   CGContextRelease(context);    
   free(imgData);

   /*********** Convert from RGBA to RGB **********/
   CFDataRef rawData = CGDataProviderCopyData(CGImageGetDataProvider(imageWithMousePointerRescaledInBox));
   UInt8* buf = (UInt8*) CFDataGetBytePtr(rawData);

   for (int x = 0; x < 1280; x++)
   {
      for (int v = 0; v < 720; v++)
      {
         int pixelStartIndex = (x + (v * 1280)) * 4;
         int newPixelStartIndex = (x + (v * 1280)) * 3;
         UInt8 R  = buf[pixelStartIndex + 1];
         UInt8 G = buf[pixelStartIndex + 2];
         UInt8 B = buf[pixelStartIndex + 3];
         bmpimg[newPixelStartIndex] = R;
         bmpimg[newPixelStartIndex + 1] = G;
         bmpimg[newPixelStartIndex + 2] = B;
      }
   }
}

Now, I realise that I miss some information to go any further. For instance, what is bmpimg ?

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ Yeah, I'm sorry for the messy function. I realize that it needs a clean-up. So I appreciate your feedback a lot! To answer your question, bmpimg is a global variable (char array). I intend to change that so that the function has this signature instead : void screenCapture(char* bmpimg) \$\endgroup\$
    – user1884325
    Oct 9, 2013 at 18:24
  • \$\begingroup\$ My bigger concern is if the function is generating memory leaks. I'm not sure what I have to free and when to do that. \$\endgroup\$
    – user1884325
    Oct 9, 2013 at 18:25
  • \$\begingroup\$ Just free whatever you malloc. \$\endgroup\$
    – SylvainD
    Oct 10, 2013 at 10:24
  • \$\begingroup\$ I know that I should free whatever I directly malloc. What I'm not sure about is those SDK calls that I'm making. For example: Should I call release on 'image' which holds the stuff returned by CGWindowListCreateImage ??? \$\endgroup\$
    – user1884325
    Oct 10, 2013 at 20:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.