iOS sound support #1875
iOS sound support #1875
Conversation
|
|
||
| - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions { | ||
|
|
||
| - (BOOL)application:(UIApplication*)application didFinishLaunchingWithOptions:(NSDictionary*)launchOptions |
There was a problem hiding this comment.
See, it's lines like this where I feel clang-format hasn't done its job right.
- ( BOOL ) application: ( UIApplication* ) application didFinishLaunchingWithOptions: ( NSDictionary* ) launchOptions
but I don't know Objective-C...
There was a problem hiding this comment.
too many spaces for Obj-C (referring to other review comment with lots of spaces)
Most places I have worked like a space between the type and the *
- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions
There was a problem hiding this comment.
Is that "too many" as in "won't compile" or as in "not normal"?
There was a problem hiding this comment.
Oh it's just purely a stylistic comment - there are religious discussions about how to format Obj-C....
It makes no difference to compilation.
|
@ngocdh to keep the commit history clean, you should use amend style changes to the last commit and then git push --force the new commit to your branch ;-). |
ios/sound.mm
Outdated
| [[AVAudioSession sharedInstance] requestRecordPermission:^( BOOL granted ) { | ||
| if ( granted ) | ||
| { | ||
| // ok |
There was a problem hiding this comment.
It should probably show an error message. (Which also doesn't happen on macOS #1576)
ios/sound.mm
Outdated
| { | ||
| // regular case: no mixing input channels used | ||
| lNumInChanPlusAddChan = lNumInChan; | ||
| qDebug ( "Sound exception ...." ); |
There was a problem hiding this comment.
Generally I've seen this style in the code:
qDebug() << "Sound exception";
Is there a reason to do it differently?
There was a problem hiding this comment.
Just 2 ways to do the same thing. Although your way requires #include
There was a problem hiding this comment.
That's why we've code style guidelines that say "do it like it's done elsewhere" rather than "do whatever you like".
There was a problem hiding this comment.
I actually have a valid reason to limit the libraries included. Initially, the iOS code had a few more unnecessary #includes, and @ann0see iPhone couldn't even launch Jamulus because it took too long. Launched after removing libs, so I prefer to stay away from #include when possible.
The qDebug() << syntax requires #include <QDebug>.
There was a problem hiding this comment.
#include changes don't change the launch time.
If the function call requires the library to work then I don't see how you can avoid it.
As far as start up performance goes, you have to not block the main thread for long periods or you'll be killed by the system. It's pretty established that this is the ::init call happening too many times.
There was a problem hiding this comment.
#include changes don't change the launch time.
It seemed to do though. We did a test with @ann0see iphone which at first couldn't launch because it took too long. Removed some unused libs, and it launched.
If the function call requires the library to work then I don't see how you can avoid it.
The thing here is qDebug("something") can be called without #include QDebug, while qDebug() << "something" can't. Although I'm sure QDebug is already included in other ios-unrelated parts of the code, I'm just trying to optimize because ios launch time is yet to be improved.
As far as start up performance goes, you have to not block the main thread for long periods or you'll be killed by the system. It's pretty established that this is the ::init call happening too many times.
Yeah I proposed multithreading...
I actually see another potential issue: MAX_NUM_CHANNELS (default value=150). If I'm not mistaken, at launch, Jamulus creates MAX_NUM_CHANNELS faders in the mixer, and hide unused ones.
However, seeing that other OSes don't seem to have problem with this, the problem might be CSound::Init, which is specific to each system.
ios/sound.mm
Outdated
| } | ||
| catch ( const CGenErr& generr ) | ||
| { | ||
| qDebug ( "Sound Init exception ...." ); |
There was a problem hiding this comment.
And again regarding qDebug() << "message";
ios/sound.mm
Outdated
| } | ||
| catch ( const CGenErr& generr ) | ||
| { | ||
| qDebug ( "Sound Start exception ...." ); |
ios/sound.mm
Outdated
| } | ||
| catch ( const CGenErr& generr ) | ||
| { | ||
| qDebug ( "Sound Stop exception ...." ); |
I actually have no idea what you're talking about because of my very limited experience, but thanks I'll keep that in mind. |
|
No problem ;-). Let’s say you did a commit and changed a typo. After committing and pushing, you realise that there’s another typo. Ok, let’s do another commit. Oh no. Now I accidentally deleted a closing bracket —> next commit. Argh, no there’s another typo —> commit etc. As you see, you now have lots of unneeded commits. What if you could modify these commits and just fix typos in one commit? That’s the place where you could use git commit --amend/ git rebase,… https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History And then Last year I also committed a bunch of unneeded commits to my repo and it was part of a PR (#807). It got merged as is into master and now there’s a lot of garbage in there. |
|
Ok so I went thru the very thorough reviews from @pljones. Now my code is much better, but I'm refraining from sending a commit to keep history clean because there's still the clang-format to sort out. I say let's just let those style issues slide for now. When we have a better .clang-format, it's just one command away to correct the ObjC style for the whole project. What does the team think? |
|
You could amend 6ac16f6 And later (after we have a working .clang_format we‘d apply it to all mm files. |
|
Yep, we'll have to comply with the exist clang-format checks on this PR. |
ios/sound.mm
Outdated
| //CSoundBase::Start(); | ||
| } | ||
| // release the malloc'ed data in the buffer we created earlier | ||
| free ( bufferList.mBuffers[0].mData ); |
There was a problem hiding this comment.
There should be no (absolute) allocation and freeing of memory inside the audio processing loop. It's a major source of performance hits. Admittedly, I'm not familiar with how it's done for other platforms but if possible, allocate the buffer once when the app starts, then re-use it.
There was a problem hiding this comment.
You might be right. I was following this example: https://github.com/Epskampie/ios-coreaudio-example/blob/master/IosAudioController.m which really helped me.
About performance, I've been playing with my iPhone to listen to people and jam for hours and didn't really have any problem.
I'll keep that in mind and see if I can do anything to make it better.
There was a problem hiding this comment.
I was trying to use a pointer to store the buffer address, so that the buffer only needs to be initialized once and can be reused. However, since recordingCallback is a static function, the pointer must also be static. When compiling in xCode, I had this linker error (couldn't access that pointer from CSound or something).
Do you know how to overcome this error?
There was a problem hiding this comment.
I'd suggest making recordingCallback not static, as a member of a single-instance class it should still be unique - so make sure the class it belongs to is single-instance. Then the buffer pointer should be a member of the same single-instance class, pointing to memory allocated when that class is instantiated.
I don't know iOS or ObjectiveC, though, to know how best to make that happen.
There was a problem hiding this comment.
I tried, but it seems the callback has to be static :(
There was a problem hiding this comment.
ha ha got it working! The secret is the pointer has to be called like this: pSound->pointer, not just pointer. Now 1 malloc only in constructor, which is freed in destructor. It seems to work, but I'm testing a bit more before committing.
There was a problem hiding this comment.
Also I checked the MacOS sound code, and there're mallocs there ...
| @@ -241,57 +266,73 @@ void CSocket::SendPacket ( const CVector<uint8_t>& vecbySendBuf, const CHostAddr | |||
| // char vector in "const char*", for this we first convert the const | |||
| // uint8_t vector in a read/write uint8_t vector and then do the cast to | |||
| // const char *) | |||
| if ( HostAddr.InetAddr.protocol() == QAbstractSocket::IPv4Protocol ) | |||
|
|
|||
| for ( int tries = 0; tries < 2; tries++ ) // retry loop in case send fails on iOS | |||
There was a problem hiding this comment.
Wouldn't an ifdef for iOS be better here (if it's only needed on iOS)?
There was a problem hiding this comment.
It's @softins code. I don't think iOS needs it though, but it might be a good idea to leave the loop there for all OSes, just as a safety mesure.
There was a problem hiding this comment.
I thought the whole point of using UDP was that it was lossy. How did a retry mechanism end up being needed here?
There was a problem hiding this comment.
@emlynmac in iOS when you lock the device, all inactive sockets are closed. When device is unlocked, and user tries to connect to a server for example, the app crashes because of the closed socket.
As far as I understand, the status check in the loop only concerns the socket (because it can not know if packet reached its destination), if socket is closed, then status < 0, that's when we need to reinit the socket and send the packet again.
There was a problem hiding this comment.
@emlynmac Application level reliability is common for ensuring a message gets through. However, for audio/video stream, retry/retransmission is not used.
|
There's a few comments I made that it looked like were intended to have been fixed -- but they don't appear to have been. I'm not sure if in the rebasing some commit got lost or what. |
|
Hmm. Maybe. But the comments should still be there. |
|
Thanks. Just saw it. |
My Mac is having trouble synchronizing with test devices. I sadly can't do anything right now. |
|
Ok, so I checked @pljones comments and made two (really) minor changes accordingly: a typo (steoreo -> stereo) and and a break in a loop. The third comment couldn't be taken into account because iOS works with float, not integer, so conversion is required. Now if you're all ok with the above, I will send my last commit (hopefully) for this PR. |
|
I‘d be ok with that |
|
I don't think I can review the code here. It's not following the way the rest of Jamulus works, it's Apple-specific. So it needs someone who knows the platform to review it. |
ann0see
requested review from
pljones and
ann0see
and removed request for
pljones
I don't have any means to test this code; the fact that QT is so old coupled with the version of Xcode required means you cannot build on Big Sur. |
|
OK, can we mark the build on Apple Store as experimental? Or does that break Apple Store distribution? |
There will be no apple store distribution - #1874 needs resolving. |
I'm confused: you can't build because you must maintain the same environment as Jamulus autobuild? |
Well, it should always be the same environment as the official build yes. But, additionally, the QT5 build environment is not functional with big sur. I used to have an old laptop with Catalina on it, but that is no more... |
|
Hmm. I‘d say since building for IOS worked and the CI is green, this should be OK to merge. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Short description of changes
What's included in this PR:
Context: Fixes an issue?
As discussed in #1865, here is the new iOS-only PR.
Does this change need documentation? What needs to be documented and how?
Yes. Compilation for iOS is already documented.
Status of this Pull Request
Waiting on review; iOS sound is working. Some more real world tests.
What is missing until this pull request can be merged?
Real world tests; review;
Later on: decide if we do a squash and merge.
Checklist
PR-Template applied by @ann0see