Advancing multiple frames results in artifacts

[PauGuillamon]

When advancing multiple frames without calling getCurrentFrame(), the pixels contain artifacts. I have changed the sample app to call advance() twice before getting the frame, I got this:

The use case for this would be integrating Gif into a rendering engine where advancing frames is calculated on each frame update. If the frame rate of the engine is slower than the gif's, then advance() needs to be called multiple times in a frame resulting in this issue. Temporary workaround would be to explicitly call getCurrentFrame() after each advance() call, but I'm sure we can improve this :)

I am not sure if calling getCurrentFrame() internally when advance() is called would be the best solution, but it would fix the problem. I am not into the details of decoding, but it sounds like we need to decode frame by frame?

[redwarp]

So, GIFs are iterative: frame 1 depends on frame 0, frame 2 depends on frame 1... That's why there is no way around it, you must calculate each frame, even if you intend to skip displaying it.

[PauGuillamon]

Right, that makes sense. But the client's code doesn't need to know that, right?

If that's anyway needed, I would suggest to do it internally when advance() is called. What do you think about this?

[redwarp]

I agree with you that the API is confusing! It would make sense to calculate things on advancing for sure. Maybe advance could also return a boolean, like the getCurrentFrame method, to tell if advanced was successful. Or an enum, as several case could happen, like end of loop, error, ...

[redwarp]

Likewise, getFrameAtIndex should probably be smart enough to call advance as many times as needed to produce a non artifacty result.

[redwarp]

At this point, we can still change the API 100% I think. What would be your ideal set of methods to get frames and advance them?

[PauGuillamon]

For my use case advance() and getCurrentFrame(pixels) work pretty well. So far I don't use getFrame(index: Int, inPixels: IntArray).

If advance() forced the processing of the new frame automatically would be good for me, no need to change the API ;)

[redwarp]

Sounds reasonable, I'll make the changes!

[redwarp]

@PauGuillamon "fun" fact, modifying so that advance compute the current frame made me discover a bug in getCurrentFrame(pixels): if you call getCurrentFrame twice in a row after advance, then the second time, the picture will be corrupted:

• the first time it's called, the actual calculation of the current frame is done, and copied in the inPixels.
• the second time, it simply copies the content of framePixels into the passed inPixels buffer, because no need to recalculated everything twice.

BUT! the method that does calculation also does disposal. And so, the content of the framePixels the second time around contains an image where gif's disposal method has already been applied, and so kind of currupted in some cases. (Basically the prep work for the next frame has already been applied.

Not sure if that will make much sense to you if you are not familiar with the gif spec, but anyway, all to say that there is a bug, that will be corrected while I'm modifying the code to let "advance()" responsible for unpacking the frame.

[redwarp]

So basically, this test:

@Test
    fun getCurrentFrame_multipleCalls_sameResult(){
        val gif = Gif.from(File("../assets/domo.gif")).unwrap()

        gif.advance()

        val firstCall = IntArray(gif.dimension.size)
        val secondCall = IntArray(gif.dimension.size)
        gif.getCurrentFrame(firstCall)
        gif.getCurrentFrame(secondCall)

        assertArrayEquals(firstCall, secondCall)
    }

fails

[PauGuillamon]

wow nice catch! As I see now the disposal is done before processing the new frame. It looks good! :)

[redwarp]

Alright, it's now fixed on main, and I released a version 0.8.0 just now with the change. Though be aware that there are a few other breaking change in the new release that might impact you: I replaced my implementation of Result with the official kotlin one, now stable. And I consolidated the Gif class a bit to replace all returns type that were of type boolean or nullable types, to use Result instead.

[PauGuillamon]

Yeah, I saw it ;) thanks for the heads up 👍