Re: possible bug: libnsgif does not handle DISPOSE_PREVIOUS correctly

Hi John,

On 06/07/2020 23:31, jcupitt@gmail.com wrote:

> First, line 438 modifies the input buffer:
>
> http://source.netsurf-browser.org/libnsgif.git/tree/src/libnsgif.c?id=8442a27c2bb8df48029ceea6e64c4930106a57fc#n438
>
> This feels a bit ugly to me, and it doesn't work for us, since the
> input buffer can be a mmaped file. I patched this to just return
> GIF_INSUFFICIENT_FRAME_DATA.

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.

> Second, gif_decode_frame(gif, frame_number) seems like the wrong API.
> You can only call this function with frame_number ascending, and you
> must always start from 0 and loop, but this restriction is not obvious
> from the function.

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.

> How about having frame_number as a member, and renaming it to
> gif_decode_next_frame(gif)? It could handle looping too.
>
> gif_decode_frame() could be marked as deprecated, and just call
> gif_decode_next_frame() without using the frame_number parameter. It
> should be compatible and the API would be simpler and clearer. I may
> well have misunderstood things, of course
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/
_______________________________________________
netsurf-dev mailing list -- netsurf-dev@netsurf-browser.org
To unsubscribe send an email to netsurf-dev-leave@netsurf-browser.org