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

Fixed issue #76 #79

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

GautamMKGarg
Copy link

Hello @nullket and @firsttris

Tried fixing the issue #76. Issue is almost fixed. When I try to enter any youtube URL or any other URL in sendToKodi plugin directly, data is getting fetched but video/audio is not getting played. But when I send the URL via Kore, video is getting played.
I am testing with same url in both the cases. Not sure if I am doing something wrong. Could you please suggest me changes if you are available?

@nullket
Copy link
Collaborator

nullket commented Sep 17, 2022

I really appreciate that you want to contribute! Thanks. I am currently on mobile but looking forward to fix this issue together with you.

Anyway, before we starting the code, we should generally decide what the plugin should do. While I understand your idea with the user input I don’t think it will be ever used at all (my personal opinion). IMHO the plugin is called SENDtokodi not „input url at Kodi“. So in MY opinion it should maybe just show an info on how to use it and maybe something like a QR code to download one of the companion apps?

As this is not a technical issue I would really like to hear some other voices on what is really useful and should be the intended functionality.

@GautamMKGarg
Copy link
Author

@nullket I understand your point. But in my opinion, this plugin is not actually sending to Kodi, It's a middle layer which is receiving URL from multiple sources like Kore, Chrome extension etc, converting URL to playable URL with the help of yt-dlp and play on Kodi. So, even sendToKodi name is also not fully correct, because main role of the plugin is to receive and play.
As it's receiving URL from multiple sources, if we can feature to users to add URL directly in Kodi will be like adding support for one more source (ie User Input). It will not change main role of the plugin which is converting received url and play.
In short, in my opinion, it's just an another source.

I think, while using kodi on PC, if we will have option to just copy url from browser and paste it in kodi, It will be useful feature.

@firsttris
Copy link
Owner

firsttris commented Sep 18, 2022

i actually made a chrome extension todo this:

https://github.com/firsttris/chrome.sendtokodi

but i was so lazy not to extend my chrome developer account and it was deleted from the store.
i have to bring it back. add a github-action pipeline. etc..

the kodi addon was acutally intended as service addon.

still i understand your idea to add some functionality if you open the addon in kodi
i agree with @nullket that i also dont see any use case where i would use this.
but also you never know what crazy usecases people are coming up with kodi.
if there is no other tradeoff i would be ok with it.

best regards
tristan

@GautamMKGarg
Copy link
Author

@nullket and @firsttris
Thanks fro the inputs. So what do you finally suggest, should we continue this development or not?

@nullket
Copy link
Collaborator

nullket commented Sep 20, 2022

but also you never know what crazy usecases people are coming up with kodi.
if there is no other tradeoff i would be ok with it.

Aye. I second this. But I would also argue that one should work on the root of the issue (e.g. sending the link directly from your browser would be much more convenient than navigating to the add-on). But that means not just reactivating your chrome developer account firsttris but somewhat support for firefox and maybe even safari.

So in conclusion to that, either showing an info page (on how to use the add-on) or providing an input makes sense. @GautamMKGarg so I would say go on with it. Your designed input should not cause any issues (at least I hope so). But your introduction of the play function is a good indicator that a proper refactoring with sub-modules, maybe a class based approach, respecting pep8, etc. is really needed. Especially building and providing custom manifests (#34) would add a ton of code and thus complexity.

Copy link
Collaborator

@nullket nullket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments to the code but all of those are basically really minor thins and just suggestions - so please do not get shocked of 6 comments on the approx 3 changed lines of code ;) Oh and btw it seems there is a merge conflict (try to merge the current master in your branch an resolve the issue from there)

service.py Outdated Show resolved Hide resolved
service.py Outdated Show resolved Hide resolved
service.py Outdated Show resolved Hide resolved
service.py Outdated Show resolved Hide resolved
service.py Outdated
@@ -60,6 +70,10 @@ def showErrorNotification(message):
def getParams():
result = {}
paramstring = sys.argv[2]

if paramstring == '':
paramstring = "?" + get_user_input()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't know this but for what reason is the ? needed? May maybe something like the following would be more direct

if not paramstring:
  result['url'] = paramstring[1:]
else:
  result['url'] = get_local_user_input()
  

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that in future we might need to give option to user to select video quality, or pass any other customer parameters. That's why I did not pass URL directly in url variable. Kindly suggest.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would even then suggest to explicitly return the result dict already correctly assigned to the keys instead of parsing it from a string.

Forcing a quality via the resolver will not really be possible anymore. Basically all streams are now fragmented with a manifest. Many still provide one legacy all in one stream where there is not much to choose from. For Kodi you can set the desired quality of a fragmented stream within the adaptive input stream addon. We partially supported manifest since the last PR we were testing so much.

service.py Outdated Show resolved Hide resolved
@GautamMKGarg
Copy link
Author

But that means not just reactivating your chrome developer account firsttris but somewhat support for firefox and maybe even safari.

Yes, You are right @nullket. Different users have different browser preference. I like using Firefox and Brave. Brave is also chromium based, but some of the chrome addons don't work on Brave. I would like to add one more point. Adding third-party addons make browser slower. And also some users don't like to install addons due to their security concern. People use web browser to do banking and stuff, so not all users like to install a addon on browser. I know that sendToKodi is an opensource safe plugin, but not all users know that. Secondly, if someone use Kodi regularly then it's fine to install an addon, but if user use it once in a while, then they will not prefer to install addon. Just coping URL will be more convenient. for them.
Media players like MPC-HC and VLC also started providing inbuilt feature/Addon to support
yt-dlp. In these players, we can just enter youtube URLs and they start playing it.

either showing an info page (on how to use the add-on) or providing an input makes sense.

We can keep both options. For time being we can provide user input option. Later on when documentation is ready, we can create a Menu, where we can provide both options,

  1. Help
  2. Enter URL
  3. Setting
  4. Any other menu item

@GautamMKGarg
Copy link
Author

GautamMKGarg commented Sep 20, 2022

I added some comments to the code but all of that is basically really minor stuff and just suggestions - so please do not get shocked of 6 comments on the approx 3 changed lines of code ;)

I am fine with the suggestions you have given. If you want to edit the code on your own, I am fine with it. Or if you want me to do changes, then also I can work on the same. Please suggest, how would you like to proceed?

I am beginner for sendToKodi, that's why I have made many basic mistakes. You know the code better.

@GautamMKGarg
Copy link
Author

@nullket
Almost all the suggested changes are done.

When I try to enter any youtube URL or any other URL in sendToKodi plugin directly, data is getting fetched but video/audio is not getting played.

As mentioned earlier, while trying to play video, Video playable URL is getting fetched properly. But the video is not getting played. Will you be able to fix the bug in your free time?

URL: https://www.youtube.com/watch?v=g6fnFALEseI

2022-09-22 22:12:52.277 T:20476    INFO <general>: initializing python engine.
2022-09-22 22:12:54.256 T:15984    INFO <general>: Loading skin file: DialogKeyboard.xml, load type: KEEP_IN_MEMORY
2022-09-22 22:13:06.237 T:20476    INFO <general>: plugin.video.sendtokodi: could not find an original manifest or manifest is not supported falling back to best all-in-one stream
2022-09-22 22:13:06.237 T:20476    INFO <general>: plugin.video.sendtokodi: creating list item for url https://rr1---sn-npupj5cax-qxal.googlevideo.com/videoplayback?expire=1663886586&ei=mpAsY6eTH92Sz7sP0Ke68AE&ip=45.249.87.125&id=o-AJbJFISldbyvtrfwXD66WPuOHJNYTV5NFDQ5-bfm2AXn&itag=22&source=youtube&requiressl=yes&mh=b3&mm=31%2C29&mn=sn-npupj5cax-qxal%2Csn-qxaelnel&ms=au%2Crdu&mv=m&mvi=1&pl=24&gcr=in&initcwndbps=738750&spc=yR2vpxENxZgfxiId2lW5Niypcrq7Q94&vprv=1&svpuc=1&mime=video%2Fmp4&cnr=14&ratebypass=yes&dur=267.308&lmt=1662944603261902&mt=1663864675&fvip=1&fexp=24001373%2C24007246&c=ANDROID&txp=4532434&sparams=expire%2Cei%2Cip%2Cid%2Citag%2Csource%2Crequiressl%2Cgcr%2Cspc%2Cvprv%2Csvpuc%2Cmime%2Ccnr%2Cratebypass%2Cdur%2Clmt&sig=AOq0QJ8wRQIgSTPw5-D7UBgoBY6aAFKer4cRG_MeHAuSXn5vU3n0ODACIQCOwkE20QTZxEIJ2B2mHk8f_L-8Sxo8PYUBu3hJxwMLsg%3D%3D&lsparams=mh%2Cmm%2Cmn%2Cms%2Cmv%2Cmvi%2Cpl%2Cinitcwndbps&lsig=AG3C_xAwRAIgbLAdUiosuVmsJwzqxJaoGpLCPW9qcZnFSRTfLJORQ1wCIDOGNCBwO2YyY718KukUEcGTn35FkH-JAeTxXqL-xVJh
2022-09-22 22:13:06.241 T:20476    INFO <general>: CPythonInvoker(10, C:\Users\Gautam\AppData\Roaming\Kodi\addons\plugin.video.sendtokodi\service.py): script successfully run
2022-09-22 22:13:06.396 T:20476    INFO <general>: Python interpreter stopped

@nullket
Copy link
Collaborator

nullket commented Sep 25, 2022

I had a longer look at this and I don't know why the playback does not start. In my understanding setResolvedUrl should start the playback if the second parameter is play.

@firsttris can you explain how calling the addon via the json api works and how the start of the playback work? I assume in the json is a Player.Open call or something?

@firsttris
Copy link
Owner

{
	"jsonrpc": "2.0",
	"method": "Player.Open",
	"params": {
		"item": {
			"file": "plugin://plugin.video.sendtokodi/?https://soundcloud.com/spinnin-deep/sam-feldt-show-me-love-edxs-indian-summer-remix-available-june-1"
		}
	},
	"id": 1
}

@nullket
Copy link
Collaborator

nullket commented Sep 27, 2022

Thanks!

@GautamMKGarg that means you either need something that opens the player (and does not try to open it twice when called via the api) or call the plugin recursively with the api call shown above and the manual retrieved url.

@anohren
Copy link
Collaborator

anohren commented May 30, 2023

I have only had a cursory glance at the changes and the issue description, but it seems to me that this relies on functionality intended for script addons (async user input), as opposed to plugin addon functionality (which this currently is).

This plugin is already using script functionality (according to consultation with Kodi forum users). We should probably not be tricked into spending time asking why something doesn't work at that point — we're already off the map, and might end up rolling a boulder uphill. My understanding thus far is that a plugin addon should always be able to resolve into Kodi navigation tree nodes without additional user input. Correct me if I'm wrong.

If we want these things I still suspect we'd need a script addon or something. Possibly a combination of both. But I'm no Kodi expert.

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.

5 participants