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

image/gif: transparency lost when encoding a wrapped *image.Paletted #30995

Open
splace opened this issue Mar 22, 2019 · 10 comments

Comments

Projects
None yet
6 participants
@splace
Copy link

commented Mar 22, 2019

go 1.12.1 linux/amd64

What did you do?

package main

import "image"
import "image/png"
import "image/gif"
import "os"
import "encoding/base64"
import "strings"


type offsetImage struct{
	image.Image
	Rect image.Rectangle	
}

func (i offsetImage) Bounds() image.Rectangle{
	return i.Rect
}

func main() {
	tgif:=`R0lGODlhAAGAAPcAAAAAAP///1UATQBFAE4AVABTAFwAUABJAFIAQQBUAEUAIABGAEwAQQBHAC4A
RwBJAEYAAAAAAG6PX0oAAEcAdCALAB0LvxaYj4Y4bo9fSgAAAAAAAAAAAAAAAAAABvBWAMzvVgAS
P/e/AwAAAPdB97+QlPy/+c33v5CU/L8I9FYAA/BWAAAAAACs71YAAAAAAM5EWABfgfu/4hP3v2cB
AADFEve/AGlWADBVcNDGL/m/4hP3v2cBAADFEve/oHEAAAgAAAAoUfe/A0MAAAj0VgAAAAAA3AFp
AAAAAADMRFgACAAARkFUMzIA8FZDOlwAAAgAAACw8lYAAAAAAMSN+L8AAgAAfPBWACCx8n9Y8FYA
5KlngVQVQAAwVXDQWPBWAA6h978ms/e/AABAAAAAAAADAAAAWBVAACAAAABUFUAAwPJWANsU8n8A
AEAAAAAAAFgVQAAIFfJ/DOD8f1gVQABxFfJ/WBVAAAp09H9YFUAAAAAAAAQBAACxc/R/sPJWABT3
VgAAAAAAIAAAADs297/MBAAARwAAAAAAAAAy8VYAHwAAACsRQABS91YA/PBWAF+B+78EFLGBAAIA
AFL3VgArEUAAHwAAAD8AsYEw8VYABBSxgT8AsYEU91YACBFAAEwRQAA48VYAQdH3vwQUsYEU91YA
VPdWAAwRQABAAAAAPwCxgTDxVgAAAgAAQAAAABT3VgAAAAAARPFWAAACAABg8VYAnyj0fwAAAAB4
8VYA5KlngQgRQAAwVXDQePFWAA6h978ms/e/AABAAAAAAAAfAAAADBFAAOj0VgADAAAAAAAAgEH/
97/A8lYATFBUAAMAAAAAAAAAAQAAAAwRQABxFfJ/DBFAAGtz9H8MEUAAFPdWAAAAAAAgAAAAQzpc
TXkgRG9jdW1lbnRzXHBpcmF0ZSBmbGFnLmdpZgAU8lYAjAAAANwOUABEAAAAAwAAAAAAAAAs8lYA
tBEAAHg5aYEgAAAAoKP3vwBgYIGYOWmBtBEAAAAAAAAMYGCBAGBggSH5BAEAAAEALAAAAAAAAYAA
AAj/AAMIHEiwoMGDCBMqXMiwocOHECNKnEixokWCVTJe3Mixo8ePIEOKVJhR48iTKFOqXMnSYMmX
LWPKnEmz5subVWrq3MmzJ0ScN30KHUqUJtCjRZMqXXrxKFCmUKNKPei06tSrWIdW3Zq1q9eYW8N+
HUsWZNizJsuqXesQ7Vm2cONSdetWrt21dOne3fs1r1++gKX6HZwzsGGihAcfXtwzsWPGkME6Thy5
csrJmC1r/oi58+bPFjuLBk36p+jThUurdol69OrXA1vLhq1atm3aoG3rxq1Zt++0vA3/9h188fDj
xfkeR57c7vLnzeE+n546el/q0K1fx05dO1bu3L1PNQVPXjxT8ujNJ0XPXr1W9vCru58ZH/58nfXz
35ecv/7+lf0F+B9KARYo34AcGVgggpwpqCCDxU05+CCEFEloIYURWajhgRjOtaGEHS704Yghsjai
hiUWdOKKKa7oYoguxohhjDQCd1+NNCKI44437ojjfD4GaV6QRHLIW5FEWofkksUtyeSRTjpJW5RU
vkbllbVdqaWRxm2J5WZehllZmGRCRuaZXZ6JJmBqtslmm3ByOVacccpF551s3amnnILtieecfvrZ
VaCCZkXooeMdiihUijZ6XqOQ8klfpI4iRimlPl16aWOaaopfp6BOCmqnMo1qqo0jnXqqSqq2p3pS
q7CKBOusZs1qq6QN3WprgrrqWlGvwE4E7LAZDmtsW8Yey1CyzKIaW7PMIgTttCpOS61A1tJaUmwe
ZqtnANtiiytJ3m4pLn/lkgjuuEal2966qbG7nrutwcttmvQ6ZS+2GK2WLbyFpSWvlbuue2+KABca
rrMIi2juwgM3XCyLEPcrsU0LhgvuxYkOt+/GHNupmMYChxwZUgbza/KRK7fs8sswqxUQADs=`

	i,_,err:=image.Decode(base64.NewDecoder(base64.StdEncoding, strings.NewReader(tgif)))

	if err!=nil{panic(err)}

	w,_:= os.Create("1.png")
	png.Encode(w,i)
	w,_= os.Create("1.gif")
	gif.Encode(w,i,nil)

	i2:=offsetImage{i,i.Bounds()}   // no offset, same image

	w,_= os.Create("2.png")
	png.Encode(w,i2)
	w,_= os.Create("2.gif")
	gif.Encode(w,i2,nil)
	
}

What did you expect to see?

images the same

What did you see instead?

gif has transparency converted to black, png retains origin transparency.

output:

  • 1
  • 1
  • 2
  • 2

@dmitshur dmitshur added this to the Go1.13 milestone Apr 18, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@splace Thanks for the issue report.

When you say "images the same" under "what did you expect", can you clarify? There are 4 images, which ones did you expect to be the same?

@splace

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

the images are in the order the code produced them, they should all be the same since the image is not being modified at any point.

details....
the image (made from inline gif in base64) has transparency, when this is saved as a png or a gif the transparency is there in both files, but when the image is embedded in a new type, offsetImage, (still the same Image to the interface) then saving as png is fine, but saved as gif causes the transparency to be replaced by black. so it seems the gif encoder is in some way 'bypassing' the interface, being aware and varying what it does depending on the specific type.

@dmitshur dmitshur changed the title image/gif: transparency lost when encoding image/gif: transparency lost when encoding a wrapped *image.Paletted Apr 19, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

The concrete type returned by image.Decode in your snippet is *image.Paletted.

I suspect this issue is happening because wrapping the *image.Paletted in a concrete type such as offsetImage makes the type assertion in gif.Encode no longer report positively:

pm, ok := m.(*image.Paletted)

/cc @robpike @nigeltao Do you think this is working as intended, or is this a bug in encoding/gif? Should it handle situations where the concrete type of the passed image.Image is not precisely *image.Paletted the same as if it were?

@splace

This comment has been minimized.

Copy link
Author

commented Apr 19, 2019

FYI this was encountered when i put together a simple image geometry transforming utility. (any supported format input, any output.) with the very simple technique of wrapping the Image and modifying the x and y of the method At(x,y) then calling the embedded Image. works as expected, except for this.

@nigeltao

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Yeah, it's a bug in encoding/gif, although not one that's trivial to fix. The

pm, ok := m.(*image.Paletted)
if !ok || len(pm.Palette) > opts.NumColors {
  etc
}

check should be something like

pm, _ := m.(*image.Paletted)
if pm == nil {
  // Either one or both of
  if pm2, ok := m.(image.PalettedImage) {
    // Create a new pm, based on pm2.
    // Note that image.PalettedImage is an interface; image.Paletted is a concrete type.
    // Yeah, the names are confusing. In hindsight, I'd do it differently.
  }
  // or
  if _, ok := m.ColorModel().(color.Palette); ok {
    // Create a new pm, based on m.
  }
}
if (pm == nil) || (len(pm.Palette) > opts.NumColors) {
  etc
}

But that's not great. The first approach might not be feasible if it requires your wrapper type to also or conditionally implement the image.PalettedImage interface.

The second approach would probably work. It won't be super-efficient to, for each pixel, call At and look up the color.Palette to turn it back into an index. But given the Go 1.x image interfaces that we have, it could be feasible.

@rsc

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@nigeltao, what do you suggest we do?

@nigeltao

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

If somebody has the free time to fix the bug as I suggested (preferably the second approach), that's great.

I don't have the free time, though.

If nobody has the free time, then I think we just have to leave it as is for now. It's not a regression.

@kawakami-o3

This comment has been minimized.

Copy link

commented May 14, 2019

@nigeltao, I tried your second approach, and it works fine.

test-image-gif

The first and second pictures are the results from @splace 's test.
The third pictures are the results from the test which contains offset.
Every gif picture has a transparent background.

My code is as follows,

  pm, _ := m.(*image.Paletted)
  if pm == nil {
    if cp, ok := m.ColorModel().(color.Palette); ok {
      pm = image.NewPaletted(b, cp)
      for i:=b.Min.X ; i < b.Max.X ; i++ {
        for j:=b.Min.Y ; j < b.Max.Y ; j++ {
          pm.Set(i,j, cp.Convert(m.At(i, j)))
          //pm.Set(i,j, m.At(i, j)) // also works 
        }
      }
    }
  }
  if (pm == nil) || (len(pm.Palette) > opts.NumColors) {
    // TODO: Pick a better sub-sample of the Plan 9 palette.
    pm = image.NewPaletted(b, palette.Plan9[:opts.NumColors])
    if opts.Quantizer != nil {
      pm.Palette = opts.Quantizer.Quantize(make(color.Palette, 0, opts.NumColors), m)
    }
    opts.Drawer.Draw(pm, b, m, image.ZP)
  }

Is this code what you expected?

@nigeltao

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Yeah, that looks OK, broadly speaking. I guess that it wasn't as complicated as I first feared. It's not super-efficient, but it will work, and we can optimize later.

There's some style nits (e.g. I'd call the variables x and y instead of i and j, and I'd also iterate y as the outer loop, not the inner one), but we can fix that in code review.

@gopherbot

This comment has been minimized.

Copy link

commented May 15, 2019

Change https://golang.org/cl/177377 mentions this issue: image/gif: transparency lost when encoding a wrapped *image.Paletted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.