-
Notifications
You must be signed in to change notification settings - Fork 0
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
avcodec/jpeg2000: Fix FF_DWT97_INT to pass the conformance testing defined in ISO/IEC 15444-4 #12
base: master
Are you sure you want to change the base?
Conversation
bdc2d08
to
fcd1485
Compare
@@ -268,7 +268,7 @@ static void init_band_stepsize(AVCodecContext *avctx, | |||
av_log(avctx, AV_LOG_ERROR, "stepsize out of range\n"); | |||
} | |||
|
|||
band->i_stepsize = lrint(band->f_stepsize * (1 << 15) + 0.5f); | |||
band->i_stepsize = (int)floorf(band->f_stepsize * (1 << 15)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@palemieux With this correction, I got the following results.
❯ ./dwt97-diff.sh ~/Documents/data/images/ppm/ElephantDream_small.ppm 0
.rw-rw-r-- 1,497,613 osamu 21 Oct 16:39 encoded-HEAD.j2c
.rw-rw-r-- 1,498,160 osamu 21 Oct 16:39 encoded-PATCH.j2c
PSNR decoded by exact-HEAD
90.2224 (0.902224)
PSNR decoded by exact-PATCH
91.6198 (0.916198)
PSNR decoded by float-HEAD
79.0372 (0.790372)
PSNR decoded by float-PATCH
81.9933 (0.819933)
Now, the mismatch between float97 and int97 seems to have disappeared. For both float and int, the larger codestream size gives better quality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Two questions:
- why move
#define F_LFTG_ALPHA
to a difference source file? - why use
//
sometimes and/*
some other times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing!
-
A1: I thought
jpeg2000dwt.h
was included intests/jpeg2000dwt.c
. But I found thatjpeg2000dwt.c
was included. So, this change makes no sense. I would revert this in the next commit. -
A2: No reason exists. In the next commit, I will unify the commenting style into
//
.
0eed928
to
1a4e3be
Compare
…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.
1a4e3be
to
3227b41
Compare
@@ -268,7 +268,7 @@ static void init_band_stepsize(AVCodecContext *avctx, | |||
av_log(avctx, AV_LOG_ERROR, "stepsize out of range\n"); | |||
} | |||
|
|||
band->i_stepsize = lrint(band->f_stepsize * (1 << 15) + 0.5f); | |||
band->i_stepsize = (int)floorf(band->f_stepsize * (1 << 15)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Two questions:
- why move
#define F_LFTG_ALPHA
to a difference source file? - why use
//
sometimes and/*
some other times?
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 source and reference files for affected FATE tests.