search

dloebl / cgif

GIF encoder written in C
MIT License
113 stars 15 forks source link

Simplify LZW-code length calculation #16

Closed kleisauke closed 3 years ago

kleisauke commented 3 years ago

This supersedes PR #15, as that doesn't seem to generate identical images and doesn't work for MSVC.

MCLoebl commented 3 years ago

We appreciate your previous PRs! Thanks for starting the discussion on calcInitCodeLen with PR #15. We agree that one should modify PR #15 since the case of one or two color tables must be covered as well. I think the following implementation of calcInitCodeLen / calcInitCodeSize is probably the best to keep things short and simple as well as compatible with the C99 standard (For the overall speed of the program, this function does anyway play practically no role):

static uint8_t calcInitCodeLen(uint16_t numEntries) {
  int index = 0;
  while (numEntries > (1uL << index)) ++index;
  return (index < 3) ? 3 : index + 1;
}

This function works with the current main branch. If you want it to be compatible with your changes in the function cgif_addframe, you would have to change the return value by 1. But we would prefer to keep the function return the same as in the main branch. The reason is that it is really the number of bits that represent one LZW-code (also in the final GIF-file). If we change it by one that would be a bit confusing.

kleisauke commented 3 years ago

Thanks for the review! I've updated the PR to avoid using GCC/Clang built-in functions (which is indeed not very portable) and to ensure the calcInitCodeLen function returns the same values as on the main branch. The other minor changes to initMainHeader, cgif_newgif and cgif_addframe are still retained, but I can also revert them if that is preferred.