[webvtt] Implement the 'time marches on' algorithm

VERIFIED FIXED

Status

()

P2
normal
VERIFIED FIXED
5 years ago
9 days ago

People

(Reporter: reyre, Assigned: bechen)

Tracking

(Blocks: 4 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel ?, firefox48+ wontfix, firefox49+ verified)

Details

(Whiteboard: [platform-rel-BBC], URL)

Attachments

(7 attachments, 9 obsolete attachments)

58 bytes, text/x-review-board-request
rillian
: review+
Details
58 bytes, text/x-review-board-request
rillian
: review+
Details
58 bytes, text/x-review-board-request
rillian
: review+
Details
58 bytes, text/x-review-board-request
rillian
: review+
Details
58 bytes, text/x-review-board-request
rillian
: review+
Details
58 bytes, text/x-review-board-request
rillian
: review+
Details
33.87 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
This is a big part of keeping the cues up to date on as things are changing.
(Reporter)

Updated

5 years ago
Blocks: 629350, 882717
Depends on: 882713
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: nobody → andrew.quartey
(Reporter)

Comment 1

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

Comment 2

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

Comment 4

5 years ago
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.
Blocks: 778077
Created attachment 822842 [details] [diff] [review]
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
Attachment #822842 - Flags: review?(giles)
Created attachment 822843 [details] [diff] [review]
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.
Attachment #822843 - Flags: review?(giles)
Created attachment 822845 [details] [diff] [review]
Part d: Create a named simple event to be fired at a TextTrackCue.

This handles the event preparation part of the algorithm.
Attachment #822845 - Flags: review?(giles)
Created attachment 822846 [details] [diff] [review]
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.
Attachment #822846 - Flags: feedback?(giles)
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?
Attachment #822842 - Flags: review?(giles) → review+
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().
Attachment #822843 - Flags: review?(giles) → review-
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.
Attachment #822844 - Flags: review?(giles) → review-
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?
Attachment #822845 - Flags: review?(khuey)
Attachment #822845 - Flags: review?(giles)
Attachment #822845 - Flags: review+
(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 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!
Attachment #822846 - Flags: feedback?(giles) → feedback-
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.
Attachment #822845 - Flags: review?(khuey) → review-
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.)
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
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?
Flags: needinfo?(giles)
Yes. Or if not we should try to align them.
Flags: needinfo?(giles)
Created attachment 8338911 [details] [diff] [review]
Part a: Add list of newly introduced cues to TextTrackManager. v2

Removed one of the update paths as suggested in comment 10
Attachment #8338911 - Flags: review?(giles)
Created attachment 8338913 [details] [diff] [review]
Part b: monotonic playback flag prior to "Time Marches On" call. v2
Attachment #822843 - Attachment is obsolete: true
Attachment #822844 - Attachment is obsolete: true
Attachment #8338913 - Flags: review?(giles)
Created attachment 8338917 [details] [diff] [review]
Part c: Create a named simple event to be fired at a TextTrackCue. v2

This incorporates Olli's suggestion in comment 17 as well.
Attachment #822845 - Attachment is obsolete: true
Attachment #8338917 - Flags: review?(khuey)
Attachment #8338917 - Flags: review?(giles)
Created attachment 8338918 [details] [diff] [review]
WIP - Implementation of time marches on algorithm v2
Attachment #822846 - Attachment is obsolete: true
Attachment #8338918 - Flags: feedback?(giles)
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,'
Attachment #8338917 - Flags: review?(khuey) → 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.

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.
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.
Attachment #8338911 - Flags: review?(giles) → review+
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.
Attachment #8338913 - Flags: review?(giles) → review-
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.
Attachment #8338917 - Flags: review?(giles)
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.
Attachment #8338918 - Flags: feedback?(giles) → feedback+
(Reporter)

Updated

5 years ago
Blocks: 952130
(Reporter)

Comment 33

5 years ago
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?
(Reporter)

Updated

5 years ago
Duplicate of this bug: 960184
(Reporter)

Updated

5 years ago
Blocks: 882669
>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.
(Reporter)

Comment 36

5 years ago
(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!
(Reporter)

Comment 37

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

Updated

5 years ago
Depends on: 996331
(Reporter)

Updated

5 years ago
Depends on: 996333
(Reporter)

Updated

5 years ago
No longer blocks: 882669
(Reporter)

Updated

5 years ago
Blocks: 882669
(Reporter)

Comment 38

4 years ago
Andrew, if you don't mind I'm thinking about finishing this up now. Are you currently working on this anymore?
(Reporter)

Comment 39

4 years ago
See comment 38.
Flags: needinfo?(andrew.quartey)
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.
Flags: needinfo?(andrew.quartey)
(Reporter)

Comment 41

4 years ago
No worries. Thanks for the update.
Racing hard to finish this...cant tonight, will need more time; if not, all yours.
Flags: needinfo?(rick.eyre)
(Reporter)

Comment 43

4 years ago
Great, thanks for the your hard work Andrew :-). Let me know if you need anything.
Flags: needinfo?(rick.eyre)

Comment 44

4 years ago
What is the current status on this?  It seems that either this depends on 1033144 or the other way around.
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?
Flags: needinfo?(rick.eyre)
Component: Audio/Video → Audio/Video: Playback
(Assignee)

Comment 46

3 years ago
Hi Andrew,
Are you still working on this bug?
I want to try to finish the bug if you don't mind.
Flags: needinfo?(andrew.quartey)
(Assignee)

Updated

3 years ago
Assignee: andrew.quartey → bechen
Flags: needinfo?(rick.eyre)
Flags: needinfo?(andrew.quartey)
(Assignee)

Comment 47

3 years ago
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??
Flags: needinfo?(giles)
(Assignee)

Comment 48

3 years ago
Created attachment 8751655 [details]
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/
Attachment #8751655 - Flags: review?(giles)
Attachment #8751656 - Flags: review?(giles)
(Assignee)

Comment 49

3 years ago
Created attachment 8751656 [details]
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/
(Assignee)

Comment 50

3 years ago
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 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.
Attachment #8751655 - Flags: review?(giles) → review+
Comment on attachment 8751656 [details]
MozReview Request: Bug 882718 - triggerTimeMarchesOn. r=rillian

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

seems legit
Attachment #8751656 - Flags: review?(giles) → review+
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?
Flags: needinfo?(giles)
(Assignee)

Comment 54

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

Comment 55

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

Comment 56

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

Comment 57

2 years ago
Created attachment 8754292 [details]
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/
Attachment #8754292 - Flags: review?(giles)
Attachment #8754293 - Flags: review?(giles)
(Assignee)

Comment 58

2 years ago
Created attachment 8754293 [details]
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/
(Assignee)

Comment 59

2 years ago
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/
(Assignee)

Comment 60

2 years ago
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/
(Assignee)

Comment 61

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

Comment 62

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

Comment 63

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

Comment 64

2 years ago
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 on attachment 8754292 [details]
MozReview Request: Bug 882718 - Implement ActiveFlag at TextTrackCue object. r=rillian

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

sure
Attachment #8754292 - Flags: review?(giles) → review+
Attachment #8754293 - Flags: review?(giles) → review+
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.
Priority: -- → P2
(Assignee)

Comment 67

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

Comment 68

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

Updated

2 years ago
Blocks: 1274884
(Assignee)

Updated

2 years ago
Blocks: 1274885
(Assignee)

Comment 69

2 years ago
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/
(Assignee)

Comment 70

2 years ago
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/
(Assignee)

Comment 71

2 years ago
Created attachment 8755748 [details]
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/
Attachment #8755748 - Flags: review?(giles)
(Assignee)

Comment 72

2 years ago
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/
(Assignee)

Comment 73

2 years ago
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/
(Assignee)

Comment 74

2 years ago
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/
(Assignee)

Comment 75

2 years ago
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/
(Assignee)

Updated

2 years ago
Blocks: 1131952
(Assignee)

Updated

2 years ago
Duplicate of this bug: 996333
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.
Attachment #8755748 - Flags: review?(giles)
(Assignee)

Comment 78

2 years ago
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/
Attachment #8755748 - Attachment description: MozReview Request: Bug 882718 - 1. Fix testcase crash. 2. The cuechange event should be fired in TimeMarchesOn. r=rillian → MozReview Request: Bug 882718 - 1. Fix testcase crash/failed . 2. The cuechange event should be fired in TimeMarchesOn. r=rillian
Attachment #8755748 - Flags: review?(giles)
(Assignee)

Comment 79

2 years ago
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/
(Assignee)

Comment 80

2 years ago
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/
(Assignee)

Comment 81

2 years ago
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/
(Assignee)

Comment 82

2 years ago
https://reviewboard.mozilla.org/r/54788/#review51578

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

Comment 83

2 years ago
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 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.
Attachment #8755748 - Flags: review?(giles) → review+
(Assignee)

Comment 85

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

Updated

2 years ago
Blocks: 1275808
(Assignee)

Comment 86

2 years ago
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/
(Assignee)

Comment 87

2 years ago
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]
(Assignee)

Comment 88

2 years ago
Created attachment 8757250 [details]
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/
Attachment #8757250 - Flags: review?(giles)
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.
Attachment #8757250 - Flags: review?(giles) → review+
(Assignee)

Updated

2 years ago
Attachment #822842 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8338911 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8338913 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8338917 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8338918 - Attachment is obsolete: true
(Assignee)

Comment 92

2 years ago
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/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 93

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
There are more issues with assertions in the other web platform tests (symbol "W").
(Assignee)

Comment 96

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

Comment 99

2 years ago
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/
(Assignee)

Comment 100

2 years ago
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/
(Assignee)

Comment 101

2 years ago
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/
(Assignee)

Comment 102

2 years ago
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/
(Assignee)

Comment 103

2 years ago
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/
(Assignee)

Comment 104

2 years ago
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/
(Assignee)

Updated

2 years ago
Flags: needinfo?(bechen)
Keywords: checkin-needed

Updated

2 years ago
Blocks: 985915

Comment 105

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago2 years ago
Resolution: --- → FIXED
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 :)
Flags: needinfo?(bechen)
(Assignee)

Updated

2 years ago
Flags: needinfo?(bechen)
Whiteboard: [leave-open]
Benjamin - can you make sure this is in aurora? Do you think it is safe to uplift to beta?
Flags: needinfo?(bechen)
(Assignee)

Comment 110

2 years ago
(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?
Flags: needinfo?(bechen)
(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.
Flags: needinfo?(bechen)
Tracking for 49 to make sure we land this in aurora and verify the fix.
status-firefox49: --- → affected
tracking-firefox49: --- → +
Flags: qe-verify+
(Assignee)

Comment 113

2 years ago
I'm trying to uplift this bug and bug 1280814 to beta, but I hit compile error because Bug 1265927 is not in beta.
Flags: needinfo?(bechen)
So can you ask the patches in bug 1265927 to uplift?
What do you think?
Flags: needinfo?(bechen)
(Assignee)

Comment 115

2 years ago
Created attachment 8768357 [details] [diff] [review]
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
Flags: needinfo?(bechen)
Attachment #8768357 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(bechen)
Anthony, can you explain to QE either here or in email to Cornel Ionce, how to test this BBC compat issue on aurora? Thanks!
status-firefox48: --- → affected
tracking-firefox48: --- → +
Flags: needinfo?(ajones)
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.
status-firefox49: affected → fixed
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?
Flags: needinfo?(bwu)
(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.
Flags: needinfo?(bwu)
Per comment #120, won't fix in 48.
status-firefox48: affected → wontfix
Comment on attachment 8768357 [details] [diff] [review]
bug882718beta.patch

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

Won't fix in 48.
Attachment #8768357 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Comment 123

2 years ago
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.
platform-rel: --- → ?
Whiteboard: [platform-rel-BBC]
Flags: needinfo?(ajones)
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.
Flags: needinfo?(bbcrddave)

Comment 125

2 years ago
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
Flags: needinfo?(bbcrddave)
See Also: → bug 1286497

Updated

2 years ago
Depends on: 1294142
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.
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
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified

Updated

2 years ago
Depends on: 1310162
Flags: needinfo?(bechen)
You need to log in before you can comment on or make changes to this bug.