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

Refactored JuicyPixels to use Data.Vector #6

Merged
merged 3 commits into from
Jan 16, 2012
Merged

Conversation

dagit
Copy link
Contributor

@dagit dagit commented Jan 15, 2012

Hello,

I really like your Juicy Pixels library. I would like to use it for games and visualizations in OpenGL. Please allow me to justify why I refactored your code to use Data.Vector:

  • OpenGL expects Ptr a for the data it accepts. The standard Haskell Arrays cannot support this without copying the image data elsewhere, which is O(ImageSize) of course.
  • Data.Vector is often faster than the normal Arrays due to clever implementation tricks.
  • Data.Vector is easier to use with Repa, which would then allow your users to do data parallel computations quite easily on their images.

I have not removed all uses of Arrays from your code. I simply started at the base types that you define and I converted things to Data.Vector.Storable and Data.Vector.Storable.Mutable as needed until I could compile and run the tests. I've tried to clean up warnings but it's possible I missed some as my GHC is spewing out lots of SpecConstr messages. More work could be done in the future to remove all uses of Array, but I didn't see that as important.

I ran the test suite and as far as I could tell I did not break anything. I have included a fix for issue #5. I also had to add primitive and vector as dependencies. Primitive is a dependency of vector and I needed it to give types to some of the functions. You might want to check the ranges I specified in the cabal file. I have only tested on Windows but I have no reason to believe that I broke anything on other platforms.

I have used unsafeFreeze in quite a few places, but I believe those uses to be safe. The unsafe part just means that you should not continue to mutate the data via an existing handle. I'm just trying to reduce copying the data, so if any use of unsafeFreeze looks unsafe please replace it with freeze.

Thank you for releasing JuicyPixels!

Jason

@TomMD
Copy link

TomMD commented Jan 15, 2012

If this is merged I will port JuicyPixels-Repa, allowing us to obtain the Repa array in O(1). No porting is needed, strictly speaking, as it appears the high level (per-pixel) interface has been maintained.

@Twinside
Copy link
Owner

This is impressive, there is a serious speedup while loading huge.png, I'm stunned :)

That being said, I won't merge it directly, some png images seems to be broken when bit depth is 1, 2 or 4. But it will definitively be merged somehow.

The free speedup is incredible :)

Twinside added a commit that referenced this pull request Jan 16, 2012
Refactored JuicyPixels to use Data.Vector
@Twinside Twinside merged commit a5e4d0e into Twinside:master Jan 16, 2012
@dagit
Copy link
Contributor Author

dagit commented Jan 16, 2012

I can help fix the png breakage if you tell me how to reproducibly test it.

The first thing I would try as a debug is to change all the uses of
unsafeFreeze to freeze to see if I screwed that up.

Jason

On Mon, Jan 16, 2012 at 9:58 AM, Vincent
reply@reply.github.com
wrote:

This is impressive, there is a serious speedup while loading huge.png, I'm stunned :)

That being said, I won't merge it directly, some png image seems to be broken when bit depth is 1, 2 or 4. But it will definitively be merged somehow.

The free speedup is incredible :)


Reply to this email directly or view it on GitHub:
#6 (comment)

@Twinside
Copy link
Owner

Don't worry, I already fixed the problem in the commit d67b761

@dagit
Copy link
Contributor Author

dagit commented Jan 16, 2012

Great! Sorry about getting the indexes wrong. Looks like there were
places where I should have changed from (0, N) to (N + 1) but I
accidentally did (0, N) to N. Easy mistake to make unfortunately :(

Thanks!
Jason

On Mon, Jan 16, 2012 at 11:18 AM, Vincent
reply@reply.github.com
wrote:

Don't worry, I already fixed the problem in the commit d67b761


Reply to this email directly or view it on GitHub:
#6 (comment)

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.

3 participants