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

RESOLVED FIXED in mozilla29

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: reyre, Assigned: reyre)

Tracking

(Blocks: 1 bug)

unspecified
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 629350, 865407
(Assignee)

Updated

5 years ago
Assignee: nobody → rick.eyre
(Assignee)

Comment 1

5 years ago
Created attachment 811295 [details] [diff] [review]
v1: Fix bugs found in TextTrack::GetActiveCues() r=rillian
Attachment #811295 - Flags: review?(giles)
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.
Attachment #811295 - Flags: review?(giles) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 812083 [details] [diff] [review]
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.
Attachment #811295 - Attachment is obsolete: true
Attachment #812083 - Flags: review?(giles)
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.
Attachment #812083 - Flags: review?(giles) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 812670 [details] [diff] [review]
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.
Attachment #812083 - Attachment is obsolete: true
Attachment #812670 - Flags: review?(giles)
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.
Attachment #812670 - Flags: review?(giles) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 819031 [details] [diff] [review]
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
(Assignee)

Updated

5 years ago
Attachment #812670 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 8347301 [details] [diff] [review]
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.
Attachment #819031 - Attachment is obsolete: true
Attachment #8347301 - Flags: review?(giles)
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?
Attachment #8347301 - Flags: review?(giles) → review+
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 8350088 [details] [diff] [review]
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
Attachment #8347301 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Wow. It's _still_ failing.. I swear I got this sorted out.
(Assignee)

Comment 14

5 years ago
Created attachment 8350436 [details] [diff] [review]
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
Attachment #8350088 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Try looks green.
Keywords: checkin-needed
Keywords: checkin-needed
(Assignee)

Comment 16

5 years ago
Created attachment 8350670 [details] [diff] [review]
v8: Fix bugs found in TextTrack::GetActiveCues() r=rillian

Rebasing against current. Carrying forward r=rillian.

Please land bug 882299 before this.
Attachment #8350436 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-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
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/9ca7e64ef0d0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

5 years ago
Created attachment 8350935 [details] [diff] [review]
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.
Attachment #8350670 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/630011cb0501
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.