bugfix(audio): Reapply Bink movie volume on first frame - #3083
bugfix(audio): Reapply Bink movie volume on first frame#3083CryoTheRenegade wants to merge 10 commits into
Conversation
Fixes TheSuperHackers#2850. Movie audio ignored the Options volume sliders because BinkSetVolume was called once in createStream(), before Bink's audio output had started, so it never took effect. Extract the volume calc into calculateMovieAudioVolume() and reapply it on every decoded frame in frameDecompress() so it takes effect and tracks live slider changes.
Fix the floor comment (formula is unchanged from retail, floor is 327 not 1) and drop the live-slider-change claim from the per-frame comment; the Options menu isn't reachable during movie playback. The reason to reapply every frame is that the one-shot createStream() call ran before Bink's audio output started, so it never took effect.
xezon
added
Audio
Per review: don't reapply the volume every frame. The original defect was that BinkSetVolume ran in createStream(), before Bink's audio output existed, so it was discarded. Apply it once on the first BinkDoFrame(), the earliest point it can actually take effect. Drops the createStream() call and the per-frame poll in favor of a one-shot latch.
|
I don't think the title is correct, as the volume is not applied every frame, but only once. |
CryoTheRenegade
changed the title
- Return the 327 floor instead of 0 on the null-audio path so it does not trip Bink's 'play at full volume' quirk. - Trim the redundant formula comment. - Restore retail tab alignment on m_handle/m_memFile and align m_volumeSet.
fixed |
Per xezon's review. Replace the per-frame latch with a push model: - VideoPlayerInterface gains a setVolume(Real) virtual (no-op default in VideoPlayer); BinkVideoPlayer overrides it to push the volume to every open stream's Bink audio output. - MilesAudioManager::processPlayingList() pushes the speech volume to TheVideoPlayer whenever the volume changes, so movies that bypass the Miles mixer follow the sliders. - BinkVideoPlayer::update() applies the volume once on the first frame a stream exists, fixing the original too-early set in createStream() (Bink's audio output is not running yet at creation, so it was lost). - calculateMovieAudioVolume() now takes the speech volume as a parameter.
|
You are clearly using AI. While there is in principle nothing against it, I would strongly recommend you review the changes it makes - per our contribution guide. |
| TheAudio->getVolume(AudioAffect_Speech), mod, volume)); | ||
| BinkSetVolume( stream->m_handle,0, volume); | ||
| DEBUG_LOG(("BinkVideoPlayer::createStream() - set volume")); | ||
| BinkSetVolume( stream->m_handle, 0, calculateMovieAudioVolume(TheAudio->getVolume(AudioAffect_Speech)) ); |
There was a problem hiding this comment.
Was the first-frame apply meant to come out in the 08-16 push? 9130511 moved the one-shot into BinkVideoPlayer::update() and dropped this createStream() call because it runs before Bink's audio output exists. On the current head update() is back to just VideoPlayer::update();, and this is the only per-stream BinkSetVolume left.
The MilesAudioManager push handles slider changes during playback, but opening a movie with the slider already low goes back to full volume, which is the #2850 symptom.
There was a problem hiding this comment.
Restored the delayed one-shot in BinkVideoPlayer::update() and dropped the createStream() BinkSetVolume call. Miles still pushes slider changes; this path covers opening a movie with the slider already low. The latch resets when no stream is left so the next movie gets the current volume too.
There was a problem hiding this comment.
Thanks, I see the push. One thing - the normal playMovie() replacement paths close the old stream and open the new one synchronously, so BinkVideoPlayer::update() never observes firstStream() == nullptr. m_volumeApplied remains TRUE, and the replacement movie can miss the delayed volume set. Could we reset when createStream() adds a stream instead?
There was a problem hiding this comment.
Good catch. playMovie() close+open never lets update() see an empty stream list, so the latch stayed set. createStream() now clears it when a stream is added; update() still applies once the new stream is live.
|
|
||
| virtual void notifyVideoPlayerOfNewProvider( Bool nowHasValid ) override { } | ||
|
|
||
| virtual void setVolume( Real volume ) override { } |
There was a problem hiding this comment.
FFmpegVideoPlayer derives from VideoPlayer, so it picks up this no-op and the volume plumbing is Bink-only. Override it there too?
There was a problem hiding this comment.
FFmpegVideoPlayer now overrides setVolume and pushes the speech volume to the OpenAL movie stream. Initial volume is also applied when the stream starts, since that backend is already live by then.
There was a problem hiding this comment.
Wait do we have OpenAL in upstream yet? This might not work until we get that landed :(
There was a problem hiding this comment.
Yeah, OpenAL is not in this repo yet. The FFmpeg override is behind RTS_USE_OPENAL so Miles/Bink builds keep a no-op, and the OpenAL gain path is just plumbing until that backend lands.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Fixes #2850.
Movie audio (Bink) bypasses the Miles mixer and plays straight through DirectSound via
BinkSoundUseDirectSound, so the only volume control isBinkSetVolume(). It was called exactly once inBinkVideoPlayer::createStream(), before Bink's audio output had actually started, so it never took effect. Movies played at full volume regardless of the Options sliders.The fix extracts the volume calc into
calculateMovieAudioVolume()and reapplies it on every decoded frame inBinkVideoStream::frameDecompress(), so the setting takes effect once audio is running and live slider changes during playback are picked up.The volume formula itself is unchanged from retail: floor of 327 out of 32768, never literal 0, because Bink interprets 0 as "play at full volume". That quirk is why the original defensive comment exists; the floor isn't the bug.