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

Delta calculations #44

Closed
wants to merge 16 commits into from
Closed

Delta calculations #44

wants to merge 16 commits into from

Conversation

liudger
Copy link
Contributor

@liudger liudger commented Jun 5, 2023

Add support for Delta calculation #25. Greatly improving the accuracy for identifying the phonemes.

Interface update and new length of mfcc array
image

Animator is not playing an AnimatorController
When set with code in the editor  the hashes are not generated
this makes sure that there are no small values sticking around after a period of time.
Make sure you can't change the delta when array's are created. Add explanation
bufferMelCepOffset is used to populate, move memory in the buffer correctly
This could maybe need a refactor? Also it include the creation of the mfcc array for the delta.
use the buffer to calculate the delta
@liudger
Copy link
Contributor Author

liudger commented Jun 6, 2023

There are some fixes coming. Don't review yet

needs te be before the dispose?
need to find a different way to setup the buffer if we want to change the buffer to accommodate delta delta
should of out of the loop. Also shouldn't we use for loop as foreach in Unity takes more memory?
We need in future to look at unsafe copy. We could make separate util for copy unsafe?
@liudger
Copy link
Contributor Author

liudger commented Jun 7, 2023

It should be good to go @hecomi

@liudger
Copy link
Contributor Author

liudger commented Jun 7, 2023

We could still do some optimisations for copy arrays. It now iterates over the arrays. We could use unsafe memcopy in the future

@hecomi
Copy link
Owner

hecomi commented Jun 17, 2023

Thank you for your pull request. I'm curious about the effectiveness of adding the Delta part to the MFCC. At present, we simply compare pre-acquired MFCCs with current ones using simple distance calculations or cosine similarity. With this, I'd like to investigate how much this additional 12 dimensions actually contribute to the accuracy. If it proves beneficial, I'd be keen on accepting your PR. However, if the impact is minimal, I would prefer to keep the project simple and may consider putting it on hold for the time being. I'd appreciate your thoughts on this.

@liudger
Copy link
Contributor Author

liudger commented Jun 17, 2023

I didn't do actual scientific test to see how much it improves. But from just using it the result looked promising. I guess we can do a comparison by using an audio file and creating sample from this file for both calculation. (it's optional the calculation) and see what is more accurate. Currently I am already on another project so this testing would be done later.

@hecomi
Copy link
Owner

hecomi commented Jul 18, 2023

Apologies for the delayed response. I have now reviewed your code.

There are changes that I would like to accept aside from the Delta calculation. It would be helpful if such changes could be provided in separate PRs in the future 🙇

As for the Delta calculation, I am considering accepting it as an experimental feature.

Please understand that I will be modifying your contributions to fit my coding style:

  • I plan to adjust some of the indentations.
  • Typically, I do not add comments to the internal implementation.

Thank you for your contributions and understanding.

@hecomi
Copy link
Owner

hecomi commented Jul 30, 2023

I've read through the code, and it seems to be computing a weighted average instead of performing differentiation for the calculation of Delta MFCC. Is this code working as intended? Please correct me if I'm wrong.

https://github.com/hecomi/uLipSync/pull/44/files#diff-623438b2ee4a24923c5ba18390fed6fac4f97e80a67a1543759f696a9e715a12R508-R534

@liudger
Copy link
Contributor Author

liudger commented Aug 3, 2023

Let me check again the calculation 🙈

@liudger
Copy link
Contributor Author

liudger commented Sep 5, 2023

I'll make separate pull request for the fixes and later make a new pull request for delta MFCC's

@liudger liudger closed this Sep 5, 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

Successfully merging this pull request may close these issues.

2 participants