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

Chrome extension update #123

Merged
merged 10 commits into from
Feb 3, 2024
Merged

Chrome extension update #123

merged 10 commits into from
Feb 3, 2024

Conversation

lightwastak3n
Copy link
Contributor

  • Chrome extension:
    • Change model dropdown to include all models - multilingual and English-only
    • Sort the dropdown list for the languages alphabetically
    • Add English to the languages so that it shows when the recognized language gets send back to the extension
    • Default value for select language is Automatically detect - sounds more user friendly since that's what it does
    • Center the transcript div - on some websites it gets hidden behind navbar in the top left.
    • Remove multilingual checkbox and everything connected to it ([Bug] Duplicate text while using chrome extension #117 (comment))
      • We use model name and chosen language for this
  • Server - might be some conflict with Save transcript #121
    • self.client_uid in recv_audio was being accessed during exception without being declared
    • ServeClientFasterWhisper
      • Language is chosen based on model name and language dropdown, we don't need self.multilingual
      • Change get_model_size function to just check_valid_model since we don't need the multilingual part anymore.
  • Client
    • self.language and self.multilingual are assigned twice

I'm new to this so any advice is appreciated.
I've checked the extension on different combinations of languages and models and didn't find any problems.
The multilingual arg is still used in some places and I don't know what you've planned with it so I left it in server.ServeClientFasterWhisper.
If extension changes get approved I'll do the Firefox one.

@makaveli10
Copy link
Collaborator

Hello, thanks for putting this together, can you do a quick merge with main and we can start looking into this.

@lightwastak3n
Copy link
Contributor Author

So like this? Sorry, still learning stuff.

@@ -154,7 +155,7 @@ def recv_audio(self,
options["model"] = faster_whisper_custom_model_path
client = ServeClientFasterWhisper(
websocket,
multilingual=options["multilingual"],
multilingual=False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We dont need this as well. This can be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can run the FasterWhisper backend without multilingual, this can be removed from this backend. So we can remove these lines as well:

multilingual=False,

multilingual=options["multilingual"],

Which brings me to the client, and client already sends the language with the model so, it doesn't need to worry about sending the multilingual option as well. If you don't mind updating the client i.e. removing the multilingual option from there that would be really appreciated, else we can take a look at it as well.

The TensorRT backend would need it for the tokenizer.

@@ -129,7 +119,6 @@ document.addEventListener("DOMContentLoaded", function () {
startButton.disabled = isCapturing;
stopButton.disabled = !isCapturing;
useServerCheckbox.disabled = isCapturing;
useMultilingualCheckbox.disabled = isCapturing;
modelSizeDropdown.disabled = isCapturing;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
taskDropdown.disabled = isCapturing

I guess it was missed initially.

…per backend. Disable task dropdown when capturing in chrome extension.
@lightwastak3n
Copy link
Contributor Author

I didn't even notice that drop-down wasn't getting disabled.
I removed multilingual from client and faster whisper. I haven't touched anything TensorRT.

I haven't looked into tokenizer but faster whisper is choosing it here and it gets multilingual based on the model right? That's why we don't care about passing it.

@makaveli10 makaveli10 merged commit 08fa183 into collabora:main Feb 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants