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

video/audio: add videojs previewer for a/v files #194

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

fenekku
Copy link
Contributor

@fenekku fenekku commented Dec 14, 2023

Screenshots

Audio previewer
audio_previewer

Initial Video

video_previewer_with_innate_poster

Paused Video

video_paused

Notes

  • We will be in the process of testing this internally over the next couple of weeks to see what else to be considered.
  • The CI is failling in general for tinymce it looks like.

fenekku added a commit to fenekku/invenio-app-rdm that referenced this pull request Dec 18, 2023
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)
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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

@Samk13
Copy link
Member

Samk13 commented Dec 18, 2023

Excellent contribution, much appreciated! 🚀

@@ -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
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! done.

@fenekku
Copy link
Contributor Author

fenekku commented Dec 19, 2023

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).

Copy link
Contributor

@tmorrell tmorrell left a 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
Copy link
Contributor

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

invenio_previewer/extensions/audio_videojs.py Show resolved Hide resolved
invenio_previewer/extensions/audio_videojs.py Show resolved Hide resolved
@fenekku fenekku force-pushed the 7_video_audio_player_videojs branch from 008d1d5 to 6414db8 Compare March 19, 2024 20:51
@fenekku
Copy link
Contributor Author

fenekku commented Mar 19, 2024

Rebased. I don't have the repo rights to merge this one though. @lnielsen or someone else can do it now.

@ntarocco ntarocco merged commit 8b6c140 into inveniosoftware:master Mar 25, 2024
4 checks passed
fenekku added a commit to fenekku/invenio-app-rdm that referenced this pull request Apr 1, 2024
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)
fenekku added a commit to inveniosoftware/invenio-app-rdm that referenced this pull request Apr 1, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released | Done 🚀
Development

Successfully merging this pull request may close these issues.

extensions: video previewer
6 participants