-
Notifications
You must be signed in to change notification settings - Fork 48
feat: Implemented HLS stream source as new plugin type. #84
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
base: master
Are you sure you want to change the base?
Conversation
Also implemented show_controls and autoplay attributes on the video element. These are particularly useful when showing a live stream.
605e6ad
to
b83f60b
Compare
djangocms_video/templates/djangocms_video/default/hls_stream_source.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sepi !
Thanks for the PR! Looks good to me. I've added two comments.
In addition, can you add to the README the support of HLS streams?
Finally, can you add tests for the new plugin and model?
djangocms_video/templates/djangocms_video/default/hls_stream_source.html
Outdated
Show resolved
Hide resolved
djangocms_video/templates/djangocms_video/default/hls_stream_source.html
Outdated
Show resolved
Hide resolved
@sepi Would be great if you can address these review comments so that we can work together quickly to get this merged. |
I reacted to all of your comments except for the tests. I don't really see any functionality I could test correctly. Maybe I also don't believe in tests or am just lazy :P |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @fsbraun Please have a look once too.
@vinitkumar @sepi To me, it seems, there's just a few small things ti fix:
|
@sepi I had a look, and the change of I made some updates to fix the migrations, superfluous spaces in the template and the test setup. (For some reason, I cannot compile the python 3.9 requirements on my machine - that can be fixed separately.) PS: The existing tests revealed that your change to the video player template added unnecessary spaces... |
Also implemented show_controls and autoplay attributes on the video element. These are particularly useful when showing a live stream.
Description
In order to be able to show an rtsp live stream, an HLS video source is implemented using HLS.js. The script is currently embedded using a CDN.
In order to stream rtsp to the web, you first need to make an .m3u8 playlist file available using for example ffmpeg. The URL of this file can be used as URL of the stream source.
Related resources
Checklist
master
Slack to find a “pr review buddy” who is going to review my pull request.