881976 - [webvtt] Implement TextTrackCue::ComputedLinePosition()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

TextTrackCue also has a computed line position. The biggest user of this is the processing model.

http://dev.w3.org/html5/webvtt/#dfn-text-track-cue-computed-line-position

Comment 1

[Caitlin Potter (:caitp)]

It's not clear that it makes sense to stick this in TextTrackCue, but for now I don't have a problem with it really

Comment 2

[Rick Eyre (:reyre)]

I'm thinking it should just be a method on the TextTrackCue class. That seems to be what the WEBVTT data model spec is describing.

Comment 3

[Rick Eyre (:reyre)]

We won't need to implement this in C++ since we're planning to do the processing model in the JS WebVTT parser. See https://github.com/andreasgal/vtt.js/issues/86 for updates.

We should leave this open until the relevant code is landed in the JS parser.

Comment 4

[Marcus Saad (:msaad)]

Rick, according to https://github.com/mozilla/vtt.js/issues/86, can we mark this as fixed ?

Comment 5

[Rick Eyre (:reyre)]

Nope not yet. This is a part of a portion of the processing model that I haven't finished yet (the overlap avoidance part). For some reason I got that link wrong (oops) it should actually be https://github.com/mozilla/vtt.js/issues/103.

Comment 6

[Rick Eyre (:reyre)]

Attachment: Part 1 v1: Expose TextTrack::TextTrackList to Chrome JS r=rillian,bz

Comment 7

[Rick Eyre (:reyre)]

Attachment: Part 2 v1: Expose TextTrackList's MediaElement to chrome JS. r=rillian,bz

Comment 8

[Rick Eyre (:reyre)]

Attachment: Part 3 v1: Refactor TextTrack classes to store references to eachother in a way that makes more sense. r=rillian

Comment 9

[Rick Eyre (:reyre)]

There's one more patch after this which is currently going through the process of landing in vtt.js. See https://github.com/mozilla/vtt.js/pull/228.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8366719 [details] [diff] [review]
Part 1 v1: Expose TextTrack::TextTrackList to Chrome JS r=rillian,bz

Review of attachment 8366719 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me. Please document _why_ a change is being made in the body of your commit message though.

::: content/media/TextTrack.cpp
@@ +205,5 @@
>  
> +TextTrackList*
> +TextTrack::GetTextTrackList()
> +{
> +  return mTextTrackList.get() ? mTextTrackList : nullptr;

Wouldn't an unitialized mTrextTackList already return nullptr here?

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8366721 [details] [diff] [review]
Part 3 v1: Refactor TextTrack classes to store references to eachother in a way that makes more sense. r=rillian

Review of attachment 8366721 [details] [diff] [review]:
-----------------------------------------------------------------

This is just looking up the parent media element, if any, via the new TextTrackList reference instead of holding a direct reference? I'd shorten the commit title to 'Part 3: Refactor TextTrack class references.' and put the rest in the body.

Comment 12

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #10)

> Wouldn't an unitialized mTrextTackList already return nullptr here?

Yes, I think so. I was over thinking it with the smart pointer. I'll just return it directly. I can change the other couple places I do this in the patch as well.

Comment 13

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #11)

> This is just looking up the parent media element, if any, via the new
> TextTrackList reference instead of holding a direct reference?

Yes. I think it's better to do it this way since a TextTrack will never belong directly to a MediaElement, only through a TextTrackList will it. It also simplifies getting the computed position a little as well.

> I'd shorten the commit title to 'Part 3: Refactor TextTrack class references.' and put
> the rest in the body.

Okay I'll do that. Thanks for review :).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8366719 [details] [diff] [review]
Part 1 v1: Expose TextTrack::TextTrackList to Chrome JS r=rillian,bz

>+  return mTextTrackList.get() ? mTextTrackList : nullptr;

Just: "return mTextTrackList;"

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8366720 [details] [diff] [review]
Part 2 v1: Expose TextTrackList's MediaElement to chrome JS. r=rillian,bz

>+  return mMediaElement.get() ? mMediaElement : nullptr;

  return mMediaElement;

r=me

Comment 16

[Rick Eyre (:reyre)]

Attachment: Part 1 v2: Expose TextTrack::TextTrackList to Chrome JS r=rillian,bz

Updated commit message and changed TextTrackList getter.

Carrying forward r=rillian,bz.

Comment 17

[Rick Eyre (:reyre)]

Attachment: Part 2 v2: Expose TextTrackList's MediaElement to chrome JS. r=rillian,bz

Updating commit message and changing HTMLMediaElement getter.

Carrying forward r=rillian,bz.

Comment 18

[Rick Eyre (:reyre)]

Attachment: Part 3 v2: Refactor TextTrack classes. r=rillian

Updating commit message.

Carrying forward r=rillian.

Comment 19

[Rick Eyre (:reyre)]

Thanks for review Ralph and Boris.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=4086d42b6797

Comment 20

[Rick Eyre (:reyre)]

HTMLMediaElement seems to be leaking.

Comment 21

[Rick Eyre (:reyre)]

Part 1 of this patch seems to be okay. It's part 2 that introduces the leak.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=d82cb8b2dc7f

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/183b0963a67b

Comment 23

[Rick Eyre (:reyre)]

Attachment: Part 2 v3: Expose TextTrackList's MediaElement to chrome JS. r=rillian,bz

I simplified it a bit and made TextTrackList just got to its TextTrackManager for the MediaElement when necessary. This solved the leak as well.

Carrying forward r=bz.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=31558ba562cc

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/183b0963a67b

Comment 25

[Rick Eyre (:reyre)]

Thanks for review Ralph.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8371117 [details] [diff] [review]
Part 2 v3: Expose TextTrackList's MediaElement to chrome JS. r=rillian,bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7126bb1c91a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8367308 [details] [diff] [review]
Part 3 v2: Refactor TextTrack classes. r=rillian

https://hg.mozilla.org/integration/mozilla-inbound/rev/69bae68bfc68

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7126bb1c91a
https://hg.mozilla.org/mozilla-central/rev/69bae68bfc68