Haiku port #1490

mmuman · 2025-02-16T00:00:18Z

It seems we had patches getting dust. I rebased them and dropped stuff that didn't seem needed anymore.
All tests pass though I haven't tried the audio or MIDI outputs, but the code worked in previous versions.

mmuman · 2025-02-16T00:13:49Z

Ugh, seems they missed the proper Haiku check 🤷

mmuman · 2025-02-16T00:37:50Z

That should fix it.

mmuman · 2025-02-16T01:13:29Z

For some reason tests 25 & 27 fail now… !?

mmuman · 2025-02-16T01:18:20Z

Oh, yeah… nothing LANC=C LC_ALL=C can't fix. (comma vs decimal point)

derselbst

Thanks for submitting. However, this is not yet in a shape that is ready to accept. It needs some work. Comments below. Also, it would be very helpful to have a Haiku CI build in place, to avoid breaking stuff in the future.
Edit: Here's an example of how SDL coped with Haiku CI: libsdl-org/SDL#6739

derselbst · 2025-02-16T13:11:44Z

CMakeLists.txt

 include ( TestBigEndian )
 test_big_endian ( WORDS_BIGENDIAN )
+endif ( NOT HAIKU )


Why is that if-ed out?

The macros were broken on Haiku I believe.

derselbst · 2025-02-16T13:12:15Z

CMakeLists.txt

+# Haiku
+if ( HAIKU )
+set ( CMAKE_CXX_FLAGS "-fpermissive")
+endif ( HAIKU )


Why is that needed?

IIRC it was needed because the compiler would start spitting out errors for what was seemingly valid C++ code for non-permissive builds.

derselbst · 2025-02-16T13:13:52Z

cmake_admin/report.cmake

+  message ( "Haiku MediaKit support: yes" )
+else ( HAIKU_SUPPORT )
+  message ( "Haiku MediaKit support: no" )
+endif ( HAIKU_SUPPORT )


This is redundant to above and should be removed.

derselbst · 2025-02-16T13:15:26Z

CMakeLists.txt

@@ -94,6 +94,8 @@ option ( enable-dsound "compile DirectSound support (if it is available)" on )
 option ( enable-wasapi "compile Windows WASAPI support (if it is available)" on )
 option ( enable-waveout "compile Windows WaveOut support (if it is available)" on )
 option ( enable-winmidi "compile Windows MIDI support (if it is available)" on )
+option ( enable-haiku "compile Haiku MediaKit audio support (if it is available)" on )


Suggested change

option ( enable-haiku "compile Haiku MediaKit audio support (if it is available)" on )

option ( enable-mediakit "compile Haiku MediaKit audio support (if it is available)" on )

IMO, the naming is unfortunate. enable-haiku would enable the audio driver and not support for Haiku in general. I'd suggest to rename it mediakit.

derselbst · 2025-02-16T13:16:35Z

CMakeLists.txt

@@ -94,6 +94,8 @@ option ( enable-dsound "compile DirectSound support (if it is available)" on )
 option ( enable-wasapi "compile Windows WASAPI support (if it is available)" on )
 option ( enable-waveout "compile Windows WaveOut support (if it is available)" on )
 option ( enable-winmidi "compile Windows MIDI support (if it is available)" on )
+option ( enable-haiku "compile Haiku MediaKit audio support (if it is available)" on )
+option ( enable-midikit2 "compile Haiku MidiKit2 MIDI support (if it is available)" on )


Both of these cmake options should be wraped inside a if ( CMAKE_SYSTEM MATCHES "Haiku" ), as we do it below, since they are platform-specific options.

src/drivers/fluid_midikit2.cpp

derselbst · 2025-02-16T13:30:52Z

src/drivers/fluid_midikit2.cpp

+        driver->m_consumer->Release();
+        delete_fluid_midi_parser(driver->parser);
+        FLUID_FREE(driver);
+        return NULL;


You're leaking driver->m_consumer here. Pls. avoid these manual partial cleanup logic and instead, properly do the cleanup in delete_fluid_midikit2_driver

derselbst · 2025-02-16T13:31:12Z

src/drivers/fluid_midikit2.cpp

+        driver->m_consumer->Unregister();
+        driver->m_consumer->Release();
+        delete_fluid_midi_parser(driver->parser);
+        FLUID_FREE(driver);


You're leaking driver->m_consumer.

Release() will delete it by itself as it's ref-counted. I'll add a comment.

But how is it supposed to know that m_consumer (an instance of fluid_midikit2_input) was allocated via new? And not via new[]? And not on the stack either?? I have no doubt that Release() will cleanup the internal state of the BMidiEndpoint object (i.e. all the things that were ref-counted allocated in its constructor). But it surely cannot release the memory of the derived fluid_midikit2_input, esp. because fluid_midikit2_input currently has a non-virtual destructor.

Meanwhile I discovered the implementation of BMidiEndpoint: https://github.com/haiku/haiku/blob/c8252422b8b3a2a293fb3be039eec6f87b30b467/src/kits/midi2/MidiEndpoint.cpp#L118-L135

So it turns out that they do make a hard call to delete this. If you used new[] to allocate the endpoint this will cause undefined behavior. And if you stack allocated this you might get a log message: https://github.com/haiku/haiku/blob/c8252422b8b3a2a293fb3be039eec6f87b30b467/src/kits/midi2/MidiEndpoint.cpp#L258-L260

Poor implementation, IMO. But still: You must declare an empty destructor for fluid_midikit2_input and mark it virtual to make this cleanup work as intended.

derselbst · 2025-02-16T13:31:54Z

src/synth/fluid_synth.c

+	char midiSettings[PATH_MAX] = "";
+	if (find_directory(B_USER_SETTINGS_DIRECTORY, -1, false, midiSettings, sizeof(midiSettings)) == B_OK) {
+		strcat(midiSettings, "/Media/midi_settings");
+		if( access( midiSettings, F_OK ) != -1 ) {
+			FILE *inFile = fopen(midiSettings, "rt");
+			if (inFile) {
+				char sf2file[PATH_MAX];
+				if (fscanf(inFile, "# Midi\n\tsoundfont \"%[^\"]\"", sf2file)) {
+					fluid_settings_register_str(settings, "synth.default-soundfont", sf2file, 0);
+				}
+				fclose(inFile);
+			}
+		}
+	}


What does that do??

Seemingly fetches the first path to the sound fonts from the Media preferences, as it is used internally.

sonarqubecloud · 2025-02-16T13:42:47Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mmuman · 2025-02-17T09:55:20Z

Thanks for submitting. However, this is not yet in a shape that is ready to accept. It needs some work. Comments below.

Well I didn't write most of this code, just trying to upstream before it bit-rots, so I'll have to figure it out as well.

Also, it would be very helpful to have a Haiku CI build in place, to avoid breaking stuff in the future.
Edit: Here's an example of how SDL coped with Haiku CI: libsdl-org/SDL#6739

Yes, though our cross-compiler is only built as part of our build system, but there are a few solutions now.

mmuman · 2025-02-17T10:13:05Z

poke @Cacodemon345 @threedeyes :-)

Cacodemon345 · 2025-02-17T11:22:44Z

I can't directly push to the branch (and I don't have a working Haiku OS installation at the moment either at the moment to help out with the changes).

threedeyes and others added 5 commits February 16, 2025 00:00

Add Haiku MediaKit support

7cee5ef

Get rid of excessive realtime audio output latency

e99318b

Add Haiku MidiKit2 MIDI backend

b2c1fd6

CMake: Report Haiku audio & MIDI

92865cc

Haiku: no need for <type_traits> and C++2011

e2f6025

mmuman mentioned this pull request Feb 16, 2025

Upstream Fluidsynth haikuports/haikuports#11790

Open

CMake: properly check for Haiku headers

ff9691f

derselbst requested changes Feb 16, 2025

View reviewed changes

cbjeukendrup mentioned this pull request Feb 16, 2025

Feature request: thirdparty libs should be linkable from system installed musescore/MuseScore#11572

Open

Haiku: cleanup

1ef3aab

mmuman force-pushed the haiku-port branch from 53165fc to 1ef3aab Compare February 17, 2025 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Haiku port #1490

Haiku port #1490

mmuman commented Feb 16, 2025 •

edited

Loading

mmuman commented Feb 16, 2025

mmuman commented Feb 16, 2025

mmuman commented Feb 16, 2025

mmuman commented Feb 16, 2025 •

edited

Loading

derselbst left a comment •

edited

Loading

derselbst Feb 16, 2025

Cacodemon345 Feb 17, 2025

derselbst Feb 16, 2025

Cacodemon345 Feb 17, 2025

derselbst Feb 16, 2025

derselbst Feb 16, 2025

derselbst Feb 16, 2025

derselbst Feb 16, 2025

derselbst Feb 16, 2025

mmuman Feb 17, 2025 •

edited

Loading

derselbst Feb 17, 2025

derselbst Feb 17, 2025

derselbst Feb 16, 2025

mmuman Feb 17, 2025 •

edited

Loading

sonarqubecloud bot commented Feb 16, 2025

mmuman commented Feb 17, 2025

mmuman commented Feb 17, 2025

Cacodemon345 commented Feb 17, 2025

	option ( enable-haiku "compile Haiku MediaKit audio support (if it is available)" on )
	option ( enable-mediakit "compile Haiku MediaKit audio support (if it is available)" on )

Haiku port #1490

Are you sure you want to change the base?

Haiku port #1490

Conversation

mmuman commented Feb 16, 2025 • edited Loading

mmuman commented Feb 16, 2025

mmuman commented Feb 16, 2025

mmuman commented Feb 16, 2025

mmuman commented Feb 16, 2025 • edited Loading

derselbst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmuman Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmuman Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 16, 2025

Quality Gate passed

mmuman commented Feb 17, 2025

mmuman commented Feb 17, 2025

Cacodemon345 commented Feb 17, 2025

mmuman commented Feb 16, 2025 •

edited

Loading

mmuman commented Feb 16, 2025 •

edited

Loading

derselbst left a comment •

edited

Loading

mmuman Feb 17, 2025 •

edited

Loading

mmuman Feb 17, 2025 •

edited

Loading