Add support for reading DPI information from JPEG2000 images #5568
Conversation
radarhere
changed the title
|
I'm not sure what's up with the pypy-3.6 x86 build failing on Windows. It worked on my fork and I haven't even modified the file that seems to be causing problems, Perhaps it's just a random failure and running the Windows build again will fix it? |
|
I ran the job again and it is passing now |
radarhere
force-pushed
the
jpeg2000-resolution
branch
from
4cbae0d
to
c4567e8
Compare
radarhere
force-pushed
the
jpeg2000-resolution
branch
from
c4567e8
to
8045ecc
Compare
src/PIL/Jpeg2KImagePlugin.py
Outdated
| @@ -154,9 +154,6 @@ def _parse_jp2_header(fp): | |||
| if reader.read_fields(">4s")[0] == b"jpx ": | |||
| mimetype = "image/jpx" | |||
|
|
|||
| if header is None: | |||
| raise SyntaxError("Could not find JP2 header") | |||
There was a problem hiding this comment.
You're right, this is unreachable, reader.has_next_box() will never return False (because reader = BoxReader(fp) is initialized without a length), so the only possible outcome for a malformed image not containing a "jp2h" header is that reader.next_box_type() will eventually fail with some kind of exception.
While not exactly elegant, that's definitely better than having an if statement that misleads the reader. 👍
| @@ -33,12 +33,12 @@ def __init__(self, fp, length=-1): | |||
| self.remaining_in_box = -1 | |||
|
|
|||
| def _can_read(self, num_bytes): | |||
| if self.has_length and self.fp.tell() + num_bytes > self.length: | |||
| # Outside box: ensure we don't read past the known file length | |||
| return False | |||
| if self.remaining_in_box >= 0: | |||
There was a problem hiding this comment.
This could technically also be an elif for symmetry and whatnot -- not that it matters, since we return from the previous if branch anyway. 👍 on spotting and fixing the bug where we could read past the parent box length!
src/PIL/Jpeg2KImagePlugin.py
Outdated
| ) | ||
| return (254 * num * (10 ** exp)) / (10000 * denom) | ||
| if denom != 0: | ||
| return num / denom * (10 ** exp) * 0.0254 |
There was a problem hiding this comment.
I should've probably clarified this in a code comment: the reason for my roundabout way of calculating the resolution was to remain precise by working with ints for as long as possible, and to only have a single (division) operation which introduces floating point errors.
While your code is more elegant and more easily readable, there are now 3 floating point operations that can introduce slight numeric errors.
I'm not sure whether my approach is overkill, though. It's not like those last few mantissa bits are ever realistically going to matter.
There was a problem hiding this comment.
Ah, ok. I was thinking of being consistent with how this value is used elsewhere in Pillow.
I'll switch back to your version.
|
Also, thank you for the additional test cases! 🚀 |
|
LGTM! Thank you for all of the code improvements 😄 |
Changes proposed in this pull request:
This PR seeks to remedy this issue by also parsing the "resc" JPEG2000 header box if it is present.
BoxReader), see the first commit.This is my first contribution to Pillow, so please let me know if I messed something up :)