1. Please add solution description to the PR. It'll be great to have something relevant in git history.
2. What if add cv::Mat cover for the "static" frame and totally exclude it from frames and timestamps. IMHO it's more consistent in terms of API.

1. Please add solution description to the PR. It'll be great to have something relevant in git history.
2. What if add cv::Mat cover for the "static" frame and totally exclude it from frames and timestamps. IMHO it's more consistent in terms of API.

i will add detailed description soon.

APNG file format can be in two different chunk order

PNG Signature
IHDR
acTL (Animation control chunk indicating the number of frames and plays)
fcTL (first animated frame control)
IDAT (first animated frame data)
fcTL (second animated frame control)
fdAT (second animated frame data)
fcTL (third animated frame control)
fdAT (third animated frame data)
IEND

PNG Signature
IHDR
acTL (Animation control chunk indicating the number of frames and plays)
IDAT (first frame - hidden frame)
fcTL (first animated frame control)
fdAT (first animated frame data)
fcTL (second animated frame control)
fdAT (second animated frame data)
IEND

in the proposed solution new property has_hidden_frame added to cv::Animation. When the format is apng 2. format imreadanimation sets Animation.has_hidden_frame as true.

imwriteanimation if Animation.has_hidden_frame is true the apng will be saved in 2. format. Also when writing in other animation formats such as GIF AVIF WEBP the firs frame will not written.

this is the original APNG

this is saved by OpenCV before this pach

After the patch:

Consecutive imreadanimation and imwriteanimation operations will preserve the original chunk order.

When converting to other formats (GIF, AVIF, WEBP), the hidden frame will be omitted as it's not needed in those formats.

frankly i tried to add some explanation to imreadanimation and imwriteanimation about the changes in the PR. but it is hard to explain briefly. let me wait @vrabaud remarks. also i wonder what @Kumataro thinks about this PR

@vrabaud @Kumataro From my point of view, an existing bug has been fixed with the solution provided. And this solution has been provided with a small amount of code. If the solution will fixed as you suggest, there will be more code and complications.

let me try to provide more documentation about the PR and request review again

also i will add CV_PROP_RW cv::Mat still_image; solution to compare

Looks good in general. Please add test case like you have in the issue report.

@asmorkalov the PR is still WIP. no need to start CI yet.

@vrabaud could you help about 16U blending
original 16 bit apng :

imreadanimation & imwriteanimation result (before last changes i made) :

imreadanimation & imwriteanimation result (after last changes i made)

The reason for the failed test is that before this patch, imread was reading the IDAT chunk of the APNG file. After this patch, imread now reads the fDAT chunk because IDAT is interpreted as a still image.

Indeed, the values read now are correct (the test files first frame have all zeros), but comparing these values to just zeros in the test seems a bit meaningless. What’s your opinion on this test?

List failed tests (first 10):
Imgcodecs_APNG.imread_animation_16u :
/build/precommit_linux64/4.x/opencv/modules/imgcodecs/test/test_animation.cpp:681: Failure
Expected equality of these values:
  65280
  img.at(0, 2)
    Which is: 0
/build/precommit_linux64/4.x/opencv/modules/imgcodecs/test/test_animation.cpp:687: Failure
Expected equality of these values:
  76
  img.at(0, 0)
    Which is: '\0'
/build/precommit_linux64/4.x/opencv/modules/imgcodecs/test/test_animation.cpp:694: Failure
Expected equality of these values:
  255
  img.at(0, 2)
    Which is: '\0'
/build/precommit_linux64/4.x/opencv/modules/imgcodecs/test/test_animation.cpp:699: Failure
Expected equality of these values:
  255
  img.at(0, 0)
    Which is: '\0'
/build/precommit_linux64/4.x/opencv/modules/imgcodecs/test/test_animation.cpp:706: Failure
Expected equality of these values:
  19519
  img.at(0, 0)
    Which is: 0
/build/precommit_linux64/4.x/opencv/modules/imgcodecs/test/test_animation.cpp:713: Failure
Expected equality of these values:
  65280
  img.at(0, 2)
    Which is: 0
/build/precommit_linux64/4.x/opencv/modules/imgcodecs/test/test_animation.cpp:718: Failure
Expected equality of these values:
  65280
  img.at(0, 0)
    Which is: 0

The alpha compositing is tricky when the alphas are both non full. https://ciechanow.ski/alpha-compositing/ expiains it well. Your formula in uint16_t and uint8_t should be the same.

Looks good to me! @vrabaud Do you have any comments?

imread reads before (hidden frame)

imread reads now (first frame)

this is expected.
also imcount returns only frames count. before this PR imcount was returning frames count + ( 1 if the first frame hidden)

I would say that "this is expected.
also imcount returns only frames count. before this PR imcount was returning frames count + ( 1 if the first frame hidden)" is bad idea. I propose to revert behaviour of imread to the original version.

I would say that "this is expected. also imcount returns only frames count. before this PR imcount was returning frames count + ( 1 if the first frame hidden)" is bad idea. I propose to revert behaviour of imread to the original version.

sorry for confusing. i think i give a wrong info "before this PR imcount was returning frames count + ( 1 if the first frame hidden)"

let me prepare detailed before and after report soon.

imcount change is good. I propose to return still_image from imread (preview frame), but no the first animation frame. It's APNG design solution and it's previous behaviour.

@sturkmen72 Thanks a lot for the updates. I rebased your branch on current 4.x and added a test imread vs Animation.stll_image to ensure that existing behaviour was presumed. The implementation is functional and pass all tests. Looks good to me in general. The only implementation detail that bothers me is readData recursion and setjmp inside of it. In case of longjump from libpng we get a lot leaked objects. @vrabaud What do you think about it?

@sturkmen72 Thanks a lot for the updates. I rebased your branch on current 4.x and added a test imread vs Animation.stll_image to ensure that existing behaviour was presumed. The implementation is functional and pass all tests. Looks good to me in general. The only implementation detail that bothers me is readData recursion and setjmp inside of it. In case of longjump from libpng we get a lot leaked objects. @vrabaud What do you think about it?

@asmorkalov let me work a bit more on this weekend to solve memory leaks

Great job! Thanks @sturkmen72 for the contribution.