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

Maybe a memory leak when using canvas to show images. #11378

Closed
liyuanqiu opened this Issue May 23, 2017 · 25 comments

Comments

Projects
None yet
7 participants
@liyuanqiu
Copy link

liyuanqiu commented May 23, 2017

Description of the problem

JSFiddle here

I record the performance in Chrome when running this JSFiddle, it shows:
1024x1024
The memory peak value getting higher and higher, but the valley value always keeps a low value.

Then uncomment the two lines in line number 30 and 31:

width = 1000;
height = 1100;

The canvas size is not power of two, and the memory usage increases rapidly

the performance record shows:
1000x1100
The memory peak value and valley value both getting higher and higher.

I call the dispose method of material and texture explicitly, and remove the mesh from scene in every loop.

Three.js version
  • Dev
  • r85
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • [] All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@WestLangley

This comment has been minimized.

Copy link
Collaborator

WestLangley commented May 23, 2017

Please try again after properly disposing of your geometry:

s.geometry.dispose();
@liyuanqiu

This comment has been minimized.

Copy link
Author

liyuanqiu commented May 23, 2017

@WestLangley If I set the canvas's size not power of 2 (for example 1000 x 1100), the memory usage still getting higher and higher, until it reached 4GB, then browser crashed.

@WestLangley

This comment has been minimized.

Copy link
Collaborator

WestLangley commented May 23, 2017

Do you mind correcting your fiddle, as I suggested?

Also, can you successfully duplicate the issue by modifying this three.js example to create a NPOT CanvasTexture?

@liyuanqiu

This comment has been minimized.

Copy link
Author

liyuanqiu commented May 23, 2017

Correcting as you suggested:
https://jsfiddle.net/liyuanqiu/wkdgt8h7/7/

Based on this three.js example
https://jsfiddle.net/liyuanqiu/j9Lzfu8g/1/
I only change the two lines:

canvas.width = 1000;
canvas.height = 1100;

in function createImage()

Both crashed.

@WestLangley

This comment has been minimized.

Copy link
Collaborator

WestLangley commented May 23, 2017

OK. That is what I expected.

@mrdoob, I expect this is related to the instantiation of a secondary POT canvas in the NPOT case.

@tommytee

This comment has been minimized.

Copy link

tommytee commented May 23, 2017

Here is my fix: tommytee@a61af5a

function makePowerOfTwo( image ) {

    if ( image instanceof HTMLImageElement || image instanceof HTMLCanvasElement ) {

      if ( ! POTCanvas )
        POTCanvas = document.createElementNS( 'http://www.w3.org/1999/xhtml', 'canvas' );

      POTCanvas.width = _Math.nearestPowerOfTwo( image.width );
      POTCanvas.height = _Math.nearestPowerOfTwo( image.height );

      var context = POTCanvas.getContext( '2d' );
      context.drawImage( image, 0, 0, POTCanvas.width, POTCanvas.height );

      console.warn( 'THREE.WebGLRenderer: image is not power of two (' + image.width + 'x' + image.height + '). Resized to ' + POTCanvas.width + 'x' + POTCanvas.height, image );

      return POTCanvas;

    }

    return image;

}

maybe POTCanvas should be declared somewhere else?

@WestLangley

This comment has been minimized.

Copy link
Collaborator

WestLangley commented May 23, 2017

@tommytee Does your solution work if there are multiple, concurrent, NPOT textures in the scene?

@tommytee

This comment has been minimized.

Copy link

tommytee commented May 23, 2017

Seems like it might be an issue.

a quick test worked fine. (3 textures)
https://vrview.io/branch/npot-multiple/examples/hotspots/index.html

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented May 23, 2017

I think it should work with makePowerOfTwo() because only gets called on single map upload. But clampToMaxSize() is used by cubemaps and this approach would definitely break.

https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLTextures.js#L248

I do like the solution though, I think it makes sense to only have one canvas.

I'll clean this all up.

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Jun 3, 2017

@tommytee Does your solution work if there are multiple, concurrent, NPOT textures in the scene?

Actually, I probably should check this with browsers...

@liyuanqiu

This comment has been minimized.

Copy link
Author

liyuanqiu commented Jun 18, 2017

Have you solved this problem?

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 18, 2017

@liyuanqiu did you try my fix?

@liyuanqiu

This comment has been minimized.

Copy link
Author

liyuanqiu commented Jun 19, 2017

@tommytee
An online little test using your fix:
http://139.196.28.110/npot/index.html

You can see the warning:
[Modified] THREE.WebGLRenderer: image is not power of two (240x260). Resized to 256x256
Notice that I added a "[Modified]" in front of this warning sentence to make sure that your fix is in use.

This page's memory usage increased rapidly.

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 19, 2017

When I go to that link my page memory stays stable and does not rise. ( latest Chrome, OSX )

Unless I open the console. I believe it is the console messages that are consuming the memory.
Try it with the console closed.

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 19, 2017

But the memory does go up for me when I use Firefox.
Could it be that Firefox is logging the console output even when the console is closed?

Try removing the warning from the code.

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 19, 2017

@liyuanqiu Is your code (fork of three.js) in a public repo?

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 19, 2017

I commented out the warning: //console.warn( 'THREE.WebGLRenderer: image is too big...

and can confirm there is no longer a memory issue with Firefox.

So far on OSX I have checked: Chrome, Safari, Opera and Firefox. None are having the memory issue.

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 19, 2017

And remember that demo is resizing 60 images per second, the warning-taking-up-memory issue won't be nearly as of as bad for normal use anyway.

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 19, 2017

@liyuanqiu

This comment has been minimized.

Copy link
Author

liyuanqiu commented Jun 20, 2017

@tommytee

Is your code (fork of three.js) in a public repo?

https://github.com/mrdoob/three.js/raw/dev/build/three.js
Replace the "makePowerOfTwo" function, add "var POTCanvas".

I change the two lines in index.html from:

canvas.width = 240;
canvas.height = 260;

to:

canvas.width = 2400;
canvas.height = 2600;

to use more memory, and commented the warning log.

The browser crashed in about 10 seconds, because of memory peak over than 4GB

Then I change to:

canvas.width = 1000;
canvas.height = 1100;

The memory peak keeps in about 1GB, and the valley is about 80MB, the memory usage curve is jagged in a high frequency.

Seems that your fix really taking effects.

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 20, 2017

@liyuanqiu Thanks. Are you getting the same basic results with all the browsers?

@tommytee

This comment has been minimized.

Copy link

tommytee commented Jun 20, 2017

@mrdoob for the clampToMaxSize canvas I created a canvas array and pass the loop index when calling clampToMaxSize

https://github.com/tommytee/three.js/blob/d1269b972b581646242a46af25b5da32d0aa8584/src/renderers/webgl/WebGLTextures.js

This demo sets the maxSize to 500 (within clampToMaxSize) for testing:
https://vrview.io/branch/canvas-test/examples/?q=cube#webgl_panorama_cube

@mrdoob mrdoob removed this from the r91 milestone Feb 14, 2018

@haolly

This comment has been minimized.

Copy link
Contributor

haolly commented May 22, 2018

It seems like this issue is still exist in r92, I changed canvas size to 1000 * 1000 in webgl_test_memory.html , chrome crash the page after 10s

@makc makc referenced this issue May 22, 2018

Closed

Texture.dispose not work #14109

3 of 13 tasks complete

@mrdoob mrdoob reopened this May 22, 2018

@mrdoob mrdoob added this to the r94 milestone May 22, 2018

@eliasasaid1

This comment has been minimized.

Copy link

eliasasaid1 commented Jun 21, 2018

this issue will later be resolved in the next milestone? because am suffering from it in an application am working on

@cherniavskii

This comment has been minimized.

Copy link
Contributor

cherniavskii commented Jul 16, 2018

For those who need workaround before patch is released.
Add this code before your threejs code:

// See https://github.com/mrdoob/three.js/pull/14483
const consoleWarn = window.console.warn;
window.console.warn = function() {
  // filter "image is not power of two" and "image is too big" warnings from three.js
  if (typeof arguments[0] === 'string' &&
     (arguments[0].includes('image is not power of two') || arguments[0].includes('image is too big'))) {
    // log it to console without second argument, which contains reference to `image`
    consoleWarn.call(null, arguments[0]);
  } else {
    // pass other warnings without changes
    consoleWarn.apply(null, arguments);
  }
};
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.