-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support playlists with reasonable performance (proof of concept) #20
Conversation
Proof of concept. Does not support random access to playlist items using the Kodi GUI for some reason.
amazing. good work. like your idea. you say does it play the next item after the first item is finished? |
Yes, [1:] is the same string slicing syntax you used further up to remove the ? from the query string. xbmc.Player() is supported in script addons. You have written a plug-in addon. The call initially caused a crash when the plugin recursed. I worked around that by making sure the first item in the queue was already resolved. Every other url is pointing back to the plugin itself. It hands itself YouTube-dl video ids ('urls') in this way. Exactly. It works if you use the "next" button in the player. |
putting kodi in debug mode and checking log might help what is going wrong with random access? |
That's an idea, didn't think of that . Although I think what's wrong is simply 'Kodi', since the plugin isn't even involved at that point. It shouldn't be this hard. The documentation doesn't explain functions or types, and doesn't even list so called "properties" that you can set on list items. Not to mention explain their semantics. It's just a stab in the dark and an endless search on the forum. That it's python doesn't exactly help 🙂 "a beautiful language" — if runtime exceptions are beautiful |
i feel like this is a super cool improvement but it also introduces a new bug. (random access of playlists) i really want to test and look into this, but currently i have limited time do you feel like this is ready for production or is it just a proof of concept ? |
I would not release it. It's not up to my standards as long as the bug is present. After all, what point is a long playlist if you can't jump in it? I have yet to receive a reply in my thread about it in the Kodi forums. |
Random access seems to work in Kodi Matrix on macOS. Just needs two quick fixes. Not sure about what to do about earlier versions. It requires a python 3 specific fix, and I'm not sure how/if you separate python2 and python3 versions somehow. |
Should be python2/3 compatible now. I'll leave it to you @firsttris to evaluate backwards compatibility and requirements. If the random access bug correlates with certain Kodi versions I suppose it would be possible to probe for that. |
will merge soon, just did some repository changes, where is splitted the repo for kodi 17 python 2 & 18 python3 |
Isn't it easier to be 2 & 3 compatible considering there are so few lines of code? |
i thought the source code is compatible with python 2 as well as 3, and its only a matter of the kodi xml files. |
You know best about that. I just assumed you mentioned splitting up the repo in order to fork the code for some reason. Does sound like more maintenance to have two repos though. |
the sourcecode remains in this repo, i meant there are now 2 different kodi addon repositories to install the extension from this was requested because the extension is now supported by the official android kodi app "kore". |
@firsttris Do you mean to enable it only for Matrix and later? Random access play queue was broken as late as Leia. I have no idea why, but then I've found that the queue feels broken most of the time. It usually seems to just teleport me away somewhere at the slightest problem. |
@firsttris |
@anohren I am picking up here to work on the playlists first. I feel like I am overlooking something, can you help me here? Why is it faster with If we start using We have a merge conflict and at least the |
Yes, that's it. This is how I explained it to Tristan:
So the plug-in is called back for each item when it's actually played, to allow lazy evaluation.
Yeah I expect that the URL parsing will be more esthetic in Python than in Lua.
Well, maybe convenience was the reason. But in all honesty it's just one file that you can paste right into while Kodi is running so I've never seen that as a big problem during development, and I've never imagined asking non-devs to help test — at least not such a simple thing as this. I'll resolve this conflict if there's interest in moving the PR forward. After that we would need a decision on:
I think we should. Playlists are pretty pointless already if they take minutes to parse. As it currently stands, design-wise we're just conditioning users not to send playlists at all. |
I think it's also interesting to keep in mind that SendToKodi should actually be a script addon instead of a plug-in addon:
We're currently "mixing and matching". I imagine that if we were a script instead we could do things like:
|
Sure lets work on this. Thanks for the explanation. But just to be clear: So kodi calls the plugin with the url when one item in the playlist ends and the next entry wants to start? So it simply goes into the
From reading the above, what is the exact issue? What behaves wrong in which way? You have kodi 17 & 18 and I have kodi 19 we can get this tested, fixed and merged.
Maybe we should open another issue for that and include a bit of refactoring etc.
That sounds a lot cleaner to me. The downside would be that the youtube playlist with a starting index thing would get a bit more complicated again. We might not be able to add videos to a kodi playlist before the current playing one? Is this important at all? I mean it would be nice to have the whole youtube playlist but the "playlist so far" should be enough right? |
Ok, I'll take a look at this then.
You've got it.
I think the best description I can give is "the play queue implodes". I haven't tried it for some time, but when I select a queued item the entire queue in the GUI is replaced with some broken version of your standard Kodi library/file browser. Or it all just disappears -- I don't remember exactly which. You'll know it when you see it.
Sure. It can't hurt to play around with the idea at least. I don't know exactly what it would entail since I've never written a Kodi addon script.
I just made sure that queued items before the currently playing one are at least retained in the queue. That should be enough for our purposes regardless of if we're a script or a plugin. Preloading is just best effort, and the default will always be to resolve URLs lazily. It's a bit sad how neglected (personal opinion) the Kodi play queue is -- it feels like it's been hidden away under an obscure button in an invisible side bar far outside of the actual video player screen ever since it ran on the original Xbox. If it were simly exposed on the player screen, the same way the Plex addon does it, it would be about 100 times more useful. |
Perfect! I just integrated the building as I am far too lazy to copy code around for testing!
Are you sure? In my understanding a script runs just one time e.g. when we submit the url initially and we would then have to parse all URLs in the background and not in a lazy way on demand? A plugin surely enabled lazy resolving as discussed above.
I get what you mean but I do not use them at all. I don't know why, maybe because they are the way the are? Maybe it is just not my style. Anyway I am more than happy to resolve the issues anyway maybe this brings enough motivation to work on input stream adaptive.. |
What the... Now random access works in 18.9 Media Center Kodi on OSMC. Kodi 17.6 couldn't run it due to Well, I can't seem to recreate the problem anymore on OSMC (Rpi3) or 32 bit Ubuntu 18.04. |
No such method on Kodi 17 and lower
Last problem I experienced while testing is that custom youtube-dl options that were passed with the initial call to the plugin are not preserved when jumping to the other playlist entries. That's to be expected. They're not passed along with the playlist items. I guess they can just be added to the Update: Fixed. This should be ready for testing now. |
If the plugin was called with extra parameters to control e.g. youtube-dl these are now preserved for every lazily resolved item in a playlist
I have testes the build artifact of the latest commit on kodi 19.
It seems like the url is percent encoded when submitted to the plugin. Expand to see kodi log
|
I don't think I'm even going to think about this further than Let's try that env variable first of all and see what happens. Then if it works we'll figure out why it thinks out URL is a file system path. Update: seems this message is logged by YouTube-dl. Are we sure that not YouTube-dl is returning these mangled URLs? It's kind of strange since we're using the result's Maybe we'll try using the video ID field instead and see where that takes us. @nullket Oh and what environment are you running in, which exact Kodi version, and which playlist? |
Yes I also have the feeling that it might be kodis fault but the LC_ALL warning is coming from yt_dlp.
I did a quick test in ipython, it seems to work correct. In [5]: url = "https://youtube.com/playlist?list=PLr_CZLgMkHeUexzcA64PfjCW0UUoO8Twi"
In [6]: dl_opts = {'format': 'best', 'extract_flat': 'in_playlist'}
In [7]: result = ydl.extract_info(url, download=False)
In [8]: result['entries'][2]['url']
Out[8]: 'https://www.youtube.com/watch?v=KzSoabcQYMg'
In [9]: result['entries'][2]['url']
Out[10]: 'https://www.youtube.com/watch?v=YWiwVaP0JQM'
https://youtube.com/playlist?list=PLr_CZLgMkHeUexzcA64PfjCW0UUoO8Twi
|
Nice, thanks. I'll see if I can recreate it. |
Hmm. I tried but failed, and when inspecting it I was apparently not testing this PR. Did I misunderstand where to install from? I just pulled 0.9.171 from the python3 repo. |
Okay I've tried it, and could reproduce the issue. It only happens when using yt_dlp, so I'll assume that your manual test was mistakenly with YouTube-dl, which was my suspicion. I'll preliminarily conclude that yt_dlp does sanity checks that are reaching outside its scope. If this is the case then the assumption that output in a field called Update: and apparently my suspicion was unfounded — I can't recreate it manually with yt_dlp. |
Yes, I am not releasing an unmerged PR for everybody. Follow the checks here in the PR or go to the actions tab and choose the latest run for your PR/branch. Download the correct zip (for python3 it would be "Addon-artifacts-Matrix", extract the download once to get the zipped plugin (there is no way to avoid the double zipping caused by github actions) https://github.com/firsttris/plugin.video.sendtokodi/actions/runs/2373042603
Sorry if I was unclear about that I aborted testing after it did not work with yt_dlp.
That is why I also tested it real quick with a standalone python session. So just to clarify (I am not at home to test), does it work with a kodi matrix instance (I assume you now have one for testing?) and youtube_dl? |
Yes, exactly, weirdly it works when youtube-dl is selected in the settings. That's what's throwing me off. It's easy to fix, but I'm still a bit weary about putting arbitrarily formatted strings into |
It seems youtube-dl accepts percent escaped URLs as input, and yt-dlp doesn't..?
Yes, me too but we still do not really know if it is not yt_dlp (I mean it works outside of kodi but who knows...). Can we look at the |
If this works for everyone I think we're ready to merge. Let's test random access in the play queue once more to make sure nothing else has been overlooked:
|
|
The generic extractor no longer generates title fields
Good catch. Fixed. Weird that it doesn't work now — it seemed to extract titles just fine when I tested these changes in the addon installed directly from the repo. Maybe I overlooked something. |
@anohren thanks for your work and especially your precision on the subject! It worked for me, I assume it worked for you so I merged it and now we will see if anybody (at all) has a problem ;) |
Thank you for helping to move it forward and for trying it out! Sounds like a good plan. |
This loads a playlist of 1200+ items in 47 seconds on a Rpi 3. The videos can be played back sequentially, but does not support random access to playlist (queue) items using the Kodi GUI for some reason.
FIxes #15