Questions about p5.RendererGL.getTexture()

[aferriss]

trafficstars

I think there may be something off with the implementation of the getTexture() function. The function looks like this:

p5.RendererGL.prototype.getTexture = function(img) {
  const textures = this.textures;

  for (const texture of textures) {
    if (texture.src === img) return texture;
  }

  const tex = new p5.Texture(this, img);
  textures.push(tex);
  return tex;
};

However, nowhere in the renderer or anywhere else in the codebase are we ever pushing anything into this.textures, so the loop will never do anything, since we're not storing textures anywhere.

Moreover, I'm suspicious of that if statement inside of the loop. Even if we were caching our textures, it seems like that check is testing to see if the Image objects are the same, which I think may be likely to be false 100% of the time, and even if they were the same, wouldn't this be a very expensive operation?

Maybe we can think of some better ways to cache and check for pre-existing textures here, or maybe we can just remove some of the other lines in there so that the function just creates and returns a new texture, until we figure out a better solution.

@stalgiag Any ideas?

[stalgiag]

@aferriss that is interesting. I did some digging and most of this logic appears to be introduced with #2112. The conversation in that PR makes the purpose of the function and intended performance trade-off a little more clear but I have to admit that I still don't fully understand. I think that when a Sampler2D uniform is set, a new texture is created if it doesn't already exist in the textures array on the Renderer. This seems to have been done to allow sharing of images across different contexts but I don't quite follow that part.

What do you think about doing the following? A) Rename textures and getTexture() to reveal more about their specific uses. B) Add an id property to p5.Texture so that the search can be more efficient.

[davepagurek]

Hi, I've spent a bit of time optimizing WebGL renders in P5 for work, so I've taken a look at the performance of textures recently!

I think that when a Sampler2D uniform is set, a new texture is created if it doesn't already exist in the textures array on the Renderer.

This is also how I understand this function!

Moreover, I'm suspicious of that if statement inside of the loop. Even if we were caching our textures, it seems like that check is testing to see if the Image objects are the same, which I think may be likely to be false 100% of the time, and even if they were the same, wouldn't this be a very expensive operation?

I can confirm that it's hitting the early return. My main use cases are drawing p5.Image objects, p5.Graphics objects, and video p5.MediaElement objects, and all of these are persistent objects, so object equality checks work for them.

There's a noticeable performance impact when calling gl.texImage2D to update data on the GPU. p5.Graphics objects, for which we can't (easily) detect changes in and need to do this every frame, are significantly slower on Firefox than p5.Images, which only have to update GPU data once thanks to caching of textures.

Maybe we can think of some better ways to cache and check for pre-existing textures here

Essentially what the existing code is doing is looking up a p5.Texture based on a source object. While a Javascript object can only have string keys, a Javascript Map object can have anything as keys. Maybe a minimal change could be to replace the array with a Map?