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

WIP Replace Jpeg Decoder #274

Merged
merged 156 commits into from
Sep 14, 2017
Merged

WIP Replace Jpeg Decoder #274

merged 156 commits into from
Sep 14, 2017

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Jul 10, 2017

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practise as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This is a WIP pull request to allow us to work collaboratively on a replacement for the current jpeg decoder.

While baseline encoding is fairly straightforward, decoding is much more difficult due to the required support for both baseline and progressive jpegs + correct handling of images encoded using buggy encoders.

Our current decoder can handle some cases but struggles with others. It's fairly fast but the code is extremely difficult to navigate due to the nature of the original Golang implementation. It also produces different output to other implementations with an error in the +-3 range per pixel component. There are hints of a memory leak there also. #151 #224

The new jpeg decoder is based upon a jpeg implementation found in Mozilla's PDF.js with several key enhancements.

  1. Fast table lookups for colorspace transforms
  2. Better handling of broken images
  3. Faster Huffman and IDCT implementations

We can now decode the images in #159 correctly throw in #214 and produce much closer output to the reference implementations in #155 and #245

However, the new decoder is 2X slower that our original and will require optimization. To do that we need to do the following:

  1. Modularize the code more to allow improved testing
  2. Perform unit and load testing on the decoder components
  3. Optimize the decoder based on test findings.

Decoding a jpeg has two areas of high complexity which require extreme optimization: Huffman decoding and Inverse Discrete Cosine Transform; we can make large gains in performance in focusing our efforts there.

The Huffman decoder requires an accurate fast lookup table implementation See here and here for a reference implementation in Java and IDCT can be sped up using SIMD. (I would like to see if we can do it using Vector<short> rather than the Block8x8F implementation we have in the current decoder to keep memory usage down. Perhaps @mellinoe can give us some hints there?)

Please have a good dig through the code. If you have any questions please ask. I'd like us to get a much better understanding of the format than we currently have.

Update by @antonfirsov

I'm on a bit different and more critical opinion towards this change, and I also have a plan to speed up the decoder while keeping it correct and memory friendly. I'm quite sure there is no easy way for this, and I really appreciate any help you can provide with the tasks I managed to identify.

@antonfirsov antonfirsov mentioned this pull request Aug 18, 2017
@antonfirsov
Copy link
Member

antonfirsov commented Aug 18, 2017

For everyone's information:
The new decoder should be based on this port, because the stream parsing and the CPU-intensive processing logic is clearly separated here. This makes work on progressive jpeg-s easier. So good job @JimBobSquarePants, this decoder is a really-really good thing! (Despite it's messy and slow IDCT and colorspace transformation and logic, which has to be replaced :P)

However, I suggest to open a new clean WIP PR, removing the noise we gathered here. I could describe my implementation plan there, and track the work. I also wiped the out the description in #192 to keep all information up-to-date.

There are some possibilities for incremental work. We can release our beta with the initial slow variant of the PdfJs decoder (if we find it good enough for a beta), and improve performance later. We need to keep all the classes of the original golang port in the repository though, to keep the useful logic and tests refactored as we add changes.

@JimBobSquarePants thoughts?

@JimBobSquarePants
Copy link
Member Author

Hey @antonfirsov ! The colorspace stuff I thought was pretty good! 😝

I agree, Let's close this PR and open a new one. Make sure we capture the EXIF fixes I added also though.

I think the PdfJS based decoder is good enough for beta despite the comparative decrease in speed. It can already successfully decode every single rogue image across all our jpeg issues and produces a very similar output to libjpeg. Make it work; make it work fast.

During beta we can tackle performance issues and ensure we don't regress using your new qa lab code.

So let's do two PR's, one to switch out the decoder, then a second WIP one for performance improvements.

@antonfirsov
Copy link
Member

@JimBobSquarePants I just merged jpeg-port into my jpeg-lab branch, organizing tests+classes+namespaces in a cleaner way.

For a while I'm planning to do refactors that keep both decoders working, so we can just merge back jpeg-lab into jpeg-port and use this (#274) PR for the switch-out process.

@JimBobSquarePants
Copy link
Member Author

Great! 👍

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

Successfully merging this pull request may close these issues.

5 participants