1328966 - VTT files with CR line endings do not parse

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Brad Dougherty]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/602.3.12 (KHTML, like Gecko) Version/10.0.2 Safari/602.3.12

Steps to reproduce:

Try to play a video with subtitles from a VTT file that has CR line endings. See the Spanish subtitles on this video: https://vimeo.com/198237283/5f5f685091


Actual results:

The subtitles don't show. You can see in the network inspector that the file was properly loaded and if you inspect the <track> you will see that the track.cues CueList is empty.


Expected results:

The subtitles should show.

Comment 1

[Maire Reavy [:mreavy] Plz needinfo]

Hi Ralph -- Can you take a look at this?  Can you reproduce?  If so, can you take this?  Thanks!

Comment 2

[Chris Pearce [:cpearce (GMT+13)]]

Blake: Is there someone on your team that can look into this?

Comment 3

[Ralph Giles (:rillian) | needinfo me]

I was able to look at this a bit too. The spec has been updated to allow all three line ending variations, so we need to modify vtt.jsm. I have a potential started working on tests, but haven't verified. If someone else wants to take over, feel free. Otherwise I'll continue tomorrow.

Comment 4

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #2)
> Blake: Is there someone on your team that can look into this?
Yeah. I have cc-ed Benjamin in this bug. But since Ralph has some ideas already, it would be better to have him finish it continuously.  

Ralph, 
Thanks!

Comment 5

[Ralph Giles (:rillian) | needinfo me]

Attachment: Bug 1328966 - Accept \r as a valid line ending.

See https://w3c.github.io/webvtt/#webvtt-line-terminator for
allowed values.

Review commit: https://reviewboard.mozilla.org/r/104654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104654/

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8826776 [details]
Bug 1328966 - Accept \r as a valid line ending.

https://reviewboard.mozilla.org/r/104654/#review105560

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7f25749589e
Accept \r as a valid line ending. r=alwu

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b7f25749589e

Comment 9

[Chris Peterson [:cpeterson]]

Ralph, can we uplift this VTT fix to Aurora 52 since it fixes a web compat issue? It's probably too late to uplift to Beta 51, which will be released next week.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

I guess 52 is affected too.

Comment 11

[Ralph Giles (:rillian) | needinfo me]

Comment on attachment 8826776 [details]
Bug 1328966 - Accept \r as a valid line ending.

I think uplift is a good idea.

Approval Request Comment
[Feature/Bug causing the regression]: WebVTT subtitles
[User impact if declined]: webcompat issue: Some video subtitles will not display.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes

[Needs manual test from QE? If yes, steps to reproduce]: Would be good to confirm. Visit https://vimeo.com/198237283/5f5f685091, press play, and select Spanish (Español) subtitles from the 'CC' player control. Confirm subtitles appear starting 5 seconds into the video.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: No.
[Why is the change risky/not risky?]:  It is a single-line change which should not affect parsing of files we handled without the change. If it causes problems, it is easy to revert.

[String changes made/needed]: None

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8826776 [details]
Bug 1328966 - Accept \r as a valid line ending.

OK, let's take it then, thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/811c33f33507