New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dependecy toward libsndfile on all OS #757

Merged
merged 21 commits into from Mar 9, 2015

Conversation

Projects
None yet
9 participants
@mantognini
Member

mantognini commented Dec 22, 2014

This implements #604. It's a rebase of feature/no_libsndfile_osx onto master (which includes feature/no_libsndfile) so be extra careful when proofreading it. Hopefully I haven't made any mistake but there were a lot of conflicts so...

What's done & what's to be done:

  • Android (all libs included)
  • Android – make loading sound compatible with APK
  • iOS (all libs included)
  • Linux (no binary to be provided)
  • OS X (all libs included)
  • Window (all libs included)

It also includes the base step toward #620.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 23, 2014

Member

I've fixed Android compiling and linking in my own branch.

Commit 3b05ee1 fixes code issues, while 2498028 includes precompiled binaries.

There's still the open issue of the new audio file loading not trying to load ressources bundled inside the APK though (will have a look at this as soon as possible before opening another PR on this branch).

Member

MarioLiebisch commented Dec 23, 2014

I've fixed Android compiling and linking in my own branch.

Commit 3b05ee1 fixes code issues, while 2498028 includes precompiled binaries.

There's still the open issue of the new audio file loading not trying to load ressources bundled inside the APK though (will have a look at this as soon as possible before opening another PR on this branch).

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 23, 2014

Member

@MarioLiebisch you should probably drop a line here otherwise ChronicRat might work for nothing.

Member

mantognini commented Dec 23, 2014

@MarioLiebisch you should probably drop a line here otherwise ChronicRat might work for nothing.

@mantognini mantognini self-assigned this Dec 23, 2014

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 26, 2014

Member

Since only Android is missing, and it requires more works, I suggest we go on with reviewing this and merge it. Android stuff can be added later.

Member

mantognini commented Dec 26, 2014

Since only Android is missing, and it requires more works, I suggest we go on with reviewing this and merge it. Android stuff can be added later.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 26, 2014

Member

Just keep in mind that it breaks Android.

Member

MarioLiebisch commented Dec 26, 2014

Just keep in mind that it breaks Android.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 26, 2014

Member

That's a good point. I'll add your first two commits I guess.

Member

mantognini commented Dec 26, 2014

That's a good point. I'll add your first two commits I guess.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 26, 2014

Member

Here, can you check that it's working on Android? Thanks. (I haven't added config_types.h because it's not used by ogg.h nor by os_types.h and it's a file auto generated for a specific platform.)

Member

mantognini commented Dec 26, 2014

Here, can you check that it's working on Android? Thanks. (I haven't added config_types.h because it's not used by ogg.h nor by os_types.h and it's a file auto generated for a specific platform.)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 26, 2014

Member

It will build and link properly, but in this state it's unable to load asset files. Not sure what happens if you try to load files off the sd card/internal memory (might be a starting point for testing).

config_types.h is there since it's included and won't be found when compiling (haven't had a closer look though).

Member

MarioLiebisch commented Dec 26, 2014

It will build and link properly, but in this state it's unable to load asset files. Not sure what happens if you try to load files off the sd card/internal memory (might be a starting point for testing).

config_types.h is there since it's included and won't be found when compiling (haven't had a closer look though).

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 26, 2014

Member

config_types.h is there since it's included and won't be found when compiling (haven't had a closer look though).

Ha, I missed the #else in os_types.h...

Maybe it should go into a dedicated directory for android (like we used to do it with sndfile on Windows/Mac/Android) since it's platform specific. And does this file change with the different Android architectures?

Member

mantognini commented Dec 26, 2014

config_types.h is there since it's included and won't be found when compiling (haven't had a closer look though).

Ha, I missed the #else in os_types.h...

Maybe it should go into a dedicated directory for android (like we used to do it with sndfile on Windows/Mac/Android) since it's platform specific. And does this file change with the different Android architectures?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Dec 30, 2014

Member

Would be another idea. It shouldn't change, since it really just holds the names of integers with a fixed length. Theoretically it might change between compilers, but considering the Android version compiles with GCC by default anyway...

Member

MarioLiebisch commented Dec 30, 2014

Would be another idea. It shouldn't change, since it really just holds the names of integers with a fixed length. Theoretically it might change between compilers, but considering the Android version compiles with GCC by default anyway...

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 8, 2015

Member

@MarioLiebisch Just to be sure, there's absolutely no difference in config_types.h across all different supported archs (mips, armeabi, armeabi-v7a, x86)?

Assuming it is the case, then:

@LaurentGomila (and others): until now we had extlibs/headers/libsndfile/{android,osx,windows}. Should we also have extlibs/headers/ogg/{android,ios,osx,windows} to follow the same spirit or do we just don't care and add config_types.h directly in extlibs/headers/ogg? Alternatively we could edit os_types.h to contain android default value (with the proper #elif statement). What is your opinion on the subject?

Member

mantognini commented Jan 8, 2015

@MarioLiebisch Just to be sure, there's absolutely no difference in config_types.h across all different supported archs (mips, armeabi, armeabi-v7a, x86)?

Assuming it is the case, then:

@LaurentGomila (and others): until now we had extlibs/headers/libsndfile/{android,osx,windows}. Should we also have extlibs/headers/ogg/{android,ios,osx,windows} to follow the same spirit or do we just don't care and add config_types.h directly in extlibs/headers/ogg? Alternatively we could edit os_types.h to contain android default value (with the proper #elif statement). What is your opinion on the subject?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 8, 2015

Member

If it's the same then there's no reason to duplicate it per platform. There's no "spirit", let's just be pragmatic ;)

Member

LaurentGomila commented Jan 8, 2015

If it's the same then there's no reason to duplicate it per platform. There's no "spirit", let's just be pragmatic ;)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jan 8, 2015

Member

From what I can tell, the header file is exactly the same for all Android architectures (compared armeabi, x86, and x86_64) as long as we're forcing GCC to be used.

Not sure regarding other platforms and/or compilers though.

Member

MarioLiebisch commented Jan 8, 2015

From what I can tell, the header file is exactly the same for all Android architectures (compared armeabi, x86, and x86_64) as long as we're forcing GCC to be used.

Not sure regarding other platforms and/or compilers though.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 8, 2015

Member

If it's the same then there's no reason to duplicate it per platform.

Alright. So I've checked and got the same file on OS X than @MarioLiebisch on Android. I don't know about iOS, Linux or Windows (@eXpl0it3r, since you've build them, do you know?).

For reference, config_types.h for Android is here.

Member

mantognini commented Jan 8, 2015

If it's the same then there's no reason to duplicate it per platform.

Alright. So I've checked and got the same file on OS X than @MarioLiebisch on Android. I don't know about iOS, Linux or Windows (@eXpl0it3r, since you've build them, do you know?).

For reference, config_types.h for Android is here.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 8, 2015

Member

I've only built the FLAC libraries which didn't require any config_types.h. The OGG libs etc. were built by @LaurentGomila.

Member

eXpl0it3r commented Jan 8, 2015

I've only built the FLAC libraries which didn't require any config_types.h. The OGG libs etc. were built by @LaurentGomila.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 9, 2015

Member

Right, sorry about that. So, Laurent? ;)

Member

mantognini commented Jan 9, 2015

Right, sorry about that. So, Laurent? ;)

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 29, 2015

Member

I just checked on Linux and the file is different. Don't know about Windows.

Even though we don't provide Linux binaries, it's not that hard to make a special rules for Android in src/SFML/Audio/CMakeLists.txt.

Member

mantognini commented Jan 29, 2015

I just checked on Linux and the file is different. Don't know about Windows.

Even though we don't provide Linux binaries, it's not that hard to make a special rules for Android in src/SFML/Audio/CMakeLists.txt.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 29, 2015

Member

@MarioLiebisch I've updated (rebased + force pushed) the branch, can you check that it now build fine on Android? Thanks.

Member

mantognini commented Jan 29, 2015

@MarioLiebisch I've updated (rebased + force pushed) the branch, can you check that it now build fine on Android? Thanks.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jan 29, 2015

Member

Okay, this just needs a minor fix for the paths (the way you did it won't work since the relative path (ogg/config_types.h) won't work. I'm fixing this with the next commit I push right after this comment.

This leaves just the minor issue of the Android version trying to read the audio files from the file system rather than the assets. I've modified FileInputStream for testing and it works, but I think we should make ResourceStream a public AssetInputStream or RessourceInputStream to allow users to pick either one. SFML funtions taking paths should use that one by default.

What do you think?

Member

MarioLiebisch commented Jan 29, 2015

Okay, this just needs a minor fix for the paths (the way you did it won't work since the relative path (ogg/config_types.h) won't work. I'm fixing this with the next commit I push right after this comment.

This leaves just the minor issue of the Android version trying to read the audio files from the file system rather than the assets. I've modified FileInputStream for testing and it works, but I think we should make ResourceStream a public AssetInputStream or RessourceInputStream to allow users to pick either one. SFML funtions taking paths should use that one by default.

What do you think?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 29, 2015

Member

Okay, this just needs a minor fix for the paths (the way you did it won't work since the relative path (ogg/config_types.h) won't work. I'm fixing this with the next commit I push right after this comment.

Right!

I've modified FileInputStream for testing and it works, but I think we should make ResourceStream a public AssetInputStream or RessourceInputStream to allow users to pick either one. SFML funtions taking paths should use that one by default.

I'm not sure to fully grok how things work on Android -- I personally trust your judgment. ;-)

Member

mantognini commented Jan 29, 2015

Okay, this just needs a minor fix for the paths (the way you did it won't work since the relative path (ogg/config_types.h) won't work. I'm fixing this with the next commit I push right after this comment.

Right!

I've modified FileInputStream for testing and it works, but I think we should make ResourceStream a public AssetInputStream or RessourceInputStream to allow users to pick either one. SFML funtions taking paths should use that one by default.

I'm not sure to fully grok how things work on Android -- I personally trust your judgment. ;-)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 4, 2015

Member

Fixed already - really just been two changes at the same position in the file.

Member

MarioLiebisch commented Mar 4, 2015

Fixed already - really just been two changes at the same position in the file.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 4, 2015

Member

👍

Member

mantognini commented Mar 4, 2015

👍

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 5, 2015

Member

have all issues been fixed or am I mistaken?

Member

mantognini commented Mar 5, 2015

have all issues been fixed or am I mistaken?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 5, 2015

Member

Yes, I think we are good to go.

Member

LaurentGomila commented Mar 5, 2015

Yes, I think we are good to go.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 9, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Mar 9, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 9, 2015

Member

I've got some, sorry. :-P

I'm pretty much done reviewing it and just spot minor typos. Won't be long.

Member

mantognini commented Mar 9, 2015

I've got some, sorry. :-P

I'm pretty much done reviewing it and just spot minor typos. Won't be long.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 9, 2015

Member

Done. Nothing else for me.

Member

mantognini commented Mar 9, 2015

Done. Nothing else for me.

@a-maly

This comment has been minimized.

Show comment
Hide comment
@a-maly

a-maly Mar 9, 2015

It is not possible to register your own file reader/writer that has been internally registered, just for the fact that you used to search forward instead of backward.

for (ReaderFactoryArray::const_iterator it = s_readers.begin(); it != s_readers.end(); ++it)

I'm just writing to let you know.

a-maly commented Mar 9, 2015

It is not possible to register your own file reader/writer that has been internally registered, just for the fact that you used to search forward instead of backward.

for (ReaderFactoryArray::const_iterator it = s_readers.begin(); it != s_readers.end(); ++it)

I'm just writing to let you know.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 9, 2015

Member

@a-maly Hm... yeah, overwriting built-in filters sounds like a neat tidbit some might want to do.

Member

MarioLiebisch commented Mar 9, 2015

@a-maly Hm... yeah, overwriting built-in filters sounds like a neat tidbit some might want to do.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 9, 2015

Member

I suggest that we release it now, get some feedback, and work more on it later if needed. Unless something is broken, this PR must be merged now :p

Member

LaurentGomila commented Mar 9, 2015

I suggest that we release it now, get some feedback, and work more on it later if needed. Unless something is broken, this PR must be merged now :p

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 9, 2015

Member

I guess we could add a priority system, or something similar, in SFML 3 but I believe that for now this is enough. Most people will simply use the default implementation (which should be good enough in any case) I guess.

So let's proceed as-is. :-)

Member

mantognini commented Mar 9, 2015

I guess we could add a priority system, or something similar, in SFML 3 but I believe that for now this is enough. Most people will simply use the default implementation (which should be good enough in any case) I guess.

So let's proceed as-is. :-)

@eXpl0it3r eXpl0it3r merged commit aa9a6de into master Mar 9, 2015

@eXpl0it3r eXpl0it3r deleted the feature/no_libsndfile_all_os branch Mar 9, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 9, 2015

Member

👏 🎉 🍸

Now let's wait for all the bug reports. 😄

Member

eXpl0it3r commented Mar 9, 2015

👏 🎉 🍸

Now let's wait for all the bug reports. 😄

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 13, 2015

Member

Now let's wait for all the bug reports.

Got one! 😋

It's a slight oversight that can lead to some hard-to-debug issues: InputStream is copyable. While it should be alright with the current implementation of MemoryInputStream, it is not good with FileInputStream.

There is one thing I'm not sure of: since InputStream prior to that merge was not exported, no one could be using it before, right?

If that's the case we can just make it inherit from NonCopyable and we're done. Otherwise we can make FileInputStream (and optionally MemoryInputStream too) inherit from NonCopyable.

Member

mantognini commented Apr 13, 2015

Now let's wait for all the bug reports.

Got one! 😋

It's a slight oversight that can lead to some hard-to-debug issues: InputStream is copyable. While it should be alright with the current implementation of MemoryInputStream, it is not good with FileInputStream.

There is one thing I'm not sure of: since InputStream prior to that merge was not exported, no one could be using it before, right?

If that's the case we can just make it inherit from NonCopyable and we're done. Otherwise we can make FileInputStream (and optionally MemoryInputStream too) inherit from NonCopyable.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 13, 2015

Member

FileInputStream was not copyable when it used a std::ifsteam, but now with FILE* it is indeed a problem. But there's no reason to disallow copy at the base class level, so let's just do it for FileInputStream.

Member

LaurentGomila commented Apr 13, 2015

FileInputStream was not copyable when it used a std::ifsteam, but now with FILE* it is indeed a problem. But there's no reason to disallow copy at the base class level, so let's just do it for FileInputStream.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 13, 2015

Member

Alright, will create a PR for that then.

Member

mantognini commented Apr 13, 2015

Alright, will create a PR for that then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment