This is a master bug for discussing improvements for animated GIFs. A typical example is this page: http://www.spc.noaa.gov/misc/AbtDerechos/casepages/jun292012page.htm
Based on the dicussion with pkasting@ there are a couple things we can do here in subsequent steps: 1. GIFImageDecoder::frameCount() shouldn't redecode for every new byte received This is captured in https://bugs.webkit.org/show_bug.cgi?id=98098. The details of this improvement is that GIFImageReader can parse the entire file and get the frame count progressively without decoding. It will save decoding information along the way. This means subsequent decoding can be much cheaper and file is parsed once. The above bug has a preliminary patch implements this parse-once-decode-many algorithm. It is a substantial change and needs a lot of unittesting to back it. 2. GIFImageDecoder shouldn't be destroyed when cached is cleared Current implementation of ImageSource.cpp is that it deletes the entire GIFImageDecoder when it is instructed to remove all frame cache. If (1) is landed redecode will be cheaper without parsing. 3. Simplify caching logic for multiframe GIFs The logic for multiframe caching is too complicated. It has this clear-before-frame logic that allows some of the frames to be removed from cache. This seems overly complicated and doesn't do much. This logic also makes it much harder for us to implement frame skipping. I propose a simple logic that we decide upfront to cache everything for small GIFs or only the last frame (and background frame) for large GIFs. 4. Use parsed frame information to skip frames When a paused GIF image is bring into viewport the image leaps to the current frame. We don't need to decode all frames in between and can safely skip DisposeOverwriteBgcolor and DisposeOverwritePrevious.
(In reply to comment #1) > 2. GIFImageDecoder shouldn't be destroyed when cached is cleared > > Current implementation of ImageSource.cpp is that it deletes the entire GIFImageDecoder when it is instructed to remove all frame cache. If (1) is landed redecode will be cheaper without parsing. I'm not convinced we should do this. "Destroy everything" seems like it should mean "destroy everything". We already have some granularity in our clearing logic that lets us nuke the framebuffer data but keep the metadata. It's not obvious anything else should change here. > 3. Simplify caching logic for multiframe GIFs > > The logic for multiframe caching is too complicated. It has this clear-before-frame logic that allows some of the frames to be removed from cache. This seems overly complicated and doesn't do much. This logic also makes it much harder for us to implement frame skipping. This all seems untrue. The clear-before-frame logic is precisely how we implement keeping just the most recent frames, so it's not the case that it "doesn't do much". I don't see how it's overly complicated (for what it needs to do) or makes frame skipping hard, either. I think the code here is doing what you want and need it to do already. There are another two items you didn't cover. (5) We should extend the frame metadata to save the byte offsets within the file at which each frame begins. This way, decode passes 2+ can avoid having to parse anything out of the file to skip through it. (6) If we pay attention while writing the pixels to the frame buffer, we can detect cases where the frame is marked as needing the previous frame but in fact draws over all of it. Then we can mark these as "dispose overwrite previous" in our metadata and allow for more frameskipping in future passes.
> This all seems untrue. The clear-before-frame logic is precisely how we implement keeping just the most recent frames, so it's not the case that it "doesn't do much". I don't see how it's overly complicated (for what it needs to do) or makes frame skipping hard, either. I probably has a bad understanding of what this logic does. My point is to have this logic of "keep the last frame necessary" exist only in the decoder. clearFrameBufferCache(clearBeforeFrame) is always called with clearBeforeFrame = currentFrame, would it be simpler to just have a clearFramesBeforeLast() method? In the API level this doesn't give control of what frames to be cleared to users of the GIF decoder. Under the hood GIFImageDecoder is still doing what's doing now and this will be a simple API change. > > There are another two items you didn't cover. > > (5) We should extend the frame metadata to save the byte offsets within the file at which each frame begins. This way, decode passes 2+ can avoid having to parse anything out of the file to skip through it. This is the same optimizations I listed in (2) but a different implementation. Instead of saving this byte offsets as metadata in BitmapImage and destroy ImageDecoder I suggest we push the handling of clear(destroyAll = true) down to each decoder. For formats other than GIF this can be implemented by destroying the corresponding ImageReader. For GIF it handles by keeping a copy of frame offsets. I think this optimization of saving byte offsets is specific to GIF and should be kept in GIFImageDecoder. > > (6) If we pay attention while writing the pixels to the frame buffer, we can detect cases where the frame is marked as needing the previous frame but in fact draws over all of it. Then we can mark these as "dispose overwrite previous" in our metadata and allow for more frameskipping in future passes. Agreed.
(In reply to comment #3) > > This all seems untrue. The clear-before-frame logic is precisely how we implement keeping just the most recent frames, so it's not the case that it "doesn't do much". I don't see how it's overly complicated (for what it needs to do) or makes frame skipping hard, either. > > I probably has a bad understanding of what this logic does. My point is to have this logic of "keep the last frame necessary" exist only in the decoder. clearFrameBufferCache(clearBeforeFrame) is always called with clearBeforeFrame = currentFrame, would it be simpler to just have a clearFramesBeforeLast() method? In the API level this doesn't give control of what frames to be cleared to users of the GIF decoder. > > Under the hood GIFImageDecoder is still doing what's doing now and this will be a simple API change. But the decoder doesn't know what frame the BitmapImage is on. (It isn't guaranteed to be the same as the decoder's last frame.) > > (5) We should extend the frame metadata to save the byte offsets within the file at which each frame begins. This way, decode passes 2+ can avoid having to parse anything out of the file to skip through it. > > This is the same optimizations I listed in (2) but a different implementation. > > Instead of saving this byte offsets as metadata in BitmapImage and destroy ImageDecoder I suggest we push the handling of clear(destroyAll = true) down to each decoder. For formats other than GIF this can be implemented by destroying the corresponding ImageReader. For GIF it handles by keeping a copy of frame offsets. I think this optimization of saving byte offsets is specific to GIF and should be kept in GIFImageDecoder. But there isn't a separate reader class for some of these types. And the decoder carries around state that is needless and wastes memory. Plus formats like ICO have multiple frames and could thus use the frame offsets too, to avoid re-parsing the header. Our frame metadata already carries enough other state that I don't see adding this as problematic.