processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.62k stars 3.31k forks source link

Memory Leak #2373

Closed jlp6k closed 6 years ago

jlp6k commented 6 years ago

Nature of issue?

Most appropriate sub-area of p5.js?

Which platform were you using when you encountered this?

Details about the bug:

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 commented 6 years ago

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

limzykenneth commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

Thanks a lot for your investigations and advices.

julian-weinert commented 6 years ago

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 commented 6 years ago

@julian-weinert same here.