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

GIF transparent pixels of non-initial frame read opaque #5480

Closed
zhuyifei1999 opened this issue May 10, 2021 · 8 comments
Closed

GIF transparent pixels of non-initial frame read opaque #5480

zhuyifei1999 opened this issue May 10, 2021 · 8 comments
Labels

Comments

@zhuyifei1999
Copy link

zhuyifei1999 commented May 10, 2021

What did you do?

Extraction of pixel values from 585738106976731142,gif, frame 1, pixel (0, 0):

from PIL import Image

img = Image.open('585738106976731142.gif')
img.seek(1)
rgba = img.convert('RGBA')
print(rgba.getpixel((0, 0)))

What did you expect to happen?

Expected: (151, 151, 151, 0) (Fully transparent)

What actually happened?

Got: (34, 24, 21, 255) (Opaque dark orange)

What are your OS, Python and Pillow versions?

  • OS: Gentoo Linux
  • Python: 3.8.10
  • Pillow: 8.2.0

Git bisect shows first bad commit as 18854dc, part of PR #5333 (CC @radarhere)

@radarhere radarhere added the GIF label May 10, 2021
@radarhere
Copy link
Member

radarhere commented May 10, 2021

Testing, part of #3434 - using transparency instead of the background color for disposal method 2 - I get (242, 179, 15, 0). Still not what you said you expected, but I imagine you'd be satisfied with that.

My problem with including that change is that it goes against the GIF specification as far as I can see.

@zhuyifei1999
Copy link
Author

I imagine you'd be satisfied with that.

Yeah, As long as alpha is 0 that's good. I'm uncertain what are the true colors of the GIF if the alpha channel were removed, (151, 151, 151, 0) was just the output from Pillow 8.1.2.

Though, I just tried to read the pixel in GIMP and it does appear that bright orange instead of grey should be behind the alpha, so (242, 179, 15, 0) is probably more correct.

I haven't read the GIF specification so what a fix / workaround should look like I have no idea.

@zhuyifei1999
Copy link
Author

I tried #3434 more closely. It generates bad data for frame 2 pixel (0, 0). It returns (151, 151, 151, 255) whereas Pillow 8.1.2 returns (211, 29, 6, 0). The expected alpha is 0 (as seen if you open the GIF in a browser). So no I don't think #3434 fixes it, assuming it is related to the same issue.

@ObaraEmmanuel
Copy link

I am also facing the same issue. Pillow 8.1.2 was handling alpha in GIF correctly but 8.2.0 suddenly displaying opaque pixels in place of the transparent pixels. I haven't done any further probing and I may not be able to provide details at this time but @zhuyifei1999 has raised a real issue here.

@zhuyifei1999
Copy link
Author

zhuyifei1999 commented Jun 9, 2021

I just did a bit of testing:

diff --git a/src/PIL/GifImagePlugin.py b/src/PIL/GifImagePlugin.py
index 5c93de2c..6b08f841 100644
--- a/src/PIL/GifImagePlugin.py
+++ b/src/PIL/GifImagePlugin.py
@@ -291,6 +291,7 @@ class GifImageFile(ImageFile.ImageFile):
         if interlace is not None:
             transparency = -1
             if frame_transparency is not None:
+                print(frame, frame_transparency)
                 if frame == 0:
                     self.info["transparency"] = frame_transparency
                 else:

This prints:

0 5
1 1
2 2
3 1
4 1
5 2
6 2
7 1
8 1
9 1
10 6
11 5
12 5
13 5
14 5
15 1
16 5
17 1
18 1
19 5

So the transparent indexes of each frame is different.

I then did

diff --git a/src/libImaging/GifDecode.c b/src/libImaging/GifDecode.c
index 301f604b..e3307e38 100644
--- a/src/libImaging/GifDecode.c
+++ b/src/libImaging/GifDecode.c
@@ -273,6 +273,8 @@ ImagingGifDecode(Imaging im, ImagingCodecState state, UINT8 *buffer, Py_ssize_t
         for (c = 0; c < i; c++) {
             if (p[c] != context->transparency) {
                 *out = p[c];
+            } else {
+                *out = 5;
             }
             out++;
             if (++state->x >= state->xsize) {

This fixed the transparency in the image in every frame, but it makes so sense to me. *out without assigning is uninitialized 0 in my testing. Why would the decoder assume all the transparent indexes are 0?

@zhuyifei1999
Copy link
Author

I'm doing this as a workaround:

diff --git a/src/PIL/GifImagePlugin.py b/src/PIL/GifImagePlugin.py
index 5c93de2c..ec441a7a 100644
--- a/src/PIL/GifImagePlugin.py
+++ b/src/PIL/GifImagePlugin.py
@@ -300,7 +300,7 @@ class GifImageFile(ImageFile.ImageFile):
                     "gif",
                     (x0, y0, x1, y1),
                     self.__offset,
-                    (bits, interlace, transparency),
+                    (bits, interlace, transparency, self.info["transparency"]),
                 )
             ]
         else:
diff --git a/src/decode.c b/src/decode.c
index 7bcbfdee..c903f810 100644
--- a/src/decode.c
+++ b/src/decode.c
@@ -431,7 +431,8 @@ PyImaging_GifDecoderNew(PyObject *self, PyObject *args) {
     int bits = 8;
     int interlace = 0;
     int transparency = -1;
-    if (!PyArg_ParseTuple(args, "s|iii", &mode, &bits, &interlace, &transparency)) {
+    int global_transparency = -1;
+    if (!PyArg_ParseTuple(args, "s|iiii", &mode, &bits, &interlace, &transparency, &global_transparency)) {
         return NULL;
     }
 
@@ -450,6 +451,7 @@ PyImaging_GifDecoderNew(PyObject *self, PyObject *args) {
     ((GIFDECODERSTATE *)decoder->state.context)->bits = bits;
     ((GIFDECODERSTATE *)decoder->state.context)->interlace = interlace;
     ((GIFDECODERSTATE *)decoder->state.context)->transparency = transparency;
+    ((GIFDECODERSTATE *)decoder->state.context)->global_transparency = global_transparency;
 
     return (PyObject *)decoder;
 }
diff --git a/src/libImaging/Gif.h b/src/libImaging/Gif.h
index 91132e2e..dcca4b90 100644
--- a/src/libImaging/Gif.h
+++ b/src/libImaging/Gif.h
@@ -32,6 +32,7 @@ typedef struct {
 
     /* The transparent palette index, or -1 for no transparency. */
     int transparency;
+    int global_transparency;
 
     /* PRIVATE CONTEXT (set by decoder) */
 
diff --git a/src/libImaging/GifDecode.c b/src/libImaging/GifDecode.c
index 301f604b..e642d98f 100644
--- a/src/libImaging/GifDecode.c
+++ b/src/libImaging/GifDecode.c
@@ -273,6 +273,8 @@ ImagingGifDecode(Imaging im, ImagingCodecState state, UINT8 *buffer, Py_ssize_t
         for (c = 0; c < i; c++) {
             if (p[c] != context->transparency) {
                 *out = p[c];
+            } else {
+                *out = context->global_transparency;
             }
             out++;
             if (++state->x >= state->xsize) {

No idea how correct this is.

@RLaursen
Copy link

RLaursen commented Jun 19, 2021

The problem here is "Only set info transparency on first frame " from commit #5333.

Why would the decoder assume all the transparent indexes are 0?

It doesn't, *out is not 0 by default, *out, if not changed, is whatever the same spot in the previous frame's image8 array was, an integer representing the index of a color in the palette.

@radarhere
Copy link
Member

Thanks to #5557 and then #5756, testing, this is now be fixed in 8.4.0.

from PIL import Image

img = Image.open('585738106976731142.gif')
img.seek(1)
rgba = img.convert('RGBA')
print(rgba.getpixel((0, 0))) # (242, 179, 15, 0)

img.seek(2)
rgba = img.convert('RGBA')
print(rgba.getpixel((0, 0))) # (242, 179, 15, 0)

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

No branches or pull requests

4 participants