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: EncodeAll fails to allow non looping animated gif #15768

Closed
ghost opened this issue May 20, 2016 · 6 comments
Closed

image/gif: EncodeAll fails to allow non looping animated gif #15768

ghost opened this issue May 20, 2016 · 6 comments
Assignees
Milestone

Comments

@ghost
Copy link

@ghost ghost commented May 20, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version go1.6.2
  2. What operating system and processor architecture are you using (go env)?
    darwin/amd64
  3. What did you do?
    In package image/gif, I'm trying to use EncodeAll() to create a non-looping animated gif with several frames, but it will always loop. In the GIF specifications 0 means forever, so -1 should the way to go to disable looping, unfortunately setting this value does not work in go.
  4. What did you expect to see?
    I should be able to set LoopCount = -1 and my resulting gif should not loop.
    I want an animated gif, but not a looping animated gif.
  5. What did you see instead?
    My output gif is looping forever (same as LoopCount = 0).

The first problem lies in src/image/gif/write.go

   299      if g.LoopCount < 0 {
   300          g.LoopCount = 0
   301      }

This should be removed to allow a negative value.

The second problem is that Netscape Looping extension header is written in the file whenever there are several frames:

   147      if len(e.g.Image) > 1 {

I believe this should be changed to:

if len(e.g.Image) > 1 && e.g.LoopCount >= 0 {

Another way to fix the problem would be to change the meaning of LoopCount '0' value to actual "no loop", but this could break the expected behaviour for some people. As in other languages, setting -1 sounds ok as long as it's commented somewhere.

@ghost ghost changed the title image/gif: EncodeAll fails to set LoopCount=-1 (non looping animated gif) image/gif: EncodeAll fails to allow non looping animated gif May 20, 2016
@josharian
Copy link
Contributor

@josharian josharian commented May 20, 2016

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 20, 2016

@chriwup I believe what you want to do is set the loop count to 1 and not -1. IMO -1 doesn't make sense for a loop count to mean no loop. I have a small script that you can use to play around with the loop count values here https://play.golang.org/p/zIPjxYjuCV and using 1 should cut the deal. Here is the code below inlined

package main

import (
    "flag"
    "fmt"
    "image/gif"
    "net/http"
    "os"
)

func exitIfError(err error) {
    if err == nil {
        return
    }

    fmt.Fprintf(os.Stderr, "%v\n", err)
    os.Exit(1)
}

func main() {
    gifURL := flag.String("url", "https://brobible.files.wordpress.com/2016/03/joker1.gif", "the URL of the GIF")
    loopCount := flag.Int("loop-count", 1, "the max loop count of the GIF")
    flag.Parse()

    res, err := http.Get(*gifURL)
    exitIfError(err)
    gifImg, err := gif.DecodeAll(res.Body)
    _ = res.Body.Close()
    exitIfError(err)

    gifImg.LoopCount = *loopCount
    _ = gif.EncodeAll(os.Stdout, gifImg)
}
$ go run gif-loop-count.go --url https://j.gifs.com/Z6ozLJ.gif --loop-count 2 > lc2.gif # Make it loop twice
$ go run gif-loop-count.go --loop-count 1 > lc1.gif # Should loop only once.

Let me know what you think.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 20, 2016

Actually I stand corrected, loop=1 sets it to loop once which is not what you want. Your original issue still stands, I've also corrected my script to include flag.Parse()

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented May 22, 2016

Yeah, it's unfortunate that, in the underlying "Netscape Looping Application Extension" format, 0 means loop forever and 1 means loop twice. To loop exactly once, we need to omit the extension entirely.

Given the back-compat constraints, I think that "negative means loop exactly once" is the right fix, in Go 1.8.

@nigeltao nigeltao added this to the Go1.8 milestone May 22, 2016
@nigeltao nigeltao self-assigned this May 22, 2016
@quentinmit quentinmit added the NeedsFix label Oct 7, 2016
@rsc
Copy link
Contributor

@rsc rsc commented Oct 26, 2016

Punting to Go 1.9.

I started to do this but it seems important to harmonize both Read and Write, and it was not entirely clear to me how to do that.

@rsc rsc modified the milestones: Go1.9, Go1.8 Oct 26, 2016
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 May 22, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@andybons andybons self-assigned this Feb 9, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 9, 2018

Change https://golang.org/cl/93076 mentions this issue: image/gif: support non-looping animated gifs (LoopCount=-1)

@gopherbot gopherbot closed this in ecba371 Feb 13, 2018
@golang golang locked and limited conversation to collaborators Feb 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.