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

[DLNA] [NextTrack] [Gapless] Fix 5644 and 5294 #5659

Merged
merged 5 commits into from May 24, 2021

Conversation

ssenart
Copy link
Contributor

@ssenart ssenart commented Mar 30, 2021

[DLNA] [NextTrack] [Gapless] Fix 5644 and 5294

Fix 5644 : [DLNA] [Music] Next track command from any DLNA device does not do anything
Fix 5294 : gaps between tracks ...

Changes

Implement the new DLNA message SetNextAVTransport() that permits to the renderer to prefetch the next track to play in the list.
It will have an effect if the renderer implement that feature :

  • On my Sonos, it works perfectly.
  • On my Marantz, I have to select 'replay once' option for it to work => strange.
  • On JRiver Player, it works perfectly.
  • On BubbleUPnP Android Player, it does not work at all : Possibly SetNextAVTransport() feature is not supported.

The other positive effect is that it removes the gap between tracks.

Feedbacks on other renderers are welcome !

Issues

Fixes #5644
Fixes #5294

@BaronGreenback
Copy link
Contributor

Note to whomever: SetNextAVTransport isn't supported by all devices - so when mentioning gapless playback in any documentation should be qualified with "if supported".

@BaronGreenback
Copy link
Contributor

Not particular to this change, and would welcome comment as I may be over cautious - but shouldn't all playlist array lookups be behind some form of locks.

Only say this as it would be possible for the dlna devices trigger events which effect the list at the same time.
eg. OnDevicePlaybackStopped clears the playList - so theoretically cause an index out of bounds.

@BaronGreenback
Copy link
Contributor

Nice bit of coding. However, I suspect this will fix #5664 only under certain conditions; namely the device supporting this command, and the command having enough time to be transmitted and processed by the device. I suspect clicking next/next/next/next will cause some issues on a slow server, and #5564 will return.

Hadn't seen #5664, but I suspect the underlying issue is with the next/last/random functionality in the SendPlayCommand method. The version in the playTo plugin (https://github.com/BaronGreenback/Jellyfin-DLNA-PlayTo/tree/master/Jellyfin.Plugin.DlnaPlayTo/Main) is a modified verson which seems to work for me.

@ssenart
Copy link
Contributor Author

ssenart commented Mar 30, 2021

Not particular to this change, and would welcome comment as I may be over cautious - but shouldn't all playlist array lookups be behind some form of locks.

Only say this as it would be possible for the dlna devices trigger events which effect the list at the same time.
eg. OnDevicePlaybackStopped clears the playList - so theoretically cause an index out of bounds.

Since all is coroutine (async/await), should not all this code be executed by the same single thread ?

@BaronGreenback
Copy link
Contributor

Not particular to this change, and would welcome comment as I may be over cautious - but shouldn't all playlist array lookups be behind some form of locks.
Only say this as it would be possible for the dlna devices trigger events which effect the list at the same time.
eg. OnDevicePlaybackStopped clears the playList - so theoretically cause an index out of bounds.

Since all is coroutine (async/await), should not all this code be executed by the same single thread ?

Again - confusing with the callbacks on the next version.

@ssenart
Copy link
Contributor Author

ssenart commented Mar 30, 2021

I agree. SetNextAVTransport() support is mandatory on Device side. Otherwise, it behaves as before without the fix.

You're right, clicking next/next/next/next without waiting few seconds between may do nothing. The explanation is still with this Timer that resynchronize with the Renderer every 10 seconds (hardcoded). The click on next button is taken in account just at resynchronization time and a new SetNextAVTransport() message is then sent.

Emby.Dlna/PlayTo/Device.cs Outdated Show resolved Hide resolved
Emby.Dlna/PlayTo/Device.cs Outdated Show resolved Hide resolved
Co-authored-by: Cody Robibero <cody@robibe.ro>
Emby.Dlna/PlayTo/Device.cs Outdated Show resolved Hide resolved
@cvium cvium merged commit 75704ef into jellyfin:master May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DLNA] [Music] Next track command from any DLNA device does not do anything gaps between tracks ...
4 participants