Skip to content

imgcodecs: gif: support animated gif without loop#26971

Merged
asmorkalov merged 9 commits intoopencv:4.xfrom
Kumataro:fix26970
Mar 19, 2025
Merged

imgcodecs: gif: support animated gif without loop#26971
asmorkalov merged 9 commits intoopencv:4.xfrom
Kumataro:fix26970

Conversation

@Kumataro
Copy link
Copy Markdown
Contributor

Close #26970

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

@asmorkalov
Copy link
Copy Markdown
Contributor

@sturkmen72 Is the Looping option relevant to APNG, AVIF, etc? M.b. make it generic.

@sturkmen72
Copy link
Copy Markdown
Contributor

sturkmen72 commented Feb 28, 2025

@sturkmen72 Is the Looping option relevant to APNG, AVIF, etc? M.b. make it generic.

it is possible adding enums

IMWRITE_ANIMATION_LOOP = IMWRITE_GIF_LOOP;
IMWRITE_ANIMATION_SPEED =IMWRITE_GIF_SPEED;

and add relevant code to other encoders
( i can add this feature if requested after this PR merged )

@asmorkalov
Copy link
Copy Markdown
Contributor

IMWRITE_ANIMATION_LOOP = IMWRITE_GIF_LOOP; IMWRITE_ANIMATION_SPEED =IMWRITE_GIF_SPEED;

We have Animation structure. I would say that we need single place for such parameters and drop format specific enums and props.

@sturkmen72
Copy link
Copy Markdown
Contributor

imho

enums
IMWRITE_ANIMATION_LOOP = 1024
IMWRITE_ANIMATION_SPEED = ?

will be useful if these properties wanted to be write with imwrite and imwritemulti functions

up to you dropping IMWRITE_GIF_LOOP if IMWRITE_ANIMATION_LOOP will be defined instead

@sturkmen72
Copy link
Copy Markdown
Contributor

there is also IMWRITE_AVIF_SPEED i should check for what functionality

@sturkmen72
Copy link
Copy Markdown
Contributor

i think IMWRITE_GIF_SPEED is unfunctional now ( overlooked when adopted gif encoder to imwriteanimation )

@Kumataro
Copy link
Copy Markdown
Contributor Author

IMO, IMWRITE_GIF_* should be refactored.

  • IMWRITE_GIF_SPEED sets duration between frames. however IMWRITE_AVIF_SPEED changes blance of compress time and data size. They have similar enum names, but their purposes are different.
  • IMWRITE_GIF_QUALITY and IMWRITE_GIF_DITHER are releated quality of output GIFs, It should be merge into one parameter.
  • IMWRITE_GIF_TRANSPARENCY is hard to image what it effects. Can we use this parameter ?
  • IMWRITE_GIF_COLORTABLE is hard to image what it effets too. Need to introduce parameter specified enum, GIF_WITHOUT_LOCALTABLE or GIF_WITH_LOCALTABLE. But it also hard to use it ....

@sturkmen72
Copy link
Copy Markdown
Contributor

i vote for
dropping IMWRITE_GIF_LOOP and IMWRITE_GIF_SPEED (use writeanimation)

@Kumataro
Copy link
Copy Markdown
Contributor Author

Kumataro commented Mar 1, 2025

I dropped IMWRITE_GIF_SPEED and IMWRITE_GIF_LOOP.

Currently IMWRITE_GIF_RESERVED_0 and IMWRITE_GIF_RESERVED_1 are kept to save values of other IMWRITE_GIF_* enum.

Do we have to remove them and reassign other enums ?

@asmorkalov
Copy link
Copy Markdown
Contributor

@Kumataro I restored constants and added warning.
@sturkmen72 @vrabaud please take a look again.

@Kumataro
Copy link
Copy Markdown
Contributor Author

@asmorkalov Thank you to resolve my worry about IMWRITE_GIF_SPEED and IMWRITE_GIF_LOOP. I agree with you !!

//! Background color of the animation in BGRA format.
CV_PROP_RW Scalar bgcolor;
//! Duration for each frame in milliseconds.
/*! @note (GIF) Due to file format limitation
Copy link
Copy Markdown
Contributor

@sturkmen72 sturkmen72 Mar 15, 2025

Choose a reason for hiding this comment

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

please consider

    /*! @note (GIF) Due to file format limitation
     *  - Durations must be multiples of 10 milliseconds. Any provided value will be rounded down to the nearest 10ms (e.g., 88ms → 80ms).
     *  - 0ms(or smaller than expected in user application) duration may cause undefined behavior, e.g. it is handled with default duration.
     *  - Over 65535 * 10 milliseconds duration is not supported.
     */

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This example is helpful to know about duration about 10ms unit, thank you for your comment !

  • Over 65535 milliseconds duration is not supported.

For GIF89a specification, I think it seems to have to change from 655350.

https://www.w3.org/Graphics/GIF/spec-gif89a.txt

  1. Graphic Control Extension.
    vii) Delay Time - If not 0, this field specifies the number of
    hundredths (1/100) of a second to wait before continuing with the
    processing of the Data Stream. The clock starts ticking immediately
    after the graphic is rendered. This field may be used in
    conjunction with the User Input Flag field.

We can storeDelay Time value as 16 bit unsigned integer with 10ms unit, so a range from 10ms to 655350ms are valid.

        // GIF file stores duration in 10ms unit.
        const int frameDelay10ms = cvRound(frameDelay / 10);
        CV_LOG_IF_WARNING(NULL, (frameDelay10ms == 0),
                          cv::format("frameDelay(%d) is rounded to 0ms, its behaviour is user application depended.", frameDelay));
        CV_CheckLE(frameDelay10ms, 65535, "It requires to be stored in WORD");

Both 65535ms(about 1min) and 655350ms(about 11min) are a very long durations as animation frames.
So this restriction is not expected to affect user experience.

To make compatibility with other codecs, should I limit with CV_CheckLE(frameDelay, 65535, "Unsupported duration") ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both 65535ms(about 1min) and 655350ms(about 11min) are a very long durations as animation frames.
So this restriction is not expected to affect user experience.

To make compatibility with other codecs, should I limit with CV_CheckLE(frameDelay, 65535, "Unsupported duration") >>?

i think the PR as is good.
up to you
using in documentation - Durations must be multiples of 10 milliseconds. Any provided value will be rounded down to the nearest 10ms (e.g., 88ms → 80ms).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reply ! I fixed it and to use "millisecondsinstead ofmillseconds`.

@asmorkalov
Copy link
Copy Markdown
Contributor

@vrabaud Do you have any notes?

@asmorkalov asmorkalov self-assigned this Mar 19, 2025
@asmorkalov asmorkalov merged commit 3e43d0c into opencv:4.x Mar 19, 2025
27 of 28 checks passed
@asmorkalov asmorkalov mentioned this pull request Apr 29, 2025
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.

imgcodecs: gif: support animated gif without loop

4 participants