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.

Sign up for GitHub

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

Jump to bottom

improve voice message quality #2714

Merged
merged 1 commit into from
Jan 25, 2023
Merged

improve voice message quality #2714

merged 1 commit into from
Jan 25, 2023

Conversation

mahibi
Copy link
Collaborator

@mahibi mahibi commented Jan 23, 2023 •
edited
Loading

resolve #1613

Signed-off-by: Marcel Hibbe dev@mhibbe.de

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔖 Capability is checked or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

@mahibi mahibi added this to the 16.0.0 milestone Jan 23, 2023
@mahibi
Copy link
Collaborator Author

mahibi commented Jan 23, 2023

/backport to stable-15.1

@mahibi mahibi force-pushed the bugfix/1613/improveVoiceMessageQuality branch from 16af5c1 to 0e87387 Compare January 23, 2023 12:19
@mahibi mahibi mentioned this pull request Jan 23, 2023
Closed
@mahibi mahibi self-assigned this Jan 23, 2023
@mahibi mahibi added the 3. to review Waiting for reviews label Jan 23, 2023
@nickvergessen
Copy link
Member

Can you send one with this to Ivan, so he can test playing it in Safari and the iOS app?
I remember issues back then

@mahibi
Copy link
Collaborator Author

mahibi commented Jan 23, 2023

Can you send one with this to Ivan, so he can test playing it in Safari and the iOS app? I remember issues back then

@Ivansss i sent you a voice message on sermo. could you test and let me know if this works/ how the quality is?

@Ivansss
Copy link
Member

Ivansss commented Jan 23, 2023

Sounds good on the iOS app 👍

But I get an error in Safari 😕
Screenshot 2023-01-23 at 18 00 38

@mahibi
Copy link
Collaborator Author

mahibi commented Jan 24, 2023

Sounds good on the iOS app +1

But I get an error in Safari

so we tested and somehow sampling rate 22050 recorded on android works on safari (But it's strange that 44100 won't, because that the value that the iOS app uses and safari can handle these recordings from iOS).

@mahibi mahibi force-pushed the bugfix/1613/improveVoiceMessageQuality branch from bb7c998 to b3dc866 Compare January 25, 2023 10:21
@mahibi mahibi requested a review from Ivansss January 25, 2023 10:22
Ivansss
Ivansss approved these changes Jan 25, 2023
View reviewed changes
Copy link
Member

@Ivansss Ivansss left a comment

Choose a reason for hiding this comment

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

Worked in our tests on iOS app and Safari 👍

@github-actions
Copy link

Codacy

Lint

TypemasterPR
Warnings111111
Errors00

SpotBugs

CategoryBaseNew
Correctness1212
Dodgy code174174
Internationalization55
Malicious code vulnerability33
Performance1010
Security22
Total206206

mahibi added a commit that referenced this pull request Jan 25, 2023
@mahibi
improve voice message quality
5ebb7e4 
first it was tried with sampling rate 44100 but this showed an error on safari when try to playback.

by direct comparison to 44100, 22050 has lower quality but it's totally okay compared to the quality before when no sampling rate was set. I'm not sure why it failed on safari, because the iOS app also uses 44100.

see #2714

Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
@mahibi mahibi force-pushed the bugfix/1613/improveVoiceMessageQuality branch from b3dc866 to 5ebb7e4 Compare January 25, 2023 10:39
@mahibi
improve voice message quality
86290d8 
first it was tried with sampling rate 44100 but this showed an error on safari when try to playback.

by direct comparison to 44100, 22050 has lower quality but it's totally okay compared to the quality before when no sampling rate was set. I'm not sure why it failed on safari, because the iOS app also uses 44100.

see #2714

Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
@mahibi mahibi force-pushed the bugfix/1613/improveVoiceMessageQuality branch from 5ebb7e4 to 86290d8 Compare January 25, 2023 10:43
@mahibi mahibi merged commit b46c96a into master Jan 25, 2023
@delete-merged-branch delete-merged-branch bot deleted the bugfix/1613/improveVoiceMessageQuality branch January 25, 2023 10:44
backportbot-nextcloud bot pushed a commit that referenced this pull request Jan 25, 2023
@mahibi @backportbot-nextcloud
improve voice message quality
fe3aa9c 
first it was tried with sampling rate 44100 but this showed an error on safari when try to playback.

by direct comparison to 44100, 22050 has lower quality but it's totally okay compared to the quality before when no sampling rate was set. I'm not sure why it failed on safari, because the iOS app also uses 44100.

see #2714

Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 25, 2023
Merged
@nickvergessen
Copy link
Member

👍🏼

@github-actions
Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/2714-talk.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

@mahibi
Copy link
Collaborator Author

mahibi commented Jan 25, 2023 •
edited
Loading

confirmed it also works on Edge browser...

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

@Ivansss Ivansss Ivansss approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees

@mahibi mahibi

Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
16.0.0
Development

Successfully merging this pull request may close these issues.

Poor audio quality of voice messages
3 participants
@mahibi @nickvergessen @Ivansss