Add the ability to scale down images during decode (IDCT scaling) #117
Conversation
Currently only adds 1/8 scale because the 1x1 IDCT is trivial, but this adds the infrastructure to easily support the others.
The reference images were generated with djpeg: djpeg -scale 1/8 tests/reftest/images/rgb.jpg | convert - tests/reftest/images/rgb_63x42.png djpeg -scale 2/8 tests/reftest/images/rgb.jpg | convert - tests/reftest/images/rgb_125x84.png djpeg -scale 4/8 tests/reftest/images/rgb.jpg | convert - tests/reftest/images/rgb_250x167.png
|
👍 on this, this is a great feature to have ! |
|
Failing tests are due to the use of the |
|
I like the idea of exposing this functionality. Would it be possible to control the scale factor via a separate modifier function on Decoder rather than making a new constructor? That way the user would be able to see the metadata of the image before deciding whether to try rescaling, and would also be compatible with adding other similar sorts of functionality without an exponential blowup in the number of constructors required. I'm also not completely sold on taking minimum output dimensions as a argument. If the only possible downscale factors are 2, 4 and 8, why can't we just have the user pass one of those directly? Another point is that I'd expect users would often either want to decode to exactly the target size or to something substantially larger so that they can get a nicer downsampled result? |
Sure, I'll try that. It will take some refactoring of
It's possible to add other scale factors N/8 by adding a NxN IDCT. |
|
I also think the interface that exposes Dimensions will both be easier to use and less likely to require a breaking change in the future. |
src/decoder.rs
Outdated
| /// Creates a new `Decoder` using the reader `reader` that returns a | ||
| /// scaled image that is equal to or larger than the requested size in at | ||
| /// least one axis, or the full size of the image if the requested size is | ||
| /// larger. |
There was a problem hiding this comment.
As written, this description says we can always just return the original image size and only hints that it sometimes won't. Perhaps it could instead say something along the lines of "scales by the smallest supported scale factor that produces an image larger or equal to (min_width, min_height) if possible. Otherwise scales by the largest supported factor".
This is the piece I was missing, your design is cleaner if a bunch more scale factors could be added. One more question I have is the "rounding mode". The current strategy is to always round up to a large image size when picking between two scale factors. Other options would be round towards closest and round towards the original size (so decreasing the size rounds up, but increasing image size rounds down). The current choice seems reasonable to me, but I want to double check there isn't a reason we might later want to prefer a different strategy |
|
I chose to round up for the use case you mentioned previously: you want the IDCT to scale to larger than the desired size, then follow it with a resample to the final size. If we were to later add a N>8 IDCT for upscaling, I think the desired behavior would be to round towards the original image size. That is, round up when downscaling, and round down when upscaling. Since this does not add upscaling support, the difference would only be how it is documented. (You could see the "or the full size of the image if the requested size is larger" as the degenerate case of this. Since there is no N>8 IDCT, a larger size always rounds down to 8/8 scaling). |
|
Updated to make the API be |
|
@fintelia Is there anything else you'd like changed before merging? We're using this in production and have run about 100k photos through it so far. |
| buffer[y * component.size.width as usize + x] = data[0][y * line_stride + x]; | ||
| for y in 0 .. width { | ||
| for x in 0 .. height { | ||
| buffer[y * width + x] = data[0][y * line_stride + x]; |
There was a problem hiding this comment.
#125 seems to have been introduced by this line
There was a problem hiding this comment.
Looks like I swapped width and height in the for loop ranges when extracting those expressions as variables. 🤦♂
Also, this presumably means there are no single-channel JPEG images in the test suite?
There was a problem hiding this comment.
Wow, I missed that ! I fixed that too in #126
There was a problem hiding this comment.
The test suite could clearly be more extensive !
| let height = component.size.height as usize; | ||
|
|
||
| let mut buffer = vec![0u8; width * height]; | ||
| let line_stride = width * component.dct_scale; |
There was a problem hiding this comment.
I think this was inadvertently changed from component.block_size.width to component.size.width, causing #125
Add the ability to scale down images during decode (IDCT scaling)
When loading a large image to generate a small thumbnail,
jpeg-decodercurrently requires allocating a large buffer and decoding the full image. It's possible to use a smaller IDCT to directly decode a JPEG image at a fraction of the full size, saving decode time and memory. This adds IDCT implementations to handle 1/8, 1/4, and 1/2 size decoding.The interface is a new
Decoderconstructor that accepts a requested image size. The implementation rounds this up to the nearest scale for which an IDCT implementation exists. This seems nicer thanlibjpeg's scaling API, which represents the size as a fraction and requires the user to know which scale factors are available.