883173 - [webvtt] Update TextTrack::ActiveCues to function properly

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

This should be returning a list of cues that start after the current playback time and end after the current play back time.

Comment 1

[Rick Eyre (:reyre)]

Attachment: v1: Bug 883173 - Implement TextTrack::ActiveCues

Comment 2

[Caitlin Potter (:caitp)]

Comment on attachment 763690 [details] [diff] [review]
v1: Bug 883173 - Implement TextTrack::ActiveCues

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

In TextTrackCueList::AddCue(TextTrackCue& aCue):
>  for (int32_t i = mList.Length() - 1; i >= 0; i--) {
>    if (aCue.StartTime() >= mList[i]->StartTime()) {
>      mList.InsertElementAt(i + 1, &aCue);
>      return;
>    }
>  }
>  mList.InsertElementAt(0, &aCue);

Could be more clearly written as
>  int i = mList.length();
>  for( ; i > 0 && aCue.StartTime() < mList[i - 1]->StartTime(); --i );
>  mList.InsertElementAt(i, &aCue);

(It might also be desirable to cache the result of aCue.StartTime(), depending on what code the compiler generates)
Fewer lines of code, no second return statement, blah blah blah. I don't necessarily think this is the ideal way to perform a sorted insert, however, but simple is good.

You probably don't want to use operator[] unless you are absolutely positive that it will be valid, since there's no real way to notify you that an error occurred, without C++ exceptions. If we're sharing the TextTrack between multiple threads, a locking mechanism will be helpful, as well.

I don't really agree with the GetActiveCues() mechanics, if we had usable reftests we'd probably find problems with it fairly easily. But it's just a feeling, so I'm not going to say much more about that. We definitely need reftest style stuff though.

That's all for today, take it or leave it.

Comment 3

[Rick Eyre (:reyre)]

(In reply to Caitlin Potter (:caitp) from comment #2)
>  int i = mList.length();
>  for( ; i > 0 && aCue.StartTime() < mList[i - 1]->StartTime(); --i );
>  mList.InsertElementAt(i, &aCue);

Looks like a better solution.

> You probably don't want to use operator[] unless you are absolutely positive
> that it will be valid, since there's no real way to notify you that an error
> occurred, without C++ exceptions. If we're sharing the TextTrack between
> multiple threads, a locking mechanism will be helpful, as well.

All the text track stuff happens on the main thread I believe. There's very few things in Gecko that actually happen off the main thread. Could be a problem though, I don't really know.

Comment 4

[Caitlin Potter (:caitp)]

It's not clear whether or not it will be shared or not. In order to implement the track parser as an Incremental Parser (in which the buffer is shared between a parser thread and a listener thread), a locking mechanism will definitely be desirable. We aren't there yet, but it's likely something that we'll be doing in the next 6 months.

However even before that, I have my doubts about any "guarantees" about whether we can expect operations to be completely synchronous. See what Ralph thinks about it I guess.

Comment 5

[Rick Eyre (:reyre)]

Attachment: Part 1 v2: Rename cue param

Comment 6

[Rick Eyre (:reyre)]

Attachment: Part 2 v2: Implement insertion sort for TextTrackList::AddCue

Comment 7

[Rick Eyre (:reyre)]

Attachment: Part 3: Implement TextTrack::GetActiveCues

Comment 8

[Rick Eyre (:reyre)]

This probably won't be able to land until the Android stuff is fixed as I've included tests -- unless we plan to move those tests over to 833386 like the rounded ones.

Comment 9

[Caitlin Potter (:caitp)]

Blocking on Android is complete nonsense. It doesn't make any sense at all -- Skip the tests on Android until you get them working, but don't block on it, that's ridiculous.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 764317 [details] [diff] [review]
Part 1 v2: Rename cue param

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

You misspelled 'parameter' in the commit message. You might want to put the Part # in the first line as well:

Bug 883173 - Part 1: Rename cue parameter. r=rillian

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 764318 [details] [diff] [review]
Part 2 v2: Implement insertion sort for TextTrackList::AddCue

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

r+ Some nits below.

You should document why we chose an insertion sort, either as a comment or in the body of the commit message. Something like, "We expect most files to already be sorted, so an insertion sort starting from the end of the current array should be more efficient than a general sort step after all cues are loaded."

::: content/media/test/Makefile.in
@@ +257,4 @@
>  		detodos.opus \
>  		notags.mp3 \
>  		id3tags.mp3 \
> +		bug883173.vtt \

This probably needs to be rebased to eliminate the conflict now that bug 833386 has landed.

::: content/media/test/test_bug883173.html
@@ +27,5 @@
> +    document.getElementById("content").appendChild(video);
> +    video.appendChild(trackElement);
> +    video.addEventListener("loadedmetadata",
> +      function run_tests() {
> +        // Re-que run_tests() at the end of the event loop until the track

s/Re-que/Re-queue/

@@ +39,5 @@
> +        var cueList = trackElement.track.cues;
> +        is(cueList.length, 5, "Cue list length should be 5.");
> +
> +        is(cueList[0].startTime, 1, "First cue's start time should be 1");
> +        is(cueList[0].endTime, 2, "First cue's end time should be 2.");

I don't object to the repetition here. It's easy to read and not too long, but you could certainly make this shorter by iterating over an array of expected results. Something like:

expected = [ [1, 2], [1, 3], ... ];
is(expected.length, cueList.length, "unexpected cue count");
expected.forEach(function(value, i) {
  is(cueList[i].startTime, value[0], "Incorrect start time");
  is(cueList[i].endTime, value[1], "Incorrect end time");
});

Or you can do it the other way around with
Array.slice(cueList, 0).forEach( function(item, index) { ... });

I'm sure there are other ways to make it neater. Anyway, up to you if you want to try to be more succinct.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 764320 [details] [diff] [review]
Part 3: Implement TextTrack::GetActiveCues

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

Looks reasonable to me, but I haven't been able to review it in detail. Chris, would you mind taking this part? It touches HTMLMediaElement.

Comment 13

[Rick Eyre (:reyre)]

Attachment: Part 1 v3: Rename cue param r=rillian

(In reply to Ralph Giles (:rillian) from comment #10)
> You misspelled 'parameter' in the commit message. You might want to put the
> Part # in the first line as well:
> 
> Bug 883173 - Part 1: Rename cue parameter. r=rillian

Done. Thanks for the review.

Carrying forward r=rillian

Comment 14

[Rick Eyre (:reyre)]

Attachment: Part 2 v3: Implement insertion sort for TextTrackList::AddCue r=rillian

(In reply to Ralph Giles (:rillian) from comment #11)
> You should document why we chose an insertion sort, either as a comment or
> in the body of the commit message. Something like, "We expect most files to
> already be sorted, so an insertion sort starting from the end of the current
> array should be more efficient than a general sort step after all cues are
> loaded."

Added into the header file for AddCue() and the commit message.

> This probably needs to be rebased to eliminate the conflict now that bug
> 833386 has landed.

Rebased.

> s/Re-que/Re-queue/

Fixed.

> expected = [ [1, 2], [1, 3], ... ];
> is(expected.length, cueList.length, "unexpected cue count");
> expected.forEach(function(value, i) {
>   is(cueList[i].startTime, value[0], "Incorrect start time");
>   is(cueList[i].endTime, value[1], "Incorrect end time");
> });

Made it more succinct by iterating over an array like you suggested.

Thanks for the review.

Carrying forward r=rillian.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 764320 [details] [diff] [review]
Part 3: Implement TextTrack::GetActiveCues

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

r+ with the changes I suggest below.

::: content/media/TextTrack.cpp
@@ +35,5 @@
>    , mLanguage(aLanguage)
>    , mMode(TextTrackMode::Hidden)
>    , mCueList(new TextTrackCueList(aParent))
>    , mActiveCueList(new TextTrackCueList(aParent))
> +  , mCuePos(0)

Shouldn't you initialize mDirty here? Otherwise chances are this TextTrack will be considered dirty by default (since the memory location that mDirty corresponds to will probably contain something non-zero, i.e. a true), but it may not be if the memory happens to be 0 (i.e. false).

@@ +104,5 @@
> +  // to have changed like a seek or an insert for a cue, than we need to rebuild
> +  // the active cue list from scratch.
> +  if (mDirty) {
> +    mCuePos = 0;
> +    mActiveCueList->RemoveAll();

Set mDirty to false here? Otherwise you'll take this branch every time this function is called.

Comment 16

[Rick Eyre (:reyre)]

Attachment: Part 3 v3: Implement TextTrack::GetActiveCues r=cpearce,rillian

Thanks for the review Chris.

Carrying forward r=cpearce,rillian.

Comment 17

[Rick Eyre (:reyre)]

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

Comment 18

[Rick Eyre (:reyre)]

Attachment: Part 1 v4: Rename cue param r=rillian

Rebased against current m-c. 

Carrying forward r=rillian

Comment 19

[Rick Eyre (:reyre)]

Attachment: Part 2 v4: Implement insertion sort for TextTrackList::AddCue r=rillian

Rebased against current m-c

Carrying forward r=rillian

Comment 20

[Rick Eyre (:reyre)]

Attachment: Part 3 v4: Implement TextTrack::GetActiveCues r=cpearce,rillian

Rebased against current m-c

Carrying forward r=rillian, cpearce

Comment 21

[Rick Eyre (:reyre)]

Attachment: Part 1 v4: Rename cue param r=rillian

Fixing cut off commit message.

Carrying forward r=rillian

Comment 22

[Rick Eyre (:reyre)]

Attachment: Part 2 v4: Implement insertion sort for TextTrackList::AddCue r=rillian

Fixing cut off commit message.

Carrying forward r=rillian

Comment 23

[Rick Eyre (:reyre)]

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

Comment 24

[Rick Eyre (:reyre)]

Attachment: Part 3 v4: Implement TextTrack::GetActiveCues r=cpearce,rillian

Fixing commit message to contain "Part 3"...

Carrying forward r=rillian,cpearce

Comment 25

[Rick Eyre (:reyre)]

Attachment: Part 2 v5: Implement insertion sort for TextTrackList::AddCue r=rillian

Just rebasing to current.

Carrying forward r=rillian

Comment 26

[Rick Eyre (:reyre)]

Attachment: Part 3 v5: Implement TextTrack::GetActiveCues r=cpearce,rillian

Rebasing against current.

I've updated TextTrack to have a SetDefaultSettings() for its constructors.

Comment 27

[Ralph Giles (:rillian)]

Comment on attachment 807830 [details] [diff] [review]
Part 3 v5: Implement TextTrack::GetActiveCues r=cpearce,rillian

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

Looks reasonable to me. Some style suggestions.

Are you planning to call GetActiveCues() in TextTrackCueList::Update() is a subsequent patch?

::: content/media/TextTrack.cpp
@@ +54,2 @@
>  {
>    SetIsDOMBinding();

I think it would be less error-prone to remove the settings from the initializer section here too. Call SetDefaults, and then have three assignment settings to override the defaults in the ctor body from the extra arguments.

::: content/media/TextTrack.h
@@ +8,4 @@
>  #define mozilla_dom_TextTrack_h
>  
>  #include "mozilla/dom/TextTrackBinding.h"
> +#include "mozilla/dom/TextTrackCue.h"

Do you need to add this to the header? The new references seem to only be in the .cpp file.

::: content/media/TextTrackCueList.cpp
@@ +50,5 @@
>  
>  TextTrackCue*
> +TextTrackCueList::operator[](uint32_t aIndex)
> +{
> +  return aIndex < mList.Length() ? mList[aIndex] : nullptr;

This could be 

  return mList.SafeElementAt(aIndex, nullptr);

...but there's no corresponding SafeRemoveElementAt, so it's not much easier to read.

@@ +103,5 @@
> +
> +void
> +TextTrackCueList::RemoveAll()
> +{
> +  mList.RemoveElementsAt(0, mList.Length());

This could be mList.Clear().

Comment 28

[Rick Eyre (:reyre)]

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

> Are you planning to call GetActiveCues() in TextTrackCueList::Update() is a
> subsequent patch?

Yep. We can call this to get the cues that we need to pass to the JS processing model. We need this though as it's in the TextTrack IDL too.

> ::: content/media/TextTrack.cpp
> @@ +54,2 @@
> >  {
> >    SetIsDOMBinding();
> 
> I think it would be less error-prone to remove the settings from the
> initializer section here too. Call SetDefaults, and then have three
> assignment settings to override the defaults in the ctor body from the extra
> arguments.

Can you clarify? Do you mean updating SetDefaults() to take arguments for mMediaElement and mParent?

Comment 29

[Rick Eyre (:reyre)]

Attachment: Part 2 v6: Implement insertion sort for TextTrackList::AddCue r=rillian

I've updated TextTrackCueList::AddCue() to use a Comparator to sort the list.

Comment 30

[Ralph Giles (:rillian)]

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

> Can you clarify? Do you mean updating SetDefaults() to take arguments for
> mMediaElement and mParent?

I just mean calling SetDefaults() without arguments from both constructors, and then overriding the defaults with explicit assignment in the argument version of the constructor, reducing the code duplication.

Comment 31

[Ralph Giles (:rillian)]

Comment on attachment 808630 [details] [diff] [review]
Part 2 v6: Implement insertion sort for TextTrackList::AddCue r=rillian

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

::: content/media/TextTrackCueList.cpp
@@ +10,4 @@
>  namespace mozilla {
>  namespace dom {
>  
> +class CompareCueByTimes

CompareCuesByTime?

Comment 32

[Rick Eyre (:reyre)]

Attachment: Part 1 v5: Rename cue param r=rillian

Just rebasing against current m-c.

Carrying forward r=rillian.

Comment 33

[Rick Eyre (:reyre)]

Attachment: Part 2 v7: Implement insertion sort for TextTrackList::AddCue r=rillian

Rebased to current m-c and renamed Comparator class to CompareCuesByTime.

Carrying forward r=rillian,cpearce.

Comment 34

[Rick Eyre (:reyre)]

Attachment: Part 3 v6: Implement TextTrack::GetActiveCues r=cpearce,rillian

Rebased to current m-c.

I hope this addresses your ctor comments Ralph. I may have misunderstood you.

Comment 35

[Ralph Giles (:rillian)]

Comment on attachment 808827 [details] [diff] [review]
Part 3 v6: Implement TextTrack::GetActiveCues r=cpearce,rillian

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

Yep, this is what I meant. Thanks!

::: content/media/TextTrack.cpp
@@ +29,5 @@
>  NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
>  
> +TextTrack::TextTrack(nsISupports* aParent)
> +  : mParent(aParent)
> +  , mMediaElement(nullptr)

The nsRefPtr constructor will initialize it to nullptr, so you shouldn't need this line.

Comment 36

[Rick Eyre (:reyre)]

Attachment: Part 3 v7: Implement TextTrack::GetActiveCues r=cpearce,rillian

Thanks for review Ralph!

Carrying forward r=rillian.

Comment 37

[Rick Eyre (:reyre)]

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

Comment 38

[Rick Eyre (:reyre)]

Attachment: Part 2 v8: Implement insertion sort for TextTrackList::AddCue r=rillian

I incorrectly removed the code, while rebasing, that checks if the cue is already in the cue list when AddCue is called.

Carrying forward r=rillian.

Comment 39

[Rick Eyre (:reyre)]

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

Comment 40

[Rick Eyre (:reyre)]

Attachment: Part 2 v9: Implement insertion sort for TextTrackList::AddCue r=rillian

Was failing on Android as we need to set preload="auto" on the video element in order for the video to be loaded on Android.

Carrying forward r=rillian.

Comment 41

[Rick Eyre (:reyre)]

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

Comment 42

[Rick Eyre (:reyre)]

Looks green.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1658d1924f42
https://hg.mozilla.org/integration/fx-team/rev/29686472fe07
https://hg.mozilla.org/integration/fx-team/rev/a1f4f274e5b8

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1658d1924f42
https://hg.mozilla.org/mozilla-central/rev/29686472fe07
https://hg.mozilla.org/mozilla-central/rev/a1f4f274e5b8