Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #980 +/- ##
==========================================
- Coverage 21.87% 21.84% -0.03%
==========================================
Files 63 63
Lines 8686 8696 +10
Branches 1411 1414 +3
==========================================
Hits 1900 1900
- Misses 6767 6777 +10
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fezster
force-pushed
the
fix-default-audiostream-selection
branch
from
a96157b to
db0e4e9
Compare
|
oddstr13
added
the
needs-testing
|
@coderabbitai review |
WalkthroughThe changes adjust how the "AudioStreamIndex" is handled in both playback initialization and reporting. The index is now decremented by 1 when set during playback setup and incremented by 1 when reported back to the server, ensuring consistent mapping between internal and server-side representations. No public interfaces were changed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PlayUtils
participant Player
participant JellyfinServer
User->>PlayUtils: Start playback
PlayUtils->>PlayUtils: Set AudioStreamIndex = DefaultAudioStreamIndex - 1
PlayUtils->>Player: Pass playback info (AudioStreamIndex)
Player->>JellyfinServer: Report playback (AudioStreamIndex + 1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jellyfin_kodi/helper/playutils.py(1 hunks)jellyfin_kodi/player.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test Jellyfin-Kodi
jellyfin_kodi/helper/playutils.py
[error] 211-217: Prettier formatting check failed. File would be reformatted by Black. Run 'black --write' to fix code style issues.
🔇 Additional comments (1)
jellyfin_kodi/player.py (1)
385-385: LGTM: Correct indexing conversion for server communication.The change properly converts from internal 0-based indexing to the server's expected 1-based indexing when reporting playback progress. This ensures the Jellyfin server receives the correct AudioStreamIndex value.
jellyfin_kodi/helper/playutils.py
Outdated
| self.info["AudioStreamIndex"] = self.info.get("AudioStreamIndex") or source.get( | ||
| "DefaultAudioStreamIndex" | ||
| ) | ||
| self.info["AudioStreamIndex"] = self.info.get("AudioStreamIndex") or (source.get("DefaultAudioStreamIndex") - 1) |
There was a problem hiding this comment.
Fix potential TypeError and address formatting issues.
This change has two issues that need to be addressed:
- Critical Issue: If
source.get("DefaultAudioStreamIndex")returnsNone, the subtraction will raise aTypeError. - Formatting Issue: The pipeline indicates this line needs Black formatting.
Apply this diff to fix both issues:
- self.info["AudioStreamIndex"] = self.info.get("AudioStreamIndex") or (source.get("DefaultAudioStreamIndex") - 1)
+ self.info["AudioStreamIndex"] = self.info.get("AudioStreamIndex") or (
+ source.get("DefaultAudioStreamIndex", 0) - 1
+ )This ensures:
- A default value of 0 if
DefaultAudioStreamIndexis missing - Proper code formatting to pass Black style checks
- Safe conversion from server's 1-based to internal 0-based indexing
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.info["AudioStreamIndex"] = self.info.get("AudioStreamIndex") or (source.get("DefaultAudioStreamIndex") - 1) | |
| self.info["AudioStreamIndex"] = self.info.get("AudioStreamIndex") or ( | |
| source.get("DefaultAudioStreamIndex", 0) - 1 | |
| ) |
🤖 Prompt for AI Agents
In jellyfin_kodi/helper/playutils.py at line 216, the current code risks a
TypeError if source.get("DefaultAudioStreamIndex") returns None because it
attempts to subtract 1 from None. To fix this, provide a default value of 1 when
retrieving "DefaultAudioStreamIndex" to ensure subtraction is safe, then
subtract 1 to convert from 1-based to 0-based indexing. Also, reformat the line
according to Black style guidelines to resolve formatting issues.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Don't mind the bot too much, mainly testing it to see if it proves to be a good tool with aiding in PR reviews.
Fix default audio stream selection
fezster
force-pushed
the
fix-default-audiostream-selection
branch
from
db0e4e9 to
affda25
Compare
|
|
@coderabbitai Can you review this PR and the related issue description (in linked forum post) in light of what has been uncovered for subtitles with PR #996 ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you analyze this PR in the context of PR #996's subtitle findings. Let me first gather information about PR #996 and trace the relevant code paths. 🏁 Script executed: Length of output: 17528 🏁 Script executed: Length of output: 411 🏁 Script executed: Length of output: 24389 🏁 Script executed: Length of output: 15083 🏁 Script executed: Length of output: 59 🏁 Script executed: Length of output: 56 🏁 Script executed: Length of output: 11016 🏁 Script executed: Length of output: 14924 @oddstr13, I've thoroughly analyzed both PRs and traced the complete code paths. Here's my assessment of whether the same underlying issue from PR #996 could be relevant to PR #980: Analysis SummaryPR #996 Discovery: Fixed subtitle stream indexing where the code was incorrectly using array positions instead of actual PR #980 Issue: Addresses a race condition on Fire TV devices where Key Differences in Stream Index HandlingAudio Streams (PR #980): # In playutils.py - subtracts 1 from server default
self.info["AudioStreamIndex"] = source.get("DefaultAudioStreamIndex", 0) - 1
# In player.py - adds 1 back when reporting to server
"AudioStreamIndex": (item["AudioStreamIndex"] + 1)Subtitle Streams (no adjustment): # In playutils.py - uses server default directly
self.info["SubtitleStreamIndex"] = source.get("DefaultSubtitleStreamIndex")Assessment of RelationshipDifferent Root Causes:
Potential Shared Vulnerability: Code Path Analysisflowchart TB
A[Playback Start] --> B{Platform Type}
B -->|Normal| C[report_playback]
B -->|Fire TV| D[set_audio_subs called early]
C --> E[detect_audio_subs]
E --> F[Audio index + 1]
F --> G[set_audio_subs]
G --> H[Kodi setAudioStream with index - 1]
D --> I[Uses DefaultAudioStreamIndex directly]
I --> J[Wrong track selected]
K[PR #980 Fix] --> L[DefaultAudioStreamIndex - 1]
L --> M[Later +1 when reporting back]
ConclusionWhile both PRs address stream indexing issues, they target different specific problems:
The underlying complexity is similar (stream indexing is tricky), but PR #980's approach with -1/+1 adjustments suggests the audio path already had different handling than subtitles. The fix appears targeted and appropriate for the specific Fire TV race condition rather than a broader indexing mismatch like PR #996 addressed. Recommendation: The fix looks sound for the specific issue described. However, it would be worth verifying that the same array-vs-index issues from PR #996 don't affect other audio stream handling paths beyond this race condition scenario. 🏁 Script executed: Length of output: 2906 Perfect! Now I have the complete context from the forum post. This provides crucial details about the specific race condition issue. Let me update my analysis: Updated Analysis with Forum ContextThe forum post provides critical details that clarify the relationship between these PRs: The Specific Race Condition (PR #980)Normal Sequence (works correctly):
Fire TV Sequence (broken):
Key Insight from Forum AnalysisThe user's logs show:
This confirms it's a timing issue where the wrong default is applied before the correct logic can run. Relationship to PR #996's Indexing FixDifferent but Related Issues:
Shared Indexing Complexity: Both PRs deal with the fundamental challenge that Jellyfin stream indexes ≠ Kodi internal indexes Code Path VerificationLooking at the audio stream handling in # This code already uses actual stream["Index"] values (like PR #996's fix)
for stream in streams:
index = stream["Index"] # ✅ Uses actual index, not array position
if stream_type == "Audio":
audio_streams.append(index)Good news: The audio stream selection code already uses proper Assessment of PR #980's FixThe # playutils.py: Prepare for Kodi (0-based)
source.get("DefaultAudioStreamIndex", 0) - 1
# player.py: Report back to Jellyfin (1-based)
item["AudioStreamIndex"] + 1This ensures consistent offset handling regardless of whether Final ConclusionThe PRs address related but distinct issues:
No additional vulnerability: The audio code already uses proper PR #980's fix is sound: The offset adjustment resolves the Fire TV race condition by ensuring consistent index handling regardless of call sequence. The underlying stream indexing complexity is shared between both issues, but PR #980's targeted fix appropriately addresses the specific race condition without introducing the broader indexing problems that PR #996 solved for subtitles. |
|
What is the requested change by the bot? I read through it's report and it just seems to say that the pull request is valid, it doesn't seem to request any change, but Github is saying a change was requested. Have I misunderstood something or is the bot acting up? There are earlier replies that note potential issues, but the latest May 27th reply suggests that the PR is valid so I'm not following what the requested change is. Asking because it seems like it's been a while without any activity and I'm running into an issue that I think may be related. (not on a Fire TV device, LibreElec on Pi4, but I'm getting similarly inconsistent audio streams with no obvious pattern, and this being a race condition with that exact effect seems like it'd check out as the root cause.) |
|
I'm not familiar enough with these parts to properly review it, and my spider senses are tingling saying there is something strange going on somewhere here. I have not dedicated the required hours to get to the bottom of what's going on here, mainly because I haven't personally noticed any issues with it. Feel free to apply these changes on top of latest release/master, and report back your findings. |
3 participants
Discussion thread here: https://forum.jellyfin.org/t-wrong-audio-track-selected-by-default-jellyfin-for-kodi?pid=52729#pid52729
The current implementation does not work on Fire TV devices (Firestick 4k Max, FireCube V3). The issue seems to stem from a race condition in player.py.
If report_playback() -> detect_audio_subs() is called first, the current audio track is defaulted and this is then stored. Later, set_audio_subs() is called and this same audio track is applied. This is what should happen and, I guess, is expected to happen.
On the FireStick, though, this never happens. The set_audio_subs() is called first - which means the DefaultAudioStreamIndex returned by Jellyfin is used. The logic is then not applied correctly and the wrong audio track is applied. Later, report_playback is called, but by then the audio track has already been set incorrectly and so it never corrects itself.
By applying the correct logic to DefaultAudioStreamIndex, this then works as well.
The correct logic, I believe, should be to subtract 1 from the index returned by Jellyfin.
Summary by CodeRabbit