Closed Bug 1213390 Opened 9 years ago Closed 9 years ago

The Video app should display captions, when a captions file exists along side the video file

Categories

(Firefox OS Graveyard :: Gaia::Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: djf, Assigned: djf)

References

Details

Attachments

(3 files)

The Gecko <video> element has the ability to display captions for a video using a <track> element and an independent WebVTT file. This bug is to detect when a file of captions exists for a given video and to set a <track> element to get those captions displayed.

The video app will look for a .vtt file that has the same base filename and directory as the video itself and will display those captions if they exist. Possibly it will do some localization if there are captions in more than one language. (Or maybe not: since the user has to manually copy captions to their SD card, we can probably assume that they have copied the captions in their preferred language.)

This bug will not implement any user controls for turning captions on or off. Right now the only way the user will be able to get captions is to manually copy them to the device somehow, so that is presumed to be enough signal that the captions are desired.

This bug is not going to attempt to delete captions when the corresponding video is delete.

This bug is not going to do anything to facilitate the sharing of captions. When a video is transferred via NFC, for example, it will not transfer any associated captions.
Assignee: nobody → dflanagan
I'm using the word "captions" in this bug to mean both captions and subtitles: http://screenfont.ca/learn/ (Or maybe I'm being even more vague and mean any kind of .vtt file at all.)
The sample video and subtitles here seem like they will be useful for testing: https://github.com/iandevlin/iandevlin.github.io/tree/master/mdn/video-player-with-captions
I was hoping that I'd be able to find some existing filename convention for .vtt files that embed language and type into the filename. If Android had a convention for subtitle files, for example, I'd just copy that.

If a video is named title.mp4, then the simplest thing to do is to look for title.vtt and use that.

But, assuming I'm running in en-US locale, maybe it would be better to look for track files in this order:

  title-en-us.vtt
  title-en.vtt
  title.vtt

That way, if there are captions for multiple locales, we'll get the most appropriate one.  This should probably be done with a case-insensitive compare.

We could also go a little further here if we want to distinguish captions from subtitles and look for files of the form: title-type-language.vtt or something like that. But since I'm not going to be implementing anything to let the user switch between captions and subtitles, it would be premature to try to make that distinction.
I've got a work-in-progress here: https://github.com/mozilla-b2g/gaia/compare/master...davidflanagan:bug1213390

This is displaying captions the first time I play a video. But if I then switch to another video and come back to the first, the captions no longer display.  I suspect that is a gecko bug and I need to work around it. Maybe a race condition between loading metadata and setting the track element? On subsequent plays, the metadata is already cached or something?
Depends on: 1214027
I filed bug 1214027 for the underlying gecko bug that is causing me problems. I suspect I can workaround it by creating a new <video> element each time I play a new video rather than reusing the same element each time. That's a bigger change than I wanted to make, and it might change the performance characteristics of the app.  But it actually might help the video app release memory while in the background, so maybe it will be for the best...
feature-b2g: --- → 2.2r+
It looks like bug 1214027 isn't a bug at all, and my code needs to set track.track.mode = 'showing' after adding a new track to the video element.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1214027#c12 and https://bugzilla.mozilla.org/attachment.cgi?id=8674660 from that bug.

Thanks to Gerald Squelart for figuring this out for me.

I'm still working on this, but 2.5+ bugs are taking priority right now. Since I have a WIP patch and am no unblocked, I don't think there will be any trouble finishing this up.
Comment on attachment 8680959 [details] [review]
[gaia] davidflanagan:bug1213390 > mozilla-b2g:master

This patch enables very basic vtt captions/subtitles for the video app.  If you have a video in /sdcard/movies/foo.mp4, then when you start playing it, this patch will look for a file named /sdcard/movies/foo.vtt. If it finds it, it will use that .vtt file with a <track> element to display the subtitles.

Russ,

This bug was going to be 2.2r+, but apparently it is not anymore, so we don't have to land it, but it is still a cool (if very rudimentary) and I think we should consider landing it in 2.5. It does check an important accessibility box for us.

What do you think? Does it seem safe and simple enough to land?

You can find test movies and .vtt files here: https://github.com/iandevlin/iandevlin.github.io/tree/master/mdn/video-player-with-captions

Note that it adds a new permission to the manifest so you'll have to do a make reset-gaia to test it.

Hema, I'm setting the feedback flag for you to get your input on whether we should land this simple (and safe, I think) patch before the 2.5 deadline.
Attachment #8680959 - Flags: review?(rnicoletti)
Attachment #8680959 - Flags: feedback?(hkoka)
According to the parent bug, this is no longer a 2.2r+ blocker.
feature-b2g: 2.2r+ → ---
Comment on attachment 8680959 [details] [review]
[gaia] davidflanagan:bug1213390 > mozilla-b2g:master

It looks good, and safe, to me, although I did not test it. It feels a bit rushed to land it for 2.5 but if your comfortable, I'm fine.
Attachment #8680959 - Flags: review?(rnicoletti) → review+
(In reply to David Flanagan [:djf] from comment #8)
> Comment on attachment 8680959 [details] [review]
> [gaia] davidflanagan:bug1213390 > mozilla-b2g:master
> 
> This patch enables very basic vtt captions/subtitles for the video app.  If
> you have a video in /sdcard/movies/foo.mp4, then when you start playing it,
> this patch will look for a file named /sdcard/movies/foo.vtt. If it finds
> it, it will use that .vtt file with a <track> element to display the
> subtitles.
> 
> Russ,
> 
> This bug was going to be 2.2r+, but apparently it is not anymore, so we
> don't have to land it, but it is still a cool (if very rudimentary) and I
> think we should consider landing it in 2.5. It does check an important
> accessibility box for us.
> 
> What do you think? Does it seem safe and simple enough to land?
> 
> You can find test movies and .vtt files here:
> https://github.com/iandevlin/iandevlin.github.io/tree/master/mdn/video-
> player-with-captions
> 
> Note that it adds a new permission to the manifest so you'll have to do a
> make reset-gaia to test it.
> 
> Hema, I'm setting the feedback flag for you to get your input on whether we
> should land this simple (and safe, I think) patch before the 2.5 deadline.

Can you get no-jun to test this out today and verify everything is good with this patch and then I am okay with landing it. I will NI Mahe (release manager) and Wilfred
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(npark)
Flags: needinfo?(mpotharaju)
(In reply to Russ Nicoletti [:russn] from comment #10)
> Comment on attachment 8680959 [details] [review]
> [gaia] davidflanagan:bug1213390 > mozilla-b2g:master
> 
> It looks good, and safe, to me, although I did not test it. It feels a bit
> rushed to land it for 2.5 but if your comfortable, I'm fine.

Russ, please test it! Thanks!

Hema
Flags: needinfo?(rnicoletti)
Attached image captions test
Two observations from my initial testing:

1) The captions are being displayed with the words vertical as opposed to horizontal (see attached screenshot). I don't see anything in the 'vtt' file specific to layout so I'm guessing it is a gecko issue. Or perhaps this is expected?
2) The extension of the "vtt" file is case-sensitive. I'm guessing it is standard for the "vtt" extension to be lowercase. I only mention it because the names, including the extension, produced by our camera are all uppercase. I suppose those folks who want to add captions to their videos will known that the "vtt" extension must be lowercase. Perhaps it's worth looking for ".VTT" if ".vtt" is not found?
Flags: needinfo?(rnicoletti) → needinfo?(dflanagan)
(In reply to Russ Nicoletti [:russn] from comment #13)

> 1) The captions are being displayed with the words vertical as opposed to
> horizontal (see attached screenshot). I don't see anything in the 'vtt' file
> specific to layout so I'm guessing it is a gecko issue. Or perhaps this is
> expected?

Looks like you took captions from a landscape-mode movie and added them to a portrait mode movie recorded on our phone. Those movies have a CSS rotation to get them to display correctly.  I wouldn't expect this to ever happen in real-life. Captions will be for professional media recorded in landscape mode.  So I'm not too worried about that corner case.

> 2) The extension of the "vtt" file is case-sensitive. I'm guessing it is
> standard for the "vtt" extension to be lowercase. I only mention it because
> the names, including the extension, produced by our camera are all
> uppercase. I suppose those folks who want to add captions to their videos
> will known that the "vtt" extension must be lowercase. Perhaps it's worth
> looking for ".VTT" if ".vtt" is not found?

For most of our devices, the sdcard and internal storage are VFAT and are case-insensitive, so .vtt or .VTT will work. I think that Aries internal storage is not this way, so it might be an issue in that case. But not anywhere else.

For a given movie foo.mp4, the right thing to do is to look for foo-en-US.vtt, foo-en-US.VTT, foo-en.vtt, foo-en.VTT, foo.vtt and foo.VTT and use the first one we find. But this is an undocumented feature that we're putting in just for hard-core power users out there and as a preliminary experiment with captions, I'd rather not take the performance hit that would come with doing that full search every time we play a movie.

So my preference is to keep this dead simple. I'm purposly ignoring locale for now, and given that most sdcard will be formatted with a case-insensitive filesystem, I'd say let's just ignore the case issue for this release.
Flags: needinfo?(dflanagan)
After discussing with Hema (who discussed with Wilfred) we've decided:

1) I'll add a unit test for this

2) We'll set qawanted to request verification that this works as expected

3) Once it has been verified, we'll request uplift to 2.5

Also, I should clarify the comment above about not supporting locales.

The idea here is that if the user has some movie named foo.mp4, and they want to watch it with subtitles or captions, they'll find an .vtt file for the movie (apparently there are lots of these online) and save it to their sdcard with the name foo.vtt. And with this patch it will just work. We're relying on the user to choose subtitles in the language they want and to manually copy them to the phone. The captions will work regardless of what locale the user is running in. We just rely on the user to pick subtitles in the language that they want.

In the future, if we have some kind of FirefoxOS Video Store or some kind of service for downloading movies, we might have something where movies are downloaded along with subtitles in a variety of languages. In that case, we would have to modify the video app to allow the user to turn subtitles on and off, and we would want to automatically choose the subtitles that best match the user's preferred language. 

For now, though, I'm proposing to land just the most rudimentary feature. It will help users who know how to find .vtt files and copy them to their sdcard.  Apparently this is a thing that android users do manually, so for parity, it would be nice to have the feature on FirefoxOS as well.
ktucker, 

No-jun is on pto. I spoke to pallavi and she said you may be able to help verify this before we land. 

thanks
hema
Flags: needinfo?(ktucker)
Setting qawanted to test the patch from commment 8.
Flags: needinfo?(ktucker)
Comment on attachment 8680959 [details] [review]
[gaia] davidflanagan:bug1213390 > mozilla-b2g:master

Russ,

I updated the patch with tests, and as part of that refactored the captions code into a separate captions.js file.  Do you want to review again, or can I carry your r+ forward?
Attachment #8680959 - Flags: feedback?(hkoka) → feedback?(rnicoletti)
KTucker or JMercado or whoever responds to the qawanted flag: Note that in order to get captions to display your video file and your vtt file have to be in the same directory, and have to have the same base filename.

I used a test video from here: https://github.com/iandevlin/iandevlin.github.io/tree/master/mdn/video-player-with-captions/video

I downloaded sintel-short.webm and renamed it to sintel.webm.

Then I grabbed a .vtt file from here: https://github.com/iandevlin/iandevlin.github.io/tree/master/mdn/video-player-with-captions/subtitles/vtt

I downloaded sintel-en.vtt and renamed it to sintel.vtt.

Then I pushed both sintel.webm and sintel.vtt to my sdcard and ran the video app.  

Note that if you use the regular gaia build system when testing, it is not enough to just do APP=video make install-gaia.  This patch changes app permissions, so it needs a full make reset-gaia to make the permissions take effect.
Tested on Flame central and Aries using the provided test videos/subtitles, they work fine as far as making the subtitles appear on the video.

Two problems we observed:

1) For a given file 'test.mp4', only subtitle file named 'test.vtt' will make the subtitles appear. Other file names such as 'test-en.vtt' or 'test-de.vtt' does NOT work.

2) The size for the subtitles seems a bit too small for comfortable reading. Attaching a screenshot taken from Aries.

Additionally, I observed another issue that could be a non-issue which is, I recorded a landscape video using Aries camera, and put the test subtitle along with it with the same file names, and observed that the subtitles appear upside down on the video. I assume this is not an issue that a regular user will see because somehow orientations are predetermined in movie files?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Keywords: qawanted
Hema please see comment 20 for the results of our testing.
Flags: needinfo?(jmercado) → needinfo?(hkoka)
(In reply to Pi Wei Cheng [:piwei] from comment #20)
> Created attachment 8681560 [details]
> screenshot of subtitles working (size too small)
> 
> Tested on Flame central and Aries using the provided test videos/subtitles,
> they work fine as far as making the subtitles appear on the video.
> 
> Two problems we observed:
> 
> 1) For a given file 'test.mp4', only subtitle file named 'test.vtt' will
> make the subtitles appear. Other file names such as 'test-en.vtt' or
> 'test-de.vtt' does NOT work.

That is intentional at this point.

> 2) The size for the subtitles seems a bit too small for comfortable reading.
> Attaching a screenshot taken from Aries.

I agree that the size of the text is just barely usable. That is a separate gecko issue, however. This patch just lands the basic capability of using the .vtt file if it exists. We may want to improve the way gecko displays the captions.

> Additionally, I observed another issue that could be a non-issue which is, I
> recorded a landscape video using Aries camera, and put the test subtitle
> along with it with the same file names, and observed that the subtitles
> appear upside down on the video. I assume this is not an issue that a
> regular user will see because somehow orientations are predetermined in
> movie files?

I think it is a non-issue. Movies recorded with camera phones often need to be rotated to be displayed correctly. But those movies are not typically captioned.  If this turns out to be a problem in practice, then I think the fix will be in gecko.
Okay, this has automated tests now and QA verification. I would land it now except that the gaia tree is closed.
(In reply to Jayme Mercado [:JMercado] from comment #21)
> Hema please see comment 20 for the results of our testing.

thanks for testing this in a short notice, appreciate it!

I will keep the NI on wmathanaraj for any additional input on the test findings and limitations on gecko (text size smaller and no support for locale based vtt files) for this initial landing of this feature. 

hema
Flags: needinfo?(hkoka)
Landed to master: https://github.com/mozilla-b2g/gaia/commit/b54d316dbd6494b0b2a4c1596413e8207388049a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(npark)
Thanks for the NI Hema and the automated tests.

Will ensure this lands on 2.5.
Flags: needinfo?(mpotharaju)
Attachment #8680959 - Flags: feedback?(rnicoletti)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: