Closed welljsjs closed 5 years ago
Wow that's a huge work. I'll try to review it on weekend. Also, is it possible to drop h264 support in favor of native framework?
That's already done, I forgot to add it to the list. However, I'm still not happy with this PR as it's really messy. I'll try to clean this up asap (I'm also relying on pjsip, that's why I'm doing all this).
this does not ship with 32-bit compiled libraries for macOS (see above)
Are you okay with that chebur?
@welljsjs I was reviewing the PR and found out a typo in openssl.sh
It's better to keep original file indentations and keep diff as small as possible to make it easier to track changes.
Thanks @chebur!
It's better to keep original file indentations and keep diff as small as possible to make it easier to track changes.
Sure, you're absolutely right, that's due to my foolish VSCode extension that automatically re-indents when saving. Will solve that.
I rebuilt the libraries and some of the flags were reset back to previous state:
openssl currently puts its header files under include/. This needs to be fixed to include/openssl/.
Actually, openssl headers were included in build/openssl/include/openssl/openssl
folder. Notice doubled openssl/openssl
in the end.
I rebuilt the libraries and some of the flags were reset back to previous state:
That's nothing I was messing around with. They're set by pjsip itself.
Did you set them manually back then?
I also don't exactly know what these settings are used for; I just stumbled over
#define PJ_SSL_SOCK_IMP PJ_SSL_SOCK_IMP_OPENSSL
. That's weird, as it got replaced by
PJ_SSL_SOCK_IMP_NONE
. You sure it built successfully with OpenSSL?
Would you be so kind and attach the logs? Just in order to reconstruct the issue.
openssl currently puts its header files under include/. This needs to be fixed to include/openssl/. Actually, openssl headers were being included in build/openssl/include/openssl/openssl folder. Notice doubled openssl/openssl in the end.
Yeah, that's what I just thought about might have happened to you. I'll fix that as well, that should be a minor mistake I think. You can still build without errors when putting the folder in the right place manually.
I got many things to do atm, trying to dedicate to this asap, so don't worry if it takes a little longer.
Yes. I rebuild the library with fixed openssl include path and it seems PJ_HAS_SSL_SOCK
flag now okay.
But the PJMEDIA_HAS_OPENCORE_AMRNB_CODEC
flag is still 0
. I'm not sure if the absence of OpenCORE AMR-NB codec is really affecting something.
Overall the PR looks good. You made really good job.
To sum up, we still have to fix:
and we will be good to go and merge it in
I'm not sure if the absence of OpenCORE AMR-NB codec is really affecting something.
That's basically G.722.2 and I doubt anyone would make use of that.
@chebur Referring to the indentation, default is 4 spaces and I used tabs, is that what you were talking about?
Please review the latest commit (see below).
No way, 2.9 was released just a few days ago, the 06/13/19. Should we upgrade? That upgrade could include OpenSSL 1.1.1c
Seems like 2.9 comes with some new features regarding SSL:
- Security framework availability will be automatically detected for Mac OS and iOS, and if found, Darwin SSL will be the default SSL backend.
- OpenSSL availability will still be checked regardless of the above result, as it may be needed for DTLS.
- Add configure option --disable-darwin-ssl to disable the automatic detection
Meaning that we could possibly drop the OpenSSL dependency.
@welljsjs I reindented openssl.sh file to make it looks closer to original file
Sorry, I made you wait so long. The PR was merged and podspec will soon be pushed too.
@chebur I don't want to wake up this PR again, this comment has a different intent; however, it seemed wrong to me to file a bug report for this, so please take this as a follow-up.
To come straight to the point: I'd be very glad to become a collaborator on this repository.
Because I'm heavily borrowing from and relying on this repo (as an individual btw), I'd like to do my part by giving back and contributing further to this repo. I would pay special attention to the latest bug reports in order to keep the list clean (and remove distracting issues/reports). Additionally, I personally think that this repo has some great potential regarding the fact that it's pretty unique out there (afaik, there're no other repos that aim to make pjsip available for literally all Apple devices).
I'd be glad to hear your opinion on this (take your time).
Sure, you're the most concerned person so far. I appreciate your efforts on committing the PR and taking a discussion on opened issues. I agree the repo has a potential and I'd be glad to invite you to be a collaborator.
Changes
OpenSSL-Universal
anymore)VideoToolbox
framework starting version 2.7)include/
. This needs to be fixed toinclude/openssl/
.Todos
Libraries for the following platforms and architectures are included:
iOS
i386 x86_64 armv7 armv7s arm64
macOS
x86_64
For further reference, see me old PR
Solves #5