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

Allow calculating PSNR for Y/U/V components #3824

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Dec 13, 2024

by requesting this either as a option for all frames in
SEncParamExt.bPsnrY (and U/V)
or per-frame using
SSourcePicture.bPsnrY (and U/V)

The resulting data goes into
SLayerBSInfo.rPsnr
with is a three-element array for the Y/U/V components of the PSNR.

This is disabled by default. Also removes the ENABLE_PSNR_CALC preprocessor define in favor of the API.

See
https://www.researchgate.net/publication/383545049_Low-Complexity_Video_PSNR_Measurement_in_Real-Time_Communication_Products
by @YCSun-Meta for a research paper explaining the background.

by requesting this either as a option for all frames in
  SEncParamExt.bPsnrY (and U/V)
or per-frame using
  SSourcePicture.bPsnrY (and U/V)

The resulting data goes into
  SLayerBSInfo.rPsnr
with is a three-element array for the Y/U/V components of the PSNR.

This is disabled by default.
Also removes the ENABLE_PSNR_CALC preprocessor define in favor of the API.

See
  https://www.researchgate.net/publication/383545049_Low-Complexity_Video_PSNR_Measurement_in_Real-Time_Communication_Products
by @YCSun-Meta for a research paper explaining the background.
@BenzhengZhang
Copy link
Collaborator

@fippo Have you done any performance testing?

@YCSun-Meta
Copy link
Contributor

YCSun-Meta commented Dec 26, 2024

Hi @BenzhengZhang ,

We did performance test on a mid-range device (Pixel 4 phone), and the psnr calculation takes ~350us.
I attached the research paper, which contains the detailed numbers.
ICME2024_Low-ComplexityVideoQualityMeasurementinReal-timeCommunicationProducts_v13_0526.pdf

In addition, the patch also provides frame-level control.
If a app has concern on the additional ~350us computation, it can do the temporal subsampling of video frames for PSNR calculation or disable the feature at all.

Meta have tested and enabled the feature on the RTC apps on all devices. Its reliability as a metric at scale has been
validated.
We also had discussion with Google, the numbers are acceptable to them. Google will enable this feature in WebRTC implementation as well.

@BenzhengZhang BenzhengZhang merged commit 33f7f48 into cisco:master Jan 6, 2025
8 checks passed
@fippo fippo deleted the psnr branch January 6, 2025 15:48
@joakim-tjernlund
Copy link

if this is an API change, should you not increase .so version as well?

@BenzhengZhang
Copy link
Collaborator

if this is an API change, should you not increase .so version as well?

Should be increased in next release.

@@ -3956,6 +3956,19 @@ int32_t WelsEncoderEncodeExt (sWelsEncCtx* pCtx, SFrameBSInfo* pFbi, const SSour
(iLayerSize << 3));
#endif//LAYER_INFO_OUTPUT

pLayerBsInfo->rPsnr[0] = NAN;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change broke compiling with MSVC 2010 and 2012; they lack the NAN constant in math.h.

Not sure if MSVC 2012 really is within the scope of targets we need to support - but it did compile fine before this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or put another way - does this strictly need to be NAN, or could we initialize it to some other trivial value, like 0 or -1 or something like that?

Copy link
Collaborator

@BenzhengZhang BenzhengZhang Jan 15, 2025

Choose a reason for hiding this comment

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

Hi @fippo,
As @mstorsjo mentioned, this patch broke the build for older versions of MSVC. Could you look into making it compatible with those versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think setting it to 0.0 should work, will do a PR

fippo added a commit to fippo/openh264 that referenced this pull request Jan 24, 2025
which is not supported in some older compilers as pointed out in cisco#3824
Applications should always check whether they requested calculating
PSNR before evaluating that field in the result.
BenzhengZhang pushed a commit that referenced this pull request Jan 26, 2025
which is not supported in some older compilers as pointed out in #3824
Applications should always check whether they requested calculating
PSNR before evaluating that field in the result.
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.

5 participants