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

PSNR result mismatch for some 16 bit streams #1115

Closed
avstrakhov opened this issue Nov 11, 2022 · 12 comments
Closed

PSNR result mismatch for some 16 bit streams #1115

avstrakhov opened this issue Nov 11, 2022 · 12 comments

Comments

@avstrakhov
Copy link

We have implemented a library for a stream metrics calculatiion using Intel IPP library.
After compare of our results with VMAF we discovered some differences in the PSNR results for some 16 bit streams.
Our investigation showed that there is an issue in the current VMAF implementation of PSNR calculation.

libvmaf/src/feature/integer_psnr.c:
175 const int32_t e = ref[j] - dis[j];
176 sse += e * e;
178 }
179 ref += ref_pic->stride[p] / 2;
(gdb) n
175 const int32_t e = ref[j] - dis[j];
(gdb) p ref[j]
$11 = 56832
(gdb) p dis[j]
$12 = 9472
(gdb) n
176 sse += e * e;
(gdb) p e
$14 = 47360 // the difference is signed 32 bit int
(gdb) p ee
$15 = -2051997696 // the product's is signed 32 bit int and has the sign bit set
(gdb) p/x e
e
$16 = 0x85b10000
(gdb) p sse
$13 = 30446604648448 // the sum before addition
(gdb) n
172 for (unsigned j = 0; j < ref_pic->w[p]; j++) {
(gdb) p sse
$17 = 30444552650752 // the sum after addition is lesser than before
(gdb)
So the calculated MSE and PSNR values are not valid for the frame.

The proposed fix:
index f24d29b4..092075e4 100644
--- a/libvmaf/src/feature/integer_psnr.c
+++ b/libvmaf/src/feature/integer_psnr.c
@@ -173,7 +173,7 @@ static int psnr_hbd(VmafPicture *ref_pic, VmafPicture *dist_pic,
for (unsigned i = 0; i < ref_pic->h[p]; i++) {
for (unsigned j = 0; j < ref_pic->w[p]; j++) {
const int32_t e = ref[j] - dis[j];

  •            sse += e * e;
    
  •            sse += (uint32_t)(e * e);
           }
           ref += ref_pic->stride[p] / 2;
           dis += dist_pic->stride[p] / 2;
    

After the fix applied, VMAF and IPP results for the above streams matched.

@waveletbeam
Copy link

waveletbeam commented Nov 11, 2022

Do you know why PSNR is limited at 72dB for 10bpc? I think this is wrong! For distribution encodes we cannot reach this high PSNR values but if you like to use PSNR for other workflow steps, higher PSNR values are important.

@st599
Copy link

st599 commented Nov 11, 2022

As has been described in detail on here before, 72 and 60 are approximations of the maximum PSNR available in a 10 and 8 bit video system respectively. This is due to the Quantisation Noise. To get a higher PSNR, you need finer quantisation steps.

@waveletbeam
Copy link

waveletbeam commented Nov 11, 2022

Sorry I don't agree
For the "quantisation noise of the digitisation process" you are right but in each further workflow step you can get tiny difference. E.g. changing one pixel in the blue channel (10,10) of a 10bpc image from the value 80 to 81. This can be reflected via PSNR and this is very important for QA.

@st599
Copy link

st599 commented Nov 11, 2022

The maximum performance of the system is dependent on bit depth. Netflix have a description of why this is the case in their FAQs.

To look at it another way, to get higher than the max PSNR, you would have to remove or reduce noise. The quanitsation noise is an inherent part of the system.

Your measure of the fidelity of a copy may be valid, but it's not the SNR or PSNR.

For example, the SNR of a 16bit audio CD is about 96dB. You can make a perfect copy - but the noise floor in the copied CD is the same in and the SNR is the same.

@kylophone
Copy link
Collaborator

@kylophone
Copy link
Collaborator

Copying your diff here for future reference, the formatting is messed up in your original post. Your changes look good, I will take a closer look later this afternoon.

--- a/libvmaf/src/feature/integer_psnr.c
+++ b/libvmaf/src/feature/integer_psnr.c
@@ -173,7 +173,7 @@ static int psnr_hbd(VmafPicture *ref_pic, VmafPicture *dist_pic,
         for (unsigned i = 0; i < ref_pic->h[p]; i++) {
             for (unsigned j = 0; j < ref_pic->w[p]; j++) {
                 const int32_t e = ref[j] - dis[j];
-                sse += e * e;
+                sse += (uint32_t)(e * e);
             }
             ref += ref_pic->stride[p] / 2;
             dis += dist_pic->stride[p] / 2;

@richardpl
Copy link

With FFmpeg psnr implementation there is possibility to get higher PSNR:

ffmpeg -i ../Videos/derf/west_wind_easy_1080p.y4m -vf "split[a][b];[b]gblur=0.06[b];[a][b]psnr" -frames 1 -f null -
[Parsed_psnr_2 @ 0x55ed287f8780] PSNR y:111.298053 u:inf v:inf average:114.308353 min:114.308353 max:114.308353

And difference is extremely subtle with two input videos.

@waveletbeam
Copy link

waveletbeam commented Nov 11, 2022

So the FFMpeg implementation of PSNR is different in comparison to the NETFLIX VAMF version. Additionally, I think the Netflix capped PSNR is misleading because PSNR is THE tool for measuring tiny difference between videos and with the capped PSNR of 60dB (8bpc) and 72dB (10bpc) this measuring range is lost.

@kylophone
Copy link
Collaborator

So the FFMpeg implementation of PSNR is different in comparison to the NETFLIX VAMF version. Additionally, I think the Netflix capped PSNR is misleading because PSNR is THE tool for measuring tiny difference between videos and with the capped PSNR of 60dB (8bpc) and 72dB (10bpc) this measuring range is lost.

If it is important to you, the functionality could be added as a non-default option to the PSNR feature extractor.

@waveletbeam
Copy link

So the FFMpeg implementation of PSNR is different in comparison to the NETFLIX VAMF version. Additionally, I think the Netflix capped PSNR is misleading because PSNR is THE tool for measuring tiny difference between videos and with the capped PSNR of 60dB (8bpc) and 72dB (10bpc) this measuring range is lost.

If it is important to you, the functionality could be added as a non-default option to the PSNR feature extractor.

Yes, also a non-default option to the PSNR feature extractor would be helpful. We are working on topics like Film Grain Synthesis and to have proper measuring tools would be helpful. As I understood a solution for Film Grain Synthesis will be added to the VMAF feature set in the future. This will change everything!

@avstrakhov
Copy link
Author

@kylophone: Thank You!

@nilfm
Copy link
Contributor

nilfm commented Nov 10, 2023

FYI: #1231 addresses a case where signed integer overflow can happen, which leads to UB and potentially negative PSNR results. Closing this issue since it's been inactive for a while.

@nilfm nilfm closed this as completed Nov 10, 2023
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

No branches or pull requests

6 participants