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

Captions #44

Closed
wants to merge 5 commits into from
Closed

Captions #44

wants to merge 5 commits into from

Conversation

elizoller
Copy link
Member

@elizoller elizoller commented Jan 11, 2021

GitHub Issue:
Islandora/documentation#1305
Islandora/documentation#1003
https://www.drupal.org/project/drupal/issues/3057144
https://www.drupal.org/project/drupal/issues/3056714

What does this Pull Request do?

Adds a caption field for audio and video files and allows them to played in the HTML5 player

What's new?

  • adds a caption file field for VTT files to the audio and video media types
  • displays that caption field on the form for audio and video
  • adds field formatters for the audio and video file fields that pulls in the captions file if it exists
  • changes the view modes for audio and video to use the new field formatter by default
  • adds templates for the HTML5 player with captions
  • allows adding the thumbnail for a video as the "poster" in the player

How should this be tested?

  • spin up an islandora site
  • pull in this islandora_defaults PR
  • do a partial config import on the config/install directory of islandora_defaults
  • cache clear
  • add an audio and/or video node
  • add an audio and/or video original file media attached to that node
  • when the service file derivative is created, add a VTT caption file to that service file media in the new captions field
  • view the node - in the menu on the HTML5 player, you should be able to toggle on and off captions
  • if it was a video and it got a thumbnail derivative - it should display as the "poster" before playing the video in the player

For testing I used the following video and captions: https://github.com/iandevlin/iandevlin.github.io/blob/master/mdn/video-player-with-captions/video/sintel-short.mp4 and https://github.com/iandevlin/iandevlin.github.io/blob/master/mdn/video-player-with-captions/subtitles/vtt/sintel-en.vtt

Additional Notes:

  • should the captions should live on the original file or the service file? - ultimately it depends on which you're displaying to the user. islandora_defaults ships with the service file being displayed so that's why my instructions say to add it there
  • is this where this stuff should go? or should it be part of islandora_audio and islandora_video in islandora/islandora? or split up somehow into FieldFormatters there and config here?

Interested parties

@Islandora/8-x-committers
@Natkeeran

@seth-shaw-unlv
Copy link
Contributor

I'm split. Normally I would say move the code to islandora and leave the config here in defaults.... however, the code is hard-coding some assumptions about config which makes me less inclined to separate them. So, I'm inclined to keep it here as it is until someone wants to put in the effort to make those hard-coded bits configurable. I wouldn't delay this PR on those grounds. I would like a second opinion before we merge.

On an unrelated note, there are some errant UUID properties in a few of the configs that need to be removed.

I haven't spun it up yet, hopefully soon.

@seth-shaw-unlv
Copy link
Contributor

The video captions work as advertised. Although, it would be nice to add multiple caption files (for multiple-language support), although I guess that would be getting into a custom field that can store the language code unless we enforce a naming convention and pull the language code from that.

@elizoller
Copy link
Member Author

Yeah, that's a good point - maybe something to bring up at the tech call

@elizoller
Copy link
Member Author

Ok, so i've been looking at this in more detail. https://www.drupal.org/files/issues/2019-10-07/3056714-16.patch
One question i have is that there are changes to the FileMediaFormatterBase which is then extended in FileVideoFormatter and FileAudioFormatter. without modifying the ones in core, i could extend off of the core FileMediaFormatterBase or do another version of it with a new name and then do new FileVideoFormatter and FileAudioFormatters off of that new base. (or i could just write the changes from the patch into each new FileVideoFormatter and FIleAudioFormatter without writing a new base).
but whichever way around that is the best way to go, we need new FieldFormatters

the rest of the patch seems like we could just pull it in and then wire up the configuration with the new formatters and such

so i'm looking for thoughts on the community preferred way to create updated FieldFormatters.
thanks

@elizoller
Copy link
Member Author

@seth-shaw-unlv any thoughts on this?

@seth-shaw-unlv
Copy link
Contributor

I don't have any strong feelings between making a new base or just pulling in those base changes to the new formatters. That said, phpcpd might not like the latter option, so perhaps a new base would avoid the most trouble.

This was referenced Mar 9, 2021
@elizoller elizoller closed this Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants