Skip to content

[FEATURE] Integrate media player from slub_web_sachsendigital#1320

Merged
sebastian-meyer merged 135 commits intokitodo:mainfrom
fschoelzel:dev-mediaplayer
Apr 1, 2025
Merged

[FEATURE] Integrate media player from slub_web_sachsendigital#1320
sebastian-meyer merged 135 commits intokitodo:mainfrom
fschoelzel:dev-mediaplayer

Conversation

@fschoelzel
Copy link
Contributor

This PR integrates the video/audio player from slub_web_sachsendigital into a new MediaPlayer plugin.
The PR is based on #796 and #818 from @dvoracek-slub

For testing this within the DFG Viewer, I have prepared a DDEV system at https://github.com/dvoracek-slub/ddev-dfgviewer-dist/tree/mediaplayer. It relies on https://github.com/dvoracek-slub/dfg-viewer/tree/dev-mediaplayer and includes a demo METS video document.

There also is a work-in-progress section in the documentation that describes some features and technical aspects of the player. composer docs:serve can be used for local preview.

Update (2022-07-06). Most notable changes:

* Kitodo in general
  
  * Read production place and person names in `Mods` format class. I use this to test GND linking of some audio-related metadata.
  * Add option to show full TOC.

* Media player
  
  * It is now possible to set and export markers/segments in the media. The list is shown in audio mode (press F2).
  * When the METS links preprocessed waveform data, this can be shown (and be used to edit markers). See the DDEV system for an example.
  * First version of handling recordings that are split across multiple media files. Currently, this only works via page reloads.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@sebastian-meyer sebastian-meyer self-requested a review September 9, 2024 14:06
@sebastian-meyer sebastian-meyer added the ↷ feature A new feature or enhancement. label Sep 9, 2024
@sebastian-meyer
Copy link
Member

sebastian-meyer commented Sep 9, 2024

You placed a lot of JavaScript files in Resources/Private/JavaScript/. Are you sure those shouldn't be put in Resources/Public/JavaScript/ instead, because they need to be accessible by the client?

Also, please put all unchanged third-party libraries in their own sub-directories separate from your own JS files. There are several reasons for that:

  • It's easier to update dependencies if you know which files are part of a third-party library and don't contain any custom changes.
  • We can exclude third-party libraries from all static analysis checks, especially regarding their code styling. (This will most likely greatly reduce the number of Codacy issues currently reported!)

@fschoelzel fschoelzel force-pushed the dev-mediaplayer branch 3 times, most recently from f8e7a39 to b591463 Compare September 9, 2024 16:09
@fschoelzel
Copy link
Contributor Author

fschoelzel commented Sep 9, 2024

You placed a lot of JavaScript files in Resources/Private/JavaScript/. Are you sure those shouldn't be put in Resources/Public/JavaScript/ instead, because they need to be accessible by the client?

This is correct and was done on purpose. All the files in the Resources/Private/JavaScript/ folder are written to a compressed file called DlfMediaPlayer.js during the build process using webpack. This and the associated .map and vendor file are the only files that the client needs when loading the player.
Additional information to code organization: MediaPlayer/Developers.rst

@fschoelzel
Copy link
Contributor Author

Also, please put all unchanged third-party libraries in their own sub-directories separate from your own JS files. There are several reasons for that:

  • It's easier to update dependencies if you know which files are part of a third-party library and don't contain any custom changes.

  • We can exclude third-party libraries from all static analysis checks, especially regarding their code styling. (This will most likely greatly reduce the number of Codacy issues currently reported!)

As far as i know this two third-party files were copied from the original repository and rewritten by @dvoracek-slub to meet the ES6 typescript requirements:
Resources/Private/JavaScript/DlfMediaPlayer/3rd-party/EventTarget.js
Resources/Private/JavaScript/DlfMediaPlayer/3rd-party/VideoFrame.js

The only other file I found is:
Resources/Private/JavaScript/SlubMediaPlayer/components/equalizer/filters.js

Which in my opinion should not easily be updated, like the other two files, without deeply testing the whole function of the Player.

Apart from that, all other third-party dependencies are located in the Build/node_modules folder and get compressed into the DlfMediaVendor.js file

@fschoelzel
Copy link
Contributor Author

I agree that the files created after the build process should be stored in the Resources/Private/JavaScript/DlfMediaPlayer folder instead of just the JavaScript folder.

Copy link
Member

@sebastian-meyer sebastian-meyer left a comment

Choose a reason for hiding this comment

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

Some general remarks in addition to the detailed comments below (as discussed in our call):

  • Please remove the DevServer or make it a dev-only dependency if that's possible.
  • Please try to merge DlfMediaPlayer, SlubMediaPlayer and lib/ as much as possible, since those are dependencies of each other anyway which won't work as stand-alone implementations.
    ** At the moment it is really difficult to tell apart what those do, where they are used and how they depend on each other.
    ** The best way of handling this would be to have just two directories: One containing the third-party Shaka player (and possibly other unchanged third-party libraries) and one containing all the Kitodo-specific code.
  • Please add license and author information to every non-third-party file.

Reverts variable renaming as it would break compatibility with waveform-data library:
- Library expects snake_case properties i.e. 'sample_rate'
- TypeScript definitions enforce 'sample_rate' naming
Adds proper existence check for showFull configuration value to prevent PHP undefined array key warning.
causing eslint errors: document.fullscreenElement() and fullscreenEnabled() is not supported in iOS Safari 15.6-15.8
Remove Superglobal and add dependency injection for SiteLanguage request and update deprecated getTwoLetterIsoCode
@fschoelzel
Copy link
Contributor Author

Regarding to this: #1320 (review)

Some general remarks in addition to the detailed comments below (as discussed in our call):

* Please remove the DevServer or make it a dev-only dependency if that's possible.

The few files to Test the Media Player have been moved to Build/Webpack/DevServer/

* Please try to merge DlfMediaPlayer, SlubMediaPlayer and lib/ as much as possible, since those are dependencies of each other anyway which won't work as stand-alone implementations.
  ** At the moment it is really difficult to tell apart what those do, where they are used and how they depend on each other.
  ** The best way of handling this would be to have just two directories: One containing the third-party Shaka player (and possibly other unchanged third-party libraries) and one containing all the Kitodo-specific code.

Because this is already hard enough to handle, i would not vote to merge this all together. The reason, why it was arranged this way is clarified in the Kitodo/Mediaplayer Documentation. One Solution to seperate it for future JS libaries in the Private/JavaScript/ folder, could be another parent folder for the Mediaplayer.
For example:

  • Resources/Private/JavaScript/DlfMediaPlayer/
    • DlfMediaPlayer or MediaPlayer
    • SlubMediaPlayer
    • lib

What do you think?

* Please add license and author information to every non-third-party file.

I added the Kitodo License Information to the Javascript files in the Resources/Public/JavaScript/DlfMediaPlayer/ folder. Does it need to be in every Private/ JS file? Which the user never see?

@fschoelzel fschoelzel marked this pull request as ready for review March 26, 2025 18:05
Copy link
Collaborator

@beatrycze-volk beatrycze-volk left a comment

Choose a reason for hiding this comment

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

After Codacy errors related to PHP files are solved, the PR could be merged. The JS Codacy errors are mostly irrelevant or false positives like missing declarations which in fact are not missing.

Copy link
Member

@sebastian-meyer sebastian-meyer left a comment

Choose a reason for hiding this comment

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

Wow, outstanding work! What an achievement, especially considering how many different developers already worked on this code!

The javascript with all its cross-dependencies is still mostly a big mess, but that's not your fault and we can fix that later. I'll merge it for now in order to finally get to a stable 6.0 release.

Again: thank you very much for this big contribution! Great job!

@sebastian-meyer sebastian-meyer merged commit ef5d445 into kitodo:main Apr 1, 2025
8 of 10 checks passed
@codecov
Copy link

codecov bot commented Apr 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (bdc95db) to head (fcab388).
Report is 1 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #1320   +/-   ##
============================
============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

↷ feature A new feature or enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants