Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement an image scaling function. #98

Merged
merged 7 commits into from
Mar 30, 2023

Conversation

IcaroJam
Copy link
Contributor

The mlx_scale_image function first uses mlx_resize_image to create a copy of the given dimensions, then copies the raw pixel data of the original image into the new one, skipping or repeating pixels in order to make make the scaled image as faithful to the original as possible.

I've tried keeping readability and performance in mind, but some things could surely be improved.

@W2Wizard W2Wizard added the Improvement Adds an implementation or improves on something label Mar 30, 2023
@W2Wizard
Copy link
Collaborator

Thank you for this PR! ✨

I think this is worthy of a merge, my only question is if it might be better to just merge mlx_resize_image and mlx_scale_image into one because I feel like this might cause a bit of confusion.

Afterall I think for most people the common sense thought would be that resizing the image also scales the content along with it. So having two functions do roughly the same but not quite seems a bit unnecessary to me.

Additionally if you could provide some unit tests that would be great, if not I'll add them later myself.

@IcaroJam
Copy link
Contributor Author

I myself had the problem of thinking resize would also scale the function when I first used it, so merging both seems a great idea to me!
I can take care of making both into a single function, or leave it to you if you'd prefer doing it yourself, just let me know :)

About the unit tests, I've never written proper ones, but I'll see what I can do.

@W2Wizard
Copy link
Collaborator

W2Wizard commented Mar 30, 2023

Alright, please merge them then I'll review the code :D

Regarding tests that's alright, I need to do them anyway. Atm there's not enough of them.

@IcaroJam
Copy link
Contributor Author

IcaroJam commented Mar 30, 2023

Done!
Had to change resize's realloc to a calloc for simplicity purposes though (creating a temporal copy of the original buffer seemed like too much work just to keep the realloc, specially since I don't see any benefits of using it over calloc in this case).

include/MLX42/MLX42.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@W2Wizard W2Wizard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it myself, works great!

@W2Wizard W2Wizard merged commit cf637ec into codam-coding-college:master Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Adds an implementation or improves on something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants