Don't change the receiver's volume when audio is disabled - #33
Open
josiahbryan wants to merge 1 commit into
Open
Don't change the receiver's volume when audio is disabled#33josiahbryan wants to merge 1 commit into
josiahbryan wants to merge 1 commit into
Conversation
`volume: 0.000000` is 0 dB, which in AirPlay is full scale -- maximum, not
silence. Two paths sent it for sessions that carry no audio, so `-no-audio`
discarded whatever volume the user had set on the receiver.
setupMirrorSession sent it unconditionally (twice) during setup, so every
connect reset the receiver to maximum. On a TV, reconnecting a dashboard or a
mirrored window slammed the set to full volume each time.
SetAudioMuted had the same effect one click away. The daemon deliberately
routes mute/unmute to the receiver when audio is disabled (daemon.go's
`d.cfg.NoAudio || t.session.HasAudio()`), and unmuting sends audioVolumeBody
(false) -- the identical full-scale value. A no-audio session still negotiates
an audio stream, so HasAudio() reports true and the plasmoid offers a mute
toggle for a video-only session; the first press silenced the receiver and the
next set it to maximum.
Guard both. A session that transmits no audio has no use for the receiver's
audio state and should leave its volume exactly as it found it. Skipping is
preferable to sending the muted value, since -144 dB would be equally
destructive of the user's setting, just in the other direction.
The SetAudioMuted guard reads MirrorSession.noAudio, which until now was
assigned and never read. HasAudio() cannot substitute for it: it is true in
both modes, which is what made this reachable.
Echoing the receiver's own reported volume back instead was considered and
rejected. On the Roku Streambar Pro tested here, `initialVolume` from /info
stayed 0.0 across ten VolumeDown and six VolumeUp presses, so it does not
track that receiver's current volume and sending it back would still command
full scale. Other receivers may report it faithfully; this was not verified
beyond the one device.
TESTS
TestSetupMirrorNoAudioStillNegotiatesAudioSession asserted the two volume
SET_PARAMETERs as part of the expected RTSP sequence for a no-audio session,
and was the only test covering that sequence -- so updating it alone would
have left no coverage that audio sessions still set the volume. The shared
helper is parameterized over noAudio and there are now two named tests, one
per direction, plus TestSetAudioMutedRefusesWhenSessionHasNoAudio.
Each test was checked against a mutation rather than merely observed green:
reverting the setup fix fails the no-audio test, making the skip unconditional
fails the with-audio test, inverting the condition fails both, and removing
the SetAudioMuted guard fails the mute test.
Verified on a Roku Streambar Pro (model 9101R2):
-no-audio -> "[SETUP] no-audio session: skipping SET_PARAMETER volume"
(sent=0, skipped=1)
with audio -> "[SETUP] SET_PARAMETER volume=0 sent" (sent=1, skipped=0)
gofmt clean, go vet clean, go test ./... passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
-no-audiosets the receiver's volume to maximum on every connect.volume: 0.000000is 0 dB, which is full scale.setupMirrorSessionsends it unconditionally, twice, so a session that streams no audio at all still commands the receiver to maximum and discards whatever the user had set. Reconnecting a dashboard or a mirrored window resets a TV's volume to 100 each time.Not the same as #11
I want to be clear about this up front, because #11 set
volume: 20.000000and5f14b6freverted it — correctly. Positive dB is out of range and receivers may read it as zero gain, so picking a quieter number is not a fix.This PR does not pick a number. It declines to send a volume at all when the session carries no audio. When audio is enabled, the 0 dB behaviour from
5f14b6fis untouched.Also: the daemon's mute control
SetAudioMutedhas the same effect one click away, and it is reachable in exactly the case this PR is about.daemon.godeliberately routes mute/unmute to the receiver when audio is disabled:and unmuting sends
audioVolumeBody(false)— the identical full-scale value. A no-audio session still negotiates an audio stream, soHasAudio()is true and the plasmoid offers a mute toggle for a video-only session: first press silences the receiver, next press sets it to maximum.So
HasAudio()cannot distinguish the two modes. The guard readsMirrorSession.noAudio, which until now was assigned and never read.Why skipping is safe
A session that transmits no audio has no use for the receiver's audio state. The request also sits after
RECORD, so it is not part of the handshake, and this repo's own receiver treatsSET_PARAMETERas a bare OK with no state transition.I considered echoing the receiver's own reported volume back instead. On the Roku Streambar Pro I tested,
initialVolumefrom/infostayed at0.0across ten VolumeDown and six VolumeUp presses, so it does not track that receiver's current volume and sending it back would still command full scale. Other receivers may report it faithfully — I only have the one device, so I did not build on it.Tests
TestSetupMirrorNoAudioStillNegotiatesAudioSessionasserted the two volumeSET_PARAMETERs as part of the expected RTSP sequence for a no-audio session, and it was the only test covering that sequence — so updating it alone would have left nothing asserting that audio sessions still set the volume.The shared helper is now parameterized over
noAudio, with a named test per direction, plus one for the mute guard.Each was checked against a mutation rather than just observed green:
SetAudioMutedguard → the mute test failsWorth noting for anyone touching that harness: what actually pins the
volume: 0.000000payload is the fake receiver's body check, not the method sequence, and a mismatch there surfaces as a 5-second/feedbacktimeout rather than a useful message.gofmt,go vetandgo test ./...are clean, verified on a clean checkout of the commit rather than a working tree.Verified on hardware
Roku Streambar Pro, model 9101R2:
Volume left untouched in the first case, unchanged behaviour in the second.
Scope
I deliberately did not touch the plasmoid, which still shows a mute toggle for video-only sessions — it now fails with a clear error instead of changing the volume. Hiding the control would mean changing what
has_audioreports, which felt like a bigger decision than this fix. Happy to follow up if you'd prefer it gone.