Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

To check everything is OK before submission of v5 #11

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

osamu620
Copy link
Collaborator

@osamu620 osamu620 commented Sep 10, 2024

  • trailing spaces
  • Squash two commits
  • English usage of commit message

Please review the following commit message.

avcodec/jpeg2000: Fix FF_DWT97_INT to pass the conformance testing defined in ISO/IEC 15444-4

Fix for the integer version of the inverse 9-7 DWT processing (FF_DWT97_INT, https://trac.ffmpeg.org/ticket/10123), which is activated with -flags +bitexact. I went through the code path for the DWT 9-7 transform (integer) and improved precision to match conformance codestream. As a result, the encoded codestream size is slightly larger for a given Q value. For example, -flags +bitexact -i lena.pnm -q: 20 -format j2k -y tmp.j2c gives 13K (HEAD) and 19K (with this patch).

This commit also updates the reference files for affected FATE tests.

@palemieux
Copy link
Contributor

I tweaked the message above.

Running FATE results in

Test j2k-dwt failed. Look at tests/data/fate/j2k-dwt.err for details.

missmatch at 0 (137 != 274) decomp:15 border 151 170 140 183
threads=1

Copy link
Contributor

@palemieux palemieux left a comment

Choose a reason for hiding this comment

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

FATE fails

@osamu620
Copy link
Collaborator Author

I tweaked the message above.

Many thanks. I will change the commit message for the submission.

Running FATE results in

Test j2k-dwt failed. Look at tests/data/fate/j2k-dwt.err for details.

missmatch at 0 (137 != 274) decomp:15 border 151 170 140 183
threads=1

I just remembered that I need to remove 'GEN=1' option with the FATE test. It's my bad :(

Apparently, I have to change libavcodec/tests/jpeg2000dwt.c to adjust the binary point for the integer 9-7 test.
Test code in libavcodec/tests/jpeg2000dwt.c does the simple FDWT/IDWT tests. This won't work for the improved 9-7 DWT with the commit because the binary points at forward and inverse DWT are not the same.

I will update libavcodec/tests/jpeg2000dwt.c. Do you agree on this course of action?

@palemieux
Copy link
Contributor

I will update libavcodec/tests/jpeg2000dwt.c. Do you agree on this course of action?

Yes!

@osamu620
Copy link
Collaborator Author

I will update libavcodec/tests/jpeg2000dwt.c. Do you agree on this course of action?

Yes!

Ok, I will tackle this today. I hope it won't take long.

@osamu620
Copy link
Collaborator Author

I have updated the test code libavcodec/tests/jpeg2000dwt.c.
Pre-scaling to simulate dequantization has been introduced in the test code for FF_DWT97_INT.

The results (/tests/ref/fate/j2k-dwt) seem OK because the sum of squared error (err2) is much smaller than HEAD.

@osamu620
Copy link
Collaborator Author

The FATE tests on HEAD are OK. I will send this patch to you first, just in case, and then to the FFMPEG-devel.

…fined in ISO/IEC 15444-4

Fix for the integer version of the inverse 9-7 DWT processing
(FF_DWT97_INT, https://trac.ffmpeg.org/ticket/10123), which is activated with
`-flags +bitexact`.

I went through the code path for the DWT 9-7 transform (integer) and improved
precision to match conformance codestream.

As a result, the encoded codestream size is slightly larger for a given Q value.
For example, `-flags +bitexact -i lena.pnm -q: 20 -format j2k -y tmp.j2c`
gives 13K (HEAD) and 19K (with this patch).

This commit also updates the source and reference files for affected FATE tests.
@palemieux
Copy link
Contributor

... I was running test and, evidently, the change in step size affects both bitexact and not bitexact. This, as you note, result in a change of file size given a particular Q value.

I am asking since the patch is intended to address the bitexact bug, but somehow it is also affecting all code paths.

Is there a way for the patch to affect only the +bitexact code path, or does it also fix an issue for code path where -bitexact?

@osamu620 osamu620 force-pushed the patches/fix-FF_DWT97_INT-v5-submitted branch from 298f3ab to 0f3b233 Compare September 13, 2024 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants