ofOpenALSoundPlayer Filesystem

[dimitre]

[dimitre]

great ideas. yeah we should find all functions compatible with wstring() and use them like this PR https://github.com/openframeworks/openFrameworks/pull/7435

[dimitre]

I've converted this one to draft and opened a separate issue here to know how to properly have windows only code on OF - https://github.com/openframeworks/openFrameworks/issues/7446

[dimitre]

Thanks @artificiel, I've updated somethings in the PR accordingly. I've tried to open sf_wchar_open with filesystem directly so it outputs the correct c_str for any platform, but this revealed some flawed logic in ofConstants.h when trying to include filesystem.

I don't even think it worths correcting logic there once we are about to drop linuxarmv6l and boost itself. after this ofConstants.h can be greatly simplified

[artificiel]

OK, but does

SNDFILE* f = sf_open(path,SFM_READ,&sfInfo);

work on windows? (where native path will be wide?) (or even anywhere, as sf_open takes a char*?) -- testing those conditional includes in platform-specific situations is not straightforward...

[dimitre]

Hello @artificiel I've updated this PR with some improvements on extensions handling and using sndfile windows version with wchar. opinions are welcome

[artificiel]

excellent; notably the treatment in ofOpenALSoundPlayer::sfReadFile is correct!

for the one in ofOpenALSoundPlayer::mpg123Stream there is still a problem if: the fs::path contains incompatible wchar unicode it will throw exception / crash. we have 2 options:

OPTION A: more correct, more work: not use mpg123_open (which is POSIX-only) and use mpg123_open_fd, and handle ourselves the file descriptor to open natively and pass the descriptor:

// incomplete code
bool openThing(fs::path & filename) {
   mh = mpg123_new(NULL, &err);
#ifdef _WIN32
    HANDLE hFile = CreateFileW(filename.c_str() /*<etc> */);
    mpg123_open_fd(mh, _open_osfhandle((intptr_t)hFile, /* <etc> */));
#else
    int fd = open(filename, O_RDONLY);
    mpg123_open_fd(mh, fd);
#endif
    // use mh
    return true;
}

OPTION B: not support paths containing non-utf-8 characters (on windows), but trapping the crash, based on <optional> implementation of ofPathToString described in the other thread:

// incomplete code
void openThing(fs::path & filename) {
    if (auto posix_conformant_filename = ofPathToOptionalString(filename) {
       // the wchar path turns out to be POSIX-friendy
       int fd = open(filename, O_RDONLY);
       mpg123_open(mp3streamf, posix_conformant_filename->c_str())!=MPG123_OK
       // do the thing
       return true;
    } else {
       // the path contains untranslatable glyphs
       // somehow notify
      return false;
    }
}

the #ifdef version is more compatible and will allow all possible paths; the <optional> version is simpler to maintain, but will not work for some windows paths.

NB: the #ifdef pattern is applicable to all libraries that will take a file descriptor, which is somehow more frequent than a dedicated wchar version

NB2 actually ofPathToString() is functionally ofConformPathToNarrowString() -- maybe that makes its purpose clearer than a simple "convert to string" utility. using optional provides a clean interface to handle the failure exception, so becomes ofComformPathToOptionalNarrowString()

[artificiel]

hahahah you edited towards _fd while I was writing the comment!

[dimitre]

haha yes. I vote to keep mpg123_open now, and keep things c++14 backwards compatible just one more release. Ah, and openal was discontinued in 2009 :) so it is not worth clearing all the spider webs there

[artificiel]

ok but then error checking should be something like:

auto conformant_path = ofPathToString(path);
if (conformant_path.empty()) {
   // NOTE: the provided windows path is not POSIX-compatible with mpg123
} else {
   if(mpg123_open(f, conformant_path.c_str() )!=MPG123_OK){
        //bla
   } 
}

I still suggest renaming ofPathToString to ofConformPathToNarrowString as it's really the function of that method (not a mere practical conversion)

[dimitre]

About extensions: I personally never seen extensions with extended characters, like a file image.ção another good point of using ofToLower would be the case (rarely though) somebody uses a capitalized extension, like .Hdr

As a practical suggestion we can do things incrementally, like using fs::path in more places in core parallel with this we can start using std::basic_string in places where std::string are used, so we keep fs::path portability when needed.

[dimitre]

alternatively we can handle extensions like this

	vector <of::filesystem::path> paths {
		{ "asdf.MP3" },
		{ "asdf.Mp3" },
		{ "asdf.mp3" },
		{ "asdf.mP3" },
	};
	
	for (auto & p : paths) {
		auto ext = p.extension().generic_string();
		std::transform(ext.begin(), ext.end(), ext.begin(),
			[](unsigned char c){ return std::tolower(c); });

		if ( ext == ".mp3" ) {
			cout << ext << " : " << p << " YES" << endl;
		}
	}