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

Jpeg loading refactor #435

Closed
wants to merge 7 commits into from
Closed

Jpeg loading refactor #435

wants to merge 7 commits into from

Conversation

theuni
Copy link
Contributor

@theuni theuni commented Sep 17, 2011

On all platforms other than IOS, we use cximage to load jpegs. It is incredibly slow. Using libjpeg(-turbo) directly results in a speedup of around 2x on average, and much higher for larger images.

I believe i have osx/win32/ios/linux acks already, just wanted to do a quick pr to be sure that no one had any fundamental concerns. @jmarshallnz We discussed this a while ago when I first started poking at it, and I believe you were ok with it as long as the exif orientation was working. It currently uses the libexif that we build, and is verified working, but I'd like to rip that out and add a quick function to grab it from our source imagebuffer, that way we avoid an unnecessary file open.

Other than that, I'm quite confident in this as a drop-in for what we have. If decode fails for any reason, it falls back gracefully to cximage.

Will push this in a day or two barring a good reason not to.

theuni and others added 7 commits September 13, 2011 23:46
Should be supported by all GL.
Thanks to CrystalP for the help with DirectX
TODO: Get rid of the need for libexif, this is simple enough to do ourselves.
TODO: Add a simple encoder for writing files
@amejia1
Copy link
Contributor

amejia1 commented Sep 18, 2011

As an aside, maybe you'll want to look at replacing cximage entirely with graphicsmagick (http://www.graphicsmagick.org/). I hear they are like "libav" but for still images. graphicsmagick has things desirable to us, namely, a much more active upstream, an upstream that maintains support for every conceivable device on the planet (at least they claim), and Python bindings. There's probably other things too, but this is things that stood out to me skimming through their website.

graphicsmagick can use libjpeg for jpeg support, so I imagine libjpeg-turbo could also be used.

@amejia1
Copy link
Contributor

amejia1 commented Sep 18, 2011

Ok, so apparently, imagemagick has gone through a bunch of improvements since I last checked and looks like a better alternative to graphicsmagick now (www.imagemagick.org). They have IOS support and PythonMagick uses them now, and they use libjpeg for jpeg support.

@theuni
Copy link
Contributor Author

theuni commented Sep 18, 2011

Sounds good to me, anything to rip out cximage. But beware, cximage uses libjpeg too. It's the overhead of the lib itself that causes the slowdown.

@topfs2
Copy link
Contributor

topfs2 commented Sep 18, 2011

I did a patch which used magick instead of cximage but it was a nightmare for windows apperantly, it worked just fine for linux (probably osx too) though. http://trac.xbmc.org/ticket/8992

@theuni
Copy link
Contributor Author

theuni commented Sep 18, 2011

I asked @topfs2 quickly on IRC if imagemagick would be a possibility:
" well as I said I think magick is a no-go on windows"

If we can abstract this away in the future with some cross-platform lib that doesn't sacrifice too much speed, you certainly have my blessing to revert this. But for now, the gains are too big to pass up. And since we're not introducing any new dependencies, I don't think it can hurt.

@theuni
Copy link
Contributor Author

theuni commented Sep 18, 2011

Pushed in b8b6eec.

@theuni theuni closed this Sep 18, 2011
@amejia1
Copy link
Contributor

amejia1 commented Sep 18, 2011

I think it's worth another look (imagemagick I mean) as far as windows support goes. It seems activity has picked up for imagemagick, and whatever problems that might have existed before might have been addressed by now.

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.

None yet

3 participants