Hi John, On 06/07/2020 23:31, jcup...@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-le...@netsurf-browser.org
