"pixels" array cannot be reassigned

[limzykenneth]

Most appropriate sub-area of p5.js?

• [ ] Accessibility (Web Accessibility)
• [ ] Build tools and processes
• [ ] Color
• [x] Core/Environment/Rendering
• [ ] Data
• [ ] DOM
• [ ] Events
• [ ] Friendly error system
• [ ] Image
• [ ] IO (Input/Output)
• [ ] Localization
• [ ] Math
• [ ] Unit Testing
• [ ] Typography
• [ ] Utilities
• [ ] WebGL
• [ ] Other (specify if possible)

Details about the bug:

• p5.js version: 1.2.0
• Web browser and version: Firefox 84.0.2 (should apply to all supported browsers)
• Operating System: Linux Pop_OS 20.10
• Steps to reproduce this:

Visit this example sketch. The initial expected behaviour is a blank canvas because the img.pixels array has been assigned with the .map function to be all 0 but the image is still drawn. Replacing the .map with a for loop works as expected.

Filing this as a bug because this behaviour is different from the canvas itself (see by uncommenting code at the end of the draw() function), which reassigning the pixels array with .map still works.

The problem seems to be outdated variable referencing in p5.Image, possibly affecting p5.Graphics as well but I didn't test.

[limzykenneth]

Actually it seems to affect the canvas's pixels array as well.

[rt1301]

@limzykenneth I would like to work on this. Can you suggest me how to get started with it?

[limzykenneth]

This one is potentially a bit complicated and it still need to be determined as an actual bug first, ie. the behaviour is something that should be fixed and not something that's intended. To fix this will require looking at how updatePixels() is getting the pixels array to update in the relevant canvas image data array.

[stalgiag]

When I test on Mac OS 10.15.7 with Firefox 84.0.2 the image is transparent with the .map modification of its pixels. For example in this slight modification of your example, I only see blue. Am I missing something?

[limzykenneth]

@stalgiag If I change the second to last line in your modified sketch and instead of mapping all values to 0, map all to 255, I still only see blue where I'm expecting a white square on blue background.

[stalgiag]

Definitely should be a white square and yes it seems to happen on both the canvas and the image when using map.

[limzykenneth]

My theory is that pixels is setup to be a reference to the underlying image data array and by reassigning it the reference is replaced and updatePixels() is still looking at the underlying image data instead of the pixels variable. I haven't look much into the code base so can't say much about what the fix could be though.

[ahujadivyam]

@limzykenneth Yes, this seems to be the case. pixels is just reference to imageData data and reassigning it will just reassign the reference not change the actual data. https://github.com/processing/p5.js/blob/f2f8bb12e4969e620b1d397f3e386289bc1a73b6/src/core/p5.Renderer2D.js#L292 https://github.com/processing/p5.js/blob/f2f8bb12e4969e620b1d397f3e386289bc1a73b6/src/core/p5.Renderer2D.js#L395

[amanMahendroo]

Hi. This does not seem to be a bug in the code. The reason that the pixel update doesn't work is that the img.pixels array is a UInt8ClampedArray and not a regular javascript array. Hence it doesn't have the Array.prototype.map() function associated with it. You can update the image's pixels by using a for-loop or by using the set() function in the p5.js library. The blue screen that appears on using the for loop seems to be a bug in the p5.js web editor as this doesn't seem to happen in other editors or in the browser. Hope this helped 😊

[limzykenneth]

UInt8ClampedArray does have a map method but that is not the key for this issue. Even not using map() (which is my original use case but it's too complicated to share), I can create a new Uint8ClampedArray say called newPixels, if I assign pixels = newPixels, updatePixels() will not populate the canvas with the values I set in newPixels.

[ahujadivyam]

UInt8ClampedArray does have a map method but that is not the key for this issue. Even not using map() (which is my original use case but it's too complicated to share), I can create a new Uint8ClampedArray say called newPixels, if I assign pixels = newPixels, updatePixels() will not populate the canvas with the values I set in newPixels.

@limzykenneth As I mentioned in above comment https://github.com/processing/p5.js/issues/4993#issuecomment-770212088 , I think pixels (property / variable ) is just a reference to image data array which renderer is using internally and updating using that, and reassigning pixels array with map() or something else just makes pixels reference to other array and internal image data array is not getting affected

[limzykenneth]

@DivyamAhuja Yes, that's what I was saying.

[ahujadivyam]

@DivyamAhuja Yes, that's what I was saying.

Yes, fixing this can be troublesome, exposing internal data array to user, and I don't know if user assigns array of wrong size, will it create unnecessary error too.

[ahujadivyam]

Maybe, we can add a check if pixels array is pointing to imageData.data , and if it's not we can add some checks about size and all , if alright will copy data to imageData.data array else show user an error.

[limzykenneth]

I'm thinking instead of referencing imageData directly here we can just somehow retrieve the pixels variable via something like this.pixels and just create a new imageData object to pass in. The code will be simpler as there's no need for additional identity checks.

Checking the pixels array provided and using FES to give comprehensible error isn't that hard as the required size of the array can be calculated from the dimension of the canvas and the pixel density, checking the type is also relatively easy as well and if required we can even do type conversions automatically so a conventional array can be used instead of strictly a UInt8ClampedArray.

[amanMahendroo]

I would like to work on this. Could you please suggest how I should get started?

[limzykenneth]

@amanMahendroo This is a bit of an advanced problem but if you are interested, you can look at how loadPixels(), pixels, and updatePixels() are implemented to see how we can solve this problem. Thanks.

[ahujadivyam]

@limzykenneth I tried to create a new ImageData object if this.pixels and imageData.data were not referencing to same Uint8ClampedArray and ImageData object will be replaced. This won't affect normal for loop modification.

https://editor.p5js.org/DivyamAhuja/sketches/b-5n_pwAH

  if (pixelsState.pixels !== pixelsState.imageData.data) {
    let imgData;
    if (pixelsState.pixels instanceof Uint8ClampedArray) {
      if (pixelsState.pixels.length === pixelsState.imageData.data.length) {
        imgData = new ImageData(
          pixelsState.pixels,
          pixelsState.imageData.width,
          pixelsState.imageData.height
        );
      } else {
        let imgArr = Array.from(pixelsState.pixels);
        imgArr.length = pixelsState.imageData.data.length;
        imgData = new ImageData(
          Uint8ClampedArray.from(imgArr),
          pixelsState.imageData.width,
          pixelsState.imageData.height
        );
      }
    } else {
      if (Array.isArray(pixelsState.pixels)) {
        pixelsState.pixels.length = pixelsState.imageData.data.length;
        imgData = new ImageData(
          Uint8ClampedArray.from(pixelsState.pixels),
          pixelsState.imageData.width,
          pixelsState.imageData.height
        );
      }
    }
    pixelsState._setProperty('imageData', imgData);
    pixelsState._setProperty('pixels', imgData.data);
  }

[limzykenneth]

@DivyamAhuja I don't quite understand why a check if pixels and imageData.data is the same would be necessary. I imagine whatever the case may be, pixels is the source of truth and we can simply use it to construct the new image data object to be set by the canvas.

[ahujadivyam]

@limzykenneth I was thinking like if someone wants to use normal for loop and pixels array is referencing to imageData.data , then there wouldn't be any need for creating new ImageData object, and object will only be created when pixels array will be reassigned to something else. I think this will enhancement without affecting current working of updatePixels function.

[ahujadivyam]

@limzykenneth @stalgiag should I then submit a PR based on it or further discussion is to be done about tweaks or implementation?

[limzykenneth]

@DivyamAhuja If possible can do you a simple benchmark of whether creating a new ImageData object when calling updatePixels() vs checking if pixels and imageData.data is the same, has significant performance difference? I would like to keep the overall implementation simple, ie. just one simple implementation instead of branching into two possible implementations, but if there is significant performance difference between the two, we may need to rethink a bit.

[ahujadivyam]

@limzykenneth I was not sure how to benchmark so,

I ran this code

let img;
function preload() {
  img = loadImage(
    "https://upload.wikimedia.org/wikipedia/commons/thumb/b/b6/Sossusvlei%2C_Namibia%2C_2018-08-06%2C_DD_017.jpg/305px-Sossusvlei%2C_Namibia%2C_2018-08-06%2C_DD_017.jpg"
  );
}
function setup() {
  createCanvas(100, 100);
  background(220);
  img.loadPixels();
  let startTime = performance.now();
  for (let i = 0; i < 10000; i++) {
    img.pixels = img.pixels.map((x, i) => {
      pix = i / 4;
      row = pix / img.width;
      col = pix % img.width;
      if (col >= img.width / 3 && col <= (2 * img.width) / 3) {
        if (row >= img.height / 3 && row <= (2 * img.height) / 3) {
          if (i % 4 == 3) return 255;
          if (i % 4 == 0) return 255;
          return 0;
        }
      }
      return x;
    });
    img.updatePixels();
  }
  let endTime = performance.now();
  console.log(endTime - startTime);
}

with both with and without checks

<!DOCTYPE html>
<html lang="en">
  <head>
    <link rel="stylesheet" type="text/css" href="style.css" />
    <meta charset="utf-8" />
    <script src="p5-withChecks.js"></script>
  </head>

  <body>
    <script src="sketch.js"></script>
  </body>
</html>

for without checks , I used this code to update imageData

  let imgArr = Array.from(pixelsState.pixels);
  imgArr.length = pixelsState.imageData.data.length;
  let imgData = new ImageData(
    Uint8ClampedArray.from(imgArr),
    pixelsState.imageData.width,
    pixelsState.imageData.height
  );

and results were: Without Checks: about 265210.20000000135 ( 230k - 265k on my browser) For With Checks: about 144864.13500001072 (130k - 144k on my browser)

[ahujadivyam]

@limzykenneth Should we do it like , loadPixels will only be used to load current pixel data into pixels array and updatePixels will update imageData and put it on image based on pixels array regardless of loadPixels being called earlier or not.

[ahujadivyam]

@limzykenneth So, How should we proceed with this one?

[limzykenneth]

@DivyamAhuja I think you can go ahead with an implementation you think is most appropriate. We'll work out the final details in the PR. It's easier for me to test code available in a PR anyway. Thanks!