asterisk / asterisk

The official Asterisk Project repository.
https://www.asterisk.org
Other
1.99k stars 929 forks source link

pseudo-MOS calculation formula change #578

Closed romryz closed 3 months ago

romryz commented 3 months ago

res_rtp_asterisk.c: Correct coefficient in MOS calculation.

Media Experience Score relies on incorrect pseudo_mos variable calculation. According to forming an opinion section of the documentation, calculation relies on ITU-T G.107 standard:

https://docs.asterisk.org/Deployment/Media-Experience-Score/#forming-an-opinion

ITU-T G.107 Annex B suggests to calculate MOS with a coefficient "seven times ten to the power of negative six", 7 * 10^(-6). which would mean 6 digits after the decimal point. Current implementation has 7 digits after the decimal point, which downrates the calls.

Fixes: #597

jcolp commented 3 months ago

Please see the code contribution instructions: https://docs.asterisk.org/Development/Policies-and-Procedures/Code-Contribution/

The contributor license agreement needs to be signed, and the commit message updated to standard.

jcolp commented 3 months ago

As well, https://docs.asterisk.org/Deployment/Media-Experience-Score/ has links to what was used for reference information and formula details.

romryz commented 3 months ago

@jcolp thanks, the ITU-T G.107 suggests the coefficient 7*10^(-6), which is 0.000007 (6 digits after the decimal point) vs current 7 digits after decimal point. Corrected the commit message, signed the contributor license according to the contributor guide

github-actions[bot] commented 3 months ago

REMINDER: If this PR applies to other branches, please add a comment with the appropriate "cherry-pick-to" headers as per the Create a Pull Request process.

If you don't want it cherry-picked, please add a comment with cherry-pick-to: none so we don't keep asking.

If, after adding "cherry-pick-to" comments, you change your mind, please edit the comment to DELETE the header lines and add cherry-pick-to: none.

The currently active branches are now 18, 20, 21 and master.

seanbright commented 3 months ago

The analysis and change are correct. @romryz you need to update the commit message (git commit --amend) and then force-push your changes (git push --force origin patch-1). I wouldn't worry about creating an issue.

seanbright commented 3 months ago

cherry-pick-to: 18 cherry-pick-to: 20 cherry-pick-to: 21

seanbright commented 3 months ago

You can refer to issue #597

seanbright commented 3 months ago

@romryz the commit message is still not correct. I’ll provide you with one.

seanbright commented 3 months ago

@romryz Update to use this commit message. Just replace your entire commit message with that one and force-push the change again.

seanbright commented 3 months ago

That’s not correct still. Are you using the web UI?

romryz commented 3 months ago

no, it's the gh cli. is it the top two lines that have to be removed?

romryz commented 3 months ago

@seanbright could you check now please, updated (again)

seanbright commented 3 months ago

Perfect. Thank you for doing all that.

romryz commented 3 months ago

@seanbright thank you for supporting me on it!

seanbright commented 3 months ago

You should be able to re-request @gtjoseph’s review

gtjoseph commented 3 months ago

@romryz Do you happen to have an example of a "before and after" so we can see what the actual effect of changing the decimal places is on the MES? If not, that's fine. I'm going to test it later today anyway.

romryz commented 3 months ago

@gtjoseph we made some comparison on formulas to implement so I reused that template to showcase the difference - the only variable here is the T time from ITU-T, but gives an idea of how glissade looks like: MOS_comparison

gtjoseph commented 3 months ago

@gtjoseph we made some comparison on formulas to implement so I reused that template to showcase the difference - the only variable here is the T time from ITU-T, but gives an idea of how glissade looks like:

Prefect, thanks! I should be able to get this merged this afternoon or tomorrow morning.

gtjoseph commented 3 months ago

I just tested a call between 2 local phones using ulaw both with and without the change and got an MES of 85 without the change and 88 with the change. Those being a pseudo_mos of 4.25 and 4.4 respectively. I thought I might have referenced "85" as the theoretical max in the documentation but I didn't so I'm good.

github-actions[bot] commented 3 months ago

Successfully merged to branch master and cherry-picked to ["18","20","21"]