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

[Feature] Allow specifying the alarm volume using steps/stops #282

Merged
merged 11 commits into from
Dec 10, 2024

Conversation

orkun1675
Copy link
Collaborator

@orkun1675 orkun1675 commented Nov 11, 2024

(updated version of #276)

This PR allows alarm users to speicfy a staircase function to control the alarm volume.

It also fixes #267 by adding this line. For some reason AVAudioPlayer ignores volume changes unless the initial volume is set to zero.

PS: Overall, AVAudioPlayer has been a pain to deal with, setVolume has bugs, and it also doesn't report the real .volume value while a fade is in progress.

I've also taken the liberty to fix lint warnings in both iOS and Android native code.

I've tested using iOS and Android emulators + iOS physical device. I would appreciate if you could test this as well before merging. Thanks!

@gdelataillade
Copy link
Owner

Hi @orkun1675

Thanks for the PR ! Sure, I will test this on my devices. Are we doing the pigeon migration in this PR ?

@gdelataillade gdelataillade added the testing The PR is being tested label Nov 12, 2024
@orkun1675
Copy link
Collaborator Author

This PR is a clone of the previous one (when I moved my changes from main to a new branch GitHub closed to previous PR).

PS: Today I started the Pigeon migration, Dart + Kotlin refactorings are complete, Swift is pending.

@orkun1675
Copy link
Collaborator Author

@gdelataillade this PR is now rebased to main and updated to utilize Pigeon and a more friendly public API. PTAL.

@orkun1675
Copy link
Collaborator Author

@gdelataillade can we merge this so I can rebase and work on my follow-up PRs?

We don't need to release a new version just yet if you'd like to work on the backwards compatibility aspect.

@gdelataillade gdelataillade merged commit 5f6e53d into gdelataillade:main Dec 10, 2024
1 check passed
@gdelataillade
Copy link
Owner

@orkun1675 Yes, I intend to ensure backward compatibility before releasing version 5.0.0. However, I’m uncertain when I’ll have the time to complete this.

In the meantime, I can provide a development release, version 5.0.0-dev.1, if you’d like to incorporate it into your project.

@orkun1675 orkun1675 deleted the step-volume branch December 10, 2024 19:45
@onegigbyte
Copy link

onegigbyte commented Dec 17, 2024

Volume fade still broken in iOS (v5.0.0-dev.1)

Hey, I just wanted to report that the iOS volume fade issue from #267 is still happening in 5.0.0-dev.1. I tried setting a 30-second fade, but the volume still jumps to max in like 2-3 seconds on device (iphone 16 pro).

What I tried

I tested with the latest dev version (5.0.0-dev.1) using both the regular fade and staircase fade:

final VolumeSettings volumeSettings = VolumeSettings.fade(
    volume: 1.0,
    fadeDuration: Duration(seconds: 100),
);

What's happening

When I run this, I see the log message:
"[SwiftAlarmPlugin] Fading volume to 1.0 over 99.95 seconds."

But in reality, the volume maxes out after just 2-3 seconds.

I think I found the problem

Looking at the code in ios/Classes/api/AlarmApiImpl.swift (L397-403):

DispatchQueue.main.asyncAfter(deadline: now + startTime) {
    if !audioPlayer.isPlaying {
        return
    }
    print("[SwiftAlarmPlugin] Fading volume to \(targetVolume) over \(fadeDuration) seconds.")
    audioPlayer.setVolume(targetVolume, fadeDuration: fadeDuration)
}

I think there's a timing issue here.

  • The code is adding seconds directly to DispatchTime.now(),
  • but DispatchTime actually works in nanoseconds.
    This is likely to explain why everything's happening way faster than it should.

Suggested fix

We probably need to convert the seconds to nanoseconds before adding them to the DispatchTime:

let delayInNanoseconds = UInt64(startTime * Double(NSEC_PER_SEC))
DispatchQueue.main.asyncAfter(deadline: now + .nanoseconds(Int(delayInNanoseconds)))

@orkun1675
Copy link
Collaborator Author

In that function the startTime is of type Double, representing seconds.

DispatchTime has nanosecond precision but I can't tell from the documentation what time unit the Double addition is assuming.

You might be right about this, would you be interested in created a PR to fix this and test it?

Simpler approach would be:

DispatchQueue.main.asyncAfter(deadline: now + .seconds(startTime))

@gdelataillade
Copy link
Owner

@onegigbyte a fix for your issue was published in 5.0.0-dev.5.
Thanks @orkun1675 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing The PR is being tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS: Alarm volume does not fade
3 participants