-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat: Implemented HLS stream source as new plugin type. #84
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
{% endif %} | ||
|
||
{% addtoblock "js" %} | ||
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/hls.min.js "></script> |
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.
There is an extra space here after the hls.min.js.
djangocms_video/models.py
Outdated
""" | ||
Renders the HTML <source> element inside of <video> for an HLS stream defined by a .m3u8 URL. | ||
""" | ||
hls_source_url = models.CharField( |
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.
hls_source_url = models.CharField( | |
hls_source_url = models.URLField( |
Should this be a URLField
? Can the max_length
fall back to Django's default?
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?
hls.loadSource(hlsUrl); | ||
hls.attachMedia(video); | ||
hls.on(Hls.Events.MANIFEST_PARSED, function (event, data) { | ||
console.log('manifest loaded, found ' + data.levels.length + ' quality level',); |
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.
console.log('manifest loaded, found ' + data.levels.length + ' quality level',); |
Is this left over from debugging?
{% endif %} | ||
|
||
{% addtoblock "js" %} | ||
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/hls.min.js "></script> |
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.
Could that source be configurable and default to the cdn? Alternatively, please add a comment to the README on how to overwrite this template.
@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.