Feature add linux gstreamer support by hhoegelo · Pull Request #11 · NativeInstruments/ni-media

hhoegelo · 2018-01-05T15:35:31Z

If you like, you can pull my attempt of adding mp3, m4a, alac and wma support on linux platform by using gstreamer framework. I didn't manage to have all tests green, though. For example, the 'interlaced_by_67_frames'-tests are off by 1 sample, which I - so far - couldn't find a reason for.

Most likely other people won't notice my fork. I hope, if you pull it into the base fork, we have a higher chance that anyone can help fixing.

… library this is the initial revision, tests are not yet green, to be continued

marcrambo · 2018-01-08T10:16:19Z

Hey Henry!

That's great! Thanks a lot for the contribution. I will look at it as soon as possible. In the mean time could you please update the travis-ci to retrieve the gstreamer package so that the linux build passes?

Thanks,
Marc

kevin-- · 2018-01-11T01:08:35Z

audiostream/CMakeLists.txt


 set( COMPILE_WITH_COREAUDIO         DONT_COMPILE)
 set( COMPILE_WITH_MEDIA_FOUNDATION  DONT_COMPILE)
+set( COMPILE_WITH_GSTREAMER  				DONT_COMPILE)


pedantic: align DONT_COMPILE with above statements

kevin-- · 2018-01-11T01:10:09Z

audiostream/CMakeLists.txt

 add_src_file  (FILES_media_audio_source "src/ni/media/audio/source/core_audio_file_source.cpp"       ${COMPILE_WITH_COREAUDIO}         WITH_HEADER )
 add_src_file  (FILES_media_audio_source "src/ni/media/audio/source/media_foundation_helper.h"        ${COMPILE_WITH_MEDIA_FOUNDATION}              )
 add_src_file  (FILES_media_audio_source "src/ni/media/audio/source/media_foundation_file_source.cpp" ${COMPILE_WITH_MEDIA_FOUNDATION}  WITH_HEADER )
+add_src_file  (FILES_media_audio_source "src/ni/media/audio/source/gstreamer_file_source.cpp" 			 ${COMPILE_WITH_GSTREAMER}  			 WITH_HEADER )


pedantic: align with other entires

kevin-- · 2018-01-11T01:14:35Z

cmake/FindGStreamer.cmake

+
+
+if( GSTREAMER_FOUND )
+	if( NOT TARGET GSTREAMER::gstreamer )


pedantic: alignment

kevin-- · 2018-01-11T01:14:48Z

cmake/FindGStreamerApp.cmake

+
+
+if( GSTREAMERAPP_FOUND )
+	if( NOT TARGET GSTREAMERAPP::gstreamerapp )


pedantic: alignment

kevin--

Looks good. I would just ask that you apply the .clang-format rules found in the root of the repo, along with the other formatting in CMake files

sdroege · 2018-04-15T09:44:12Z

audiostream/src/ni/media/audio/source/gstreamer_file_source.cpp

+
+    if ( numBytesRequested )
+    {
+        tGstPtr<GstElement> sink( gst_bin_get_by_name( GST_BIN( m_pipeline.get() ), "sink" ), gst_object_unref );


It would be better to store the sink pointer somewhere instead of retrieving it all the time

sdroege · 2018-04-15T09:44:17Z

audiostream/src/ni/media/audio/source/gstreamer_file_source.cpp

+    {
+        pcm::number_type number_type = gst_format_char_to_number_type( format[0] );
+        auto             srcDepth    = std::atol( format + 1 );
+        auto             endian      = ( strcmp( format, "BE" ) == 0 ) ? pcm::big_endian : pcm::little_endian;


You probably want to use GstAudioInfo here and in the function above. it gives you a simple C struct with all these information you want to extract from the caps. Also check the GstAudioFormatInfo that is part of the GstAudioInfo.

sdroege · 2018-04-15T09:44:21Z

audiostream/src/ni/media/audio/source/gstreamer_file_source.cpp

+    if ( wait_for_async_operation() != GST_STATE_PAUSED )
+        throw std::runtime_error( "gstreamer_file_source: pipeline doesn't preroll into paused state" );
+
+    gst_element_set_state( m_pipeline.get(), GST_STATE_PLAYING );


Check return value

sdroege · 2018-04-15T09:44:23Z

audiostream/src/ni/media/audio/source/gstreamer_file_source.cpp

+
+void gstreamer_file_source::preroll_pipeline()
+{
+    gst_element_set_state( m_pipeline.get(), GST_STATE_PAUSED );


Check return value

sdroege · 2018-04-15T09:44:26Z

audiostream/src/ni/media/audio/source/gstreamer_file_source.cpp

+
+    gst_bin_add_many( GST_BIN( m_pipeline.get() ), source, decodebin, queue, sink, nullptr );
+    gst_element_link_many( source, decodebin, NULL );
+    gst_element_link_many( queue, sink, NULL );


best to check return values, things can fail

sdroege · 2018-04-15T09:44:30Z

audiostream/src/ni/media/audio/source/gstreamer_file_source.cpp

+    auto        sinkpad     = gst_element_get_static_pad( sink, "sink" );
+    auto        caps        = gst_pad_get_current_caps( sinkpad );
+    auto        caps_struct = gst_caps_get_structure( caps, 0 );
+    fill_format_info( caps_struct, container );


You're leaking the caps and sinkpad here.

sdroege · 2018-04-15T09:44:35Z

audiostream/src/ni/media/audio/source/gstreamer_file_source.cpp

+gstreamer_file_source::~gstreamer_file_source()
+{
+    gst_element_set_state( m_pipeline.get(), GST_STATE_NULL );
+    wait_for_async_operation();


Downwards state change is always sync

sdroege · 2018-04-15T09:48:41Z

@hhoegelo Can you explain the easiest way to reproduce the 1-sample-off problem and what information you gathered so far?

If I understand correctly, this happens on specific files (WMA? Or everything? All WMA files or a specific one?) and if you decode from the beginning vs. you seek to a position, you would end up one sample off? Is it always absolutely one sample, or would it sometimes be more or less, or would every seek make it one more sample off?

Re main loop: this is not required, correct. You have multiple options, but first of all: why do you care? You want to block until error or reading samples is possible? And then blocking-read the samples?

Blocking retrieval of messages from the GStreamer bus to know when pre-rolling has happened. This does not help you much here because you also block on reading samples and you want to continue getting bus message to know when an error happened
Integration of the GStreamer bus into whatever event loop you're using here. Details depend on how that event loop work
Synchronously getting messages from the bus and handling them from there. You could use that to get notified about errors once pre-rolling has happened in 1) for example. Note that you must call things like setting pipeline states from a different thread then the sync message handler.

From what I saw in the code so far I would suggest a combination of 1) and 3). You block until error or the sink is ready (you received the ASYNC_DONE message on the bus), by calling gst_bus_timed_pop_filtered(bus, whatever_timeout_you_want_in_ns, GST_MESSAGE_ERROR | GST_MESSAGE_EOS | GST_MESSAGE_ASYNC_DONE). In addition you add a sync bus handler (gst_bus_set_sync_handler) that ignores all messages until ASYNC_DONE is received. Once ASYNC_DONE is returned from gst_bus_timed_pop_filtered you are pre-rolled and can start. If instead ERROR or EOS is received you know that nothing will ever work here. You then start pulling from the appsink, but if the sync bus handler notifies you about ERROR, you asynchronously report that somehow and shut down the pipeline (gst_element_call_async might be useful for the latter).

sdroege · 2018-04-15T09:49:54Z

Sorry if some comments appeared twice, GitHub had some kind of hickup while I was doing the review and returned a few server errors

kevin-- · 2018-04-15T22:16:35Z

thanks for the thorough look @sdroege really appreciate seeing this level of collab

… library this is the initial revision, tests are not yet green, to be continued

marcrambo · 2018-04-17T07:31:48Z

audiostream/src/ni/media/audio/source/gstreamer_file_source.cpp

+    gst_element_set_state( m_pipeline.get(), GST_STATE_PAUSED );
+
+    if ( wait_for_async_operation() != GST_STATE_PAUSED )
+        throw std::runtime_error( "gstreamer_file_source: pipeline doesn't preroll into paused state" );


This line randomly throws. There seems to be a race condition when setting / checking the state

marcrambo · 2018-04-17T07:42:39Z

@hhoegelo can you please pull in my branch https://github.com/marcrambo/ni-media/tree/feature-add-linux-gstreamer-support? I reenabled the tests and commited a fix for all the tests failing due to an exception thrown while retrieving the track length. Also I improved the logging so we have a better overview of which tests are still failing.

marcrambo · 2018-04-17T07:51:06Z

@sdroege Thanks a lot for your help which is really greatly appreciated! I was wondering if it was possible to run gstreamer in "synchronous" mode i.e. without having to do "preroll pipeline" and so on. Calls to gestream_source::read don't need to be realtime capable i.e. non blocking. Actually all the other backend sources are synchronous (except itunes_source which also does some sort of asynchronous prerolling / caching as in gstreamer )

sdroege · 2018-04-17T08:21:53Z

@sdroege Thanks a lot for your help which is really greatly appreciated! I was wondering if it was possible to run gstreamer in "synchronous" mode i.e. without having to do "preroll pipeline" and so on. Calls to gestream_source::read don't need to be realtime capable i.e. non blocking. Actually all the other backend sources are synchronous (except itunes_source which also does some sort of asynchronous prerolling / caching as in gstreamer )

@marcrambo That's exactly how it would behave with my suggestion (1 + 3) from comment #11 (comment)

While GStreamer is always async to some degree, there is API that makes it easy enough to build a sync API on top

hhoegelo added 3 commits December 21, 2017 17:43

adding support for mp3, m4a and wma files on linux by using gstreamer…

717b048

… library this is the initial revision, tests are not yet green, to be continued

fixed some tests, still some failing

5274d99

improve readability

9130479

hhoegelo added 4 commits January 8, 2018 14:44

add gstreamer dependencies for ci run

693b84d

fix intendation issue

e9254c1

blindflight: configureing ubuntu packages not having ubuntu

07341d1

blindflight: configureing ubuntu packages not having ubuntu

8ec93ff

marcrambo assigned andre-bergner, kevin--, mgoldsmith, d-xo, FalconPDX and marcrambo Jan 10, 2018

kevin-- reviewed Jan 11, 2018

View reviewed changes

cmake/FindGStreamer.cmake Outdated

if( GSTREAMER_FOUND )

if( NOT TARGET GSTREAMER::gstreamer )

Copy link

kevin-- Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pedantic: alignment

kevin-- reviewed Jan 11, 2018

View reviewed changes

cmake/FindGStreamerApp.cmake Outdated

if( GSTREAMERAPP_FOUND )

if( NOT TARGET GSTREAMERAPP::gstreamerapp )

Copy link

kevin-- Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pedantic: alignment

kevin-- suggested changes Jan 11, 2018

View reviewed changes

hhoegelo added 2 commits January 11, 2018 08:12

fix formatting (apply clang-format)

f46c5d2

fix alignment

d147f11

FalconPDX requested review from FalconPDX, andre-bergner, d-xo and marcrambo January 11, 2018 16:19

FalconPDX unassigned andre-bergner, kevin--, mgoldsmith, d-xo and FalconPDX Jan 11, 2018

sdroege reviewed Apr 15, 2018

View reviewed changes

hhoegelo added 11 commits April 16, 2018 21:36

adding support for mp3, m4a and wma files on linux by using gstreamer…

4643b10

… library this is the initial revision, tests are not yet green, to be continued

fixed some tests, still some failing

8f1123b

improve readability

e6a05c1

add gstreamer dependencies for ci run

7146d73

fix intendation issue

eea5bb4

blindflight: configureing ubuntu packages not having ubuntu

48c73f2

blindflight: configureing ubuntu packages not having ubuntu

7dde3c7

fix formatting (apply clang-format)

44d9df9

fix alignment

7023f2f

incorporated some of the pull request comments

59bd834

incorporated some of sdroege's comments

54ea50d

hhoegelo force-pushed the feature-add-linux-gstreamer-support branch from 1c58318 to 54ea50d Compare April 16, 2018 20:40

hhoegelo added 2 commits April 17, 2018 08:46

fixup

4d82bdc

removed GstApp dependency

cf1e7a8

marcrambo reviewed Apr 17, 2018

View reviewed changes

merged marc boucek's fixes

b4d9d4c

FalconPDX mentioned this pull request Apr 15, 2020

Linux does not support mp4 or m4a? #41

Closed



		if( GSTREAMERAPP_FOUND )
		if( NOT TARGET GSTREAMERAPP::gstreamerapp )

Conversation

hhoegelo commented Jan 5, 2018

Uh oh!

marcrambo commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin-- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdroege commented Apr 15, 2018

Uh oh!

sdroege commented Apr 15, 2018

Uh oh!

kevin-- commented Apr 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcrambo commented Apr 17, 2018

Uh oh!

marcrambo commented Apr 17, 2018

Uh oh!

sdroege commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kevin-- left a comment •

edited

Loading