Skip to content

Conversation

neildavis
Copy link
Contributor

This commit adds bilinear interpolation scaling methods to Image. In the process, several issues with Image were found & fixed:

  • There was no destructor to free the canvas memory. This would cause a memory leak unless the client remembered to call close() A virtual destructor has been added to free this memory.

  • There was no explicit copy constructor for Image This means whenever Image was copied or passed by value a 'shallow copy' was performed resulting in both instances referencing the same shared canvas memory. This works, and several examples were depending on it, but after the addition of the destructor it caused seg faults since the memory was free()'d several times. An explicit copy constuctor for Image has been added to perform a 'deep copy' of the canvas memory.

  • Several examples and the DisplayNeoPixel::showImage() methods were passing Image instances by value. This is now inefficient due to the deep copy. Since pass-by-reference semantics are required in these cases they are now passed by reference.

  • New c'tors and getters added to Color to support RGBA access as required by the new scaling method on Image Also tidied up c'tors to use common helper methods.

  • Used new/delete instead of malloc/free for heap memory use in Image Just coz it's more conventional in C++

This commit adds bilinear interpolation scaling methods to `Image`.
In the process, several issues with `Image` were found & fixed:

* There was no destructor to free the `canvas` memory. This would
  cause a memory leak unless the client remembered to call `close()`
  A virtual destructor has been added to free this memory.

* There was no explicit copy constructor for `Image`
  This means whenever Image was copied or passed by value
  a 'shallow copy' was performed resulting in both instances
  referencing the same shared canvas memory.
  This works, and several examples were depending on it, but
  after the addition of the destructor it caused seg faults since
  the memory was free()'d several times.
  An explicit copy constuctor for `Image` has been added to
  perform a 'deep copy' of the `canvas` memory.

* Several examples and the `DisplayNeoPixel::showImage()` methods
  were passing `Image`instances by value. This is now inefficient due
  to the deep copy.
  Since pass-by-reference semantics are required in these cases they
  are now passed by reference.

* New c'tors and getters added to `Color` to support RGBA
  access as required by the new scaling method on `Image`
  Also tidied up c'tors to use common helper methods.

* Used new/delete instead of malloc/free for heap memory use  in `Image`
  Just coz it's more conventional in C++
@neildavis
Copy link
Contributor Author

Closing as further work has identified a potential issue I need to resolve first. Will reopen when resolved.

@neildavis neildavis closed this Oct 20, 2022
@wryan67
Copy link
Owner

wryan67 commented Oct 21, 2022

I really liked the idea of using the & (pass-by-reference) to speed up image processing. There were a lot of changes in this pull request and I've still not had time to review all of them. I may have some time this weekend, so I could work on the draw functions to switch them to pass-by-reference, but I'll wait if you'd rather incorporate the changes into your next pull request.

@wryan67
Copy link
Owner

wryan67 commented Oct 21, 2022

BTW, one thing I've been wanting to work on is true-type fonts, wouldn't that be awesome! If only I had a clone of myself to work on Raspberry Pi projects all day long while the other me goes to my regular job. sigh.

@neildavis
Copy link
Contributor Author

neildavis commented Oct 21, 2022

I really liked the idea of using the & (pass-by-reference) to speed up image processing.

As it stands (prior to this PR), passing Image by value vs reference is not vastly less efficient, since a 'shallow copy' of the canvas pointer is made, so just a few bytes are copied. It's still preferable to pass by reference, from both a semantic and performance perspective, but it wouldn't massively affect performance. This PR introduced a copy c'tor (but omitted an assignment operator - doh! - I'll fix that) which would perform a 'deep copy' of the canvas buffer using memcpy(), so passing by value with that would be vastly less efficient since the buffer would be copied every time an instance is passed by value (or possibly returned by value, although I'd hope and assume most modern compilers would optimize it away on return!)

The reason I closed this is that I was doing further work to add arbitrary Image rotation using the 'three shear' method. After modifying the example demo to use this I see some random 'double free or corruption' memory issues, which I haven't nailed down the cause yet. Whilst I can't reproduce it using the scale methods in this PR, I also can't see how it is caused by the new rotation code, and thus think the issue probably does lie somewhere in the changes in this PR. Err'ing on the side of caution I decided to close the PR.

but I'll wait if you'd rather incorporate the changes into your next pull request.

Your call. I hope to open a new PR soon, but rebasing a few pass-by-ref changes shouldn't cause much disruption.

@neildavis
Copy link
Contributor Author

Opened #3 which supercedes this PR. Few more changes in that one since I had to 'chase through' the const changes throughout the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants