GNOME Bugzilla – Bug 792189
srt: add recipe
Last modified: 2018-04-30 13:11:52 UTC
Created attachment 366269 [details] [review] recipes/srt: Add support SRT This recipe enables srt plugins of gst-plugins-bad.
Shouldn't we use release tarballs in the recipe? What about adding it to the -bad recipe?
Created attachment 366272 [details] [review] recipes/srt: Add support SRT
(In reply to Tim-Philipp Müller from comment #1) > Shouldn't we use release tarballs in the recipe? The current released tarball of SRT doesn't support cross-building by MinGW so I just took master. I am not sure when they release new version of SRT. Should I be waiting for them? > What about adding it to the -bad recipe? I think adding it to 'deps' is enough.
What are the chances that it builds fine on iOS/Android/macOS?
I don't think we should enable this until there's a proper release. Or maybe we can just add patches on top of the current release if the fix is not too involved?
(In reply to Sebastian Dröge (slomo) from comment #4) > What are the chances that it builds fine on iOS/Android/macOS? All of those should work in theory.. There are apps for all of those using libsrt.
Created attachment 366356 [details] [review] recipes/srt: Add support SRT Now, it fetches source from tarball,and patches some fixes which are not yet released.
Let's push it then I'd say, as long as someone stands by to fix up any build problems that might emerge :)
Pushed, let's see how the CI takes it. commit b3bfe6203583a94b34ad0c30825c592844bf4768 (HEAD -> master, upstream/master) Author: Justin Kim <justin.kim@collabora.com> Date: Thu Jan 4 15:41:03 2018 +0900 recipes/srt: Add support SRT SRT(Secure Reliable Transport) library is added to enable srt plugins in gst-plugins-bad. https://bugzilla.gnome.org/show_bug.cgi?id=792189
I ended up re-disabling it because there are problems on Android and iOS: 1. We need to build both static and shared, it seems that our builds require having dynamic libs on Android and iOS too because we build both. I'm not sure why. 2. We need to fix the PTHREAD search crap in the cmake file. I have a patch for this that I should upstream to srt. 3. On my system, cmake adds "-lrt" when linking, I don't know where it gets it from. And on Android, there is no separate librt, so that fails.
Created attachment 367279 [details] [review] srt: Enabled for Android again To build SRT plugins for Android in cerbero, it should support both static and shared libraries. In addition, the hard-coded libraries such as pthread and rt should be removed.
Review of attachment 367279 [details] [review]: ::: recipes/srt.recipe @@ +31,2 @@ if self.config.target_platform == Platform.ANDROID: + self.configure_options += ' -DANDROID_NDK=1 ' I'm not convinced we should treat the Android NDK in a special one, there is nothign android specific in here. ::: recipes/srt/0001-Build-support-for-Android-in-cerbero.patch @@ +19,3 @@ ++if (ANDROID_NDK) ++ pkg_check_modules (GNUSTL REQUIRED "gnustl") ++endif() I suggest just putting this part in the recipe file as it's so cerbero specific. @@ +29,3 @@ + endif() + ++if (NOT ANDROID_NDK) This not not android specific. I think it should just use the following unless some manual variable is specified.. And this should be upstreamed to srt: set(THREADS_PREFER_PTHREAD_FLAG ON) find_package(Threads REQUIRED) @@ +48,3 @@ +-add_library(${TARGET_srt} ${srt_libspec} ${SOURCES_srt} ${SOURCES_srt_extra}) ++add_library(${TARGET_srt} SHARED ${SOURCES_srt} ${SOURCES_srt_extra}) ++add_library(${TARGET_srt}_static STATIC ${SOURCES_srt} ${SOURCES_srt_extra}) Here, I'd really like to be able to build both static and shared in one go in a way that can be upstreamed.. Maybe with a ENABLE_BOTH or something similar. Sad that CMake doesn't help us there. @@ +81,3 @@ ++if ( LINUX AND NOT ANDROID_NDK ) + target_link_libraries(${TARGET_srt} PUBLIC rt) + set (IFNEEDED_SRT_LDFLAGS -pthread) After discussion with Mikolaj on their Slack, we can just remove the check entirely. No function from librt are used now.
Created attachment 367784 [details] [review] srt: Enabled for Android again
Regarding building static and shared libraries at once, SRT has an option in their 'dev' branch, but they don't have a plan to backport to 'master' so I just created cerbero specific patch again.
Created attachment 370686 [details] [review] srt: update to 1.2.3
Created attachment 370687 [details] [review] gst-plugins-bad: enable srt
Which builds have already been tested ? Do you want to give this a try on the CI ?
(In reply to Nicolas Dufresne (stormer) from comment #17) > Which builds have already been tested ? I have tested on android-arm64 and x86_64 linux. > Do you want to give this a try on the CI ? yes, please :)
Attachment 370686 [details] pushed as 48026d7 - srt: update to 1.2.3 Attachment 370687 [details] pushed as 618dd87 - gst-plugins-bad: enable srt
Does not work: CMake Error at CMakeLists.txt:225 (message): Failed to find pthread library. Specify PTHREAD_LIBRARY. https://ci.gstreamer.net/job/cerbero-android/8384/console Do you think this can be fixed soon or should I revert again ?
It seems to be looking in the wrong directories for the pthread headers (/usr/include instead of the ndk directories). Furthermore on android you don't need to explicitely link against pthread, it's always present in bionic. You can reproduce that failure locally with cerbero: $ cerbero-uninstalled bootstrap $ cerbero-uninstalled -c config/cross-android-universal build srt
Created attachment 370726 [details] [review] srt: add a patch to find thread by cmake option To find thread library, Just following what cmake does.
Created attachment 370727 [details] [review] gst-plugins-bad: enable srt (also add libsrt to deb)
Thank you guys, I tested buildable for android-universal and linux on docker image.
Review of attachment 370726 [details] [review]: ::: recipes/srt/0002-CMakeLists.txt-let-cmake-find-pthread.patch @@ +2,3 @@ +From: Justin Kim <justin.kim@collabora.com> +Date: Tue, 10 Apr 2018 11:57:59 +0900 +Subject: [PATCH] CMakeLists.txt: let cmake find pthread Please add upstream PR or Issues link for later tracking. @@ +42,3 @@ ++set (THREADS_PREFER_PTHREAD_FLAG ON) ++find_package (Threads REQUIRED) ++set (SRT_LIBS_PRIVATE ${SRT_LIBS_PRIVATE} ${CMAKE_THREAD_LIBS_INIT}) That's massively simpler, and leave it to cmake to handle. Did you check if this was added in a specific cmake version, and if we need to bump the requirement ?
Review of attachment 370726 [details] [review]: ::: recipes/srt/0002-CMakeLists.txt-let-cmake-find-pthread.patch @@ +2,3 @@ +From: Justin Kim <justin.kim@collabora.com> +Date: Tue, 10 Apr 2018 11:57:59 +0900 +Subject: [PATCH] CMakeLists.txt: let cmake find pthread This is a local patch because SRT has different approach. I found SRT has released 1.3.0 yesterday, but I think it's better to stay 1.2.3 for a moment because it causes linking errors when I tested in cerbero. @@ +42,3 @@ ++set (THREADS_PREFER_PTHREAD_FLAG ON) ++find_package (Threads REQUIRED) ++set (SRT_LIBS_PRIVATE ${SRT_LIBS_PRIVATE} ${CMAKE_THREAD_LIBS_INIT}) Ah, I didn't check, but in bootstrap recipe, the version of cmake is defined as 3.7.1 'THREADS_PREFER_PTHREAD_FLAG' was introduced CMake >= 3.1.
Created attachment 370761 [details] [review] srt: add a patch to find thread by cmake option Updated commit messages.
I think we should move to 1.3 as they have more patches.
Created attachment 371356 [details] linking error - srt 1.3.0 Bumping up the version shouldn't be hard, but I am stuck an error. I don't understand why it fails to resolve the symbol of 'std::istream::seekg'. > error: undefined reference to 'std::istream::seekg(long long, std::_Ios_Seekdir)' I attached a full log. Can anybody give me an advice?
Merged, let's hope the CI passes! commit 52e9e5fd5504bba71bf77fa85338afa5a74463f0 (HEAD -> master, upstream/master, gitcollabora/srt-test, gitcollabora/master, srt-test) Author: Olivier Crête <olivier.crete@collabora.com> Date: Mon Apr 30 08:41:02 2018 -0400 gst-plugins-bad-1.0: Enable SRT plugin commit 3580f3aabda248017c76d7fc997784364126dcbc Author: Olivier Crête <olivier.crete@collabora.com> Date: Fri Apr 27 16:04:16 2018 -0400 srt: Update 1.3.0 commit 74048987b69f2549dffa44697f8d243a725a0742 Author: Olivier Crête <olivier.crete@collabora.com> Date: Fri Apr 27 16:01:36 2018 -0400 gnustl: Use headers from the right architecture