Opened 3 years ago

Closed 3 years ago

#18014 closed defect (fixed)

GIF Decoder changes transparent colour to pink

Reported by: HugoElias Owned by: vadz
Priority: low Milestone:
Component: GUI-all Version: 3.0.3
Keywords: GIF wxImage Cc: maartenbent@…, ericj
Blocked By: Blocking:
Patch: no

Description

Description of the problem

Code in gifdecod.cpp changes the transparent colour of a loaded image to pink.

Normally, this isn't a problem, because that colour wouldn't be rendered, but in some circumstances, the pink shows up, and looks incorrect.

To Reproduce

The following two examples show the problem:

// Example 1: Shows pink instead of correct grey background when useMask = false
if (image.LoadFile("image_with_transparent_background.gif")
{
    bitmap = wxBitmap(image);
    image.Destroy();
    dc.DrawBitmap(bitmap, 10, 10, false);
}
// Example 2: Shows pink leaking into boundary between masked and unmasked areas of the image,
// due to resizing, even when useMask = true
if (image.LoadFile("image_with_transparent_background.gif")
{
    image.Rescale(image.GetSize().GetWidth()/2, image.GetSize().GetHeight()/2, wxIMAGE_QUALITY_BILINEAR);
    bitmap = wxBitmap(image);
    image.Destroy();
    dc.DrawBitmap(bitmap, 10, 10, true);
}

Who would experience this bug?

Anybody making an image editor, or image browser with thumbnails, that works with .GIF images.

The problem is especially serious for an image editor, where the user may need to smooth the pixels near the transparency, so that the blend nicely with the background. This would be impossible to do correctly if the transparent colour was turned pink.

Attachments (4)

Pink_Masking_01.png download (138.0 KB) - added by HugoElias 3 years ago.
Image showing the problem
gif_pink_transparency.diff download (1.7 KB) - added by HugoElias 3 years ago.
Patch to fix this problem
gif_transparency_fix.diff download (4.1 KB) - added by HugoElias 3 years ago.
Adds option to address issue. Plus documentation.
gif_transparency_2018_01_05.diff download (5.4 KB) - added by HugoElias 3 years ago.

Download all attachments as: .zip

Change History (39)

Changed 3 years ago by HugoElias

Image showing the problem

Changed 3 years ago by HugoElias

Patch to fix this problem

comment:1 Changed 3 years ago by vadz

  • Component changed from base to GUI-all
  • Keywords GIF wxImage added

Thanks for your patch and the explanations, but I'm not completely sure I understand them.

The original idea behind using pink was, AFAIR, exactly to make it very noticeable so that it could be seen immediately if the transparent colour was incorrectly used instead of being just ignored as it ought to be. So before changing this I'd like to understand better what is the actual problem and how do you end up with using the value of this colour exactly? In the explanation above you seem to be implying that you use this colour for averaging (smoothing) in your code, but why do you do this, shouldn't the smoothing code take into account alpha of the transparent pixels correctly? I.e. isn't the real bug in Rescale() (the first example isn't a bug at all IMHO because you shouldn't be drawing a bitmap ignoring its mask in the first place)?

Or did I misunderstand what the problem was?

comment:2 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

Well, yes, you have a point. In a way. Two responses:

Response 1

In the case that someone is making an image editor that supports GIFs, then this code is simply broken, and also does not match the behaviour of many image editors and browsers. Let me take you through the process of making and editing a transparent GIF to outline the problem.

Creating a transparent GIF

  • 1. Choose the background image, and choose a representative colour for it. In my case, I chose the grey 'paper' texture, and chose a light grey colour (210, 210, 210) to represent it.
  • 2. Go to my image editor, create a new image (in this case a 256 greyscale image), and set all pixels in that image to the light grey colour (210).
  • 3. Draw my cat picture on top, with nicely anti-aliased lines.
  • 4. When saving the image as a GIF, choose palette colour 210 to be the transparent colour.
  • 5. Put that image on my web page, with the paper texture background. Now the cat looks correct with correct looking anti-aliasing.

Now I decide I want to edit the image and add a little mouse next to the cat. What do I expect to happen when I load up the image in my image editor? I expect it to look exactly as it did when I saved it. I expect to see the grey background. I actually need the grey background in order to add the mouse with correct anti-aliasing.

However, if I were to create an image editor in wxWidgets, then I get the wrong behaviour, and there is no way to fix it. When I re-open my cat image, the grey background is now pink! The actual correct colour has been overwritten inside gifdecod.cpp.

Making an image browser

This is actually what my application is, and so this is where I'm having the problem.

The image browser reads all the images in a directory, and creates thumbnails of them using image::Rescale() with wxIMAGE_QUALITY_BILINEAR. This is where the pink is impossible to avoid. The bilinear rescaling algorithm obviously leaks pink all over adjacent pixels. Pink is visible no matter if I set useMask to true or false.

Importantly, using wxWidgets as it is, it would be impossible to re-create the correct functionality of either of the image editors or image browsers I currently use.

Response 2

Indeed, a *partial* fix for this problem would be to make the image bilinear rescaling algorithm much more complex, getting it to carefully ignore pink pixels in its calculation. Wouldn't it be easier just to get rid of the pink, and fix both problems properly?

Actually, I believe there is a better solution than either of the ones suggested so far. gifdecod.cpp should create the alpha channel based on the palette colour value, rather than a matching RGB value as it does now. But I believe that this solution would require much more work to implement, with essentially no visible improvement over my patch.

The original idea behind using pink was, AFAIR, exactly to
make it very noticeable so that it could be seen immediately
if the transparent colour was incorrectly used instead of
being just ignored as it ought to be.

Can you give an real-ish world example of how someone would be grateful for the existence of the pink? I'm struggling to think of one. Generally, when people are creating GIFs, then they have the option themselves to choose a contrasting colour for this very reason. I don't see how having the contrasting colour in the image viewer itself helps them.

In summary

  1. The pink colour makes it impossible to create a correctly functioning image editor or browser that supports GIFs.
  2. I doubt the pink colour actually helps solves a problem that isn't best solved inside the editor which created it.

Lastly: I'm sorry for the very long response. Thank you for your patience.

comment:3 Changed 3 years ago by vadz

First, thanks for taking the time to write this long response!

I admit I didn't think about the situation described in the first point, i.e. the need to use anti-aliasing when drawing on "transparent" background. However now that you described it, I can't help wondering why should we handle it just in the case of solid background colour instead of arbitrary backgrounds? IMO the proper way to do this would be to draw the image transparently (i.e. with useMask = true) on top of the background you want to use, whatever it is, and then -- optionally -- use anti-aliasing when drawing. I say "optionally" because a few editors use a chequered background for the (semi)transparent images by default and you don't want to use anti-aliasing when drawing over it.

As for the second point, I'm afraid there is a misunderstanding: I didn't at all mean to ignore pink pixels, but to ignore _transparent_ pixels, i.e. just handle the mask correctly, just as it already handles alpha. AFAICS the changes should be relatively straightforward: the code in wxImage::ResampleBilinear() already handles "real" alpha, so we just need to modify the parts of it doing if ( src_alpha ) to also use ... else if ( usesMask ) and do the obvious mask-to-alpha translation (alpha is wxALPHA_TRANSPARENT if the colour is the same as mask colour or wxALPHA_OPAQUE otherwise). This seems like obviously the right thing to do to me, isn't it?

I can't give you an example because I don't use GIF myself anyhow, but the idea of making it painfully obvious when you forgot to pass useMask = true to DrawBitmap() doesn't seem completely irrealistic to me. And I think really fixing Scale() is a better idea than making its absence of support for handling transparent pixels less obvious, as this patch does.

Wouldn't you agree?

comment:4 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

I'll have to take a look at the bilinear code to see exactly what's doing. But my feeling is that adding support for carefully ignoring transparent pixels will be tricky.

I didn't at all mean to ignore pink pixels, but to ignore _transparent_ pixels

That's what I meant too. I said pink but I meant transparent.

the idea of making it painfully obvious when you forgot to pass
useMask = true to DrawBitmap() doesn't seem completely irrealistic to me.

The problem is that this convenience is making it impossible for my application to function correctly. Also, useMask is true by default. The user would have to explicitly ask for the image to be rendered without transparency. There's no way to forget to do it.

Whatever we do, I'm happy as long as there's a way to load up a GIF without messing with any of its colours. As I said, I want to make an image browser and editor, which needs to be able to load up the images with all of their correct colours. I.E. gifdecod.cpp really needs to change, otherwise there's no way for me to make my application behave correctly.

This isn't just about the way images are displayed in applications like web browsers or whatever. This is about the use case where the application needs to see the GIF exactly as it is.

Perhaps an alternative option would be to load up the GIF correctly, but then give users an option or function to change the transparent colour to pink if that would help them.

comment:5 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

Is there any way we could make it so that the GIF is loaded up as it is now, but there's an option to load the GIF without modification.

Looking at the code, I'm not quite sure how to get that option to the wxGIFDecoder::ConvertToImage() function in a sensible way.

Perhaps a compile flag?

comment:6 Changed 3 years ago by vadz

  • Patch unset
  • Priority changed from normal to low
  • Status changed from new to confirmed

wxImage has a generic mechanism for passing options to the image handlers via SetOption() method, so wxGIFHandler::LoadFile() could query the option and then pass it to wxGIFDecoder via some new ctor argument or method call.

comment:7 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

I will try that, and submit a patch.

comment:8 Changed 3 years ago by vadz

TIA! But I also think that you might be overestimating the difficulty of fixing wxImage::ResampleXXX(), it seems pretty easy to make them work with the mask exactly as well as they work with alpha transparency -- which shouldn't be too bad, as many people use them with partly transparent images without any big problems.

comment:9 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

It looks like I can't use SetOption() to pass information to gifdecod.cpp.

The options reside within M_IMGDATA, which itself is not allocated until the GIF starts to load (which calls wxImage::Create(), which allocates M_IMGDATA), at which point it's too late.

One option would be to allow wxImage::SetOption() itself to allocate M_IMG_DATA. Then, when wxImage::Create() is called, it checks to see if M_IMGDATA is already allocated before attempting to allocate a new copy.

Do you think that would be sensible?

comment:10 Changed 3 years ago by vadz

I think there should be no problem with using AllocExclusive() in Create() which is, AFAICS, the only thing that needs to be changed (SetOption() does already allocate the image data, it's just that it's discarded by Create() currently).

comment:11 follow-up: Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

I think I worked out what was going wrong.

Somewhere at the beginning of wxGIFDecoder::ConvertToImage(), the options data was getting messed up. So I just called image->GetOption() right at the start, cached the result, and use it later in the function.

So if I call:
image.SetOption(wxIMAGE_OPTION_GIF_TRANSPARENCY_HANDLING, wxIMAGE_OPTION_GIF_TRANSPARENCY_UNCHANGED);

then the image looks correct.

If I call:
image.SetOption(wxIMAGE_OPTION_GIF_TRANSPARENCY_HANDLING, wxIMAGE_OPTION_GIF_TRANSPARENCY_HIGHLIGHT);

or if I set no options, then the original behaviour still happens.

Would you be happy for me to submit this as a patch?

comment:12 in reply to: ↑ 11 Changed 3 years ago by vadz

Replying to HugoElias:

I think I worked out what was going wrong.

Somewhere at the beginning of wxGIFDecoder::ConvertToImage(), the options data was getting messed up.

Yes, I didn't notice it, but it called Destroy() and then Create() again. I have no idea why did it do this, it really ought to just call Create() (which should be modified as indicated in the comment:10).

So I just called image->GetOption() right at the start, cached the result, and use it later in the function.

This could be used as a workaround but just not calling Destroy() would be better and not more difficult.

So if I call:
image.SetOption(wxIMAGE_OPTION_GIF_TRANSPARENCY_HANDLING, wxIMAGE_OPTION_GIF_TRANSPARENCY_UNCHANGED);

then the image looks correct.

If I call:
image.SetOption(wxIMAGE_OPTION_GIF_TRANSPARENCY_HANDLING, wxIMAGE_OPTION_GIF_TRANSPARENCY_HIGHLIGHT);

or if I set no options, then the original behaviour still happens.

Would you be happy for me to submit this as a patch?

Yes, thank you. I would probably shorten the option name to just wxIMAGE_OPTION_GIF_TRANSPARENCY because this is already quite long and _HANDLING doesn't really add anything.

Please add the new option and its possible values to interface/wx/image.h too (the existing options are barely documented there, but this shouldn't prevent you from providing more useful/extensive documentation for the new one), TIA and looking forward to your patch!

comment:13 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

I'm a little confused as to where to add this option. (perhaps this is an oversight in the code.)

wxIMAGE_OPTION_GIF_COMMENT is actually defined in imaggif.h. (It's also in interface/wx/image.h, but that definition doesn't seem to be the one that gifdecod.cpp uses)

Does that mean that gifdecod.cpp should be changed to include interface/wx/image.h, and the definition of wxIMAGE_OPTION_GIF_COMMENT removed from imaggif.h?

comment:14 Changed 3 years ago by vadz

Oops, sorry for the confusion. The headers under interface are only used for documentation (and related) purpose(s), they should never be included from anywhere. So you should add the new constants to the normal header files (and include/wx/imaggif.h is indeed better for GIF-specific options) and to the interface ones, with a documentation comment in the latter.

Changed 3 years ago by HugoElias

Adds option to address issue. Plus documentation.

comment:15 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

Hi,

I think this patch should solve the issue. Unfortunately I couldn't get the git clone to compile, so I moved my changed from my normal WxWidgets folder to the git clone, and made the diff there. The diff looks correct.

I also added documentation in interface/wx/image.h, but I'm not sure if I got the formatting correct.

I hope this is all OK.

Hugo

comment:16 Changed 3 years ago by vadz

  • Owner set to vadz
  • Status changed from confirmed to accepted

Thanks, I'll look at the patch soon, but what's the problem with compiling the current master? Is it due to submodules again? Please post to wx mailing list if you still have it.

comment:17 follow-up: Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

I think it was the fact that png, jpeg, expat, etc. Were missing. I tried copying them over from my normal installation, but it still complained about expat.h missing.

In the end I thought that this wasn't helping me solve the problem quickly, so I fudged it. I generated the patch from the master, but didn't check that it compiles there. Therefore I can't be 100% sure that this patch works on the master.

Anyway, I am happy with this solution because it's a small change, the default behaviour is unchanged, and it solves my problem :)

comment:18 in reply to: ↑ 17 Changed 3 years ago by vadz

Replying to HugoElias:

I think it was the fact that png, jpeg, expat, etc. Were missing.

Yes, this is due to submodules switch. See https://github.com/wxWidgets/wxWidgets/blob/master/README-GIT.md

comment:19 Changed 3 years ago by vadz

I'm sorry, but this doesn't seem to have been tested at all -- or you uploaded a wrong file somehow. In any case, the patch from comment:15 doesn't compile at all (multiple problems: wxIMAGE_OPTION_GIF_TRANSPARENCY_XXX not defined, transparencyOption variable not declared/defined, ...) and it it compiled, it wouldn't work correctly because it always sets the transparent colour as mask colour, even in wxIMAGE_OPTION_GIF_TRANSPARENCY_HIGHLIGHT case.

I've tried fixing this (as well as adding some comments) in this PR, but I don't have any testing code for this and so I hope you can test it and check that it still works as intended.

comment:20 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

Whoops. I'm not sure exactly why, but the patch seems to be missing the changes to imaggif.h. It certainly was tested, and I have it working correctly on my machine at work.

The problem might be the fact that I made the changes on my original installation of wxWidgets, but transferred them over to the git clone, and perhaps I messed up there,

Now that you've told me that I need to use --recurse-submodules, then I can get the git clone compiling, and do this properly.

I'm back at work (where my changes are) on 4th Jan, so I'll be able to send a new patch then, and test your fixes too.

Sorry about that.

Last edited 3 years ago by HugoElias (previous) (diff)

comment:21 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

I'm back on the case. I've cloned the repo with --recurse-submodules, and it compiles fine.

I have moved my changes into this repo, and they compile.

I have tested that these changes work with my application to render GIFs correctly when I use the wxIMAGE_OPTION_GIF_TRANSPARENCY_UNCHANGED option.

When I use no option, or when I use the wxIMAGE_OPTION_GIF_TRANSPARENCY_HIGHLIGHT option, the GIF is rendered with pink for the transparent pixels.

I then created a patch using git diff > gif_transparency_2018_01_05.diff from c:\wxwidgets.

Changed 3 years ago by HugoElias

comment:22 Changed 3 years ago by vadz

Thanks, the new patch looks good, but as I said, I had already changed the original one to compile in the meanwhile, so now we have slightly different changes in this patch and in the PR. Could you please check if the PR version works for you? If so, I'd rather merge it, unless there are any important differences you disagree with.

TIA!

comment:23 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

One question I have is: what is the default behaviour?

In my version, it runs the new code if the option is set to wxIMAGE_OPTION_GIF_TRANSPARENCY_UNCHANGED , otherwise it runs the original code.

In your PR, it looks like the user would have to explicitly set one option or the other.

Or did I misread the code?

comment:24 Changed 3 years ago by vadz

Oops, no, you're absolutely right, I forgot the test for the option being empty, thanks for noticing it. Added it now (and also rebased on latest master), do you see any other problems with it? Thanks again!

comment:25 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

It looks good now. I haven't had time to test it against my code. That'll have to wait until I get into work again on Monday.

I'll let you know. Have a good weekend.

comment:26 Changed 3 years ago by vadz

Did you have a chance to test this? I'd like to merge this in. Thanks!

comment:27 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

Sorry about the delay. I'm back at work now. It also took me a while to learn how to check out your PR (I'm new to git).

The sad news is that it doesn't seem to work. I'm not exactly sure why, but adding:
std::cout << "Transparency = " << transparency << std::endl; to line 152 gives: Transparency =

My code that sets the option is unchanged, and works on my modified wxWidgets.

I'm looking into it now.

comment:28 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

I've had a look into but, but I really can't figure out why it's not working. I can't work out what the problem is. However, I know that the patch I submitted does work. Could that be used instead?

comment:29 Changed 3 years ago by MaartenB

  • Cc maartenbent@… added

Doesn't work for me either. It seems

image->Create(sz.GetWidth(), sz.GetHeight());

in gifdecod.cpp:r135 clears the options. So the transparency option should be requested before calling Create, or Create should preserve the options.

comment:30 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

Is there anything else I can do to help this along?

comment:31 Changed 3 years ago by ericj

  • Cc ericj added

I didn't follow the whole discussion, so sorry if i'm not making sense...

But couldn't this be solved through a new wxBITMAP_TYPE_GIF_XXX filetype?

I checked the sources and the filetype doesn't get passed to the image load handlers, so some refactoring will be required, but maybe it would still a cleaner solution?

comment:32 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… added

Fixed it.

Line 149: const wxString& transparency ... needs to move to line 133.

Am I allowed to push to the branch?

comment:33 follow-up: Changed 3 years ago by vadz

Sorry for the delay and thanks to both Maarten and Hugo for finding the bug.

I'm not sure if Create() should preserve the options, so let's apply the fix from the comment:32. I've updated https://github.com/wxWidgets/wxWidgets/pull/647 the PR now, please retest it if possible (the absence of a test for it was a huge time saver here, it would have been really great to have an automatic unit test...).

comment:34 in reply to: ↑ 33 Changed 3 years ago by HugoElias

  • Cc hugo.elias@… removed

Tested and working!

comment:35 Changed 3 years ago by Vadim Zeitlin <vadim@…>

  • Resolution set to fixed
  • Status changed from accepted to closed

In 28bf0e868/git-wxWidgets:

Add wxIMAGE_OPTION_GIF_TRANSPARENCY for GIF image loading

Allow to keep the originally defined transparent pixels colour instead
of replacing it with bright pink (which is still the default behaviour).

Closes #18014.

Note: See TracTickets for help on using tickets.