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

Reduced the youtube processing time 90% #391

Merged
merged 19 commits into from
Nov 3, 2018
Merged

Reduced the youtube processing time 90% #391

merged 19 commits into from
Nov 3, 2018

Conversation

sansyrox
Copy link
Member

@sansyrox sansyrox commented Oct 1, 2018

Fixes #384
(Partial Fix to #309 )
Creates a custom media server rather using mpv

Copy link
Member

@hongquan hongquan left a comment

Choose a reason for hiding this comment

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

Should clean the coding style later.

@sansyrox sansyrox changed the title Reduced the youtube processing time 90% Reduced the youtube processing time 90%(WIP) Oct 3, 2018
Copy link
Member

@prateekiiest prateekiiest left a comment

Choose a reason for hiding this comment

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

Tests are still failing

@hongquan
Copy link
Member

hongquan commented Oct 5, 2018

You are introducing more software to SUSI Linux, so please update the install script to include these software.

Please note that, we are using headless Raspbian, so please include vlc-nox, instead of vlc.

Btw, according to #384 (comment), your PR still depends on youtube-dl, so please test more to point out why this PR help reduce processing time, while both solution (with and without pafy) still uses youtube-dl.

@sansyrox
Copy link
Member Author

sansyrox commented Oct 5, 2018

@hongquan , the testing is still in process as the PR is still in (WIP) . But how is this PR dependant on youtube-dl . As I am directly streaming from a youtube link and not downloading it.

Noted, thanks a lot 😄

please include vlc-nox, instead of vlc

@hongquan
Copy link
Member

hongquan commented Oct 7, 2018

I know that you are streaming video. Quote from my comment:

So, you still can use youtube-dl to retrieve the streaming URL.

So, why have to add pafy and VLC (It is OK if vlc-nox is lighter than mpv)?

@sansyrox
Copy link
Member Author

sansyrox commented Oct 8, 2018

@hongquan , I am using pafy to retrieve the best mp3 streaming URL and am using the library "python-vlc" to stream the URL. I believe that we can install vlc-nox as well

@sansyrox
Copy link
Member Author

@hongquan @cweitat , upadted

@cweitat
Copy link
Contributor

cweitat commented Oct 17, 2018

@stealthanthrax conflict

@sansyrox
Copy link
Member Author

@cweitat , updated

Copy link
Member

@hongquan hongquan left a comment

Choose a reason for hiding this comment

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

Pay attention to coding style!

Add your new packages to install script.

@@ -10,3 +10,4 @@ cp $DIR_PATH/Systemd/ss-*.service /lib/systemd/system/
systemctl enable ss-update-daemon.service
systemctl enable ss-susi-server.service
systemctl enable ss-python-flask.service
systemctl enable ss-susi-youtube.service
Copy link
Member

Choose a reason for hiding this comment

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

Add blank line, please!

return resp

if __name__ == '__main__':
app.run(debug=True,port=7070,host= '0.0.0.0')
Copy link
Member

Choose a reason for hiding this comment

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

Add whitespaces

@sansyrox
Copy link
Member Author

@hongquan , updated

@sansyrox sansyrox changed the title Reduced the youtube processing time 90%(WIP) Reduced the youtube processing time 90% Oct 20, 2018
@hongquan
Copy link
Member

You still don't update what I requested:

image

@hongquan
Copy link
Member

@stealthanthrax Note, I asked you to update install script twice, but you still ignore??

@sansyrox
Copy link
Member Author

@hongquan , I am not sure why I need to make any change with the install script

@sansyrox
Copy link
Member Author

@hongquan , travis fails If I add whitespaces at the end of a line

You still don't update what I requested

@hongquan
Copy link
Member

I am not sure why I need to make any change with the install script

You make SUSI depend on pafy, vlc. If you don't install them to RPi, how can SUSI run?

travis fails If I add whitespaces at the end of a line

@stealthanthrax Who told you to "add whitespace at the end of a line"?

@sansyrox
Copy link
Member Author

@hongquan , then where do I add them ?? 😕

Who told you to "add whitespace at the end of a line"?

@hongquan
Copy link
Member

You have had to fix PEP8 so many times, don't you?

untitled

@hongquan
Copy link
Member

@stealthanthrax You only add "python-vlc" to requirements file, then from where VLC come, for "python-vlc" to call?

@sansyrox
Copy link
Member Author

@hongquan , updated

@hongquan
Copy link
Member

@stealthanthrax

You are about to replace MPV with "VLC", why don't you remove MPV from install script??

And what the hell is this:

image

You make the checker silent, instead of fixing your error?

@sansyrox
Copy link
Member Author

If I don't disable the pylint , it won't allow us the run it at '0.0.0.0' which is an essential feature as it will allow us to control the music from mobile clients in the future.

You make the checker silent, instead of fixing your error?

@hongquan
Copy link
Member

hongquan commented Oct 28, 2018

About issue of binding to 0.0.0.0, in practice, people won't let Python app to do that. Python app will bind to localhost (127.0.0.1) only, and then use a reverse proxy like Nginx to face the outside world. The diagram is like this:

Outside world <--> Nginx (bound with 0.0.0.0) <--> Python web app (127.0.0.1)

It is because, there are many tasks that Nginx can do itself, without needing Python app's effort. Access rate limit (to avoid abusing) is also taken care by Nginx.

But in our use case, it seems to be not a time to need Nginx yet.

What I complaint in the line

app.run(debug=false,port=7070,host= '0.0.0.0')

is this issue. Just in this PR, I tell you again and again to fix, but you keep ignoring?

@sansyrox
Copy link
Member Author

@hongquan , updated

@hongquan
Copy link
Member

@stealthanthrax How about this?

You are about to replace MPV with "VLC", why don't you remove MPV from install script??

@sansyrox
Copy link
Member Author

@hongquan , updated

@sansyrox sansyrox merged commit 71845a3 into fossasia:master Nov 3, 2018
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.

6 participants