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

Fix PlayMedia builtin for smart playlists and playlists - Backport #16352

Merged
merged 1 commit into from Jul 29, 2019

Conversation

DaveTBlake
Copy link
Member

Backport of #16327

Fixes the regression in PlayMedia builtin, which is broken in Leia when called for a playlist or smart playlist that needs to be recursively expanded into actual media items e.g. a smart playlist of artist or albums. Reported in https://forum.kodi.tv/showthread.php?tid=344892

@DaveTBlake DaveTBlake added this to the Leia 18.4 milestone Jul 5, 2019
@DaveTBlake DaveTBlake merged commit d092f92 into xbmc:Leia Jul 29, 2019
@DaveTBlake DaveTBlake deleted the PlayMediaBuiltinFix_leia branch July 31, 2019 06:07
@sualfred
Copy link
Member

sualfred commented Aug 12, 2019

@DaveTBlake
This commit breaks PlayMedia for .strm files.
.strm files are playlists and before this PR it was handled correctly. After this commit only 1 item is going to be played and the next one is not added to a playlist.

Tested it with a dummy playlist file:

e:\1.mp3
e:\2.mp3

And called it with PlayMedia(e:\test.strm)

Just noticed it because Emby for Kodi, which is using .strm files, doesn't work for widgets anymore in the latest Leia nightlies.

I guess it's enough if you make somehow a exception for .strm files instead of using !item.IsPlayList()

Edit:
2019-08-12 15:18:47.330 T:19472 WARNING: CApplication::PlayMedia called to play a playlist e:\test.strm but no idea which playlist to use, playing first item

@DaveTBlake
Copy link
Member Author

DaveTBlake commented Aug 12, 2019

OK, I'll take a look. Using .strm files as music playlists has always been an undocumented feature that worked by accident as far as I can see.

Yeap, I've had @notspiff confirm that .strm were only indended to hold one url.

@sualfred
Copy link
Member

Just to prevent mistakes:
We use .strm files for video playback and generating the required playback params. It's just broke for both of them. Starting from library works fine.
I know it was an undocumented feature, but it is the correct behaviour, because .strm is used to provide multiple playback urls.

PlayMedia() is acting weird anyway. As I told you in the past it's adding items to playlistid 0 instead of 1 for videos. We've bypassed this with a workaround in our code. But that's a different story :)

@angelblue05
Copy link
Contributor

Ok thanks for looking into it. We will arrange otherwise. Maybe update documentation now that this has been settled.

@DaveTBlake
Copy link
Member Author

Maybe update documentation now that this has been settled.

I certainly would if it was clear where, I guess in the PlayMedia builtin API?

We use .strm files for video playback and generating the required playback params. It's just broke for both of them.

@sualfred I'm not someone that removes "accidents" people are using if I can help it, OTOH spiff is a reliable source regarding what Kodi was designed to do. As for PlayMedia() is acting weird I really can't comment, but it sounds like the type of item to be played being mis-identified. CPlayListPlayer::Play needs .xsp and .m3u playlists expanding before it is called, but there isn't expansion for .strm (single url expected) although CPlayListPlayer::Play it could play as a lump. The whole thing is horribly tangled.

A set of tests that cover all the routes for starting playback (GUI, plugins, builtins, JSON etc.) of all the kinds of items that can be parameters (files, folders, playlists, smart playlists, etc.) and final media types that get played (video, music, picture - but not getting into media formats) would be so useful too.

@sualfred
Copy link
Member

sualfred commented Aug 12, 2019

@DaveTBlake
I just understand snippets of the C++ code (not my playground), but I believe you 100%.
One of our main problems is that there is a main difference between PlayMedia() and a regular library playback:
The library is building a playlist, even if it's just one item. So we are able to manipulate the playlist or add next items to it, etc. With this removed .strm = playlist handling, we have no way to to do this for PlayMedia() and there is no way to check if a playback has been started from a library node or a widget.

May I ask you to change that? I mean that PlayMedia is also adding the stuff to a playlist so both playback starts are somehow similar? Strms would still be just 1 valid URL, but we would have a playlist, which we could use.

@DaveTBlake
Copy link
Member Author

May I ask you to change that? I mean that PlayMedia is also adding the stuff to a playlist so both playback starts are somehow similar?

@sualfred I will certainly look more into what is happening with .strm, but I have been travelling so give me a couple of days to get onto it.

@sualfred
Copy link
Member

@DaveTBlake
Thank you and please keep us updated. Enjoy your travelling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants