-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Support wav2vec base models #9637
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b64004f
Support wav2vec base models
kijai 66e1091
trim trailing whitespace
kijai 874a067
Merge remote-tracking branch 'upstream/master'
kijai ca2e7f3
Merge remote-tracking branch 'upstream/master'
kijai bfa45fb
Do interpolation after
kijai File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason this shouldn't be done by the node using the outputs like the s2v node? https://github.com/comfyanonymous/ComfyUI/blob/master/comfy_extras/nodes_wan.py#L892
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about that, this was the way it was done in the initial MultiTalk implementation. I guess the reason could be efficiency as this way the model has to process less? Though the models being as light as they are, I'm not sure it matters, mostly I was trying to match how it was done in the initial MultiTalk implementation to get matching results.
I can test if it makes a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best is to have the encoder nodes return the raw output from the model as much as possible which makes it easier if multiple models use the output from the same encoder but with different processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I agree with that principle of course. Tested now and there is a slight difference in results when interpolating before the encode and when interpolating the already encoded features, with the same function as S2V, should we worry about that?
AnimateDiff_00005-audio.webm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird, why is there a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, but looking at the tensors there's already a slight difference in the features if the interpolation is done before feature_projection:
and similarly between encoding interpolated features and interpolating encoded features:
It's very small, can it really be that sensitive, maybe it matters if the model is trained like that?
The original implementation is specifically overriding the Wav2vec2 -model to do the interpolation before projection and encode:
https://github.com/MeiGen-AI/MultiTalk/blob/636c774370d40214e6be1ea1ed98bc219dfbd8c4/src/audio_analysis/wav2vec2.py#L32
Another longer comparison test with better settings overall still shows a difference, but I can't really say if it's better or worse:
AnimateDiff_00002-audio.webm
Anyway there's no hurry with this PR as the native model implementation still needs to be done to have any use for this, so which way to do this can be decided later too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all the models do the same interpolation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, the normal Wav2Vec2Model implementation in transformers uses same forward for base and large, and it doesn't involve interpolation, so it seems it's just specifically this chinese model from Tencent that uses it, and it would explain why they use custom code for it. The MultiTalk paper didn't mention the fps used or anything, so I'm just guessing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did end up leaving the interpolation later as you suggested, and while the result is slightly different, it has been working fine. If there are no other issues left it would be fine by me to merge this.