Add macos text service #32
Merged
Conversation
This file contains 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
mzgoddard
commented
Jan 10, 2024
|
|
||
| class ATDriverClientUnix { | ||
|
|
||
| class SocketResource { |
There was a problem hiding this comment.
Made this RAII type to automatically handle closing the socket. In the case of how its currently used, when the resource goes out of scope in _sendEvent, it'll de-initialize and close the socket. This way we don't have to be careful of all the edge cases around throw, return, and other scenarios of the socket left hanging open.
|
Some alternatives I considered before the XPCService and BSD socket approach here:
|
mzgoddard
force-pushed
the
add-macos-text-service
branch
from
9851ade to
84f6f0c
Compare
jugglinmike
requested changes
Jan 30, 2024
There was a problem hiding this comment.
I'm unable to build the project on my system; here's the error:
Build input file cannot be found: '/Users/mike/projects/bocoup/facebook-aria/aria-at-automation-driver/src/macos/ATDriverGenericMacOS/ATDriverGenericService/ATDriverClientIP.swift'. Did you forget to declare this file as an output of a script phase or custom build rule which produces it?
I'll be happy to debug this with you on a call if that'd help, though it sounds we may just need to add a file from your machine. Let me know!
Separately, it looks like macOS allows the app to write to a directory in ~/Library/Containers. Since this isn't included among the alternatives you mentioned, I'm wondering if it's worth exploring. Would it be viable to place the socket there so that the app could pass messages to the AT Driver server without the need for the daemon?
lib/cli.js
Outdated
| const yargs = require('yargs/yargs'); | ||
| const { hideBin } = require('yargs/helpers'); | ||
|
|
||
| const createCommandServer = require('./create-command-server'); | ||
| const createVoiceServer = require('./create-voice-server'); | ||
| const NAMED_PIPE = '\\\\?\\pipe\\my_pipe'; | ||
| const WINDOWS_NAMED_PIPE = '\\\\?\\pipe\\my_pipe'; |
There was a problem hiding this comment.
This name leaves something to be desired (I'm being blunt because I chose it). Now seems like a good time to switch to a more sensible name--along the lines of the one you've proposed for macOS.
There was a problem hiding this comment.
I'm not sure what a better name would be. https://learn.microsoft.com/en-us/windows/win32/ipc/named-pipes I'd have to go look up more about them. It's not a unix socket or local socket. I guess there is some kind of system server for pipes. The name isn't a path, just path-like. Windows doesn't create a file on the file system like a unix/local socket does.
Do you mean the my_pipe part?
There was a problem hiding this comment.
WINDOWS_PIPE_NAME would have the same tense/phrasing that MACOS_SOCKET_UNIX_PATH has. Is that what you are thinking?
There was a problem hiding this comment.
I mean that at_driver_generic would be far more descriptive than my_pipe. Now that this patch is blocked on the new one, I'll do this in a follow-up.
|
It looks like the references to "ATDriverClientIP.swift" are being removed in gh-33, so maybe we just need to cherry-pick those changes here... |
Merged
|
Since this branch doesn't conflict with Also, did you see my question about |
Yes, if we had the app sandbox enabled, we'd have to use a socket path in The XPCService is used to parallelize communicating over a socket with nodejs. The parallelization is the key point I think. The alternatives I mentioned were for resolving the parallelization as well. I think we want the socket communication to occur somehow on a different thread or process than the speech synthesis request. I don't know what kind of issues would happen if we didn't do that, but I think this is a good goal for making a reliable tool. The XPCService handles a lot of parallelization issues for us, like race conditions and resource management. |
#36 helps test #35 and #37 together as well. I figured removing the socket path in this PR would make it easier to locally merge with #36 to test without rebasing #32 if we made further changes to #36. Sorry I didn't express that earlier. I'll rebase. |
ATDriverGenericService is an XPC Service that the Audio Unit extension messages to forward messages to the lib/cli.js server. This step is used to keep the AudioUnit speedy. - Add unix socket path to cli.js - Create directory for socket and unlink old leftover sockets in cli.js - Add ATDriverGenericService to xcode project - Add service dependency to Extension
mzgoddard
force-pushed
the
add-macos-text-service
branch
from
f6cd6f2 to
d4cfdb5
Compare
@jugglinmike You're probably encountering an issue with the solution using an Application Extension. The extension discovery method doesn't handle duplicates well. The safest way to test it would be with a new system like a virtual machine. Outside that you need to delete any duplicate extensions. My first step for troubleshooting this kind of thing is to clean the build folder in Xcode, and wait a few minutes before rebuilding. If you have the extension selected in Spoken Content in System Settings you should select another speech synthesizer. A hacky way you can test if its finding the latest build is to change the name in |
After running with this new version of the branch, the expand to review the error message
If contributing to this project is going to require a virtual environment, we'll definitely need to document that in the contribution guidelines (ideally within the commit which introduces the requirement).
My research into your recommendation has identified the "Extension" section of macOS's System Preferences. While the audio unit is listed there, the associated UI is so vague (an unlabeled check box) that I can't infer what it actually does:
Unchecking a box seems like an unorthodox method to remove something, but I tried it, anyway. That didn't help. Can you be a little more specific?
I've interpreted your statement to mean, "modify the file --- a/src/macos/ATDriverGenericMacOS/ATDriverGenericMacOSExtension/Common/Audio Unit/ATDriverGenericMacOSExtensionAudioUnit.swift
+++ b/src/macos/ATDriverGenericMacOS/ATDriverGenericMacOSExtension/Common/Audio Unit/ATDriverGenericMacOSExtensionAudioUnit.swift
@@ -167,7 +167,7 @@ public class ATDriverGenericMacOSExtensionAudioUnit: AVSpeechSynthesisProviderAu
logger.log("speechVoices")
}
return [
- AVSpeechSynthesisProviderVoice(name: "ATDriverGenericMacOSExtensionVoice", identifier: "ATDriverGenericMacOSExtension", primaryLanguages: ["en-US"], supportedLanguages: ["en-US"])
+ AVSpeechSynthesisProviderVoice(name: "ATDriverGenericMacOSExtensionVoice2", identifier: "ATDriverGenericMacOSExtension2", primaryLanguages: ["en-US"], supportedLanguages: ["en-US"])
]
}
set { }Can you be a little more specific? |
Oh. Interesting. Can you dump I'm thinking there are two high level possibiliities here:
The second of those is a wild guess. I'm not sure its even possible. Either way that log will hopefully give us clues.
That sounds good to me. I think we have been there from the beginning. Needing software installed or configuration changes at the user level on operating systems for it to be used by third party applications I think fulfills this. We can't have multiple installed copies of the voices.
By delete I meant for you to delete the built files from the file system. The screen you show toggles permission for the application extension by extension interface. The extensions you have installed in that screen both have
You made the change I meant. The change should should up in the dropdown for System Settings > Accessibility > Spoken Content > System voice.
If there is still an item for the extension but its the old name, I think it could be recognizing a version of the extension is installed, and its using a cached copy of the list of voices that either has not yet updated or when it tried to update an error occured and it is reusing an older cache. I don't know if reuse of an old cache is possible. From my experience the cache gets updated in a manner of minutes after a new copy of the extension is installed. |
Sure. I reverted my temporary name change, rebuilt, enabled it in the system settings, and typed log contents |
|
@jugglinmike we met on Thursday and figured out that your system did have a second copy of the extension. Deleting it we were able to get this patch working on your system. Is there any other changes we haven't addressed that you want to see before this is merged? |
|
@mzgoddard Although we got this running during our call on Thursday, I'm now seeing the same error in response to I can see just one extension installed:
And after I use "Product > Clean Build Folder..." in Xcode, that extension is gone:
...and it's likewise removed from the relevant dropdown in the System menu. After using "Product > Run" in Xcode and waiting about 20 seconds, the extension reappears in that dropdown. Selecting it and running
Any tips on what I should try next? |
|
@jugglinmike If I recall correctly we also saw that error when you selected the pre-installed Eddy voice. If you delete our voice with I remember I made a quick search for that error and saw https://forums.developer.apple.com/forums/thread/730789. That thread doesn't seem to have a solution for us and for them was likely more a bug? I suspect its some kind of process permissions issue. (It kind of makes me think of bluetooth permissions issues for apps that are not wrapped and crash.) If I recall you ran it in tmux, can you try running it outside of tmux? |
Yup, same problem with Eddy.
Yes! That was the problem! (I don't know which is more impressive: your perceptiveness or your creativity. I would not have consciously noted the presence of tmux, nor would I guess that it had anything to do with a problem like this. In any case, nice work.) |
jugglinmike
approved these changes
Feb 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Depends on #30 and #31.
Summary
Add an XPC Service, called
ATDriverGenericService, to the xcode project. This service handles communication between the AudioUnit and thelib/cli.jsnodejs executable.Changes
Description
XPC Services are useful for separating some work into another process. In this case this change uses a service to separate socket communication with a nodejs process. This way the blocking calls to bsd socket apis are done in a process other then the one the audio unit runs its synthesis in.
The extension creates an XPCConnection designating the target service and protocol. With that, the extension calls methods on a proxy with the same interface as the protocol. The methods queue messages to be sent. When the service is created and starts, it receives and processes those message. As implemented this service than produces messages in the format
cli.jsexpects and opens a unix family socket to send the message.Adding an Service in Xcode
When adding the service Xcode added the service as a build dependency to the container application. For the extension to be able to use the service it has been added as a build dependency to the extension as well.