Skip to content

Commit

Permalink
Merge pull request #5333 from radarhere/gif_frame_transparency
Browse files Browse the repository at this point in the history
  • Loading branch information
@hugovk
hugovk committed Mar 31, 2021
2 parents 54e9f3b + b216b36 commit c54a7bb
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 43 deletions.
Binary file added Tests/images/different_transparency.gif
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added Tests/images/different_transparency_merged.gif
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 15 additions & 2 deletions Tests/test_file_gif.py
View file
Expand Up @@ -468,12 +468,25 @@ def test_dispose2_background(tmp_path):
assert im.getpixel((0, 0)) == 0


def test_iss634():
def test_transparency_in_second_frame():
with Image.open("Tests/images/different_transparency.gif") as im:
assert im.info["transparency"] == 0

# Seek to the second frame
im.seek(im.tell() + 1)
assert im.info["transparency"] == 0

assert_image_equal_tofile(im, "Tests/images/different_transparency_merged.gif")


def test_no_transparency_in_second_frame():
with Image.open("Tests/images/iss634.gif") as img:
# Seek to the second frame
img.seek(img.tell() + 1)
assert "transparency" not in img.info

# All transparent pixels should be replaced with the color from the first frame
assert img.histogram()[img.info["transparency"]] == 0
assert img.histogram()[255] == 0


def test_duration(tmp_path):
Expand Down
4 changes: 2 additions & 2 deletions Tests/test_file_webp_animated.py
View file
Expand Up @@ -45,12 +45,12 @@ def test_write_animation_L(tmp_path):
# Compare first and last frames to the original animated GIF
orig.load()
im.load()
assert_image_similar(im, orig.convert("RGBA"), 25.0)
assert_image_similar(im, orig.convert("RGBA"), 32.9)
orig.seek(orig.n_frames - 1)
im.seek(im.n_frames - 1)
orig.load()
im.load()
assert_image_similar(im, orig.convert("RGBA"), 25.0)
assert_image_similar(im, orig.convert("RGBA"), 32.9)


@pytest.mark.xfail(is_big_endian(), reason="Fails on big-endian")
Expand Down
41 changes: 20 additions & 21 deletions src/PIL/GifImagePlugin.py
View file
Expand Up @@ -145,7 +145,6 @@ def _seek(self, frame):
self.dispose_extent = [0, 0, 0, 0] # x0, y0, x1, y1
self.__frame = -1
self.__fp.seek(self.__rewind)
self._prev_im = None
self.disposal_method = 0
else:
# ensure that the previous frame was loaded
Expand Down Expand Up @@ -174,6 +173,8 @@ def _seek(self, frame):
self.palette = copy(self.global_palette)

info = {}
frame_transparency = None
interlace = None
while True:

s = self.fp.read(1)
Expand All @@ -192,7 +193,7 @@ def _seek(self, frame):
#
flags = block[0]
if flags & 1:
info["transparency"] = block[3]
frame_transparency = block[3]
info["duration"] = i16(block, 1) * 10

# disposal method - find the value of bits 4 - 6
Expand Down Expand Up @@ -250,9 +251,6 @@ def _seek(self, frame):
# image data
bits = self.fp.read(1)[0]
self.__offset = self.fp.tell()
self.tile = [
("gif", (x0, y0, x1, y1), self.__offset, (bits, interlace))
]
break

else:
Expand Down Expand Up @@ -282,11 +280,26 @@ def _seek(self, frame):
except (AttributeError, KeyError):
pass

if not self.tile:
if interlace is not None:
transparency = -1
if frame_transparency is not None:
if frame == 0:
self.info["transparency"] = frame_transparency
else:
transparency = frame_transparency
self.tile = [
(
"gif",
(x0, y0, x1, y1),
self.__offset,
(bits, interlace, transparency),
)
]
else:
# self.__fp = None
raise EOFError

for k in ["transparency", "duration", "comment", "extension", "loop"]:
for k in ["duration", "comment", "extension", "loop"]:
if k in info:
self.info[k] = info[k]
elif k in self.info:
Expand All @@ -299,20 +312,6 @@ def _seek(self, frame):
def tell(self):
return self.__frame

def load_end(self):
ImageFile.ImageFile.load_end(self)

# if the disposal method is 'do not dispose', transparent
# pixels should show the content of the previous frame
if self._prev_im and self._prev_disposal_method == 1:
# we do this by pasting the updated area onto the previous
# frame which we then use as the current image content
updated = self._crop(self.im, self.dispose_extent)
self._prev_im.paste(updated, self.dispose_extent, updated.convert("RGBA"))
self.im = self._prev_im
self._prev_im = self.im.copy()
self._prev_disposal_method = self.disposal_method

def _close__fp(self):
try:
if self.__fp != self.fp:
Expand Down
4 changes: 3 additions & 1 deletion src/decode.c
View file
Expand Up @@ -430,7 +430,8 @@ PyImaging_GifDecoderNew(PyObject *self, PyObject *args) {
char *mode;
int bits = 8;
int interlace = 0;
if (!PyArg_ParseTuple(args, "s|ii", &mode, &bits, &interlace)) {
int transparency = -1;
if (!PyArg_ParseTuple(args, "s|iii", &mode, &bits, &interlace, &transparency)) {
return NULL;
}

Expand All @@ -448,6 +449,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;

return (PyObject *)decoder;
}
Expand Down
3 changes: 3 additions & 0 deletions src/libImaging/Gif.h
View file
Expand Up @@ -30,6 +30,9 @@ typedef struct {
*/
int interlace;

/* The transparent palette index, or -1 for no transparency. */
int transparency;

/* PRIVATE CONTEXT (set by decoder) */

/* Interlace parameters */
Expand Down
38 changes: 21 additions & 17 deletions src/libImaging/GifDecode.c
View file
Expand Up @@ -248,29 +248,33 @@ ImagingGifDecode(Imaging im, ImagingCodecState state, UINT8 *buffer, Py_ssize_t
/* To squeeze some extra pixels out of this loop, we test for
some common cases and handle them separately. */

/* FIXME: should we handle the transparency index in here??? */

if (i == 1) {
if (state->x < state->xsize - 1) {
/* Single pixel, not at the end of the line. */
*out++ = p[0];
state->x++;
/* If we have transparency, we need to use the regular loop. */
if (context->transparency == -1) {
if (i == 1) {
if (state->x < state->xsize - 1) {
/* Single pixel, not at the end of the line. */
*out++ = p[0];
state->x++;
continue;
}
} else if (state->x + i <= state->xsize) {
/* This string fits into current line. */
memcpy(out, p, i);
out += i;
state->x += i;
if (state->x == state->xsize) {
NEWLINE(state, context);
}
continue;
}
} else if (state->x + i <= state->xsize) {
/* This string fits into current line. */
memcpy(out, p, i);
out += i;
state->x += i;
if (state->x == state->xsize) {
NEWLINE(state, context);
}
continue;
}

/* No shortcut, copy pixel by pixel */
for (c = 0; c < i; c++) {
*out++ = p[c];
if (p[c] != context->transparency) {
*out = p[c];
}
out++;
if (++state->x >= state->xsize) {
NEWLINE(state, context);
}
Expand Down

3 comments on commit c54a7bb

@RLaursen
Copy link

@RLaursen RLaursen commented on c54a7bb Jun 14, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When decoding a gif, if the local color table is different from the color table referenced by values already in image8, the values left unchanged in image8 will use the wrong colors. Leaving image8 as is when decoding does not make sense when a local color table is present, pasting gif frames on top of one another should be handled outside of the decoder again where there isn't a reliance on a palette size of less than 256. I think this commit should be reversed and this issue should be fixed outside of the decoder.

@radarhere
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to try and understand, so bear with me.

Leaving image8 as is when decoding does not make sense when a local color table is present

I would have thought that a local color table only affects the pixels coming in from that new frame, not any of the previously loaded pixels.
Take this GIF for example.

out

The first frame has a local color table of b'\x00\x00\x00\xff\x00\x00\x00\x00\x00\x00\x00\x00' - black, red, black, black.
The second frame has a local color table of b'\x00\x00\x00\x00\xff\x00\x00\x00\x00\x00\x00\x00' - black, green, black, black.

If we followed your suggestion, wouldn't that mean that the second frame didn't have any red - when the browser clearly thinks that the second frame should have both a red and a green box?

@RLaursen
Copy link

@RLaursen RLaursen commented on c54a7bb Jun 15, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually bugged as it is now. This is the current behavior if I open that gif and save it with the following code:

from PIL import Image

f = Image.open('testy.gif')
f.save('testyd.gif', save_all = True)

testyd

As you can see, with the current behavior, transparency in subsequent frames means leaving the same indices in the same position in the image8 array, this means those indices now potentially refer to different colors in the new palette based on the new local color table.

Here is that image if I revert this behavior by passing the decoder a transparency value of -1, so it does not retain any indices in image8:

edited line 195 of GifImagePlugin.py
(bits, interlace, transparency),
to
(bits, interlace, -1),

testyd

That said, the second frame is now transparent where the first frame's red box is and the browser is layering the frames for us, the question becomes whether you want Pillow to load each frame with transparency or to do its best to stack each subsequent frame on preceding frames.

Please sign in to comment.