Skip to content

Comments

fix bugs in GIF decoding#26738

Merged
asmorkalov merged 5 commits intoopencv:4.xfrom
redhecker:fix
Jan 11, 2025
Merged

fix bugs in GIF decoding#26738
asmorkalov merged 5 commits intoopencv:4.xfrom
redhecker:fix

Conversation

@redhecker
Copy link
Contributor

@redhecker redhecker commented Jan 8, 2025

Pull Request Readiness Checklist

this is related to #25691

i solved two bugs here:

  1. the decoding setting:
    according to https://www.w3.org/Graphics/GIF/spec-gif89a.txt
    DEFERRED CLEAR CODE IN LZW COMPRESSION

    There has been confusion about where clear codes can be found in the
    data stream.  As the specification says, they may appear at anytime.  There
    is not a requirement to send a clear code when the string table is full.

    It is the encoder's decision as to when the table should be cleared.  When
    the table is full, the encoder can chose to use the table as is, making no
    changes to it until the encoder chooses to clear it.  The encoder during
    this time sends out codes that are of the maximum Code Size.

    As we can see from the above, when the decoder's table is full, it must
    not change the table until a clear code is received.  The Code Size is that
    of the maximum Code Size.  Processing other than this is done normally.

    Because of a large base of decoders that do not handle the decompression in
    this manner, we ask developers of GIF encoding software to NOT implement
    this feature until at least January 1991 and later if they see that their
    particular market is not ready for it.  This will give developers of GIF
    decoding software time to implement this feature and to get it into the
    hands of their clients before the decoders start "breaking" on the new
    GIF's.  It is not required that encoders change their software to take
    advantage of the deferred clear code, but it is for decoders.

at first i didn't consider this case, thus leads to a bug discussed in #25691. the changes made in function lzwDecode() is aiming at solving this.

  1. the fetch method of loopCount:
    in the codes at https://github.com/opencv/opencv/blob/4.x/modules/imgcodecs/src/grfmt_gif.cpp#L410, if the branch is taken, 3 more bytes will be taken, leading to unpredictable behavior.

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

// clear code
if (!(code ^ clearCode)) {
lzwExtraTable.clear();
lzwExtraTable.resize((1 << 12) + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not lzwTableSize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because (1<<12) is the maximum size of lzwtable, thus i use (1<<12)+1 here

@asmorkalov
Copy link
Contributor

@sturkmen72 Could you take a look too?

@redhecker
Copy link
Contributor Author

i add some warning logs for unexpected cases and turn 1<<12 to a const value

@sturkmen72
Copy link
Contributor

@redhecker thank you. @asmorkalov Looks perfect to me.

@asmorkalov asmorkalov self-assigned this Jan 10, 2025
@asmorkalov asmorkalov added this to the 4.12.0 milestone Jan 10, 2025
@asmorkalov
Copy link
Contributor

Run cd /home/ci/opencv
modules/imgcodecs/src/grfmt_gif.cpp:347: trailing whitespace.
+            //    * notice that if the lzw table size is full, 
Error: Process completed with exit code 2.

@asmorkalov asmorkalov merged commit 2c2866a into opencv:4.x Jan 11, 2025
29 of 30 checks passed
JSpencerPittman pushed a commit to JSpencerPittman/opencv that referenced this pull request Jan 13, 2025
Fix bugs in GIF decoding opencv#26738 

### Pull Request Readiness Checklist

this is related to opencv#25691 

i solved two bugs here:

1. the decoding setting:
according to [https://www.w3.org/Graphics/GIF/spec-gif89a.txt](https://www.w3.org/Graphics/GIF/spec-gif89a.txt)

```
    DEFERRED CLEAR CODE IN LZW COMPRESSION

    There has been confusion about where clear codes can be found in the
    data stream.  As the specification says, they may appear at anytime.  There
    is not a requirement to send a clear code when the string table is full.

    It is the encoder's decision as to when the table should be cleared.  When
    the table is full, the encoder can chose to use the table as is, making no
    changes to it until the encoder chooses to clear it.  The encoder during
    this time sends out codes that are of the maximum Code Size.

    As we can see from the above, when the decoder's table is full, it must
    not change the table until a clear code is received.  The Code Size is that
    of the maximum Code Size.  Processing other than this is done normally.

    Because of a large base of decoders that do not handle the decompression in
    this manner, we ask developers of GIF encoding software to NOT implement
    this feature until at least January 1991 and later if they see that their
    particular market is not ready for it.  This will give developers of GIF
    decoding software time to implement this feature and to get it into the
    hands of their clients before the decoders start "breaking" on the new
    GIF's.  It is not required that encoders change their software to take
    advantage of the deferred clear code, but it is for decoders.
```
at first i didn't consider this case, thus leads to a bug discussed in opencv#25691. the changes made in function lzwDecode() is aiming at solving this.

2. the fetch method of loopCount:
in the codes at https://github.com/opencv/opencv/blob/4.x/modules/imgcodecs/src/grfmt_gif.cpp#L410, if the branch is taken, 3 more bytes will be taken, leading to unpredictable behavior.

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [ ] The feature is well documented and sample code can be built with the project CMake
JSpencerPittman pushed a commit to JSpencerPittman/opencv that referenced this pull request Jan 13, 2025
Fix bugs in GIF decoding opencv#26738 

### Pull Request Readiness Checklist

this is related to opencv#25691 

i solved two bugs here:

1. the decoding setting:
according to [https://www.w3.org/Graphics/GIF/spec-gif89a.txt](https://www.w3.org/Graphics/GIF/spec-gif89a.txt)

```
    DEFERRED CLEAR CODE IN LZW COMPRESSION

    There has been confusion about where clear codes can be found in the
    data stream.  As the specification says, they may appear at anytime.  There
    is not a requirement to send a clear code when the string table is full.

    It is the encoder's decision as to when the table should be cleared.  When
    the table is full, the encoder can chose to use the table as is, making no
    changes to it until the encoder chooses to clear it.  The encoder during
    this time sends out codes that are of the maximum Code Size.

    As we can see from the above, when the decoder's table is full, it must
    not change the table until a clear code is received.  The Code Size is that
    of the maximum Code Size.  Processing other than this is done normally.

    Because of a large base of decoders that do not handle the decompression in
    this manner, we ask developers of GIF encoding software to NOT implement
    this feature until at least January 1991 and later if they see that their
    particular market is not ready for it.  This will give developers of GIF
    decoding software time to implement this feature and to get it into the
    hands of their clients before the decoders start "breaking" on the new
    GIF's.  It is not required that encoders change their software to take
    advantage of the deferred clear code, but it is for decoders.
```
at first i didn't consider this case, thus leads to a bug discussed in opencv#25691. the changes made in function lzwDecode() is aiming at solving this.

2. the fetch method of loopCount:
in the codes at https://github.com/opencv/opencv/blob/4.x/modules/imgcodecs/src/grfmt_gif.cpp#L410, if the branch is taken, 3 more bytes will be taken, leading to unpredictable behavior.

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [ ] The feature is well documented and sample code can be built with the project CMake
@asmorkalov asmorkalov mentioned this pull request Jan 15, 2025
shyama7004 pushed a commit to shyama7004/opencv that referenced this pull request Jan 20, 2025
Fix bugs in GIF decoding opencv#26738 

### Pull Request Readiness Checklist

this is related to opencv#25691 

i solved two bugs here:

1. the decoding setting:
according to [https://www.w3.org/Graphics/GIF/spec-gif89a.txt](https://www.w3.org/Graphics/GIF/spec-gif89a.txt)

```
    DEFERRED CLEAR CODE IN LZW COMPRESSION

    There has been confusion about where clear codes can be found in the
    data stream.  As the specification says, they may appear at anytime.  There
    is not a requirement to send a clear code when the string table is full.

    It is the encoder's decision as to when the table should be cleared.  When
    the table is full, the encoder can chose to use the table as is, making no
    changes to it until the encoder chooses to clear it.  The encoder during
    this time sends out codes that are of the maximum Code Size.

    As we can see from the above, when the decoder's table is full, it must
    not change the table until a clear code is received.  The Code Size is that
    of the maximum Code Size.  Processing other than this is done normally.

    Because of a large base of decoders that do not handle the decompression in
    this manner, we ask developers of GIF encoding software to NOT implement
    this feature until at least January 1991 and later if they see that their
    particular market is not ready for it.  This will give developers of GIF
    decoding software time to implement this feature and to get it into the
    hands of their clients before the decoders start "breaking" on the new
    GIF's.  It is not required that encoders change their software to take
    advantage of the deferred clear code, but it is for decoders.
```
at first i didn't consider this case, thus leads to a bug discussed in opencv#25691. the changes made in function lzwDecode() is aiming at solving this.

2. the fetch method of loopCount:
in the codes at https://github.com/opencv/opencv/blob/4.x/modules/imgcodecs/src/grfmt_gif.cpp#L410, if the branch is taken, 3 more bytes will be taken, leading to unpredictable behavior.

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [ ] The feature is well documented and sample code can be built with the project CMake
NanQin555 pushed a commit to NanQin555/opencv that referenced this pull request Feb 24, 2025
Fix bugs in GIF decoding opencv#26738 

### Pull Request Readiness Checklist

this is related to opencv#25691 

i solved two bugs here:

1. the decoding setting:
according to [https://www.w3.org/Graphics/GIF/spec-gif89a.txt](https://www.w3.org/Graphics/GIF/spec-gif89a.txt)

```
    DEFERRED CLEAR CODE IN LZW COMPRESSION

    There has been confusion about where clear codes can be found in the
    data stream.  As the specification says, they may appear at anytime.  There
    is not a requirement to send a clear code when the string table is full.

    It is the encoder's decision as to when the table should be cleared.  When
    the table is full, the encoder can chose to use the table as is, making no
    changes to it until the encoder chooses to clear it.  The encoder during
    this time sends out codes that are of the maximum Code Size.

    As we can see from the above, when the decoder's table is full, it must
    not change the table until a clear code is received.  The Code Size is that
    of the maximum Code Size.  Processing other than this is done normally.

    Because of a large base of decoders that do not handle the decompression in
    this manner, we ask developers of GIF encoding software to NOT implement
    this feature until at least January 1991 and later if they see that their
    particular market is not ready for it.  This will give developers of GIF
    decoding software time to implement this feature and to get it into the
    hands of their clients before the decoders start "breaking" on the new
    GIF's.  It is not required that encoders change their software to take
    advantage of the deferred clear code, but it is for decoders.
```
at first i didn't consider this case, thus leads to a bug discussed in opencv#25691. the changes made in function lzwDecode() is aiming at solving this.

2. the fetch method of loopCount:
in the codes at https://github.com/opencv/opencv/blob/4.x/modules/imgcodecs/src/grfmt_gif.cpp#L410, if the branch is taken, 3 more bytes will be taken, leading to unpredictable behavior.

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [ ] The feature is well documented and sample code can be built with the project CMake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants