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

Specific ICO file being shifted vertically #3403

Closed
Deimos opened this issue Oct 10, 2018 · 8 comments · Fixed by #5667
Closed

Specific ICO file being shifted vertically #3403

Deimos opened this issue Oct 10, 2018 · 8 comments · Fixed by #5667
Labels
Bug Any unexpected behavior, until confirmed feature.
Projects

Comments

@Deimos
Copy link

Deimos commented Oct 10, 2018

What did you do?

Downloaded this ICO file and used PIL to resave as a PNG: https://lauren.vortex.com/favicon.ico

What did you expect to happen?

PNG result should look the same as the icon does in the browser (and other applications) - an L with a black square around it.

What actually happened?

PNG result seems to have been shifted vertically, with the top part of the image lost:
vortex

What are your OS, Python and Pillow versions?

  • OS: Ubuntu 16.04
  • Python: 3.6.5
  • Pillow: 5.2.0

Code to reproduce

from PIL import Image
i = Image.open("favicon.ico")  # downloaded from https://lauren.vortex.com/favicon.ico
i.ico.getimage((16, 16)).save("vortex.png")
@radarhere
Copy link
Member

I presume that you have no connection to the site's favicon, and so can't give us permission to use the image as part of our test suite.

@Deimos
Copy link
Author

Deimos commented Mar 12, 2019

That's right, I'm not associated. Lauren seems like a pretty contactable person though, so you might be able to ask for his permission if it would make a good test: https://www.vortex.com/lauren

@radarhere radarhere added the Bug Any unexpected behavior, until confirmed feature. label Mar 17, 2019
@aclark4life aclark4life added this to Backlog in Pillow May 11, 2019
@aclark4life aclark4life moved this from Backlog to In progress in Pillow May 11, 2019
@radarhere
Copy link
Member

radarhere commented Apr 20, 2021

A potential solution to this is to use the "offset" and "size" from header to work backwards and read the AND image from the end, instead of moving forward.

    Read AND image from end

diff --git a/src/PIL/IcoImagePlugin.py b/src/PIL/IcoImagePlugin.py
index 5634bf8e9..65c73cdb3 100644
--- a/src/PIL/IcoImagePlugin.py
+++ b/src/PIL/IcoImagePlugin.py
@@ -189,15 +189,8 @@ class IcoFile:
             d, e, o, a = im.tile[0]
             im.tile[0] = d, (0, 0) + im.size, o, a
 
-            # figure out where AND mask image starts
             mode = a[0]
-            bpp = 8
-            for k, v in BmpImagePlugin.BIT2MODE.items():
-                if mode == v[1]:
-                    bpp = k
-                    break
-
-            if 32 == bpp:
+            if mode == BmpImagePlugin.BIT2MODE[32][1]:
                 # 32-bit color depth icon image allows semitransparent areas
                 # PIL's DIB format ignores transparency bits, recover them.
                 # The DIB is packed in BGRX byte order where X is the alpha
@@ -226,8 +219,8 @@ class IcoFile:
                 # the total mask data is
                 # padded row size * height / bits per char
 
-                and_mask_offset = o + int(im.size[0] * im.size[1] * (bpp / 8.0))
                 total_bytes = int((w * im.size[1]) / 8)
+                and_mask_offset = header["offset"] + header["size"] - total_bytes
 
                 self.buf.seek(and_mask_offset)
                 mask_data = self.buf.read(total_bytes)

I thought it might be because this is a 1 bit ICO, but comparing it with https://github.com/lordelph/icofileloader/blob/master/tests/assets/1bit-32px-sample.ico, that isn't the case.

Maybe the encoder just decided to throw some padding in the middle?

@radarhere
Copy link
Member

@laurenweinstein1 hi. In the above posts, a user has found that your site's favicon is an unusual image. Are you happy if we take that image, include it in our test suite, and distribute it under Pillow's license?

@laurenweinstein1
Copy link

Hi. Interesting that you're seeing that behavior. However, I'd prefer that this favicon not be included in the test suite and not be distributed under Pillow's license. Thanks. -L

@hugovk
Copy link
Member

hugovk commented Apr 25, 2021

@laurenweinstein1 Sure thing!

Do you happen to remember how the favicon was created?

@laurenweinstein1
Copy link

I was thinking about that. I probably designed it around 2003 when I started my blog. I remember creating it and tweaking it in a bitmap editor but unfortunately today I have no idea which one.

@radarhere
Copy link
Member

I've created PR #5667 to resolve this.

Pillow automation moved this from In progress to Closed Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Any unexpected behavior, until confirmed feature.
Projects
Pillow
  
Closed
Development

Successfully merging a pull request may close this issue.

4 participants