882718 - [webvtt] Implement the 'time marches on' algorithm

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Rick Eyre (:reyre)]

This is a big part of keeping the cues up to date on as things are changing.

Comment 1

[Rick Eyre (:reyre)]

If we're going to start working on this maybe we should land the first patch in bug 865407. Chris doesn't want extra code for VTT going directly into the HTMLMediaElement class so TextTrackManager is meant to hold that. It's been r+ so it's just a matter of testing and pushing it.

Comment 2

[Rick Eyre (:reyre)]

I'll working on landing that so you can start adding the code for this algorithm into the TextTrackmanager class where it makes sense, Andrew.

Comment 3

[Andrew Quartey [:drexler]]

(In reply to Rick Eyre (:reyre) from comment #1)
> If we're going to start working on this maybe we should land the first patch
> in bug 865407. Chris doesn't want extra code for VTT going directly into the
> HTMLMediaElement class so TextTrackManager is meant to hold that. It's been
> r+ so it's just a matter of testing and pushing it.

Hmmm..Ok. We should land that first then. I already have a few patches which would be impacted including the ones for this.

Comment 4

[Rick Eyre (:reyre)]

Yeah, I was planning to move it over when rebasing for the patch before landing so as to not block you, but it doesn't seem like I'll be able to land it for bit since I'll be travelling from next Weds for a week. Since you're starting this I think it just makes sense to land that now.

Comment 5

[Andrew Quartey [:drexler]]

Attachment: Part a: Add list of newly introduced cues to TextTrackManager.

This is needed for the time marches on implementation: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#list-of-newly-introduced-cues

Comment 6

[Andrew Quartey [:drexler]]

Attachment: Part b: monotonic playback flag prior to "Time Marches On" call.

Appends a flag to HTMLMediaElement to indicate whether playback was monotonic prior to "Time Marches On" call.

Comment 7

[Andrew Quartey [:drexler]]

Attachment: Part c: Add active flag to TextTrackCue.

Implements this part of the spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#text-track-cue-active-flag

Comment 8

[Andrew Quartey [:drexler]]

Attachment: Part d: Create a named simple event to be fired at a TextTrackCue.

This handles the event preparation part of the algorithm.

Comment 9

[Andrew Quartey [:drexler]]

Attachment: WIP - Implementation of time marches on algorithm

This is nearly complete minus the indicated steps shown in the patch, namely steps 6, 13, 15 and 16. Of the remaining steps, it's step 6 that i do have an issue in implementing especially how to interpret this condition: "if the user agent has not fired a timeupdate event at the element in the past 15 to 250ms and is not still running event handlers for such an event". I'll probably have to chat with you on IRC over that bit.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 822842 [details] [diff] [review]
Part a: Add list of newly introduced cues to TextTrackManager.

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

Sorry for taking so long to get to this. r=me with nits addressed.

Please document _why_ you're adding mNewCues to TextTrackManager.

I haven't read the other patches yet, but I'm also concerned about the general strategy. This is getting a bit spaghetti code, with the text track adding cues to the media element adding them to the manager. mNewCues isn't something the spec algorithm describes. It seems like it would be easy to forget one of the update paths between the different objects, especially during later maintenance. Is it possible to have more of this logic in the TextTrackManager and consolidate the update calls?

::: content/media/TextTrack.h
@@ +100,5 @@
>  
>    // Time is in seconds.
>    void Update(double aTime);
>  
> +  void AddCuesToMediaElement() const;

Is const appropriate here when it's modifying mMediaElement?

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 822843 [details] [diff] [review]
Part b:  monotonic playback flag prior to "Time Marches On" call.

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

r-

Per discussion with cpearce, it would be better to move this logic into TextTrackManager, which already gets a notification from HTMLMediaElement::SeekCompleted().

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 822844 [details] [diff] [review]
Part c: Add active flag to TextTrackCue.

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

I would rather this used mActiveCues from Ricks patch.

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 822845 [details] [diff] [review]
Part d: Create a named simple event to be fired at a TextTrackCue.

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

Looks ok to me, but see comments. Kyle can you sanity check the event dispatch, please?

::: content/html/content/src/TextTrackManager.cpp
@@ +117,5 @@
> +
> +  nsString mName;
> +  double mTime;
> +  TextTrack* mTrack;
> +  TextTrackCue* mCue;

Shouldn't these be nsRefPtr<>s?

::: content/html/content/src/TextTrackManager.h
@@ +15,5 @@
>  namespace mozilla {
>  namespace dom {
>  
>  class HTMLMediaElement;
> +struct SimpleEventRunner;

This is a very generic name for a class specific to the TextTrackManager. Maybe SimpleTextTrackEvent?

::: content/media/TextTrackCue.cpp
@@ +206,5 @@
>    }
>  }
> +
> +void
> +TextTrackCue::DispatchSimpleEvent(const nsAString& aEventName)

Since this is just implementing the whatwg spec text as an TrustedEvent wrapper, it makes more sense to move this to nsDOMEventTargetHelper.h?

Comment 14

[Ralph Giles (:rillian)]

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

> > +  nsString mName;
> > +  double mTime;
> > +  TextTrack* mTrack;
> > +  TextTrackCue* mCue;
> 
> Shouldn't these be nsRefPtr<>s?

And declared to the cycle collector, of course.

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 822846 [details] [diff] [review]
WIP -  Implementation of time marches on algorithm

This is fine as far as it goes, but I think you're implementing the spec too literally.

HTMLMediaElement::FireTimeUpdate is called approximately once per frame by the decoder and already handles queuing the 'timeupdate' event as in step 6. So you just need to add event dispatch to TextTrackManager::Update.

It's also not necessary to build the event lists explicitly because the cues should already be sorted in their lists; it should be possible to queue the events in order as the active cue list is updated. At least if you can figure out how to handle the only-different-by-parent-track case.

See also MediaRecorder::DispatchSimpleEvent for another simple event implementation example.

Again, sorry this took so long. Thanks for working on this!

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 822845 [details] [diff] [review]
Part d: Create a named simple event to be fired at a TextTrackCue.

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

Nothing in this patch uses CreateEvent so it's hard to see what the user looks like ... Why is it fired off a runnable?

::: content/html/content/src/TextTrackManager.cpp
@@ +117,5 @@
> +
> +  nsString mName;
> +  double mTime;
> +  TextTrack* mTrack;
> +  TextTrackCue* mCue;

Yes, these absolutely need to be strong pointers.

@@ +122,5 @@
> +};
> +
> +SimpleEventRunner*
> +TextTrackManager::CreateEvent(const nsAString& aEventName, double aTime,
> +                              TextTrack* aTrack, TextTrackCue* aCue) const

And this should return an already_AddRefed<T>.  Returning an object with a reference count of 0 is a footgun.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 822845 [details] [diff] [review]
Part d: Create a named simple event to be fired at a TextTrackCue.


>+  // Dispatches a simple named event target at |this| which does not
>+  // bubble and is non-cancelable.
>+  void DispatchSimpleEvent(const nsAString& aEventName);
Just make DispatchTrustedEvent in nsDOMEventTargetHelper public and use that and remove
DispatchSimpleEvent.

(Simple event is Event. The base interface. Everything else needs to be special cased.)

Comment 18

[Ralph Giles (:rillian)]

Thanks Kyle and Smaug. It's fired from a runnable because the spec says, "Create a task to fire a simple event named event at target." http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#time-marches-on

Comment 19

[Andrew Quartey [:drexler]]

Thanks for the reviews and feedback.

> Review of attachment 822844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would rather this used mActiveCues from Ricks patch.

So Ralph, in this case the mActiveCues becomes the |currentCues| as defined in the algorithm?

Comment 20

[Ralph Giles (:rillian)]

Yes. Or if not we should try to align them.

Comment 21

[Andrew Quartey [:drexler]]

Attachment: Part a: Add list of newly introduced cues to TextTrackManager. v2

Removed one of the update paths as suggested in comment 10

Comment 22

[Andrew Quartey [:drexler]]

Attachment: Part b: monotonic playback flag prior to "Time Marches On" call. v2

Comment 23

[Andrew Quartey [:drexler]]

Attachment: Part c: Create a named simple event to be fired at a TextTrackCue. v2

This incorporates Olli's suggestion in comment 17 as well.

Comment 24

[Andrew Quartey [:drexler]]

Attachment: WIP - Implementation of time marches on algorithm v2

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8338917 [details] [diff] [review]
Part c: Create a named simple event to be fired at a TextTrackCue. v2

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

::: content/html/content/src/TextTrackManager.cpp
@@ +110,5 @@
> +class SimpleTextTrackEvent : public nsRunnable
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(SimpleTextTrackEvent)

No, this should not be cycle collected.  Runnables should end up destroyed after they are processed by the thread.

@@ +128,5 @@
> +
> +private:
> +  nsString mName;
> +  double mTime;
> +  nsRefPtr<TextTrack> mTrack;

What do you even need mTrack for?

::: content/html/content/src/TextTrackManager.h
@@ +44,5 @@
>  
>    void PopulatePendingList();
>  
> +  already_AddRefed<SimpleTextTrackEvent>
> +  CreateEvent(const nsAString& aEventName, double aTime,TextTrack* aTrack,

space after 'aTime,'

Comment 26

[Andrew Quartey [:drexler]]

> ::: content/html/content/src/TextTrackManager.cpp
> @@ +110,5 @@
> > +class SimpleTextTrackEvent : public nsRunnable
> > +{
> > +public:
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_CLASS(SimpleTextTrackEvent)
> 
> No, this should not be cycle collected.  Runnables should end up destroyed
> after they are processed by the thread.

Ah, then WIP is definitely off with regards to this. So, briefly, only the non-runnable objects declaring and using these nsRefPtr<>'s need to be concerned with handling and declaring them to the cycle collector?

> @@ +128,5 @@
> > +
> > +private:
> > +  nsString mName;
> > +  double mTime;
> > +  nsRefPtr<TextTrack> mTrack;
> 
> What do you even need mTrack for?

It's used in sorting dispatched events to ensure the correct firing order...see: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#prepare-an-event.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The short story is that only objects that live on the heap and do not have well defined lifetimes need to be cycle collected.  Runnables should be short lived with well defined lifetimes (all references go away after they run) so there's no cycle collection needed.

You're not using mTrack in this patch, afaict.

Comment 28

[Ralph Giles (:rillian)]

Comment on attachment 8338913 [details] [diff] [review]
Part b: monotonic playback flag prior to "Time Marches On" call. v2

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

::: content/html/content/src/TextTrackManager.h
@@ +44,5 @@
>    void PopulatePendingList();
>  
>  private:
> +
> +  void ResetHasSeeked();

Since this is private, can't you just assign false directly to mHasSeeked when you need to? You should probably do that from a stub of time marches on in this patch; as it is this isn't really a complete patch, so it doesn't make sense for it to stand on its own.

Comment 29

[Ralph Giles (:rillian)]

Comment on attachment 8338917 [details] [diff] [review]
Part c: Create a named simple event to be fired at a TextTrackCue. v2

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

I don't have anything to add; this is more khuey's area.

Comment 30

[Ralph Giles (:rillian)]

Comment on attachment 8338918 [details] [diff] [review]
WIP - Implementation of time marches on algorithm v2

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

Sorry I took a while to get back to you. This looks like a positive direction. Some questions:

Can you explain the changes to the cycle collection macros? Are these necessary for the monitor?

My understanding is that jobs are dispatched from the main thread, and indeed you check for that. What other access is the monitor protecting the object from? Why isn't that already a problem with the TextTrackManager. I'd like to understand what the threading model here is.

::: content/html/content/src/TextTrackManager.h
@@ +85,5 @@
>    nsRefPtr<TextTrackCueList> mNewCues;
>    // True if the media player playback changed due to seeking prior to and
>    // during running the "Time Marches On" algorithm.
>    bool mHasSeeked;
> +  // Playback position at the time of last "Time Marches On" call

Period at the end of comments, please.

Comment 31

[Andrew Quartey [:drexler]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7bc0d9b7e881

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/7bc0d9b7e881

Comment 33

[Rick Eyre (:reyre)]

We're getting close to the point where this will be the last piece in the puzzle for support of WebVTT (besides chapter title rules and in-band text tracks, which is another beast entirely). Are you still working on this Andrew? If so, when do you expect you could have it done?

Comment 35

[Andrew Quartey [:drexler]]

>Are you still working on this Andrew? 
Yes. Sorry. Been a little busy for the past few months but i should have some free time this weekend and most of next week to blaze through this if necessary - have to see this through. 

>If so, when do you expect you could have it done?
Depends on how fast I can get review feedback for the updated patches i'll be posting.

Comment 36

[Rick Eyre (:reyre)]

(In reply to Andrew Quartey [:drexler] from comment #35)

> Yes. Sorry. Been a little busy for the past few months but i should have
> some free time this weekend and most of next week to blaze through this if
> necessary - have to see this through. 

No worries!

> Depends on how fast I can get review feedback for the updated patches i'll
> be posting.

We're hoping to get this landed in 31 so hopefully we can't get some quick reviews.

Thanks for your hard work!

Comment 37

[Rick Eyre (:reyre)]

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

> We're hoping to get this landed in 31 so hopefully we can't get some quick
> reviews.

Woops, should be _can_, heh.

Comment 38

[Rick Eyre (:reyre)]

Andrew, if you don't mind I'm thinking about finishing this up now. Are you currently working on this anymore?

Comment 39

[Rick Eyre (:reyre)]

See comment 38.

Comment 40

[Andrew Quartey [:drexler]]

Yes, still working on it. Sorry this slipped through the cracks - real life work projects intruding. If i fail to put up an updated patch come monday...6/23/2014, go ahead and assign it to yourself. Sorry for the delays.

Comment 41

[Rick Eyre (:reyre)]

No worries. Thanks for the update.

Comment 42

[Andrew Quartey [:drexler]]

Racing hard to finish this...cant tonight, will need more time; if not, all yours.

Comment 43

[Rick Eyre (:reyre)]

Great, thanks for the your hard work Andrew :-). Let me know if you need anything.

Comment 44

[syxton]

What is the current status on this?  It seems that either this depends on 1033144 or the other way around.

Comment 45

[Andrew Quartey [:drexler]]

Rick, what's the current state of this? I have some cycles to focus on this now besides, the source tree structure has changed significantly since my last login. Is this task still revelant?

Comment 46

[Benjamin Chen [:bechen]]

Hi Andrew,
Are you still working on this bug?
I want to try to finish the bug if you don't mind.

Comment 47

[Benjamin Chen [:bechen]]

Hi Ralph, I have a question about our implementation.
The purpose of the TimeMarchesOn is to fire onenter/onexit event at TextTrackCue object when cues become active/non-active.
Since we load the vtt file at the beginning of TrackElement, it is very possible that the whole cues are added into "list of newly introduced cues" before we execute TimeMarchesOn. And then the TimeMarchesOn empty the "list of newly introduced cues" at step 5.

So the problem is: if there is a 5 seconds video with texttrack, then we execute TimeMarchesOn empty the "list of newly introduced cues" at playback position 1s, we miss the 2-5 seconds onenter/onexit events??

Comment 48

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 882718 - Implement "TimeMarchesOn". r=rillian

Review commit: https://reviewboard.mozilla.org/r/52145/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52145/

Comment 49

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 882718 - triggerTimeMarchesOn. r=rillian

Review commit: https://reviewboard.mozilla.org/r/52147/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52147/

Comment 50

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52145/#review49073

::: dom/html/TextTrackManager.h:120
(Diff revision 1)
>  private:
> +  void TimeMarchesOn();
> +
> +  // Sort text tracks in the same order as the text tracks appear in the
> +  // media element's list of text tracks, and remove duplicates.
> +  void SortTextTracks(TextTrackList* aTrackList);

Useless function.

Comment 51

[Ralph Giles (:rillian)]

Comment on attachment 8751655 [details]
MozReview Request: Bug 882718 - Implement "TimeMarchesOn". r=rillian

https://reviewboard.mozilla.org/r/52145/#review49545

::: dom/html/TextTrackManager.cpp:409
(Diff revision 1)
> +private:
> +  nsString mName;
> +  double mTime;
> +  TextTrack* mTrack;
> +  RefPtr<TextTrackCue> mCue;
> +};

Can mTrack be a RefPtr<> too? TextTrack participates in cycle collection, and that seems more safe?

::: dom/html/TextTrackManager.cpp:512
(Diff revision 1)
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +  nsISupports* parentObject =
> +    mMediaElement->OwnerDoc()->GetParentObject();
> +  NS_ENSURE_TRUE_VOID(parentObject);
> +  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(parentObject);

`NS_ENSURE_*()` macros are deprecated because the embedded flow control is confusing. In new code like this, please use
```
if (NS_WARN_IF(!parentObject)) {
  return;
}
```

::: dom/html/TextTrackManager.cpp:531
(Diff revision 1)
> +    new TextTrackCueList(window);
> +  bool dummy;
> +  for (uint32_t index = 0; index < mTextTracks->Length(); ++index) {
> +    TextTrack* ttrack = mTextTracks->IndexedGetter(index, dummy);
> +    if (ttrack) {
> +      TextTrackCueList* activeCueList = ttrack->GetActiveCues();

Might as well `if (track && dummy)` here. TextTrackList::IndexedGetter() does return nullptr whenever dummy is false, but it's not good to rely on that.

If you don't like the extra check, `MOZ_ASSERT(dummy)` (probably with a better variable name) is fine too.

::: dom/html/TextTrackManager.cpp:539
(Diff revision 1)
> +          currentCues->AddCue(*((*activeCueList)[i]));
> +        }
> +      }
> +
> +      //Populate otherCues with 'non-active" cues.
> +      TextTrackCueList* cueList = ttrack->GetCues();

missing space in the comment here.

Comment 52

[Ralph Giles (:rillian)]

Comment on attachment 8751656 [details]
MozReview Request: Bug 882718 - triggerTimeMarchesOn. r=rillian

https://reviewboard.mozilla.org/r/52147/#review49549

seems legit

Comment 53

[Ralph Giles (:rillian)]

Thanks for following up on this. I didn't find a good answer to your question, but the patches seem like an improvement, so let's try them. Is there test coverage for this, for example in the web platform tests?

Comment 54

[Benjamin Chen [:bechen]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #53)
> Thanks for following up on this. I didn't find a good answer to your
> question, but the patches seem like an improvement, so let's try them. Is
> there test coverage for this, for example in the web platform tests?
I just look the google chromium's implementation, we should not process all the cues in "mNewCues" instead we can only process the cues reached by the playback position.

I will spend some time for the tests.

Comment 55

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52145/#review49545

> Can mTrack be a RefPtr<> too? TextTrack participates in cycle collection, and that seems more safe?

Since the SimpleTextTrackEvent holds TextTrackCue and TextTrackCue holds TextTrack, I think it is fine to use raw pointer or Refptr here.

Comment 56

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52145/#review50506

::: dom/html/TextTrackManager.h:102
(Diff revision 1)
> +
> +  struct TimeMarchesOnParams
> +  {
> +    TimeMarchesOnParams(double aCurrentPlaybackTime, bool aNormalPlayback)
> +    : mCurrentPlaybackTime(aCurrentPlaybackTime),
> +      mNormalPlayback(aNormalPlayback)

The mCurrentPlaybackTime is the time we call DispatchTimeMarchesOn, not equal to the playback position when we execute TimeMarchesOn (ttrack->GetActiveCues).

Comment 57

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 882718 - Implement ActiveFlag at TextTrackCue object. r=rillian

Review commit: https://reviewboard.mozilla.org/r/53870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53870/

Comment 58

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 882718 - Implement GetCueListByTimeInterval() at TextTrackCueList object. r=rillian

Review commit: https://reviewboard.mozilla.org/r/53872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53872/

Comment 59

[Benjamin Chen [:bechen]]

Comment on attachment 8751655 [details]
MozReview Request: Bug 882718 - Implement "TimeMarchesOn". r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52145/diff/1-2/

Comment 60

[Benjamin Chen [:bechen]]

Comment on attachment 8751656 [details]
MozReview Request: Bug 882718 - triggerTimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52147/diff/1-2/

Comment 61

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/53870/#review50608

For now, I move the patch of "Bug 882713 - [webvtt] Implement TextTrackCue::active" here to imeplement the active flag to achieve TimeMarchesOn step 7, 11,12.
But as Bug 882713 comment 2 says, we can use the new member of TextTrackManager |astActiveCues| to replace the active flag. I will fire a new bug if necessary.

Comment 62

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/53872/#review50610

I think the GetCueListByTimeInterval() function is very similiar to TextTrack::GetActiveCues, the difference is that we use a time interval as parameter instead a specific time.
The usage of the function is that we retrive a cueList by a time interval, then we divide the cueList into otherCueList, missCueList and activeCueList for the TimeMarchesOn().

Comment 63

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52145/#review50612

Introduce the |mLastActiveCues and GetCueListByTimeInterval| for otherCueList to fire onexit event.
Remove the |TimeMarchesOnParams| which exist in version 1. See comment 56.
Many steps are modified...

Comment 64

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52147/#review50614

At the end of TextTrackManager::UpdateCueDisplay, call TimeMarchesOn directly to fire onexit/onenter because we just render some new cues.
And for other cases, we just call  DispatchTimeMarchesOn.

Comment 65

[Ralph Giles (:rillian)]

Comment on attachment 8754292 [details]
MozReview Request: Bug 882718 - Implement ActiveFlag at TextTrackCue object. r=rillian

https://reviewboard.mozilla.org/r/53870/#review50764

sure

Comment 66

[Ralph Giles (:rillian)]

Comment on attachment 8754293 [details]
MozReview Request: Bug 882718 - Implement GetCueListByTimeInterval() at TextTrackCueList object. r=rillian

https://reviewboard.mozilla.org/r/53872/#review50766

I guess you can't start the endtime loop at the startindex because there could be overlapping queues. lgtm.

Comment 67

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52145/#review50914

::: dom/html/TextTrackManager.cpp:543
(Diff revision 2)
> +    // Seek case. Put the mLastActiveCues into otherCues.
> +    otherCues = mLastActiveCues;
> +  }
> +  for (uint32_t i = 0; i < currentCues->Length(); ++i) {
> +    TextTrackCue* cue = (*currentCues)[i];
> +    if (otherCues->GetCueById(cue->Id())) {

The cue id might be empty, so we can't use it for checking the same cue.

Comment 68

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52145/#review50924

::: dom/html/TextTrackManager.cpp:588
(Diff revision 2)
> +  if (c1 && c2 && c3) {
> +    mLastTimeMarchesOnCalled = currentPlaybackTime;
> +    return;
> +  }
> +
> +  // Step 8. Respect PauseOnExit flag if not seek.

Wrong implementation.

Comment 69

[Benjamin Chen [:bechen]]

Comment on attachment 8751655 [details]
MozReview Request: Bug 882718 - Implement "TimeMarchesOn". r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52145/diff/2-3/

Comment 70

[Benjamin Chen [:bechen]]

Comment on attachment 8751656 [details]
MozReview Request: Bug 882718 - triggerTimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52147/diff/2-3/

Comment 71

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 882718 - 1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian

Review commit: https://reviewboard.mozilla.org/r/54788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54788/

Comment 72

[Benjamin Chen [:bechen]]

Comment on attachment 8754292 [details]
MozReview Request: Bug 882718 - Implement ActiveFlag at TextTrackCue object. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53870/diff/1-2/

Comment 73

[Benjamin Chen [:bechen]]

Comment on attachment 8754293 [details]
MozReview Request: Bug 882718 - Implement GetCueListByTimeInterval() at TextTrackCueList object. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53872/diff/1-2/

Comment 74

[Benjamin Chen [:bechen]]

Comment on attachment 8751655 [details]
MozReview Request: Bug 882718 - Implement "TimeMarchesOn". r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52145/diff/3-4/

Comment 75

[Benjamin Chen [:bechen]]

Comment on attachment 8751656 [details]
MozReview Request: Bug 882718 - triggerTimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52147/diff/3-4/

Comment 77

[Ralph Giles (:rillian)]

Comment on attachment 8755748 [details]
MozReview Request: Bug 882718 - 1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian

https://reviewboard.mozilla.org/r/54788/#review51578

Looks ok, but you need to address the timeouts with dom/media/test/test_texttrackcue.html on try. Maybe the test is wrong, or time-marches-on isn't being called when it should be.

Comment 78

[Benjamin Chen [:bechen]]

Comment on attachment 8754293 [details]
MozReview Request: Bug 882718 - Implement GetCueListByTimeInterval() at TextTrackCueList object. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53872/diff/2-3/

Comment 79

[Benjamin Chen [:bechen]]

Comment on attachment 8751655 [details]
MozReview Request: Bug 882718 - Implement "TimeMarchesOn". r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52145/diff/4-5/

Comment 80

[Benjamin Chen [:bechen]]

Comment on attachment 8751656 [details]
MozReview Request: Bug 882718 - triggerTimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52147/diff/4-5/

Comment 81

[Benjamin Chen [:bechen]]

Comment on attachment 8755748 [details]
MozReview Request: Bug 882718 - 1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54788/diff/1-2/

Comment 82

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/54788/#review51578

The timeout root cause is that the TimeMarchesOn pause the MediaElement if the PauseOnExit flag is set.

Comment 83

[Benjamin Chen [:bechen]]

Comment on attachment 8755748 [details]
MozReview Request: Bug 882718 - 1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54788/diff/2-3/

Comment 84

[Ralph Giles (:rillian)]

Comment on attachment 8755748 [details]
MozReview Request: Bug 882718 - 1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian

https://reviewboard.mozilla.org/r/54788/#review51860

Thanks for tracking it down.

Comment 85

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/54788/#review52072

::: dom/html/TextTrackManager.cpp:565
(Diff revision 3)
>        }
>      }
>    }
>    // Populate otherCues with 'non-active" cues.
>    if (hasNormalPlayback) {
> +    if (mLastTimeMarchesOnCalled > currentPlaybackTime) {

This only address the backward seek, I should fix the forward seek as well.

Comment 86

[Benjamin Chen [:bechen]]

Comment on attachment 8755748 [details]
MozReview Request: Bug 882718 - 1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54788/diff/3-4/

Comment 87

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/54788/#review52134

There is a shutdow issue...

INFO - [2108] ###!!! ASSERTION: Failed NS_DispatchToMainThread() in shutdown; leaking: 'false', file /builds/slave/try-m64-d-00000000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp, line 185
INFO - 01: mozilla::dom::TextTrackManager::DispatchTimeMarchesOn() [mfbt/AlreadyAddRefed.h:101]
INFO - 02: mozilla::dom::HTMLTrackElement::UnbindFromTree(bool, bool) [mfbt/RefPtr.h:61]

Comment 88

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 882718 - Do not dispatch task to main thread when shutdown. r=rillian

Review commit: https://reviewboard.mozilla.org/r/55788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55788/

Comment 89

[Ralph Giles (:rillian)]

Comment on attachment 8757250 [details]
MozReview Request: Bug 882718 - Do not dispatch task to main thread when shutdown. r=rillian

https://reviewboard.mozilla.org/r/55788/#review52514

r=me with the try issue fixed.

::: dom/html/TextTrackManager.h:167
(Diff revision 1)
> +      }
> +      return NS_OK;
> +    }
> +
> +  private:
> +    ~ShutdownObserverProxy() {};

Making this virtual will fix the build failures, I think. See https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaRecorder.cpp#578 for an example.

Comment 90

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd28fed91408

Comment 91

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe69d2bb10ef

Comment 92

[Benjamin Chen [:bechen]]

Comment on attachment 8757250 [details]
MozReview Request: Bug 882718 - Do not dispatch task to main thread when shutdown. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55788/diff/1-2/

Comment 93

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/453431d7a2c8
Implement ActiveFlag at TextTrackCue object. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2bf27c13c37
Implement GetCueListByTimeInterval() at TextTrackCueList object. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6363fdbbf29
Implement "TimeMarchesOn". r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f61a6bd8a3d
triggerTimeMarchesOn. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/883bfabfde46
1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/830cccc16bd9
Do not dispatch task to main thread when shutdown. r=rillian

Comment 94

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for crashing in track.html with nsXBLPrototypeBinding::GetRuleProcessor()

https://hg.mozilla.org/integration/mozilla-inbound/rev/767ae8716c5b411b0e302494d5a5afc068ebf25c
https://hg.mozilla.org/integration/mozilla-inbound/rev/21c996f834375ada99da02b1eb9e1d266bb48c90
https://hg.mozilla.org/integration/mozilla-inbound/rev/acf1cdd99c985fe3b1f197ced9d4b320b2f4938e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b73a8a156585b864833261f195d0343a87e09e4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/96084c4ffc5070879fd8b88a6caacae59b156a08
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa94eb8c2df98df19bfb52f1f1e3587a5354e28

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=005a02a60fbd9e61026054cc9f5a3a3a09cbabec
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=29110050&repo=mozilla-inbound

Comment 95

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There are more issues with assertions in the other web platform tests (symbol "W").

Comment 96

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/52147/#review53508

::: dom/media/TextTrack.cpp:136
(Diff revision 5)
>  {
>    //TODO: Apply the rules for text track cue rendering Bug 865407
>    aCue.SetActive(false);
>  
>    mCueList->RemoveCue(aCue, aRv);
> +  HTMLMediaElement* mediaElement = mTextTrackList->GetMediaElement();

Sigmentation fault here.

Comment 97

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4328aa2e6249

Comment 98

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f5d906f10b4

Comment 99

[Benjamin Chen [:bechen]]

Comment on attachment 8754292 [details]
MozReview Request: Bug 882718 - Implement ActiveFlag at TextTrackCue object. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53870/diff/2-3/

Comment 100

[Benjamin Chen [:bechen]]

Comment on attachment 8754293 [details]
MozReview Request: Bug 882718 - Implement GetCueListByTimeInterval() at TextTrackCueList object. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53872/diff/3-4/

Comment 101

[Benjamin Chen [:bechen]]

Comment on attachment 8751655 [details]
MozReview Request: Bug 882718 - Implement "TimeMarchesOn". r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52145/diff/5-6/

Comment 102

[Benjamin Chen [:bechen]]

Comment on attachment 8751656 [details]
MozReview Request: Bug 882718 - triggerTimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52147/diff/5-6/

Comment 103

[Benjamin Chen [:bechen]]

Comment on attachment 8755748 [details]
MozReview Request: Bug 882718 - 1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54788/diff/4-5/

Comment 104

[Benjamin Chen [:bechen]]

Comment on attachment 8757250 [details]
MozReview Request: Bug 882718 - Do not dispatch task to main thread when shutdown. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55788/diff/2-3/

Comment 105

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e00bbe392726
Implement ActiveFlag at TextTrackCue object. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/61ffbcb578ac
Implement GetCueListByTimeInterval() at TextTrackCueList object. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12cd85eb79d
Implement "TimeMarchesOn". r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/639ff2d25ec3
triggerTimeMarchesOn. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/e130275a1db1
1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e513543f26c
Do not dispatch task to main thread when shutdown. r=rillian

Comment 106

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e00bbe392726
https://hg.mozilla.org/mozilla-central/rev/61ffbcb578ac
https://hg.mozilla.org/mozilla-central/rev/a12cd85eb79d
https://hg.mozilla.org/mozilla-central/rev/639ff2d25ec3
https://hg.mozilla.org/mozilla-central/rev/e130275a1db1
https://hg.mozilla.org/mozilla-central/rev/3e513543f26c

Comment 107

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e00bbe392726
https://hg.mozilla.org/mozilla-central/rev/61ffbcb578ac
https://hg.mozilla.org/mozilla-central/rev/a12cd85eb79d
https://hg.mozilla.org/mozilla-central/rev/639ff2d25ec3
https://hg.mozilla.org/mozilla-central/rev/e130275a1db1
https://hg.mozilla.org/mozilla-central/rev/3e513543f26c

Comment 108

[Jared Wein [:jaws] (please needinfo? me)]

Benjamin, is there any more work to do here? If not, please remove the [leave-open] whiteboard note, which is why bugherder didn't automatically close this bug :)

Comment 109

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Benjamin - can you make sure this is in aurora? Do you think it is safe to uplift to beta?

Comment 110

[Benjamin Chen [:bechen]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #109)
> Benjamin - can you make sure this is in aurora? Do you think it is safe to
> uplift to beta?

The patches had exist in aurora.
With Bug 1280814 fix, I think it is safe to uplift.

By the way, do I need to uplift bug 1280814 to aurora?

Comment 111

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Benjamin Chen [:bechen] from comment #110)
> The patches had exist in aurora.
> With Bug 1280814 fix, I think it is safe to uplift.

Can you do the uplift request? This is a compat issue that affects the BBC.
 
> By the way, do I need to uplift bug 1280814 to aurora?

It looks to me like it is worth uplifting.

Comment 112

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 49 to make sure we land this in aurora and verify the fix.

Comment 113

[Benjamin Chen [:bechen]]

I'm trying to uplift this bug and bug 1280814 to beta, but I hit compile error because Bug 1265927 is not in beta.

Comment 114

[Blake Wu [:bwu][:blakewu]]

So can you ask the patches in bug 1265927 to uplift?
What do you think?

Comment 115

[Benjamin Chen [:bechen]]

Attachment: bug882718beta.patch

Approval Request Comment
[Feature/regressing bug #]: New Feature
[User impact if declined]: Implement onenter/onexit events at Cue object.
[Describe test coverage new/current, TreeHerder]: current mochitests.
[Risks and why]: Low risk
[String/UUID change made/needed]: none

Comment 116

[Liz Henry (:lizzard) (relman/hg->git project)]

Benjamin does this include everything necessary for the uplift or do you still need work from other bugs as mentioned in comment 114? Does the patch now apply to beta?    And, can we test and verify the work on aurora?   

I could just try throwing this into the beta 6 build tonight, but it would be good to have more assurance it's worth trying and assessment of the risk.  If you can explain new features, regressing bugs, add test coverage when possible, and explain possible risk that will be helpful especially for beta uplifts.

Comment 117

[Liz Henry (:lizzard) (relman/hg->git project)]

Anthony, can you explain to QE either here or in email to Cornel Ionce, how to test this BBC compat issue on aurora? Thanks!

Comment 118

[Liz Henry (:lizzard) (relman/hg->git project)]

I also think from reading back that most of this work landed while 49 was on mozilla-central, in comment 109, so it is probably right to call it fixed in 49.

Comment 119

[Gerry Chang [:gchang]]

Hi Blake,
Can we let this ride the train on 49/50 and won't fix in 48 beta since the patch is big and is risky to me?

Comment 120

[Blake Wu [:bwu][:blakewu]]

(In reply to Gerry Chang [:gchang] from comment #119)
> Hi Blake,
> Can we let this ride the train on 49/50 and won't fix in 48 beta since the
> patch is big and is risky to me?
Agree. 
Lifting it to 48 is risky to me.

Comment 121

[Gerry Chang [:gchang]]

Per comment #120, won't fix in 48.

Comment 122

[Gerry Chang [:gchang]]

Comment on attachment 8768357 [details] [diff] [review]
bug882718beta.patch

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

Won't fix in 48.

Comment 123

[u514836]

We (BBC) would like to use this feature with our DASH solution to provide EBU-TT-D subtitles, as well as to handle more generic inband metadata. Both of these will use TextTrackCues as the syncronisation method.

The simplest example is http://dashif.org/reference/players/javascript/v2.2.0/samples/captioning/ttml-ebutt-sample.html. This should display a timecode in green with black background overlayed on the video. This example creates new Cues and uses onenter and onexit to manage display of the overlay.

I can provide more complex examples but, if this one works (which I have tested it does in at least Nightly), it will work for us.

We currently have a fallback method for Firefox where we just make a best-effort to display mostly-unstyled text, but of course this means the content is less accessible and Firefox users get a degraded experience.

Support in 49 would be great.

Comment 124

[Blake Wu [:bwu][:blakewu]]

Hi David, 
Thanks for this important info. 

(In reply to David Evans from comment #123)
> We (BBC) would like to use this feature with our DASH solution to provide
> EBU-TT-D subtitles, as well as to handle more generic inband metadata. Both
We will create a bug to support inband metadata. Do you have any link for us to verify it?
> of these will use TextTrackCues as the syncronisation method.
> 
> The simplest example is
> http://dashif.org/reference/players/javascript/v2.2.0/samples/captioning/
> ttml-ebutt-sample.html. This should display a timecode in green with black
> background overlayed on the video. This example creates new Cues and uses
> onenter and onexit to manage display of the overlay.
This bug is to support onenter and onexit event. It should work after 49. Please let us know if you hit any problems.

Comment 125

[u514836]

Hi Blake,

Sorry for the slow response.

I don't have a public demo of this available right now unfortunately, but since there is no UA standard, our application extracts the metadata from the source media in javascript and exposes it via TextTrack with kind=metadata using TextTrackCue.onenter/exit just as the captioning example does.

If, in future, a standardised method for handling and exposing the metadata directly from the UA becomes available, I will be sure to raise a bug!

Cheers,
Dave

Comment 126

[Bogdan Maris, Desktop QA]

This is still unclear for QE how we can verify this. Any steps, guidance or information on how we can test would be much appreciated.
I tried on Firefox 49 beta 8 with the link David Evans posted in comment 123 but the timecode in green with black background does not appear in the video unlike latest Nightly 51.0a1 where it does.

Comment 127

[Ciprian Georgiu, Desktop QA]

This issue is verified fixed on Firefox 49.0-build2 (2016-09-07) using:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- macOS 10.12 Sierra