Out-by-one error in flipPixelsVertically

[diarmidmackenzie]

https://github.com/aframevr/aframe/blob/v1.7.0/src/components/scene/screenshot.js

  flipPixelsVertically: function (pixels, width, height) {
    var flippedPixels = pixels.slice(0);
    for (var x = 0; x < width; ++x) {
      for (var y = 0; y < height; ++y) {
        flippedPixels[x * 4 + y * width * 4] = pixels[x * 4 + (height - y) * width * 4];
        flippedPixels[x * 4 + 1 + y * width * 4] = pixels[x * 4 + 1 + (height - y) * width * 4];
        flippedPixels[x * 4 + 2 + y * width * 4] = pixels[x * 4 + 2 + (height - y) * width * 4];
        flippedPixels[x * 4 + 3 + y * width * 4] = pixels[x * 4 + 3 + (height - y) * width * 4];
      }
    }
    return flippedPixels;
  },

The pixels array starts at 0,
y ranges from 0 to (height - 1)

So when flipping pixels, the pixel at y= 0 should go to (height -1), and the pixel at y = (height - 1) should go to 0.
But with the current code, when y = 0, we take a pixel from y = (height - 0), which is out of bounds (undefined)
And when y = (height - 1), we take a pixel from y = (height - (height -1) = 1

So the image is getting flipped, but also shifted up by 1 pixel.

This all happens in the screenshot process. Subsequently, we sometimes hit an exception here:

imageData = new ImageData(new Uint8ClampedArray(pixels), size.width, size.height);

Failed to construct 'ImageData': The input data length is not a multiple of 4.

Consulting with Claude, it suggests that this could happen as a result of adding undefined values to the array:

When you create a Uint8ClampedArray from an array containing undefined values, the resulting array can end up with an incorrect length or corrupted data. The ImageData constructor then validates that the data length equals width × height × 4, and throws the error when it doesn't match.

I have only seen this exception twice in the last week, and never before, so I don't fully understand what's going on here. But the out-by-one error in the image flip looks like a genuine bug, and putting undefined values into the Uint8ClampedArray is probably not a good thing to do.

I will submit a PR to fix this.

[mrxz]

Why are we flipping the rows on the CPU? If we create an ImageBitmap out of the pixel data and use a bitmaprenderer context, we can let the browser handle the flip. Something along the lines of:

imageBitmap = await createImageBitmap(imageData, { imageOrientation: 'flipY' });
canvas.getContext('bitmaprenderer').transferFromImageBitmap(imageBitmap);
canvas.toBlob(function (blob) { /* save to file */ }, { type: 'image/png' });

[diarmidmackenzie]

Why are we flipping the rows on the CPU? If we create an ImageBitmap out of the pixel data and use a bitmaprenderer context, we can let the browser handle the flip. Something along the lines of:

imageBitmap = await createImageBitmap(imageData, { imageOrientation: 'flipY' });
canvas.getContext('bitmaprenderer').transferFromImageBitmap(imageBitmap);
canvas.toBlob(function (blob) { /* save to file */ }, { type: 'image/png' });

That's probably better in most cases, but level of change seems risky compared to the simple fix I proposed, and I'm not confident I could anticipate all the possible side effects.

Is it possible that an experience could be GPU-bound, and this change might cause frame drops from screenshots that wouldn't happen with the current implementation?

I'm not against making the change you suggest, but the intention of my fix was to fix a simple bug where a line of bad pixels ends up at the top of screenshots, and I'm nervous about unintended consequences of a more substantial change to how screenshots are taken.