-
Notifications
You must be signed in to change notification settings - Fork 59
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
video/audio: add videojs previewer for a/v files #194
video/audio: add videojs previewer for a/v files #194
Conversation
c1ab629
to
bf756d4
Compare
This makes them available by default in any invenio-app-rdm backed instance. :warning: depends on inveniosoftware/invenio-previewer#194 being merged and released (i.e. wait for that to confirm dependency versioning too)
invenio_previewer/assets/semantic-ui/js/invenio_previewer/videojs.js
Outdated
Show resolved
Hide resolved
setup.cfg
Outdated
@@ -28,7 +29,7 @@ python_requires = >=3.7 | |||
zip_safe = False | |||
install_requires = | |||
charset_normalizer>=3.3.2 | |||
invenio-assets>=1.2.7 | |||
invenio-assets>=1.2.7,!=3.0.1 | |||
invenio-base>=1.2.10 |
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.
The exclusion of invenio-assets is a short-term fix. It's important to investigate the root cause of the errors with this version and address them for future compatibility.
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.
The problem has to do with the improper inclusion of tinymce in invenio-assets 3.0.1 . The issue is known: https://discord.com/channels/692989811736182844/1035104382519365712/1186299141257691167 and any fix would imply a new version, so I think it's reasonable to skip it. I don't know if discord links are very permanent / would make sense to be added though.
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.
Maybe @kpsherva has a better say about this.
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.
I think this has been fixed in v3.0.2
} | ||
|
||
body { | ||
justify-content: center; |
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.
This could lead to unexpected global style changes no?
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.
Only if the built css from audio_videojs.scss (this file) is placed in a "global" (or at least not in the only template it is placed in right now) would it potentially cause issues.
The previewer endpoint returns an HTML page that is rendered inside an iframe on the details page. So its style is always well prevented from affecting wider styles. Other previewers (e.g., the pdf one) take advantage of this to really customize the style to what they need.
Excellent contribution, much appreciated! 🚀 |
invenio_previewer/webpack.py
Outdated
@@ -72,6 +73,9 @@ | |||
"bottom_css": "./scss/invenio_previewer/bottom.scss", | |||
"simple_image_css": "./scss/invenio_previewer/simple_image.scss", | |||
"txt_css": "./scss/invenio_previewer/txt.scss", | |||
"videojs_js": "./js/invenio_previewer/videojs.js", # shared for audio and video # noqa |
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.
I think here you can point directly to the node_modules/video.js module
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.
Nice! done.
Tested with nginx server, with Firefox on Ubuntu client. Works well enough. Important thing is to have a fronting server like nginx that can respond to range request (mostly for video). I didn't have to make other nginx config adjustments (other than already done X-Accel-Redirect configuration which allows file serving by nginx in the first place). |
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 like a great addition, with one quick change.
setup.cfg
Outdated
@@ -28,7 +29,7 @@ python_requires = >=3.7 | |||
zip_safe = False | |||
install_requires = | |||
charset_normalizer>=3.3.2 | |||
invenio-assets>=1.2.7 | |||
invenio-assets>=1.2.7,!=3.0.1 | |||
invenio-base>=1.2.10 |
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.
I think this has been fixed in v3.0.2
008d1d5
to
6414db8
Compare
Rebased. I don't have the repo rights to merge this one though. @lnielsen or someone else can do it now. |
This makes them available by default in any invenio-app-rdm backed instance. :warning: depends on inveniosoftware/invenio-previewer#194 being merged and released (i.e. wait for that to confirm dependency versioning too)
This makes them available by default in any invenio-app-rdm backed instance. :warning: depends on inveniosoftware/invenio-previewer#194 being merged and released (i.e. wait for that to confirm dependency versioning too)
Screenshots
Audio previewer
Initial Video
Paused Video
Notes