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

[android] Simplify TTS code using AudioManagerCompat #7575

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Isira-Seneviratne
Copy link
Contributor

@Jean-BaptisteC
Copy link
Member

Is it really necessary to use media library, we try to reduce as possible the size of APK

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

What are the pros and cons of this change? Does it fix any issue that is not possible to fix otherwise?

@Isira-Seneviratne
Copy link
Contributor Author

What are the pros and cons of this change? Does it fix any issue that is not possible to fix otherwise?

It provides consistent behaviour across Android versions when requesting/abandoning audio focus.

On the other hand, it'll increase the app size slightly as mentioned, though this impact can be reduced with code shrinking.

@biodranik
Copy link
Member

  1. Which inconsistency is fixed? Is there any reported issue about it?
  2. How big is the size increase of the release apk?
  3. Did you test it on different Android versions?

@Isira-Seneviratne
Copy link
Contributor Author

Isira-Seneviratne commented Mar 14, 2024

  1. Which inconsistency is fixed? Is there any reported issue about it?

I meant that it avoids the need to call a deprecated API directly (the compat class handles that scenario), and it makes the transition to the framework API easier when the minSdk is bumped to 26 or higher.

  1. How big is the size increase of the release apk?
  2. Did you test it on different Android versions?

I'm having issues with building the app, as it's complaining about Boost being missing (I'm building on Windows, and I've cloned the repos for the 3rd party dependencies).

That being said, there shouldn't be a significant increase in the size of the app (maybe a few kilobytes) or cause any issues on different Android versions (it's from a library developed by Google, and the new focus request is in the same format as the original version).

@biodranik
Copy link
Member

Let's measure the size to be precise. Did you follow all documentation instructions about cloning and configuring the project?

@Isira-Seneviratne
Copy link
Contributor Author

Isira-Seneviratne commented Mar 15, 2024

Let's measure the size to be precise. Did you follow all documentation instructions about cloning and configuring the project?

Yes, I followed all the instructions under the Android section (I installed the NDK and CMake, and I'm using Android Studio).

Edit: I cloned again and ran the configuration script, but during the build it's still complaining about Boost being missing.

@biodranik
Copy link
Member

What build error do you see?

@Isira-Seneviratne
Copy link
Contributor Author

Isira-Seneviratne commented Mar 17, 2024

What build error do you see?

It's complaining about Boost being missing, but the repo was properly cloned with the third party dependencies.

@biodranik
Copy link
Member

It means that ./configure.sh wasn't run, or its run was unsuccessful for some reason (no available compiler in the system, etc.).

@Isira-Seneviratne
Copy link
Contributor Author

It means that ./configure.sh wasn't run, or its run was unsuccessful for some reason (no available compiler in the system, etc.).

I ran the script using WSL, and there didn't seem to be any issues (no error messages or anything like that).

@biodranik
Copy link
Member

Please paste 1) the script's output and 2) the build error output.

@Isira-Seneviratne
Copy link
Contributor Author

Isira-Seneviratne commented Mar 19, 2024

@biodranik

Configure script output (I already ran it when setting up the repo):
Configure.txt

Build errors:
Build error.txt

@biodranik
Copy link
Member

Try to move the organicmaps repository folder into another location, without spaces in the path (you have a parent Android Studio Project folder with spaces).

@Isira-Seneviratne
Copy link
Contributor Author

Try to move the organicmaps repository folder into another location, without spaces in the path (you have a parent Android Studio Project folder with spaces).

I tried removing the spaces, but it still gives the error messages about Boost not being available.

@biodranik
Copy link
Member

How do you run configure.sh? Does it create files in 3party/boost subdirectory accessible for Android Studio?

@Isira-Seneviratne
Copy link
Contributor Author

Isira-Seneviratne commented Mar 27, 2024

How do you run configure.sh? Does it create files in 3party/boost subdirectory accessible for Android Studio?

I run it through the WSL terminal. The created directories are visible in Android Studio.

image

@biodranik
Copy link
Member

The error message in your build log explicitly says that file does not exist:

fatal error: cannot open file 'C:/Users/isira/Documents/Android Studio Projects/organicmaps/3party/boost/boost/range.hpp'

Please check the existence of this file at this path. If configure was run properly, then this file (and others) should exist.

As boost uses symlinks, I assume that symlinks may not be enabled in your system, like it was mentioned in the documentation.

@Isira-Seneviratne
Copy link
Contributor Author

The error message in your build log explicitly says that file does not exist:

fatal error: cannot open file 'C:/Users/isira/Documents/Android Studio Projects/organicmaps/3party/boost/boost/range.hpp'

Please check the existence of this file at this path. If configure was run properly, then this file (and others) should exist.

As boost uses symlinks, I assume that symlinks may not be enabled in your system, like it was mentioned in the documentation.

I enabled symlinks in Git using the command mentioned in the documentation.

Also, the file is there:

image

@biodranik
Copy link
Member

Symlinks should be also enabled in the operating system.

Are you able to see that file's content when you click it in Android Studio?

@Isira-Seneviratne
Copy link
Contributor Author

Isira-Seneviratne commented Mar 28, 2024

Symlinks should be also enabled in the operating system.

Are you able to see that file's content when you click it in Android Studio?

Developer mode wasn't enabled. I enabled it, and now the build is working.

Edit - Here are the app sizes:

Before: 60426 KB
After: 60427 KB

mTts.speak(textToSpeak, TextToSpeech.QUEUE_ADD, mParams, textToSpeak);
final boolean isMusicActive = mAudioManager.isMusicActive();
AudioManagerCompat.requestAudioFocus(mAudioManager, mAudioFocusRequest);
final long delay = isMusicActive ? TTS_SPEAK_DELAY_MILLIS : 0;
Copy link
Member

@biodranik biodranik May 4, 2024

Choose a reason for hiding this comment

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

What does git blame/git log say about this delay when music is active? Why was it introduced in the first place? Is this delay really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That delay was introduced in this 2021 commit: 959dc08

@Jean-BaptisteC
Copy link
Member

@Isira-Seneviratne can you rebase your branch?

Signed-off-by: Isira Seneviratne <isirasen96@gmail.com>
@rtsisyk
Copy link
Contributor

rtsisyk commented May 26, 2024

Thank you for your patience. I don't have cycles to test this PR fully at the moment. Let's wait until other folks make the first round of testing.

@rtsisyk rtsisyk self-assigned this May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants