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: Use C++17, update to liboboe 1.6.1, improve oboe source handling #2472

Merged
merged 4 commits into from
Mar 12, 2022

Conversation

hoffie
Copy link
Member

@hoffie hoffie commented Mar 8, 2022

Short description of changes

  • Android: Build with C++17 support in preparation for oboe 1.6.1 update
    liboboe 1.6.1 (and maybe earlier versions) requires C++17 for std::timed_mutex

  • Android: Detect liboboe sources automatically
    Instead of hardcoding the list of relevant .cpp and .h files, use qmake's $$files to detect the sources. This makes liboboe updates easier and makes Jamulus.pro more readable.
    It has been confirmed that the hardcoded list of .cpp and .h files closely matches the list of available files. In other words, this commit does not change the content of the variables a lot, it just switches from hardcoding to dynamic detection. The new approach adds a single new .h file which was not listed previously:
    libs/oboe/src/flowgraph/resampler/KaiserWindow.h

  • Android: Update liboboe to 1.6.1
    This updates the included git submodule from a 01/2021 manual commit to the latest stable version.
    We should aim to use the latest supported version. There are also unresolved CodeQL warnings about our current liboboe tree, which might already be fixed in newer versions.

  • Jamulus.pro: Fix wrong indentation in Android block

CHANGELOG: Android: Improved compilation and updated liboboe to 1.6.1

Context: Fixes an issue?

  • liboboe was the source of some sporadic CodeQL failures.
  • Aim to use supported software

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Ready code-wise.

What is missing until this pull request can be merged?

  • Testing of the Android release artifact

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

hoffie added 4 commits March 8, 2022 12:22
liboboe 1.6.1 (and maybe earlier versions) requires C++17 for std::timed_mutex
Instead of hardcoding the list of relevant .cpp and .h files, use
qmake's $$files to detect the sources. This makes liboboe updates
easier and makes Jamulus.pro more readable.

It has been confirmed that the hardcoded list of .cpp and .h files
closely matches the list of available files. In other words, this commit does
not change the content of the variables a lot, it just switches from
hardcoding to dynamic detection.
The new approach adds a single new .h file which was not listed
previously:
libs/oboe/src/flowgraph/resampler/KaiserWindow.h
This updates the included git submodule from a 01/2021 manual commit to
the latest stable version.

We should aim to use the latest supported version. There are also
unresolved CodeQL warnings about our current liboboe tree.
@hoffie hoffie added this to the Release 3.9.0 milestone Mar 8, 2022
@ann0see
Copy link
Member

ann0see commented Mar 11, 2022

I can't test Android at the moment. @ngocdh @j-santander I think you know a bit more about Android?

@ngocdh
Copy link
Contributor

ngocdh commented Mar 11, 2022

Sorry I can't help here.

@hoffie
Copy link
Member Author

hoffie commented Mar 11, 2022

I've just tried it on my phone (Moto G4 Play -- rather old meanwhile). Jamulus starts, I can connect and audio is at least as "good", maybe even better than with the previous version. However, audio still sounds broken, but I suspect that's related to the phone performance and maybe the capturing buffer size (which I can't adjust due to UI problems).

@ann0see
Copy link
Member

ann0see commented Mar 12, 2022

You need to test audio over 4G/5G but probably it’s ok like that.

@pljones
Copy link
Collaborator

pljones commented Mar 12, 2022

I suspect that's related to the phone performance and maybe the capturing buffer size (which I can't adjust due to UI problems).

Which raises a good point - we need to stop concentrating on anything on the Android build (and iOS?) apart from fixing the UI. It borders on useless on my Moto G9 Power (which has a wide, short display so cuts off the bottom of every window).

@ann0see
Copy link
Member

ann0see commented Mar 12, 2022

iOS works fine for me over LTE.

@hoffie
Copy link
Member Author

hoffie commented Mar 12, 2022

I suspect that's related to the phone performance and maybe the capturing buffer size (which I can't adjust due to UI problems).

Which raises a good point - we need to stop concentrating on anything on the Android build (and iOS?) apart from fixing the UI. It borders on useless on my Moto G9 Power (which has a wide, short display so cuts off the bottom of every window).

I think that's tracked in #1529 (although I don't know about anyone working on it). I agree that it should be a priority for all Android-related things, but I don't think we should block PRs like this which are basic maintenance.

@pljones
Copy link
Collaborator

pljones commented Mar 12, 2022

but I don't think we should block PRs like this which are basic maintenance.

Not at all. But I'd rather anyone putting effort into Android was making it useable.

@softins
Copy link
Member

softins commented Mar 12, 2022

After doing git submodule update:

tony@ubuntu20:~/jamulus$ git submodule status
 855ea841a93bf304065e5152909983b1b85ffabb libs/oboe (1.5.1-109-g855ea84)

This suggests that the latest is 1.5.1, not 1.6.1 - was the latter a typo?

Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

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

I'm happy this PR does what it intends to. I can build on my Ubuntu 20 VM. I have compared the makefiles generated by qmake for this commit with those for the branch point, and verified that the $$files(... , true) calls find all the correct files. I have loaded the apk from this PR on my Android phone, and it runs equally well or badly as the previous version I had loaded. There are issues that need to be investigated and understood in the Android port, but they are independent of this PR.

@hoffie
Copy link
Member Author

hoffie commented Mar 12, 2022

After doing git submodule update:

tony@ubuntu20:~/jamulus$ git submodule status
 855ea841a93bf304065e5152909983b1b85ffabb libs/oboe (1.5.1-109-g855ea84)

This suggests that the latest is 1.5.1, not 1.6.1 - was the latter a typo?

Well, this is interesting. I was certainly planning to update to 1.6.1, which is the latest release. The release page (and 1.6.1 tag) point to the very commit you listed.
The output in brackets seems to match what git describe outputs. After some investigation, I believe it is related to this git-describe behavior:

By default (without --all or --tags) git describe only shows annotated tags. For more information about creating annotated tags see the -a and -s options to git-tag(1).

Tag objects (created with -a, -s, or -u) are called "annotated" tags; they contain a creation date, the tagger name and e-mail, a tagging message, and an optional GnuPG signature. Whereas a "lightweight" tag is simply a name for an object (usually a commit object).
Annotated tags are meant for release while lightweight tags are meant for private or temporary object labels. For this reason, some git commands for naming objects (like git describe) will ignore lightweight tags by default.

Based on the following output (I haven't found a better way to display this than `--verify`), 1.5.1 is such an annotated commit which is used by `git describe` by default, while 1.6.1 isn't:
libs/oboe$ git status
HEAD detached at 855ea84

libs/oboe$ git describe
1.5.1-109-g855ea84

libs/oboe$ git describe --tags
1.6.1

libs/oboe$ git tag --verify 1.6.1
error: 1.6.1: cannot verify a non-tag object of type commit.

libs/oboe$ git tag --verify 1.5.1
object 85f6061c46652897b99504732b664de2a84e9f2e
type commit
tag 1.5.1
tagger Don Turner <[email protected]> 1614091190 +0000

Oboe version 1.5.1
error: no signature found

So, in summary:

  • I believe PR title, commit message and actual used oboe version do match (= 1.6.1), so it is fine.
  • We might want to look into using annotated tags for our releases (but that's a totally different issue ;)).

@hoffie hoffie merged commit 6ce34af into jamulussoftware:master Mar 12, 2022
@hoffie hoffie deleted the update-liboboe-1.6.1 branch March 19, 2022 21:36
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.

5 participants