Skip to content

Add captioning support for video/audio files#1967

Merged
tdonohue merged 5 commits intoDSpace:mainfrom
yingjin:DA-8586
Jan 10, 2023
Merged

Add captioning support for video/audio files#1967
tdonohue merged 5 commits intoDSpace:mainfrom
yingjin:DA-8586

Conversation

@yingjin
Copy link
Copy Markdown
Contributor

@yingjin yingjin commented Nov 16, 2022 •
edited by tdonohue
Loading

References

Add references/links to any related issues or PRs. These may include:

Description

Add captioning support for video/audio files. Only front end angular is changed.

Instructions for Reviewers

Please add a more detailed description of the changes made by your PR. At a minimum, providing a bulleted list of changes in your PR is helpful to reviewers.

List of changes in this PR:

  • Add language helper contains a const object mapping of two letter with their Full language name
  • Add a capInfos class to save related info to captioning
  • Updated several .ts and .html files

Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.

Have an item with media bitstreams, either video/audio files, and their corresponding .vtt file named following this pattern -

the video file "greenteam.mp4" media should have a .vtt file named "greenteam.mp4-en.vtt". -en is the two letter language code and will be translated to full language name using language helper. You can have multiple .vtt files associated with it if there are multiple languages.

Refer language code here or language-helper.ts file.
https://www.w3schools.com/tags/ref_language_codes.asp

You also need to turn on the media viewer in config.yml, add new format at registries -> format. Name: WEB VTT, minetype: text/vtt and suffix: vtt.

After this, you could ingest the items with media items and vtt files and test it in media viewer. It should show you the caption off when click three dot at the right bottom corner of the media player. Choose caption and you should see a list of the language(s) listed. Choose one and test it.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • [ x ] My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@tdonohue tdonohue added improvement component: viewer Related to media viewers / inline streaming of content labels Nov 17, 2022
@tdonohue tdonohue changed the title add captioning support for video/audio files - ticket #8586 Add captioning support for video/audio files Nov 17, 2022
@jtimal
Copy link
Copy Markdown
Contributor

jtimal commented Nov 22, 2022

Hi @yingjin

I checked the operation and it works very well and I can change languages and everything is excellent.
image

@tdonohue tdonohue requested review from atarix83 and tdonohue December 1, 2022 15:37
tdonohue
tdonohue approved these changes Dec 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@yingjin : Thanks for this improvement! I gave this a test and was able to successfully get captioning working on a video using a *.vtt file.

The only thing that tripped me up was the naming convention...I had a video named test.mp4 and I originally named the VTT test-en.vtt only to realize it should be named test.mp4-en.vtt. That's just my own mistake, but we may want to make sure to write some clear documentation on this naming convention.

All that said, I have a few minor comments below on things to clean up in the code. I also think we should create a small PR for the backend to add VTT to our DSpace Format Registry -- otherwise, I'd worry someone will forget that step and find captioning doesn't work.

Comment thread src/app/item-page/media-viewer/media-viewer-video/caption-info.ts
Comment thread src/app/item-page/media-viewer/media-viewer-video/media-viewer-video.component.html Outdated
controls
></video>
>
<!-- <track default kind="captions" src="./assets/vtt/JusticeAndTransformation_caption.mp4-en.vtt" label="English" type="text/vtt"/>-->
Copy link
Copy Markdown
Member

@tdonohue tdonohue Dec 14, 2022

Choose a reason for hiding this comment

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

Small thing, could we remove this commented out code?

Comment thread src/app/item-page/media-viewer/media-viewer-video/media-viewer-video.component.ts Outdated
let filteredCapMedias: MediaViewerItem[];
let capInfos: CaptionInfo[] = [];
filteredCapMedias = this.medias
.filter((media) => media.minetype === 'text/vtt')
Copy link
Copy Markdown
Member

@tdonohue tdonohue Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We may want to add the 'text/vtt' mime type to the DSpace registry by default (this would require a separate backend PR). That way it's easier for others to reuse this code. For example, if someone forgets to add this mimeType & then uploads a VTT file, captioning won't work.

(If you could create a PR for this, that'd be great. If you don't have time to do so though, then I could always create this small PR as a compliment to this PR)

Comment thread src/app/item-page/media-viewer/media-viewer-video/media-viewer-video.component.ts Outdated
filteredCapMedias
.forEach((media, index) => {
let srclang: string = media.bitstream.name.slice(-6, -4).toLowerCase();
//capInfos[index] = new CaptionInfo(
Copy link
Copy Markdown
Member

@tdonohue tdonohue Dec 14, 2022

Choose a reason for hiding this comment

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

Again, it'd be best to remove any commented out code.

atarix83
atarix83 requested changes Dec 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

thanks @yingjin for this PR

the code looks good, but i can't find a way to test it. I've followed instructions but the captions didn't appear.
I've created an item in the demo here https://demo7.dspace.org/items/0220cb00-3eb9-467d-916c-2ab588caa9b7

could you tell me what is wrong ?

moreover while i was trying to enabled the media viewer i found a bug that is not related to this PR, but it'd be nice if you can fix.
Basically if you enable the media viewer only for video the media player is not shown

mediaViewer:
  image: false
  video: true

It's enough to fix the following lines
https://github.com/yingjin/dspace-angular/blob/DA-8586/src/app/item-page/simple/item-types/publication/publication.component.html#L27
https://github.com/yingjin/dspace-angular/blob/DA-8586/src/app/item-page/simple/item-types/untyped-item/untyped-item.component.html#L27

@yingjin
Copy link
Copy Markdown
Contributor Author

yingjin commented Dec 15, 2022 via email

@yingjin
Copy link
Copy Markdown
Contributor Author

yingjin commented Dec 15, 2022 via email

@yingjin
Copy link
Copy Markdown
Contributor Author

yingjin commented Dec 15, 2022 via email

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Dec 15, 2022

@yingjin : If you wanted to fix the image viewer issue in this PR, it's OK with me. It's not required for this PR, but you can do it if you want.

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Dec 16, 2022 •
edited
Loading

@atarix83 : I retested this PR with your example on the demo site & it works fine for me. I suspect maybe you forgot to turn on the captioning in the video viewer? By default, captioning is always off... but if you click the three-dot icon (lower right) you'll see the "Captions" menu (see screenshot). Click it and captioning will be enabled.

screenshot

So, with this PR, enabling captioning involves two main steps...

  1. First, obviously you have to have the video viewer enabled (and the image viewer cause of that bug you found):
mediaViewer:
  image: true
  video: true
  1. Then, in the video viewer, you have to turn on captioning by clicking that "Captions" menu.

@tdonohue tdonohue requested a review from atarix83 December 16, 2022 17:06
@tdonohue tdonohue added this to the 7.5 milestone Dec 16, 2022
@yingjin
Copy link
Copy Markdown
Contributor Author

yingjin commented Dec 17, 2022

As @atarix83 suggested,
https://github.com/yingjin/dspace-angular/blob/DA-8586/src/app/item-page/simple/item-types/publication/publication.component.html#L27
https://github.com/yingjin/dspace-angular/blob/DA-8586/src/app/item-page/simple/item-types/untyped-item/untyped-item.component.html#L27

And setup
mediaViewer:
image: false
video: true

Here are the result after changing the line 27,
from
<ng-container *ngIf="mediaViewer.image">
to
<ng-container *ngIf="mediaViewer.video">

Screen Shot 2022-12-16 at 5 59 51 PM

Screen Shot 2022-12-16 at 5 58 43 PM

Is that expected behavior?

Ying

@tdonohue tdonohue self-requested a review December 19, 2022 17:29
tdonohue
tdonohue reviewed Dec 20, 2022
View reviewed changes
Comment thread src/app/core/shared/media-viewer-item.model.ts Outdated
@yingjin
Copy link
Copy Markdown
Contributor Author

yingjin commented Jan 5, 2023 via email

tdonohue
tdonohue approved these changes Jan 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 Thanks @yingjin . Tested this again today and it works as described.

Looking closer, I think the bug regarding having to enable image in order to get video to work is not going to be simple to fix. So, I think it needs to be treated as a separate bug ticket. Based on how the media.viewer.component.html is written, it appears to assume that you can only enable video if image is also enabled. I think it would require some refactoring to support something like:

mediaViewer:
  image: false
  video: true

So, I'll create a separate ticket for this bug, as it's not directly related to your work here. Separate ticket created at #2035

@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Jan 10, 2023
@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Jan 10, 2023 •
edited
Loading

Merging with +1 approval for 7.5. I've moved the outstanding bug to #2035 . I've also added some basic documentation for this new captioning feature to https://wiki.lyrasis.org/display/DSDOC7x/User+Interface+Configuration#UserInterfaceConfiguration-MediaViewerSettings

Thanks again @yingjin !

@tdonohue tdonohue merged commit 6c63af3 into DSpace:main Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tdonohue tdonohue tdonohue approved these changes

@atarix83 atarix83 Awaiting requested review from atarix83

Assignees

@yingjin yingjin

Labels

1 APPROVAL pull request only requires a single approval to merge component: viewer Related to media viewers / inline streaming of content improvement

Projects

No open projects
DSpace 7.5 release
Status: ✅ Done

Milestone

7.5

Development

Successfully merging this pull request may close these issues.

Video/Audio file Captioning support

4 participants

@yingjin @jtimal @tdonohue @atarix83