Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory Leak #2373

Closed
3 of 17 tasks
jlp6k opened this issue Nov 20, 2017 · 7 comments
Closed
3 of 17 tasks

Memory Leak #2373

jlp6k opened this issue Nov 20, 2017 · 7 comments

Comments

@jlp6k
Copy link

jlp6k commented Nov 20, 2017

Nature of issue?

  • Found a bug
  • Existing feature enhancement
  • New feature request

Most appropriate sub-area of p5.js?

  • Color
  • Core
  • Data
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Other (specify if possible)

Which platform were you using when you encountered this?

  • Mobile/Tablet (touch devices)
  • Desktop/Laptop
  • Others (specify if possible)

Details about the bug:

  • p5.js version: /*! p5.js v0.5.16 October 11, 2017 */
  • Web browser and version:
    • Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
    • Safari Version 11.0.1 (12604.3.5.1.1)
  • Operating System: Mac OS X 10.12.6
  • Steps to reproduce this:

The following code produces a memory leak when the chromaKey() function is called.
I can't figure out what I may have done wrong except using the graphicBuffer - created with the createGraphics() function - as a pixel buffer in the chromaKey() function.

Even if the browser crashes after a while, the code seems to work fine. The ellipse is hollowed and let see the canvas background through it.

I stumbled upon this while I was looking for a work around for a previously closed issue #906 which wasn't solved IMO.

function setup() {
  createCanvas(500, 500);

  // slow the things down to be able to keep control over
  frameRate(1);
}

function draw() {
  background(100);

  drawHollowedShape();
}

function drawHollowedShape() {
  // it seems the graphicBuffer has a different pixel density (2) than
  // the screen. So it has to be twice the size of the screen.
  let graphicBuffer = createGraphics(width * 2, height * 2);
  graphicBuffer.background('white');
  graphicBuffer.fill(color('blue'));
  // draw a shape changing randomly
  graphicBuffer.ellipse(graphicBuffer.width / 4, graphicBuffer.height / 4,
    random(graphicBuffer.width / 2), random(graphicBuffer.height / 2));

  chromaKey(graphicBuffer, color('blue'), 0.1);

  // graphicBuffer has to be scaled down to fit the screen
  image(graphicBuffer, 0, 0, width, height);
}

function chromaKey(image, keyColor, threshold) {
  let pixelCount = image.width * image.height * 2;
  let keyRedComponent = red(keyColor);
  let keyGreenComponent = green(keyColor);
  let keyBlueComponent = blue(keyColor);

  image.loadPixels();
  for (let pixelIndex = 0; pixelIndex < pixelCount; pixelIndex++) {
    let pixelComponentsIndex = pixelIndex * 4;
    let pixelRedComponent = image.pixels[pixelComponentsIndex];
    let pixelGreenComponent = image.pixels[pixelComponentsIndex + 1];
    let pixelBlueComponent = image.pixels[pixelComponentsIndex + 2];

    let pixelToKeyDistance = dist(
      pixelRedComponent, pixelGreenComponent, pixelBlueComponent,
      keyRedComponent, keyGreenComponent, keyBlueComponent);

    if(pixelToKeyDistance < threshold) {
      // set the pixel alpha channel to full transparency
      image.pixels[pixelComponentsIndex + 3] = 0;
    }
  }
  image.updatePixels();
}
@Spongman
Copy link
Contributor

Spongman commented Nov 20, 2017

i think this is by-design. graphics objects are not automatically garbage-collected. in this case, you should be calling graphicsBuffer.remove().

@limzykenneth
Copy link
Member

You are calling createGraphics every frame and with every call to createGraphics a new canvas buffer will be created and unless explicitly remove as @Spongman mentioned, it will stay in memory (in Chrome at least, in the DOM) and over time, you will run out of memory as canvas buffers usually take up a chunk of memory each.

You can either remove the graphics when you no longer need them or even better, create a global graphics buffer on setup then just reuse that in your code.

@jlp6k
Copy link
Author

jlp6k commented Nov 21, 2017

@Spongman and @limzykenneth: your explanation are clear, I didn't know the graphicBuffer was referenced somewhere else than in my function and had to be explicitly removed. But even if I call graphicBuffer.remove() before leaving the function, it seems to consume memory as @limzykenneth suggests it. So remove() don't remove everything.

Using a global once-instantiated graphicBuffer isn't completely satisfying especially if you have to change its size. And IMHO it's a better programming scheme to avoid global variables when you only use them locally.

And even if I use a global graphicBuffer instantiated one time in the setup() function. The memory continues to leak. It stops to leak when I remove the call to chromaKey(graphicBuffer, color('blue'), 0.1). So the problem seems to come from the pixels manipulation, not from the repeated graphicBuffer instantiation.

Here is the new script following the useful but non efficient previous advices:

var graphicBuffer = null;

function setup() {
  createCanvas(500, 500);

  // it seems the graphicBuffer has a different pixel density (2) than
  // the screen. So it has to be twice the size of the screen.
  graphicBuffer = createGraphics(width * 2, height * 2);

  // slow the things down to be able to keep control over
  //frameRate(1);
}

function draw() {
  background(100);
  drawHollowedShape();
}

function drawHollowedShape() {
  graphicBuffer.background('white');
  graphicBuffer.fill(color('blue'));
  // draw a shape changing randomly
  graphicBuffer.ellipse(graphicBuffer.width / 4, graphicBuffer.height / 4,
    random(graphicBuffer.width / 2), random(graphicBuffer.height / 2));

  chromaKey(graphicBuffer, color('blue'), 0.1);

  // graphicBuffer has to be scaled down to fit the screen
  image(graphicBuffer, 0, 0, width, height);
}

function chromaKey(image, keyColor, threshold) {
  let pixelCount = image.width * image.height * 2;
  let keyRedComponent = red(keyColor);
  let keyGreenComponent = green(keyColor);
  let keyBlueComponent = blue(keyColor);

  image.loadPixels();
  for (let pixelIndex = 0; pixelIndex < pixelCount; pixelIndex++) {
    let pixelComponentsIndex = pixelIndex * 4;
    let pixelRedComponent = image.pixels[pixelComponentsIndex];
    let pixelGreenComponent = image.pixels[pixelComponentsIndex + 1];
    let pixelBlueComponent = image.pixels[pixelComponentsIndex + 2];

    let pixelToKeyDistance = dist(
      pixelRedComponent, pixelGreenComponent, pixelBlueComponent,
      keyRedComponent, keyGreenComponent, keyBlueComponent);

    if(pixelToKeyDistance < threshold) {
      // set the pixel alpha channel to full transparency
      image.pixels[pixelComponentsIndex + 3] = 0;
    }
  }
  image.updatePixels();
}

@limzykenneth
Copy link
Member

@jlp6k There isn't really nothing wrong with using global variables fundamentally, the only problem will be with variable name collisions but that will apply to all the global functions as well. However if you really want to avoid polluting the global namespace, you can have a look at instance mode and with it you can have the graphics buffer local only to the instance of p5 you are creating.

I did some profiling with your updated code and although it seemed like chromaKey is causing a memory leak and crashing the browser, it is not the source of a memory leak. There actually isn't a leak, the whole page peaks in memory at about 4.5GB and never goes higher, with or without chromaKey. The reason it is crashing with chromaKey is maybe because it is a slower build up to the 4.5GB mark and before it reaches that point, Safari panicked and thought it would go on forever and proceed to crash the process.

My advice would be to use a smaller graphics buffer, the one you are creating now is 2000x2000 which is really big even for retina screens. Having said that, I will look into the source for some optimization, especially around calls to image which caused all the memory to stack but I still think you should consider scaling down the graphics size a bit.

@jlp6k
Copy link
Author

jlp6k commented Nov 22, 2017

I'll probably be able to optimize the size of the hollowed/transparent shape.

Thanks a lot for your investigations and advices.

@julian-weinert
Copy link

julian-weinert commented Sep 17, 2018

I don't want to create a new issue, if not necessary. I ran into the same issue today.

I have a graphics context which I'm keeping and constantly add lines to. This context is then drawn onto the main canvas. As soon as I either draw the context using image() or add lines with buffer.line() the memory shoots up into the GB and keeps rising until Safari kills the process.

setup() {
    buffer = createGraphics(width, height);
    buffer.scale(1 / pixelDensity());
}

draw() {
    background(255);

    buffer.stroke(0);
    buffer.line(x1, y1, x2, y2);

    image(buffer, 0, 0, width, height);

    // more drawing on main canvas
}

@limzykenneth I read your statement that it is a quite big buffer.
I created mine in a window of size 628x365, but still, as soon as I draw my buffer using image the memory footprint is over 1.5GB, and in full screen size almost 5.

Maybe I'm missing something here, but should the buffer of that "size" really be almost 5GB and if so, how does it differ to the normal buffer used for the main canvas?

Oh and in the p5 web editor the memory footprint is extremely small compared. About 250MB which would equate to 500max, assuming it is handled in the iFrame as 1x pixel density (is that the case?)

Thanks

@Alynva
Copy link

Alynva commented Sep 30, 2018

@julian-weinert same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants