Implement AampRialtoPlayer::SetAudioVolume via IMediaPipeline - #1838
Merged
Conversation
Store the requested volume (0-100) and forward it to Rialto's IMediaPipeline::setVolume()/setMute(), mirroring the GStreamer/ InterfacePlayerRDK path: volume 0 mutes the audio source without touching setVolume(); any other value unmutes (if attached) and forwards volume/100.0 to setVolume(). Re-apply the cached volume when a pipeline is (re)created in Configure() and when the audio source newly attaches in AttachSource(), since a fresh pipeline defaults to full volume and setMute() requires a valid Rialto sourceId that is only available after attachment. Add L1 tests covering: no-pipeline safety, non-zero volume forwarding, mute-only behavior when muted, unmute+volume after un-muting, deferred mute application on delayed audio attach, and volume re-application across pipeline recreation (retune).
Contributor
There was a problem hiding this comment.
Pull request overview
Implements AampRialtoPlayer::SetAudioVolume() for the direct-Rialto sink by caching the requested volume and applying it to firebolt::rialto::IMediaPipeline (including re-applying on pipeline recreation and on delayed audio source attachment). Adds L1 coverage in AampRialtoPlayerTests for the new volume/mute behaviors.
Changes:
- Add cached audio volume state and
applyAudioVolume()helper to translate 0–100 volume into RialtosetMute()/setVolume()calls. - Re-apply cached audio volume when a pipeline is created in
Configure()and when the audio source attaches inAttachSource(). - Add L1 tests covering no-pipeline safety, mute-only behavior for volume 0, unmute+volume after muting, deferred mute-on-attach, and retune re-application.
Scores: Completeness 9/10, Assumptions 8/10, Clarity 9/10, CostRisk 3/10 | Critique: Clear intent and well-scoped diffs/tests; only minor implicit assumptions about Rialto API failure handling. | Improve: Add explicit acceptance criteria for how out-of-range volume values (e.g., >100) should be handled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
direct-rialto/AampRialtoPlayer.cpp |
Implements cached volume handling and applies it on pipeline creation and audio attach. |
direct-rialto/AampRialtoPlayer.h |
Adds m_audioVolume and declares applyAudioVolume() helper. |
test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp |
Adds L1 tests validating the new SetAudioVolume behavior across key scenarios. |
Rialto has no dedicated video-mute API: the rialto-gstreamer video sink's "show-video-window" property (PullModeVideoPlaybackDelegate) maps directly to IMediaPipeline::setMute(videoSourceId, !show), the same per-source API already used here for subtitle mute and audio-mute-at-zero-volume. Cache the requested state in m_videoMuted and apply it via m_pipeline->setMute() when the video source is attached, or defer and re-apply from AttachSource() once the video source attaches - mirroring the existing SetSubtitleMute() pattern, since setMute() requires a valid Rialto sourceId. Add L1 tests covering: mute/unmute on an attached video source, deferred mute application on delayed video attach, and no-pipeline safety.
…e::getStats The GStreamer reference (InterfacePlayerRDK::GetVideoPlaybackQuality) reads a "stats" GObject property off video_sink/video_dec, gated on the pipeline being in PLAYING or PAUSED state. Rialto's rialto-gstreamer video sink builds that same "stats" property from IMediaPipeline::getStats(sourceId, renderedFrames, droppedFrames), so call it directly here instead. This is a read-through getter, not a cached setter: query live each call, gated on PlayerStateId::PLAYING/PAUSED and the video source being attached. Add m_playbackQuality as backing storage for the returned pointer (the StreamSink API returns PlaybackQualityStruct*, so a stack local would dangle) - mirroring gstPrivateContext->playbackQuality in the GStreamer reference, not a value that needs re-applying anywhere. Add L1 tests covering: PLAYING/PAUSED success, state-gating when not playing/paused, video source not yet attached, getStats() failure, and no-pipeline safety.
Rialto has no dedicated PTS API, so derive the value from IMediaPipeline::getPosition(), converting nanoseconds to 90kHz PTS ticks using named constants instead of magic numbers. Returns 0 with a warning log if the pipeline is missing or the position query fails. Add L1 tests covering successful conversion, pipeline query failure, and the no-pipeline case.
Rialto has no video-size query API. Mirror InterfacePlayerRDK's approach by parsing the "x,y,w,h" string already stored by SetVideoRectangle() to derive width/height, leaving the caller's values unchanged when no rectangle has been set yet. Add L1 tests covering the parsed and unset-rectangle cases.
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.
Store the requested volume (0-100) and forward it to Rialto's IMediaPipeline::setVolume()/setMute(), mirroring the GStreamer/ InterfacePlayerRDK path: volume 0 mutes the audio source without touching setVolume(); any other value unmutes (if attached) and forwards volume/100.0 to setVolume().
Re-apply the cached volume when a pipeline is (re)created in Configure() and when the audio source newly attaches in AttachSource(), since a fresh pipeline defaults to full volume and setMute() requires a valid Rialto sourceId that is only available after attachment.
Add L1 tests covering: no-pipeline safety, non-zero volume forwarding, mute-only behavior when muted, unmute+volume after un-muting, deferred mute application on delayed audio attach, and volume re-application across pipeline recreation (retune).