On Fri, 3 Jul 2020 at 12:16, <jcupitt(a)gmail.com&gt; wrote: Hello again, I spent a little time looking at the code. tldr: I think GIFs which have a COMBINE frame followed by a RESTORE frame will not render correctly in libnsgif. 1. The frames in this GIF (except for frame 5) have disposal_method 1, GIF_FRAME_COMBINE, ie. the frame is not cleared and pixels accumulate. 2. Frame 5 is GIF_FRAME_RESTORE, meaning that after frame 5 is rendered, the frame should be restored to frame 4. This is detected when frame 6 rendering starts here: https://source.netsurf-browser.org/libnsgif.git/tree/src/libnsgif.c#n794 And it calls gif_internal_decode_frame() recursively to put the pixels for frame 4 back. 3. However, because frame 4 is GIF_FRAME_COMBINE, the frame 4 pixels are incorrectly painted on top of frame 5. 4. gif_internal_decode_frame() then decode the pixels for frame 6, but because frame 4 has not been restored, the wingtips are left behind. The problem is that with GIF_FRAME_COMBINE, the state of a GIF frame depends on all the previous frames. You can't just call gif_internal_decode_frame() to put the pixels back --- you need to re-render the entire animation. It seems to me that the best way to implement GIF_FRAME_RESTORE is to have an extra bitmap in the gif struct to hold the restore pixels. When processing a frame with GIF_FRAME_RESTORE set, a copy should be made of the frame before painting in the new pixels. When processing a frame whose /previous/ frame is GIF_FRAME_RESTORE, the frame should be initialised from the previous bitmap. Does this sound reasonable? I could try to make a patch if there's interest. John

On 06/07/2020 10:07, Michael Drake wrote: Please try with this: http://source.netsurf-browser.org/libnsgif.git/commit/?id=8442a27c2bb8df4... Cheers, -- Michael Drake https://www.codethink.co.uk/

Hi John, On 06/07/2020 23:31, jcupitt(a)gmail.com wrote: That sounds like a good idea. If we run into problems with partial gifs we can find another way to display what we have that doesn't involve modifying the input buffer. There are a few reasons for this behaviour. At the moment in NetSurf, if a web page is showing an animated gif, we schedule a callback with our scheduler to happen when the gif frame delay has elapsed. When the callback fires, we simply increment the number of the frame that should be shown inside NetSurf and cause the gif to notify its users that its area is "dirty" and needs to be redrawn. When that bit of the screen is redrawn, the gif redraw code decodes enough frames to get back to where it should be in the animation and shows that frame. An advantage of this is that if there's a gif off-screen further down the page, or on a tab that's not the current tab, we don't have to waste time decoding frames that aren't getting rendered. Another point is that the same gif may be used on many open tabs, and even many times on the same page. NetSurf shares contents, so all these instances share the same libnsgif instance for the animation. This keeps all instances of the animation in lock step, which is visually pleasing as well as efficient. Finally, the libnsgif code was originally stripped out of NetSurf into a separate library, and it still contains some some of this legacy in its API. NetSurf could be made to work with your proposed change, and I agree it's a cleaner API. I probably wouldn't even keep the old `gif_decode_frame()` as deprecated. Just change it. I think its API needs quite a bit of cleaning up before we call it 1.0 and stop making breaking changes. Cheers, -- Michael Drake https://www.codethink.co.uk/