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

translate.c: implement new direct comp table mode #585

Merged
merged 1 commit into from Feb 28, 2024

Conversation

zebastian
Copy link

@zebastian zebastian commented Feb 7, 2024

The new mode gives for each codec translation the actual real cost of microseconds per second cpu time taken. This allows to compare actual real cpu usage of translations and helps in evaluation of codec implementation changes regarding performance (regression testing)

sample output:
image

Resolves: #601

@InterLinked1
Copy link
Contributor

CLA has not been signed, so this can't be reviewed yet.

However, I'm curious what one is supposed to make of "999999". It's not clear how that should be interpreted, or if that is even correct.

@zebastian
Copy link
Author

zebastian commented Feb 8, 2024

CLA still pending, probably can get this by next week.

about the 999999:
these are present for the linear resamplers:
translate.c: Translator 'slin 8000khz -> 48000khz' does not produce sample frames.

this is due to the codec_resample.c not setting a sample on the translators, eg slin8_sample.
--> Not sure if this is intended or just missing.

Suggestion as both the 0 values and the 999999 values have no actual meaning in the context of evaluating the computational time in this table:

  • For values = 999999 fill in "-"
  • Also for values = 0 fill in "-"

@zebastian
Copy link
Author

zebastian commented Feb 8, 2024

see also updated patch which now yields:

as a sample in red we can now read the expected computational cost from codec2 over slin8 to gsm as
2223 + 286 = 2509 = 2.5 milliseconds / second (as measured on an intel shared cpu vps server):

image

@zebastian
Copy link
Author

Please note the alaw/ulaw and ulaw/alaw costs in the table come from a new direct alaw/ulaw|ulaw/alaw transcoder in my local environment, which runs 50% faster than the existing "A-law and Mulaw direct Coder/Decoder". (part of next patches)

    alaw:8000        To ulaw:8000       : (alaw@8000)->(ulaw@8000)

@gtjoseph
Copy link
Member

gtjoseph commented Feb 11, 2024

A few things you need to do...

  • Sign the CLA.
  • Open an issue that describes the problem.
  • Squash the commits into a single commit referencing the issue just created.
  • Update the PR description and the commit message to reference the issue.
  • Add a comment with the appropriate "cherry-pick" statements.

See
https://docs.asterisk.org/Development/Policies-and-Procedures/Code-Contribution/
https://docs.asterisk.org/Development/Policies-and-Procedures/Commit-Messages/

@zebastian
Copy link
Author

@gtjoseph
i now did the following:

  • Sign the CLA.
  • Open an issue that describes the problem.
  • Update the PR description and the commit message to reference the issue.

Not sure about the following, do you do the squashing when you merge the PR?

  • Squash the commits into a single commit referencing the issue just created.

about the cherry picking: Since it is a new feature i assume this is not backported to any previous versions but only is merged against the master branch for the next version, right?

@gtjoseph
Copy link
Member

@gtjoseph i now did the following:

  • Sign the CLA.
  • Open an issue that describes the problem.
  • Update the PR description and the commit message to reference the issue.

You still need to update the PR description and the commit message so you need to add
a line to both like:

Resolves: #601

This allows the automation to automatically close the issue when this PR merges and also tie the issue and commit together in the change logs.

Not sure about the following, do you do the squashing when you merge the PR?

  • Squash the commits into a single commit referencing the issue just created.

Nope. The automated process that does the merges doesn't squash (for many reasons) so you need to squash the commits yourself and do a force push.

about the cherry picking: Since it is a new feature i assume this is not backported to any previous versions but only is merged against the master branch for the next version, right?

If the change is a major one or one that breaks some existing behavior then yes, it'd be master-only. Most new features though are fine to backport into the currently supported branches which, at the present time, are 18, 20 and 21.

@zebastian
Copy link
Author

zebastian commented Feb 14, 2024

@gtjoseph
thanks, all done now, see squashed commit in here.

@zebastian zebastian changed the title translate.c: implement new direct comp table mode translate.c: implement new direct comp table mode #601 Feb 14, 2024
@zebastian zebastian changed the title translate.c: implement new direct comp table mode #601 translate.c: implement new direct comp table mode / Resolves: #601 Feb 14, 2024
@gtjoseph
Copy link
Member

@gtjoseph thanks, all done now, see squashed commit in here.

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

You're getting closer but the "Resolves" line needs to be in the PR description, not the summary, and the cherry-pick lines need to be by themselves in a separate comment.

I'll fix both for you but keep this in mind if you should create more PRs.

@gtjoseph
Copy link
Member

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

@gtjoseph gtjoseph changed the title translate.c: implement new direct comp table mode / Resolves: #601 translate.c: implement new direct comp table mode Feb 14, 2024
@zebastian
Copy link
Author

Thanks @gtjoseph, will keep that in mind for future pull requests

main/translate.c Outdated Show resolved Hide resolved
The new mode lists for each codec translation the actual real cost in cpu microseconds per second translated audio.
This allows to compare the real cpu usage of translations and helps in evaluation of codec implementation changes regarding performance (regression testing).

- add new table mode
- hide the 999999 comp values, as these only indicate an issue with transcoding
- hide the 0 values, as these also do not contain any information (only indicate a multistep transcoding)

Resolves: asterisk#601
Copy link

sangoma-oss-cla bot commented Feb 25, 2024

CLA assistant check
All committers have signed the CLA.

@gtjoseph gtjoseph added the cherry-pick-test Trigger dry run of cherry-picks label Feb 28, 2024
@github-actions github-actions bot added cherry-pick-testing-in-progress Cherry-Pick tests in progress cherry-pick-checks-passed Cherry-Pick checks passed cherry-pick-gates-passed Cherry-Pick gates passed and removed cherry-pick-test Trigger dry run of cherry-picks cherry-pick-testing-in-progress Cherry-Pick tests in progress labels Feb 28, 2024
@asterisk-org-access-app asterisk-org-access-app bot merged commit ea8ead4 into asterisk:master Feb 28, 2024
62 checks passed
Copy link

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

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.

[new-feature]: translate.c: implement new direct comp table mode (PR #585)
5 participants