After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 792189 - srt: add recipe
srt: add recipe
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: cerbero
git master
Other All
: Normal enhancement
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 795501
 
 
Reported: 2018-01-04 06:52 UTC by Justin Kim
Modified: 2018-04-30 13:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
recipes/srt: Add support SRT (1.43 KB, patch)
2018-01-04 06:52 UTC, Justin Kim
none Details | Review
recipes/srt: Add support SRT (2.17 KB, patch)
2018-01-04 09:32 UTC, Justin Kim
none Details | Review
recipes/srt: Add support SRT (17.65 KB, patch)
2018-01-05 04:25 UTC, Justin Kim
committed Details | Review
srt: Enabled for Android again (7.31 KB, patch)
2018-01-23 06:29 UTC, Justin Kim
none Details | Review
srt: Enabled for Android again (11.62 KB, patch)
2018-02-02 03:43 UTC, Justin Kim
none Details | Review
srt: update to 1.2.3 (27.45 KB, patch)
2018-04-09 13:05 UTC, Justin Kim
committed Details | Review
gst-plugins-bad: enable srt (2.34 KB, patch)
2018-04-09 13:05 UTC, Justin Kim
committed Details | Review
srt: add a patch to find thread by cmake option (3.04 KB, patch)
2018-04-10 07:36 UTC, Justin Kim
none Details | Review
gst-plugins-bad: enable srt (also add libsrt to deb) (3.11 KB, patch)
2018-04-10 07:37 UTC, Justin Kim
none Details | Review
srt: add a patch to find thread by cmake option (3.47 KB, patch)
2018-04-11 01:08 UTC, Justin Kim
none Details | Review
linking error - srt 1.3.0 (57.86 KB, text/plain)
2018-04-25 00:51 UTC, Justin Kim
  Details

Description Justin Kim 2018-01-04 06:52:51 UTC
Created attachment 366269 [details] [review]
recipes/srt: Add support SRT

This recipe enables srt plugins of gst-plugins-bad.
Comment 1 Tim-Philipp Müller 2018-01-04 09:04:53 UTC
Shouldn't we use release tarballs in the recipe?

What about adding it to the -bad recipe?
Comment 2 Justin Kim 2018-01-04 09:32:47 UTC
Created attachment 366272 [details] [review]
recipes/srt: Add support SRT
Comment 3 Justin Kim 2018-01-04 09:36:22 UTC
(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.
Comment 4 Sebastian Dröge (slomo) 2018-01-04 09:42:31 UTC
What are the chances that it builds fine on iOS/Android/macOS?
Comment 5 Tim-Philipp Müller 2018-01-04 09:48:29 UTC
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?
Comment 6 Olivier Crête 2018-01-04 20:22:30 UTC
(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.
Comment 7 Justin Kim 2018-01-05 04:25:32 UTC
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.
Comment 8 Tim-Philipp Müller 2018-01-08 17:34:05 UTC
Let's push it then I'd say, as long as someone stands by to fix up any build problems that might emerge :)
Comment 9 Olivier Crête 2018-01-08 19:23:43 UTC
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
Comment 10 Olivier Crête 2018-01-10 18:48:41 UTC
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.
Comment 11 Justin Kim 2018-01-23 06:29:10 UTC
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.
Comment 12 Olivier Crête 2018-01-26 22:20:36 UTC
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.
Comment 13 Justin Kim 2018-02-02 03:43:09 UTC
Created attachment 367784 [details] [review]
srt: Enabled for Android again
Comment 14 Justin Kim 2018-02-02 03:47:15 UTC
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.
Comment 15 Justin Kim 2018-04-09 13:05:16 UTC
Created attachment 370686 [details] [review]
srt: update to 1.2.3
Comment 16 Justin Kim 2018-04-09 13:05:40 UTC
Created attachment 370687 [details] [review]
gst-plugins-bad: enable srt
Comment 17 Nicolas Dufresne (ndufresne) 2018-04-09 13:51:10 UTC
Which builds have already been tested ? Do you want to give this a try on the CI ?
Comment 18 Justin Kim 2018-04-10 01:39:05 UTC
(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 :)
Comment 19 Nicolas Dufresne (ndufresne) 2018-04-10 01:52:30 UTC
Attachment 370686 [details] pushed as 48026d7 - srt: update to 1.2.3
Attachment 370687 [details] pushed as 618dd87 - gst-plugins-bad: enable srt
Comment 20 Nicolas Dufresne (ndufresne) 2018-04-10 02:15:15 UTC
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 ?
Comment 21 Edward Hervey 2018-04-10 06:19:17 UTC
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
Comment 22 Justin Kim 2018-04-10 07:36:29 UTC
Created attachment 370726 [details] [review]
srt: add a patch to find thread by cmake option

To find thread library, Just following what cmake does.
Comment 23 Justin Kim 2018-04-10 07:37:17 UTC
Created attachment 370727 [details] [review]
gst-plugins-bad: enable srt (also add libsrt to deb)
Comment 24 Justin Kim 2018-04-10 07:41:22 UTC
Thank you guys, I tested buildable for android-universal and linux on docker image.
Comment 25 Nicolas Dufresne (ndufresne) 2018-04-10 16:45:11 UTC
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 ?
Comment 26 Justin Kim 2018-04-11 00:54:13 UTC
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.
Comment 27 Justin Kim 2018-04-11 01:08:44 UTC
Created attachment 370761 [details] [review]
srt: add a patch to find thread by cmake option

Updated commit messages.
Comment 28 Olivier Crête 2018-04-17 15:15:33 UTC
I think we should move to 1.3 as they have more patches.
Comment 29 Justin Kim 2018-04-25 00:51:52 UTC
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?
Comment 30 Olivier Crête 2018-04-30 12:59:14 UTC
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