[webvtt] Update TextTrack::ActiveCues to function properly

RESOLVED FIXED in mozilla27

Status

()

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

People

(Reporter: reyre, Assigned: reyre)

Tracking

(Blocks: 1 bug)

Trunk
mozilla27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 19 obsolete attachments)

1.60 KB, patch
Details | Diff | Splinter Review
12.11 KB, patch
Details | Diff | Splinter Review
5.46 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
This should be returning a list of cues that start after the current playback time and end after the current play back time.
(Assignee)

Updated

4 years ago
Assignee: nobody → rick.eyre
Blocks: 865407, 882700
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 882713
(Assignee)

Comment 1

4 years ago
Created attachment 763690 [details] [diff] [review]
v1: Bug 883173 - Implement TextTrack::ActiveCues
Attachment #763690 - Flags: review?(giles)
Attachment #763690 - Flags: feedback?(caitlin.potter)
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.
Attachment #763690 - Flags: feedback?(caitlin.potter)
(Assignee)

Comment 3

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

Updated

4 years ago
Blocks: 833386
(Assignee)

Updated

4 years ago
Attachment #763690 - Flags: review?(giles)
(Assignee)

Comment 5

4 years ago
Created attachment 764317 [details] [diff] [review]
Part 1 v2: Rename cue param
Attachment #763690 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Created attachment 764318 [details] [diff] [review]
Part 2 v2: Implement insertion sort for TextTrackList::AddCue
(Assignee)

Comment 7

4 years ago
Created attachment 764320 [details] [diff] [review]
Part 3: Implement TextTrack::GetActiveCues
(Assignee)

Updated

4 years ago
No longer blocks: 833386
(Assignee)

Updated

4 years ago
Depends on: 884884
(Assignee)

Updated

4 years ago
Attachment #764317 - Flags: review?(giles)
(Assignee)

Updated

4 years ago
Attachment #764318 - Flags: review?(giles)
(Assignee)

Updated

4 years ago
Attachment #764320 - Flags: review?(giles)
(Assignee)

Comment 8

4 years ago
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.
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 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
Attachment #764317 - Flags: review?(giles) → review+
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.
Attachment #764318 - Flags: review?(giles) → review+
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.
Attachment #764320 - Flags: review?(giles)
Attachment #764320 - Flags: review?(cpearce)
Attachment #764320 - Flags: feedback+
(Assignee)

Comment 13

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

Comment 14

4 years ago
Created attachment 770965 [details] [diff] [review]
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.
Attachment #764318 - Attachment is obsolete: true
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.
Attachment #764320 - Flags: review?(cpearce) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 773404 [details] [diff] [review]
Part 3 v3: Implement TextTrack::GetActiveCues r=cpearce,rillian

Thanks for the review Chris.

Carrying forward r=cpearce,rillian.
Attachment #764320 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=73f7daa70455
(Assignee)

Comment 18

4 years ago
Created attachment 788195 [details] [diff] [review]
Part 1 v4: Rename cue param r=rillian

Rebased against current m-c. 

Carrying forward r=rillian
Attachment #770963 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
Created attachment 788198 [details] [diff] [review]
Part 2 v4: Implement insertion sort for TextTrackList::AddCue r=rillian

Rebased against current m-c

Carrying forward r=rillian
Attachment #770965 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Created attachment 788199 [details] [diff] [review]
Part 3 v4: Implement TextTrack::GetActiveCues r=cpearce,rillian

Rebased against current m-c

Carrying forward r=rillian, cpearce
Attachment #773404 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 788203 [details] [diff] [review]
Part 1 v4: Rename cue param r=rillian

Fixing cut off commit message.

Carrying forward r=rillian
Attachment #788195 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 788205 [details] [diff] [review]
Part 2 v4: Implement insertion sort for TextTrackList::AddCue r=rillian

Fixing cut off commit message.

Carrying forward r=rillian
Attachment #788198 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=59b03cf1f00f
(Assignee)

Comment 24

4 years ago
Created attachment 788209 [details] [diff] [review]
Part 3 v4: Implement TextTrack::GetActiveCues r=cpearce,rillian

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

Carrying forward r=rillian,cpearce
Attachment #788199 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Created attachment 807826 [details] [diff] [review]
Part 2 v5: Implement insertion sort for TextTrackList::AddCue r=rillian

Just rebasing to current.

Carrying forward r=rillian
Attachment #788205 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
Created attachment 807830 [details] [diff] [review]
Part 3 v5: Implement TextTrack::GetActiveCues r=cpearce,rillian

Rebasing against current.

I've updated TextTrack to have a SetDefaultSettings() for its constructors.
Attachment #788209 - Attachment is obsolete: true
Attachment #807830 - Flags: review?(giles)
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().
Attachment #807830 - Flags: review?(giles) → review+
(Assignee)

Comment 28

4 years ago
(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?
(Assignee)

Comment 29

4 years ago
Created attachment 808630 [details] [diff] [review]
Part 2 v6: Implement insertion sort for TextTrackList::AddCue r=rillian

I've updated TextTrackCueList::AddCue() to use a Comparator to sort the list.
Attachment #807826 - Attachment is obsolete: true
Attachment #808630 - Flags: review?(giles)
(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 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?
Attachment #808630 - Flags: review?(giles) → review+
(Assignee)

Comment 32

4 years ago
Created attachment 808825 [details] [diff] [review]
Part 1 v5: Rename cue param r=rillian

Just rebasing against current m-c.

Carrying forward r=rillian.
Attachment #788203 - Attachment is obsolete: true
(Assignee)

Comment 33

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

Comment 34

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

Comment 36

4 years ago
Created attachment 809165 [details] [diff] [review]
Part 3 v7: Implement TextTrack::GetActiveCues r=cpearce,rillian

Thanks for review Ralph!

Carrying forward r=rillian.
Attachment #808827 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=3a2f825d6578
(Assignee)

Comment 38

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

Comment 39

4 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=80ca6d6a219d
(Assignee)

Comment 40

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

Comment 41

4 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=6f9c5ba365c8
(Assignee)

Comment 42

4 years ago
Looks green.
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1658d1924f42
https://hg.mozilla.org/mozilla-central/rev/29686472fe07
https://hg.mozilla.org/mozilla-central/rev/a1f4f274e5b8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.