Skip to content
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

Restrict OpenSLES to Android via CMake #10947

Merged
merged 1 commit into from Jan 30, 2023

Conversation

Zopolis4
Copy link
Contributor

@Zopolis4 Zopolis4 commented Aug 5, 2022

OpenSLES code is only compiled on android, but instead of checking via CMake we search for OpenSLES on all platforms, and then compile that code if we find it. Just in case a non-android build finds OpenSLES, both of the files are wrapped in #ifdefs. Instead, this PR will only search for and compile the files on Android, thus not generating a false warning that OpenSLES was not found on regular builds.

@Zopolis4 Zopolis4 force-pushed the opensleuth branch 2 times, most recently from d01aa8f to 2b14ee6 Compare August 5, 2022 03:56
@phire
Copy link
Member

phire commented Aug 5, 2022

This is not going to degrade correctly if there is an android build that doesn't find OpenSLES.

Also, OpenSL ES is technically a cross-platform API. Do we really want to put it behind an Android gate? A HAVE_OPENSL_ES define might make more sense.

Edit: Or alternatively, mark it as a required library for android. It's been in there since Android 2.3. Unless there is a chance of Android deprecating it at some point?

@phire
Copy link
Member

phire commented Aug 5, 2022

Also, you remove AudioCommon/OpenSLESStream.h from the list of sources, but it's included anyway?

@Pokechu22
Copy link
Contributor

The way the audio backends currently seem to work is that the header is always included, but if the backend isn't supported it would just produce this:

class OpenSLESStream final : public SoundStream
{
};

which combines with these default values:

class SoundStream
{
protected:
std::unique_ptr<Mixer> m_mixer;
public:
SoundStream() : m_mixer(new Mixer(48000)) {}
virtual ~SoundStream() {}
static bool IsValid() { return false; }
Mixer* GetMixer() const { return m_mixer.get(); }
virtual bool Init() { return false; }
virtual void SetVolume(int) {}
// Returns true if successful.
virtual bool SetRunning(bool running) { return false; }
};

so that IsValid() always returns false. (IsValid always returns true for all subclasses that aren't removed by the preprocessor except for OpenALStream, which is dependent on both Windows being in use and the run-time existence of openal32.dll, which we don't currently ship.)

We should either replace this with something else (preprocessor checks in AudioCommon.cpp for all backends), or leave the preprocessor checks in OpenSLESStream.h/cpp alone and just make the change in CMakeLists.txt (which should remove the false warning).

(A HAVE_OPENSL_ES check would also make more sense than ANDROID, and in fact there's already a #endif // HAVE_OPENSL in OpenSLESStream.h corresponding to the #ifdef ANDROID, but HAVE_OPENSL itself is never defined or checked anywhere.)

@Zopolis4
Copy link
Contributor Author

Zopolis4 commented Aug 7, 2022

I think there are two paths to take with this PR. Either we try to add cross-platform OpenSLES support or we restrict it to just android. Which one would people prefer?

@Zopolis4
Copy link
Contributor Author

Given that it is OpenSL ES, I think it makes more sense to restrict it to just android, especially when cubeb works so well.

@AdmiralCurtiss AdmiralCurtiss merged commit 8edca20 into dolphin-emu:master Jan 30, 2023
11 checks passed
@Zopolis4 Zopolis4 deleted the opensleuth branch January 30, 2023 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants