Add subtitle/transcript capability

[bondjimbond]

JIRA Ticket: https://jira.lyrasis.org/browse/ISLANDORA-2531

What does this Pull Request do?

Allows Islandora Videojs to display subtitles if they exist on a given audio/video object.

What's new?

If a datastream containing a WebVTT file exists on the object being viewed, if the DSID matches the Islandora Videojs configuration, the player will display them.

How should this be tested?

• Check out this branch
• Add a new datastream to a video/audio object with the dsid TRANSCRIPT
• Upload a WebVTT file (doesn't have to match the actual video)
• View the object; subtitles should display.

Sample WebVTT file (save as file.vtt):

WEBVTT

00:00:00.500 --> 00:00:02.000
Here is some text

00:00:02.500 --> 00:00:04.300
and here is a bit more text.

Additional Notes:

The meat of this code was originally written by @bryjbrown for use at FSU; I only adapted it to make it a general-use PR.

Interested parties

@bryjbrown @whikloj @Islandora/7-x-1-x-committers

[Natkeeran]

@bondjimbond
Does this require changes to the video content model? I assume it will work without those changes; however, we may need to keep it consistent.
https://github.com/Islandora/islandora_solution_pack_video/blob/7.x/xml/islandora_video_ds_composite_model.xml

[bondjimbond]

@Natkeeran It does not require changes, but it would be good to update the Video CModel to make TRANSCRIPT an optional datastream and provide for it. However, this PR should be merged first.

[bondjimbond]

@adam-vessey @Natkeeran Given my responses above, does this meet standards for approval?

[bondjimbond]

@jordandukart @Natkeeran I'll take a closer look at the Video CModel (and also Audio) after this is merged.

[adam-vessey]

Really, just the access check stuff is necessary to address, I think... the other bits would just be nice to have.

[bondjimbond]

@adam-vessey XACML stuff is sorted out. If you can live with the other bit, I'm happy to let you live with it, because I don't fully understand your suggestion.

[bondjimbond]

@adam-vessey Ready to merge?

[adam-vessey]

@bondjimbond: In my mind, sure, but there was also input from others here... is it jumping the gun to merge without resolving it (or at least hearing from them)? Jordan's and Nat's comments?

[bondjimbond]

@adam-vessey I've responded to them with my comments, they never replied. If they chose not to reply, I think we're free to move forward.

[bondjimbond]

(Given there has been upward of 16 days to respond to my responses)

[DiegoPino]

Sorry for chiming in (trying to help a developer friend out here) but were not the rules 24 hours after an approval? Also, since this does not impose a condition on having the CMODEL XML definition changed, but uses DSIDs ,IF present (and CMODEL also being on another module) I personally feel approval is correct and this can be merged. I was not part of the code review but the pull is good. Thanks!

[bondjimbond]

@DiegoPino is correct - our rule, technically, is that after approval there are 24 hours open for comment before the PR can be merged. After 24 hours of no comments, community assent is presumed.

Given that this much time that has passed since both the comments and responses as well as the approval, I don't see a reason to wait any longer.

[adam-vessey]

https://github.com/Islandora/islandora/wiki/Islandora-Committers-Workflow#merging-workflow , part 15:

If there are no outstanding concerns about the pull request, and the code has been sufficiently tested, and interested parties have been given sufficient time (24 hour window minimum) to respond to the pull request, then any Committer who feels confident with the code change (excluding those who would be considered ‘internal’ by the definition above) may merge the pull request.

... there were outstanding concerns, and I've not tested it (and don't presently have the capacity to do so). Perhaps I was wrong in adding my approval and not just dismissing my stale review?

[bondjimbond]

I saw no concerns with the code, besides yours, which were addressed -- the only questions outstanding are about other modules. This improvement doesn't actually touch those other modules - it only adds the capability to work with datastreams attached to objects if the DSID exists.

I believe we're fine to move forward on this with the goal of eventually making the TRANSCRIPT datastream "official" on those other modules -- but it's not required to do so right now, especially since the DSID here is configurable.

It is very hard to even generate discussion on improvements to I7 modules - questions about modules that are not materially affected by this change should not impact whether or not the improvement should happen.

[dannylamb]

Alright, let's pull this in. It's been met with approval, albeit limited in some cases, and 24 hrs are way over. I'll be the bad (good?) guy here and merge this. If explosions occur, I'll gladly do the legwork to revert.