Fix case where cropping could be wrong

[MrStevns]

A potential fix for #1940

Though i haven't been able to reproduce the behavior myself. I looked into the cropping method, took a deep dive into QImage and noticed the way we move the cursor, which could lead to this kind of issue.

I suspect that the bug is related to the auto cropping logic, as the user points out it happens after the project has been saved and re-opened.

By going through scanline for each column, we ensure that image is traversed correctly, no matter the padding.

[scribblemaniac]

I think this is a more correct way to iterate over the pixels given that I can't find anything in the QImage documentation explicitly saying what the layout of the QImage data buffer is. So I am in favor of this change.

However, I don't think there's any chance this will fix the problem of #1940. This code will only behave differently than the original code if cursor += width isn't the memory for the pixel immediately below the current pixel. If that's the case, then one of three things can happen:

1. The memory location at cursor+width is not allocated to Pencil2D and the application crashes with a segfault. The user didn't mention the application crashing, and I don't recall any reports of v0.7.0 crashing so this can't be what happened in 🐛 [BUG] - Frame saving/importing movement glitch #1940 and is generally unlikely.
2. The memory location at cursor+width is part of the image data or some other memory allocated to Pencil2D, and the data at that address casts to a QRgb with alpha != 0. Autocrop stops cropping one side prematurely, and the frame is saved with some transparent columns on the side. Crucially, the top left position of the image is adjusted based on the number of rows/columns cropped, not the actual minimum bounds; so even if there's more "padding", that padding will never push the image to the side.
3. Same as scenario 2, but the data casts to a QRgb with alpha = 0 and all other pixels in the column also happen to map to values with alpha = 0. Autocroping potentially crops columns on the side that contain content.

In all cases, nothing would result in a translation of the frame. If autocropping was the cause of that issue, I think the cause would have to be in BitmapImage::updateBounds or the values passed to it. Reviewing that part of the code, I don't see anything obviously wrong.

[scribblemaniac]

If we're going to call constScanLine for what is effectively every pixel tested in the left and right side cropping, I'm wondering if it might be faster to do something like this (pseudocode):

minLeft = image.width;
for (y = 0; y < image.height; y++) {
  cursor = image.constScanLine(y);
  for (column = 0; column < minLeft; column++) {
    if (qAlpha(*cursor) != 0) {
      minLeft = column;
      break;
    }
    cursor++;
  }
}

In words, it's finding the minimum span of transparent pixels at the start/end of each row. With this we would only need to call constScanLine once per row (per side probably) instead of once per pixel. There might also be benefits from the sequential memory access. However for certain shapes this will end up iterating over more pixels overall (in a bad case imagine an upside down "T" that fills the image).

Thoughts?

[MrStevns]

I'm thinking that we should probably test out if it would be worth the added complexity before making such a change but it's an interesting thought that might be worth looking into.

[scribblemaniac]

Agreed. Autocrop gets called quite frequently (though most of those calls will probably exit early with the mMinBound check) so I think it's worth looking into potential performance improvements. Benchmarking is the way to go, but I'm not sure what a representative sample of test cases would look like.

[MrStevns]

It may not have to be called so often anymore though since we now have the tiled buffer to draw the stroke. The image should ideally only have to be modified on ScribbleArea::endStroke (when the scenario is drawing on the canvas).

I'd really like to be able to get rid of the many autocrop calls from within BitmapImage, in order to get a clearer picture of when we actually need to modify the image.