882664 - [webvtt] Sort the Media Elements Text Track List correctly

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

The text tracks need to be in a sorted order. I haven't seen where this matters really yet, possible in some of the algorithms later on. Lets figure out if it's needed and implement it if it is.

Comment 1

[Rick Eyre (:reyre)]

Attachment: Part 1 v1: TextTrack should hold a ref to its TrackElement, if any. r=rillian

Comment 2

[Rick Eyre (:reyre)]

Attachment: Part 2 v1: Perform sorting on TextTrackLists r=rillian.

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8383835 [details] [diff] [review]
Part 2 v1: Perform sorting on TextTrackLists r=rillian.

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

This feels really cumbersome. Can you explain for me what you're trying to accomplish and why you chose this strategy?

The VTTRegion changes feel like they're part of a different issue.

I guess most of this and Part 1 are is to implement sorting as described in the first section of whatwg 4.7.10.12.1 http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#timed-text-tracks ? Please mention that sort of thing in your commit message, it's quite helpful for people looking at the code later.

Would it be simpler to keep three lists, based on origin, and concatenate them when returning a TextTrackList element?

Why is it important to sort the text tracks in the first place?

Comment 4

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #3)
> Comment on attachment 8383835 [details] [diff] [review]
> Part 2 v1: Perform sorting on TextTrackLists r=rillian.
> 
> Review of attachment 8383835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This feels really cumbersome. Can you explain for me what you're trying to
> accomplish and why you chose this strategy?
> 
> The VTTRegion changes feel like they're part of a different issue.

Yeah, it felt weird to me. I can split it off into another issue. I think it might belong better with bug 978163.

> I guess most of this and Part 1 are is to implement sorting as described in
> the first section of whatwg 4.7.10.12.1
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-
> element.html#timed-text-tracks ? Please mention that sort of thing in your
> commit message, it's quite helpful for people looking at the code later.

I can do that.

> Would it be simpler to keep three lists, based on origin, and concatenate
> them when returning a TextTrackList element?

I was thinking of that strategy, but a lot of parts of this will have to remain. We still need to remember the TextTrackSource for sorting, as well as performing automatic track selection (another issue), and we still need the rules for each of the different types of sorting. I chose this because it keeps the implementation of the AddTextTrack function and TextTrackManager class still fairly trivial, we just have to pass in the comparator. All the logic for sorting is stored in one comparator.

If we had three separate lists then either AddTextTrack would get a lot bigger, or we would need to add multiple sort functions for each list, or have multiple comparators.

> Why is it important to sort the text tracks in the first place?

The place it matters the most is in positioning the cues on the video when the line is "auto". In that case the line number a cue will be positioned at is relative to the tracks place in its TextTrackList. So a cue that has auto positioning and is a part of a track which is at position 2 in its TextTrackList will have a default line position of -3. Which will put it 3 lines up from the bottom of the video.

Comment 5

[Ralph Giles (:rillian)]

(In reply to Rick Eyre (:reyre) from comment #4)

> Yeah, it felt weird to me. I can split it off into another issue. I think it
> might belong better with bug 978163.

That was my thought too.
> 
> > I guess most of this and Part 1 are is to implement sorting as described in
> > the first section of whatwg 4.7.10.12.1
> 
> I can do that.

Thanks.

> > Would it be simpler to keep three lists, based on origin, and concatenate
> > them when returning a TextTrackList element?
> 
> I was thinking of that strategy, but a lot of parts of this will have to
> remain. We still need to remember the TextTrackSource for sorting

I meant keep separate arrays based on origin, but you're probably right that's not any better.

> The place it matters the most is in positioning the cues on the video when
> the line is "auto". In that case the line number a cue will be positioned at
> is relative to the tracks place in its TextTrackList. So a cue that has auto
> positioning and is a part of a track which is at position 2 in its
> TextTrackList will have a default line position of -3. Which will put it 3
> lines up from the bottom of the video.

Ok, that's sounds reasonably important. Thanks for explaining.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8383834 [details] [diff] [review]
Part 1 v1: TextTrack should hold a ref to its TrackElement, if any. r=rillian

I'm fine with this approach then, but I'll cancel review until you have new patches addressing the comments.

Comment 7

[Rick Eyre (:reyre)]

Attachment: Part 2 v2: Perform sorting on TextTrackLists r=rillian.

Thanks for review Ralph.

I've moved the TextTrackRegion bits to bug 978163. I've also updated the commit message.

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8388580 [details] [diff] [review]
Part 2 v2: Perform sorting on TextTrackLists r=rillian.

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

r=me with comment addressed. Almost there!

::: content/html/content/src/TextTrackManager.cpp
@@ +64,5 @@
> +    case AddTextTrack:
> +      // aOne will always be less then aTwo when both their sources are
> +      // AddTextTrack as we just put the item being inserted at the end of
> +      // the list in this case.
> +      return true;

This violates the comparator abstraction, but I suppose looking up the actual indices in the TextTrackList and comparing those would too.

Maybe make this more explicit in the comment:

// For AddTextTrack sources the tracks will already be in the correct relative
// order in the source array. Assume we're called in iteration order and can
// therefore always report aOne < aTwo to maintain the original temporal ordering.

Please also add a test for this, with a comment about why it's important, since this is somewhat brittle. I don't think nsTArray::InsertElementSorted() could be changed to do anything but an insertion sort, but a later rewrite could violate this assumption and silently break our implementation here.

Comment 9

[Rick Eyre (:reyre)]

Thanks for review Ralph.

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

> ::: content/html/content/src/TextTrackManager.cpp
> @@ +64,5 @@
> > +    case AddTextTrack:
> > +      // aOne will always be less then aTwo when both their sources are
> > +      // AddTextTrack as we just put the item being inserted at the end of
> > +      // the list in this case.
> > +      return true;
> 
> This violates the comparator abstraction, but I suppose looking up the
> actual indices in the TextTrackList and comparing those would too.

Yeah, it made me cringe a bit when I was writing it. I don't think there is a nice way do sort with all the different cases. We have to make at trade off somewhere.

> Maybe make this more explicit in the comment:
> 
> // For AddTextTrack sources the tracks will already be in the correct
> relative
> // order in the source array. Assume we're called in iteration order and can
> // therefore always report aOne < aTwo to maintain the original temporal
> ordering.

Will do.

> Please also add a test for this, with a comment about why it's important,
> since this is somewhat brittle. I don't think
> nsTArray::InsertElementSorted() could be changed to do anything but an
> insertion sort, but a later rewrite could violate this assumption and
> silently break our implementation here.

I have added some tests. They may be a bit tricky to see. test_texttrack.html now adds four tracks to the video element. Two from track elements and two with 'addTextTrack' method. There added out of order so that we can make sure the sorting is correct. Then it loops through 'video.textTracks' and uses the 'label' value to make sure that they've been sorted correctly. I can add some comments to make what is happening in the test more clear.

Comment 10

[Ralph Giles (:rillian)]

(In reply to Rick Eyre (:reyre) from comment #9)

> I have added some tests. They may be a bit tricky to see.

Aha! Yes, I missed the first addTrack and thought you weren't testing order.

This is starting to get complicated enough the various spec features should be tested in separate functions so it's clear which parts depend on what, but adding comments is fine for this patch.

Comment 11

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) on vacation until March 31 from comment #10)
> This is starting to get complicated enough the various spec features should
> be tested in separate functions so it's clear which parts depend on what,
> but adding comments is fine for this patch.
I agree. I'll open a bug for that.

Comment 12

[Rick Eyre (:reyre)]

Splitting up tests filed as bug 983182.

Comment 13

[Rick Eyre (:reyre)]

Attachment: Part 1 v2: TextTrack should hold a ref to its TrackElement, if any. r=rillian

Rebasing.

Carrying forward r=rillian.

Comment 14

[Rick Eyre (:reyre)]

Attachment: Part 2 v3: Perform sorting on TextTrackLists r=rillian.

Rebasing and adding comments that Ralph requested.

Carrying forward r=rillian.

Comment 15

[Rick Eyre (:reyre)]

Attachment: Part 2 v4: Perform sorting on TextTrackLists r=rillian.

Forgot one comment change request from Ralph.

Carrying forward r=rillian.

Comment 16

[Rick Eyre (:reyre)]

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=9329ada6e5a3

Comment 17

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43585af186ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/824d1ef99ba2

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43585af186ec
https://hg.mozilla.org/mozilla-central/rev/824d1ef99ba2