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 Captions Support #4189

Closed
bmattb opened this issue Aug 24, 2020 · 43 comments · Fixed by #4615
Closed

Video Captions Support #4189

bmattb opened this issue Aug 24, 2020 · 43 comments · Fixed by #4615
Labels
Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). Elements: Video Group: Media Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar P1 High priority, must do soon Type: Enhancement New feature or improvement of an existing feature

Comments

@bmattb
Copy link

bmattb commented Aug 24, 2020

Feature Description

As part of expected functionality the editor should allow for the ability of the user to associate video subtitle files (VTT files) with video media assets.

Product Brief

Figma

Alternatives Considered

Additional Context


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance Criteria

Implementation Brief

@bmattb bmattb added Elements: Video Group: Media Type: Enhancement New feature or improvement of an existing feature labels Aug 24, 2020
@bmattb
Copy link
Author

bmattb commented Aug 24, 2020

@o-fernandez to detail the requirements and release for this please.

@swissspidy swissspidy added the Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). label Aug 31, 2020
@o-fernandez o-fernandez added the Status: Needs More Info Follow-up required in order to be actionable. label Sep 2, 2020
@o-fernandez
Copy link
Contributor

To keep this very simple, let's do the following:

  • When selecting a video, show a new field within Design -> Accessibility that is "captions"
  • This field should have a button/affordance for the user to start an upload, which would allow the user to select a file from their local files and upload it.
  • This captions file is then associated with the video and captions load when the video is playing, use caption/subtitle support in 🐛Propagate <track> elements in amp-story ampproject/amphtml#13751

Can @choumx @barklund @pbakaus @samitron7 please review this and see if it makes sense. Any other details you need me to spec out? I can find time to do it, but don't want to block on me or UX if eng can make progress without us.

@o-fernandez o-fernandez added Status: Brief In Review and removed Status: Needs More Info Follow-up required in order to be actionable. labels Sep 15, 2020
@spacedmonkey
Copy link
Contributor

There are two place that the caption upload button should be added.

In the design panel here under the design panel under accessibility

Screenshot 2020-09-15 at 17 42 05

There is also the edit media dialog.

Screenshot 2020-09-15 at 17 42 19

This video widget found in WordPress, has an option for caption uploads looks like

Screenshot 2020-09-15 at 17 44 12

Screenshot 2020-09-15 at 17 44 20

The upload button should open the WordPress uploader and be limited to only .vtt files. Once selected, users should be able to remove / replace caption files, if wrong file was selected.

@samitron7
Copy link

samitron7 commented Sep 15, 2020

@o-fernandez This approach looks good to me. As for where we should add the cta

  1. Add add caption button to the accessibility section.
  2. Clicking on it to trigger local upload.
  3. Once uploaded, we show the file name with the option to remove.
  4. Remove will take the users to the "Add video caption" state.

Screen Shot 2020-09-21 at 11 31 44 AM

@spacedmonkey How did you get to the video widget? I can't seem to trigger that on a regular post.

@spacedmonkey
Copy link
Contributor

@samitron7

Step to get that view.

  1. Hover over appearance on left hand sidebar. Click on widget.
  2. Click on Video. Insert into a widget area.
  3. Upload a video (mp4 file)
  4. Then click edit video.

Screenshot 2020-09-15 at 19 12 24

@o-fernandez o-fernandez added this to the Sprint 38 milestone Sep 21, 2020
@o-fernandez

This comment has been minimized.

@o-fernandez o-fernandez added P0 Critical, drop everything and removed Status: Brief In Review labels Sep 21, 2020
@samitron7
Copy link

@spacedmonkey I made edits to the mocks to show a separate area for captions. I need to create a more consistent pattern for features where you can either delete/replace but this should work for now if we need to get this out today.

@spacedmonkey
Copy link
Contributor

spacedmonkey commented Sep 21, 2020

I did some more research into subtitles and it’s more complex than I originally thought. For one, even gutenberg does not support uploading subtitles files at this time. See WordPress/gutenberg#7673 . More importantly, WordPress itself doesn’t not support uploading .vtt files ( or even srt ) which is the subtitle file format. This changes the scope of the work but a lot and needs more thought than I think we have time for right now. I think the video widget example ( I screenshoted in the ticket ) is a weird outlier in WordPress, as I was unable to get it to work in my testing. As WordPress itself doesn’t support this, I think ( as sad as it makes me ) that we should bump to 1.1. If it is not part of the existing WordPress experience, it becomes a nice to have IMO.

Screenshot 2020-09-21 at 20 11 09

@spacedmonkey
Copy link
Contributor

While diving into this I noticed that track tags have a number of required attributes, see the docs.

Fields I would flag,

  • srclang which is a language code. This required specially if the subtitles are not in english. I believe we would need a dropdown of country codes. For users to select.
  • label this is a user defined label for the subtitles.
    Optional
  • kind Type of captions uploaded, we would default to subtitles.

We would need fields to be able to input these values in the design panel.

I have a WIP PR #4615 of what I have done so far. It need tests and more design work.

@o-fernandez o-fernandez added P1 High priority, must do soon and removed P0 Critical, drop everything labels Sep 22, 2020
@bmattb
Copy link
Author

bmattb commented Sep 23, 2020

@o-fernandez / @samitron7 this requires further clarification before it can be actioned.

@spacedmonkey
Copy link
Contributor

I think that we should do as Omar says,

Deutsch (German). When a user searches, it search both native and english name. I think it is to hard to maintain a list of translations for all languages, so German in Spanish, Spanish in German etc. WordPress.org, as 201 languages, that is 201 x 201 to be translated. I think if a user does not know the native language, english is a good default, I would say,

@spacedmonkey
Copy link
Contributor

I have been thinking about this data structure.

{
  "tracks": [
    {
      "id": 123,
      "src": "https://exampe.com/file.vtt",
      "kind": "captions",
      "lang": "en"
    }
  ]
}

As we have a repeatable field to add an many track files as you want, it could be possible to add the same file twice. So each object will need a unique id to reference. I will take a look into how pages have their page id generated.

@swissspidy swissspidy changed the title Ensure that video media files support the ability to associate and support video subtitles. Video Captions / Subtitles Support Oct 12, 2020
@swissspidy
Copy link
Collaborator

  1. From the design, there is an uploading progress meter. However in my POC PR, I use the WordPress media dialog filtered to ttf files. Is this acceptable?

@o-fernandez @samitron7 Thoughts on this? Perhaps that's OK for a first iteration, but we then customize it in a next step?

(Aside: I assume you meant vtt files)


Overall it we've uncovered quite a few unknowns which, as Jonny pointed out, increases complexity. Feels like we might not be able to get this done for the next release.

@spacedmonkey could you perhaps summarize your current findings and work a bit? What are your thoughts?

@o-fernandez
Copy link
Contributor

Assuming it's vtt files, then yes, all else sounds OK as a starting point. Will wait for @spacedmonkey's update with the summary.

@spacedmonkey
Copy link
Contributor

I am still working on the PR. I will update at the end of the day.

@spacedmonkey
Copy link
Contributor

I have something working locally. It needs styling and tests to be added, but it functionally working.

I just want to confirm with @samitron7 @o-fernandez that the UX ( please ignore the styling for now ). Here is a video of the UX. https://www.youtube.com/watch?v=5wtRyFt3UjU&feature=youtu.be

The important thing to note here is the use of WordPress media picker here. This picker is limited to VTT files and will not let you pick anything but these files.

Before this can be merged, we need to work out the list of languages and how we are going to populate this date. I think there need to a fixed list for V1. But you will also need to add the users current language. For example if the fix list is german, english, french and Italian, the user should be able to pick hebrew as the language.

@swissspidy Have you had any thoughts on the list of languages / selecting the current language.

If you want to play with my PR, it can be found here. #4615

@o-fernandez
Copy link
Contributor

I'll let @samitron7 comment on the UX, I think this is acceptable with some refinements as p1/p2 to do later IMO.

Re: the list of languages, seems like we need to allow for any of these? https://r12a.github.io/app-subtags/
There's quite a few of them, so I think that the language picker that is similar to the font picker (with an input field to filter the list) seems important. The drop-down can just have the most popular language codes as the first ones shown if the user doesn't search for anything.

English (en)
Chinese (zh)
Hindi (hi)
Spanish (es)
French (fr)
Arabic (ar)
Bengali (bn)
Russian (ru)
Portuguese (pt)
Indonesian (id)

We can have more, but I think that the user should be able to easily type a language (e.g., Italian) and get the right code. Then, with the "recently used" section of the picker they could see that next time they are adding a caption (if we can easily implement that part of the picker).

@bmattb
Copy link
Author

bmattb commented Oct 14, 2020

recommendation for MVP (this ticket) - use the WP language list of ~100 languages with no preselected option
Future (1.2?) - use a list similar to fonts to provide auto-complete function

@spacedmonkey
Copy link
Contributor

As agreed, I have implemented the language dropdown with all WordPress languages. The dropdown is extremely long and not specially usable. Is this okay for an MPV - https://youtu.be/OQFlz0izD_E ?

@bmattb
Copy link
Author

bmattb commented Oct 14, 2020

@o-fernandez - in backlog grooming it was flagged that this MVP version of this will not be completed/ready for end of S39 and will take additional time in S40 to complete, however INFRA pod feels that they could prioritize this for the next week and still have it included in scope for v1.1

@barklund
Copy link
Contributor

Note that both search and recently used items require that #4749 is implemented first. I have prioritized this for the next sprint, so @miina or I might even get to that this week.

The generalization of the font picker can happen without the API for users present in that ticket to be completed in two separate PRs.

@spacedmonkey
Copy link
Contributor

spacedmonkey commented Oct 15, 2020

I feel like the infra pod has done it's work here and this should now be handed off to the editor pod, to finish off.
I think we should merge what we have now behind a feature flag ( after code review is complete ). Then create another ticket that would be style the inputs, add some more tests and implement #4749. Thoughts?

Update: ticket created for styling #4899

@swissspidy
Copy link
Collaborator

As you mentioned there's #4899 to hand off styling to Prometheus/Pea pod. I'll update that ticket with details shortly.

If you can add the feature flag to #4615 that's fine with me. Then we can code-review & merge it.

The update on #4749 is promising. I think it warrants a new ticket to switch dropdown implementation for captions, I'll create that ticket.

@o-fernandez
Copy link
Contributor

As agreed, I have implemented the language dropdown with all WordPress languages. The dropdown is extremely long and not specially usable. Is this okay for an MPV - https://youtu.be/OQFlz0izD_E ?

I would be OK with this going out in 1.1 as it is, and prioritize the styling (#4899) for 1.2. In fact, if we can do a small release between 1.1 and 1.2 that includes #4899 all the better. @choumx @swissspidy

@swissspidy
Copy link
Collaborator

No need to overcomplicate things. Adding tests and improving styling (without autoomplete dropdown) shouldn't take more than a few hours

@spacedmonkey
Copy link
Contributor

I have made some discoveries while finalizing the code.

  • The AMP stories format does not support track tag with type of subtitles. This is because video controls are disabled there is no way for a user to enable or change language, as the controls are not there to enable this.
  • The AMP stories format does support captions ( see example ), but only one set of captions. Again, because the user can not select a different language, only one track is supported.
  • Captions should be enabled on video backgrounds.

Some thoughts

  • As there can only be one caption file, we should not allow them to upload multiples. Once user uploads one caption file then add more button should be removed / disabled.
  • As there is only one caption file, we could remove the language option for now. Language is only required for tracks defined as subtitles. We can default the language to the users language for now. Captions are text of the spoken words, in the language they are spoken. It is likely that users language will match the video's language.
  • All references to subtitles should be removed from designs and code, as it is just captions for now.
  • Design may need to be updated if only on caption file is allowed.

@o-fernandez Thoughts. The above sound reasonable?

@spacedmonkey
Copy link
Contributor

Example of subtitles in amp playground.

@swissspidy swissspidy changed the title Video Captions / Subtitles Support Video Captions Support Oct 16, 2020
@swissspidy
Copy link
Collaborator

The AMP stories format does not support track tag with type of subtitles. This is because video controls are disabled there is no way for a user to enable or change language, as the controls are not there to enable this.

Not 100% accurate. Subtitles do work, at least they did for me in AMP playground. The default track always works, even with subtitles:

<track default srclang="en" label="English" kind="subtitles" src="https://example.com/sample.vtt"></track>

But given the fact that captions are P1 and subtitles are P2, I agree with the suggestion above— just going with captions for now to keep things simple (this drastically reduces complexity!)

So the next steps are:

  • Remove languages logic
  • Default to captions
  • Remove UI to add multiple tracks*

* element.tracks should still be an array, in case we need it in the future

After that, we can improve styling in #4899

@spacedmonkey
Copy link
Contributor

This ticket is ready for code review. Now the ticket is much more simple, I think we can get this merge and tested for 1.1.

@bmattb bmattb modified the milestones: Sprint 39, Sprint 40 Oct 19, 2020
@csossi
Copy link

csossi commented Oct 23, 2020

Verified in QA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). Elements: Video Group: Media Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar P1 High priority, must do soon Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants