921484 - [webvtt] Fix bugs in TextTrack::GetActiveCues()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

There are a few bugs in GetActiveCues().

* mDirty needs to be set to false after the list has been recomputed.
* The loop that adds cues to the list doesn't work properly when the list has become dirty as we check if endTime < playback time and since we start from pos 0 we will never get past the first cue.

Comment 1

[Rick Eyre (:reyre)]

Attachment: v1: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 811295 [details] [diff] [review]
v1: Fix bugs found in TextTrack::GetActiveCues() r=rillian

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

I don't think this works either. See comments.

Can we add an overlapping cue to basic.vtt (not so basic anymore?) to test that case?

::: content/media/TextTrack.cpp
@@ +143,4 @@
>    // the active cue list from scratch.
>    if (mDirty) {
>      mCuePos = 0;
> +    mDirty = false;

Sorry I missed this!

@@ +147,5 @@
>      mActiveCueList->RemoveAll();
>    }
>  
>    double playbackTime = mMediaElement->CurrentTime();
> +

Trailing whitespace. 'git diff --check' will find these for you.

@@ +160,5 @@
>    // added, that have valid start and end times for the current playback time.
>    // We can stop iterating safely once we encounter a cue that does not have
>    // valid times for the current playback time as the cue list is sorted.
> +  for (; mCuePos < mCueList->Length() &&
> +         (*mCueList)[mCuePos]->StartTime() <= playbackTime; mCuePos++) {

You still need to check endtimes.

12:00.500 -- 12:03.700
No show

12:02.000 --> 12:04.600
Show

If this runs when playbackTime is 12:04.000, it will add both cues to mActiveList when only the second should be valid.

You're right that you can't use both as a loop terminator, but you need to skip the AddCue if EndTime < playbackTime. 
I'd check the endtime first so you can short-circuit the starttime check until you

::: content/media/test/Makefile.in
@@ +74,4 @@
>  		test_bug895305.html \
>  		test_bug895091.html \
>  		test_bug919265.html \
> +		test_bug921484.html \

Instead of adding a new test file, could this be added at the end of test_texttrackcue.html? You could even reuse the cueinfo array.

Comment 3

[Rick Eyre (:reyre)]

Attachment: v2: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Thanks for review Ralph.

(In reply to Ralph Giles (:rillian) from comment #2)
> Comment on attachment 811295 [details] [diff] [review]
> v1: Fix bugs found in TextTrack::GetActiveCues() r=rillian
> 
> Review of attachment 811295 [details] [diff] [review]:
> -----------------------------------------------------------------

> Can we add an overlapping cue to basic.vtt (not so basic anymore?) to test
> that case?

Done.

> ::: content/media/TextTrack.cpp
> @@ +143,4 @@
> >    // the active cue list from scratch.
> >    if (mDirty) {
> >      mCuePos = 0;
> > +    mDirty = false;
> 
> Sorry I missed this!

Actually, I think this was my bad. Chris mentioned in a review to set it to false here (when it wasn't being set at all) and I ended up adding it in, but setting it to 'true'.

> You still need to check endtimes.
> 
> 12:00.500 -- 12:03.700
> No show
> 
> 12:02.000 --> 12:04.600
> Show
> 
> If this runs when playbackTime is 12:04.000, it will add both cues to
> mActiveList when only the second should be valid.

I've changed it up so that we check for the EndTime() within the for loop, which is what I think you were talking about.

> ::: content/media/test/Makefile.in
> @@ +74,4 @@
> >  		test_bug895305.html \
> >  		test_bug895091.html \
> >  		test_bug919265.html \
> > +		test_bug921484.html \
> 
> Instead of adding a new test file, could this be added at the end of
> test_texttrackcue.html? You could even reuse the cueinfo array.

Done.

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 812083 [details] [diff] [review]
v2: Fix bugs found in TextTrack::GetActiveCues() r=rillian

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

r+ with nits.

I think having two cues with the same timerange is good, but I think we should also have one with an overlap to catch the edge case I described in the last review. Can you add another cue with a partial overlap?

::: content/media/TextTrack.cpp
@@ +147,5 @@
>      mActiveCueList->RemoveAll();
>    }
>  
>    double playbackTime = mMediaElement->CurrentTime();
> +

trailing whitespace.

@@ +158,2 @@
>    }
> +

trailing whitespace.

@@ +169,2 @@
>    }
> +

trailing whitespace.

::: content/media/test/test_texttrackcue.html
@@ +65,5 @@
>        // Check that Cue setters are working correctly.
>        cue.id = "Cue 01";
>        is(cue.id, "Cue 01", "Cue's ID should be 'Cue 01'.");
> +      cue.startTime = 0.51;
> +      is(cue.startTime, 0.51, "Cue's start time should be 1.5.");

error messages need updating too.

@@ +74,5 @@
>  
>        // Check that we can create and add new VTTCues
>        var vttCue = new VTTCue(3.999, 4, "foo");
>        trackElement.track.addCue(vttCue);
> +      is(cueList.length, 6, "Cue list length should now be 5.");

Also here, and below.

Comment 5

[Rick Eyre (:reyre)]

Attachment: v3: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Thanks for review Ralph.

Had to redesign the testing mechanism to account for multiple cues with overlap, but I think it turned out way better then the way I had it initially.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 812670 [details] [diff] [review]
v3: Fix bugs found in TextTrack::GetActiveCues() r=rillian

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

Thanks. Sorry I took so long to get to it.

::: content/media/test/test_texttrackcue.html
@@ +131,5 @@
> +        for (var i = 0; i < cueInfo.length && !found; i++) {
> +          var cue = cueInfo[i];
> +          if (playbackTime >= cue.startTime && playbackTime <= cue.endTime) {
> +            is(activeCues.length, cue.ids.length, "There should be " + cue.ids.length + " currently active cue(s).");
> +            for (var i = 0; i < cue.ids.length; i++) {

I'd rather you used 'j' here for the iterator. The 'var' should make it safe, but it's easy to confuse with the outer loop and I worry about later edits.

Comment 7

[Rick Eyre (:reyre)]

Attachment: v4: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Thanks for review Ralph. No worries! Carrying forward r=rillian.

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

Comment 8

[Rick Eyre (:reyre)]

Starting on this again in as bug 865407 needs this to land. Try push: https://tbpl.mozilla.org/?tree=Try&rev=75a6009b6d49.

I seem to recall that there was something was failing and that's why I didn't mark it for check-in.

Comment 9

[Rick Eyre (:reyre)]

Attachment: v5: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Main problem was that we can't assume that the end times are in order when removing the cues from the active cue list as the cue list is sorted with start times first. So if a cue has a later start time, but an earlier end time than a cue previously in the list, it's going to be missed.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8347301 [details] [diff] [review]
v5: Fix bugs found in TextTrack::GetActiveCues() r=rillian

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

r=me with nit.

::: content/media/TextTrack.cpp
@@ +162,2 @@
>    }
> +

Trailing whitespace.

::: content/media/test/basic.vtt
@@ -11,5 @@
>  3
>  00:02.710 --> 00:02.910
>  A
>  
> -3

Was the duplicate id part of a another test?

Comment 11

[Rick Eyre (:reyre)]

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

> Was the duplicate id part of a another test?

Looking at the blame it doesn't look like it. Last time that was touched was when we added the very first tests for the Track APIs and I can't see any relevant tests we have now that use it. If we don't see a fail when we do a try push I think it's probably alright.

Comment 12

[Rick Eyre (:reyre)]

Attachment: v6: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Got rid of trailing whitespace.

Carrying forward r=rillian

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

Comment 13

[Rick Eyre (:reyre)]

Wow. It's _still_ failing.. I swear I got this sorted out.

Comment 14

[Rick Eyre (:reyre)]

Attachment: v7: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Okay. So it looks like it was because in the test I check for active cue times exclusively (curTime > startTime && < endTime) where as in the actual GetActiveCues() code we add them inclusively (curTime >= startTime && <= endTime) which is the correct way. Just changed the test to be correct.

Carrying forward r=rillian.

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

Comment 15

[Rick Eyre (:reyre)]

Try looks green.

Comment 16

[Rick Eyre (:reyre)]

Attachment: v8: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Rebasing against current. Carrying forward r=rillian.

Please land bug 882299 before this.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca7e64ef0d0

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2e8a63e433 because something from the push this was in introduced a new intermittent failure on Android: https://tbpl.mozilla.org/php/getParsedLog.php?id=32309229&tree=Mozilla-Inbound

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ca7e64ef0d0

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backout to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/3a2e8a63e433

Comment 21

[Rick Eyre (:reyre)]

Attachment: v9: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Okay. The issue was that I wrote the test badly. I've updated the test so that we test the right times for sets of cues that may share the same start and end times.

I've also updated it so that we don't just assume if we've found no times in our cue list that no cues should be active. Instead, I specify which times that no cues should be active. This ensures that we're not testing for no cues to be active in a time, for example 2.405, that sits on the edge between two 'cue info entries'.

Carrying forward r=rillian.

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=00bae5e573f0

I've ran the test on Android multiple times to ensure that we don't have the intermittent test failure anymore.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/630011cb0501

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/630011cb0501