Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize frame comparison #72

Merged
merged 5 commits into from
Aug 13, 2024
Merged

Optimize frame comparison #72

merged 5 commits into from
Aug 13, 2024

Conversation

tlsa
Copy link
Contributor

@tlsa tlsa commented Jun 24, 2024

Context

I have been creating lots of animated charts lately and I have found that the performance of cgif_addframe is very slow for my use case.

The GIFs I'm creating have large dimensions and many frames. Not very much changes from frame-to-frame. I enable the flag CGIF_FRAME_GEN_USE_DIFF_WINDOW to get small file sizes.

I profiled it and found that most of the time was spent in the cmpPixel function. This function is used in two places:

  1. The checking for the area that actually changed in doWidthHeightOptim. This is done when the CGIF_FRAME_GEN_USE_DIFF_WINDOW flag is set.
  2. The checking for identical frames, which is done when CGIF_GEN_KEEP_IDENT_FRAMES is not set.

Optimization

This pull request changes the library to use a faster comparison if both the current and previous frame use the global palette. When both use the global palette, it compares the pixel data directly. This can allow it to compare whole rows or the whole image with a single memcmp. I tried to keep to the project code style.

Results

These changes make my program run 30 times faster.

Considering just the time spent in CGIF it's 80 times faster.

Questions

There are a couple of things I'm unsure about.

The first is a query about the original behavior of cmpPixel. It looks at the frame before and the current frame, to check if they use the global palette. Is there a case where both the previous frame and the current frame both use the global palette, but some frame before that has used a local palette? If so, it seems like there could be a problem with the approach used in cmpPixel. If it's not correct, maybe the CGIF structure needs to contain a flag indicating whether a local palette has ever been used in an earlier frame, and check that instead of the previous frame. If that was a problem before, then this still PR maintains the same behavior.

Second I'm not sure if my optimization is okay with transIndex.

Any feedback gratefully received.

@dloebl If it would be helpful I could e-mail you an example of the GIFs I'm making. (They're about 50kb).

@tlsa
Copy link
Contributor Author

tlsa commented Jun 24, 2024

Another point is that my program already knows the areas that have changed, so if the cgif_addframe API supported it, I could just pass the rectangle in.

@dloebl dloebl self-requested a review June 26, 2024 23:09
@dloebl
Copy link
Owner

dloebl commented Jun 28, 2024

Thanks for working on this @tlsa! The change makes sense to me. I'm busy right now, but I'll take a closer look at it and address your questions once I have some spare time

@tlsa
Copy link
Contributor Author

tlsa commented Jul 5, 2024

Thanks for the response @dloebl, no worries I'm far too busy too. 😹

I think I've answered my own question by remembering that the cgif API doesn't support setting the x/y offset to the frame within the image. The API means that users always supply a complete coverage of the image when they add a new frame, so it is safe to compare against the previous palette. (So there was no problem with cmpPixel.)

@dloebl
Copy link
Owner

dloebl commented Aug 13, 2024

@tlsa Sorry for the long silence! I finally managed to take a look.

Second I'm not sure if my optimization is okay with transIndex

Yes, user defined transparency (transIndex) won't work with a memcmp. I've added some tests and added a check to skip the memcmp path if user defined transparency is present: see 94a67d1 and f931c91

Another point is that my program already knows the areas that have changed

Unfortunately, as of now the public cgif API doesn't support this use case. You could use the internal cgif_raw API instead: https://github.com/dloebl/cgif/blob/main/inc/cgif_raw.h
The only downsides are that you would have to take care of the transparency optimization yourself and that the raw API isn't stable.

Copy link
Owner

@dloebl dloebl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this!

@dloebl dloebl merged commit c234bc2 into dloebl:main Aug 13, 2024
7 checks passed
@tlsa
Copy link
Contributor Author

tlsa commented Aug 13, 2024

Thanks @dloebl!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants