imgcodecs: gif: support Disposal Method by Kumataro · Pull Request #26930 · opencv/opencv

Kumataro · 2025-02-17T14:23:58Z

Pull Request Readiness Checklist

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

Kumataro · 2025-02-17T14:29:07Z

Now this pull request is draft because we have to add tests for Disposal Method = 3.

kmtr@kmtr-VMware-Virtual-Platform:~/work/opencv_extra4/testdata/highgui/gifsuite$ git show
commit 727770994fe49acca31b8d7ffe8653febed67cb5 (HEAD -> 4.x, origin/master, origin/HEAD, origin/4.x)
Merge: eddc7acc 8ff6cb8a
Author: Alexander Smorkalov <2536374+asmorkalov@users.noreply.github.com>
Date:   Fri Feb 14 19:40:11 2025 +0300

    Merge pull request #1236 from asmorkalov:as/openvx_hal_features2d

    Regression data for FAST OpenVX case.

kmtr@kmtr-VMware-Virtual-Platform:~/work/opencv_extra4/testdata/highgui/gifsuite$ gifbuild -d *.gif | grep disposal | sort | uniq -c
      7         disposal mode 0
     87         disposal mode 1
     10         disposal mode 2

sturkmen72 · 2025-02-17T15:19:45Z

i can confirm the PR seems fixed the bug.

modules/imgcodecs/src/grfmt_gif.hpp

modules/imgcodecs/src/grfmt_gif.cpp

CMakeLists.txt

This reverts commit c98db2c.

Kumataro · 2025-02-20T23:18:40Z

I feel more refactoring seems be needed from current GifEncoder/Decoder.
Here are some points I noticed. I think these should be fixed separately from this fix.

opencv/modules/imgcodecs/src/grfmt_gif.cpp

Lines 516 to 536 in 7a2b048

    
           GifEncoder::GifEncoder() { 
        
               m_description = "Graphics Interchange Format 89a(*.gif)"; 
        
               m_height = 0, m_width = 0; 
        
               width = 0, height = 0, top = 0, left = 0; 
        
               m_buf_supported = true; 
        
               opMode = GRFMT_GIF_Cover; 
        
               transparentColor = 0; // index of the transparent color, default 0. currently it is a constant number 
        
               transparentRGB = Vec3b(0, 0, 0); // the transparent color, default black 
        
               lzwMaxCodeSize = 12; // the maximum code size, default 12. currently it is a constant number 
        
               // default value of the params 
        
               fast = true; 
        
               loopCount = 0; // infinite loops by default 
        
               criticalTransparency = 1; // critical transparency, default 1, range from 0 to 255, 0 means no transparency 
        
               frameDelay = 5; // 20fps by default, 10ms per unit 
        
               bitDepth = 8; // the number of bits per pixel, default 8, currently it is a constant number 
        
               lzwMinCodeSize = 8; // the minimum code size, default 8, this changes as the color number changes 
        
               colorNum = 256; // the number of colors in the color table, default 256 
        
               dithering = 0; // the level dithering, default 0 
        
               globalColorTableSize = 256, localColorTableSize = 0; 
        
           }

Member variables in Encoder/Decoder should be started "m_" prefix. Now it is hard to know which is it function local variable or not.
readExtensions() should be rename readGraphiControlExtension(). Now it doesn't handle other extensions.
uchar should be uint8_t.
"flags" variable is used for "Packed Fields" for some Descriptor and Extensions. But it is hard to know what does means.
GifEncoder has both (A) m_height/m_width which means "Logical Screen Width/Height" in "Logical Screen Descriptor" , and (B) height/width which means "Image Width/Height" in "Image Descriptor". I feel it is natural to introduce innner strcutors to split information.

class GifEncoder {
 + struct ImageDescriptor {
    + imageWidth;
    + imageHeight; };
 + struct LogicalScreenDescriptor {
    + logicalScreenWidth;
    + logicalScreenHeight; };
}

Kumataro · 2025-02-21T08:24:48Z

Sorry I test with ./bin/opencv_test_imgcodecs --gtest_filter="*Gif* command, so I miss this error.

     const uint8_t gcePackedFields = (GIF_DISPOSE_RESTORE_PREVIOUS << GIF_DISPOSE_METHOD_SHIFT) |
-                                    (criticalTransparency) ? GIF_TRANSPARENT_INDEX_GIVEN : GIF_TRANSPARENT_INDEX_NOT_GIVEN;
+                                    (criticalTransparency ? GIF_TRANSPARENT_INDEX_GIVEN : GIF_TRANSPARENT_INDEX_NOT_GIVEN);

It seems this code works following.

const uint8_t gcePackedFields =
  ( (GIF_DISPOSE_RESTORE_PREVIOUS << GIF_DISPOSE_METHOD_SHIFT) | criticalTransparency) )
  ? 
  GIF_TRANSPARENT_INDEX_GIVEN : 
  GIF_TRANSPARENT_INDEX_NOT_GIVEN;
- ```

asmorkalov

Looks good to me. Need to check possible out-of-bound access mentioned bellow.

modules/imgcodecs/src/grfmt_gif.cpp

asmorkalov · 2025-02-26T13:06:43Z

modules/imgcodecs/test/test_gif.cpp

+TEST(Imgcodecs_Gif, decode_disposal_method)
+{
+    const string root = cvtest::TS::ptr()->get_data_path();
+    const string filename = root + "gifsuite/disposalMethod.gif";


Looks like it's a new file. Please create PR to opencv_extra repo with the same branch name.

I'm sorry I created branch which has same name this patch so its test is passed, but I forget to create PR.

Please could you merge this pull request ? opencv/opencv_extra#1239

Kumataro added 3 commits February 17, 2025 23:12

imgcodecs: gif: support Disposal Method

1c08353

remove unnecessary line breaks

b74dc87

Restore line breaks

eeb7d40

asmorkalov added feature category: imgcodecs labels Feb 17, 2025

sturkmen72 reviewed Feb 17, 2025

View reviewed changes

modules/imgcodecs/src/grfmt_gif.hpp Outdated Show resolved Hide resolved

Kumataro added 3 commits February 19, 2025 18:41

Rename to GifDisposeMethod enum and add test.

4743de6

Temporary enable WITH_IMGCODEC_GIF for test

c98db2c

Use RestoreToPrevious to save gif

1c711e1

sturkmen72 suggested changes Feb 19, 2025

View reviewed changes

modules/imgcodecs/src/grfmt_gif.cpp Outdated Show resolved Hide resolved

Kumataro marked this pull request as ready for review February 19, 2025 10:55

asmorkalov reviewed Feb 20, 2025

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

Kumataro added 2 commits February 20, 2025 22:20

Revert "Temporary enable WITH_IMGCODEC_GIF for test"

1df4759

This reverts commit c98db2c.

Reduce masks and shifts using magic numbers

22bce43

Kumataro added 2 commits February 21, 2025 17:30

Fixed a mistake in processing using ternary operators

e7e7d51

fix for warning C4244

fc35d8e

asmorkalov added this to the 4.12.0 milestone Feb 25, 2025

asmorkalov self-assigned this Feb 25, 2025

asmorkalov approved these changes Feb 25, 2025

View reviewed changes

modules/imgcodecs/src/grfmt_gif.cpp Outdated Show resolved Hide resolved

revert to clip drawing frame

892119f

asmorkalov requested changes Feb 26, 2025

View reviewed changes

asmorkalov approved these changes Feb 26, 2025

View reviewed changes

asmorkalov merged commit a63ede6 into opencv:4.x Feb 26, 2025
28 checks passed

asmorkalov mentioned this pull request Mar 4, 2025

5.x merge 4.x #27009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

imgcodecs: gif: support Disposal Method#26930

imgcodecs: gif: support Disposal Method#26930
asmorkalov merged 11 commits intoopencv:4.xfrom
Kumataro:fix26924

Kumataro commented Feb 17, 2025 •

edited

Loading

Uh oh!

Kumataro commented Feb 17, 2025

Uh oh!

sturkmen72 commented Feb 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kumataro commented Feb 20, 2025

Uh oh!

Kumataro commented Feb 21, 2025

Uh oh!

asmorkalov left a comment

Uh oh!

Uh oh!

asmorkalov Feb 26, 2025

Uh oh!

Kumataro Feb 26, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Kumataro commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Kumataro commented Feb 17, 2025

Uh oh!

sturkmen72 commented Feb 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kumataro commented Feb 20, 2025

Uh oh!

Kumataro commented Feb 21, 2025

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asmorkalov Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

Kumataro Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kumataro commented Feb 17, 2025 •

edited

Loading

Kumataro Feb 26, 2025 •

edited

Loading