[Webkit-unassigned] [Bug 72171] <track>-related events cuechange, enter, and exit should be sorted and filtered before dispatching
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 3 09:49:51 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=72171
Eric Carlson <eric.carlson at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #120898|review? |review-
Flag| |
--- Comment #5 from Eric Carlson <eric.carlson at apple.com> 2012-01-03 09:49:51 PST ---
(From update of attachment 120898)
View in context: https://bugs.webkit.org/attachment.cgi?id=120898&action=review
> Source/WebCore/ChangeLog:30
> + * html/HTMLMediaElement.h: Added variables to keep
> + state information required by the text track update algorithm (last time
> + the algorithm was run, and whether a seeking event has occured)
> + * html/TextTrackCue.cpp:
Nit: Many people but a blank line between non-associated files to make the changelog easier to read.
> Source/WebCore/ChangeLog:8212
> +>>>>>>> master
Left over from a bad merge?
> Source/WebCore/html/HTMLMediaElement.cpp:966
> + // 12 - Sort the tasks in events in ascending time order (tasks with earlier
> + // times first).
> + if (a.first == b.first) {
> + int aCueIndex = a.second->track()->cues()->getCueIndex(a.second);
> + int bCueIndex = b.second->track()->cues()->getCueIndex(b.second);
> +
> + if (a.second->track() == b.second->track()) {
> + // 12 - Further sort tasks in events that have the same time by the
> + // relative text track cue order of the text track cues associated
> + // with these tasks.
> + return aCueIndex - bCueIndex < 0;
> + }
> + }
> +
> + return a.first - b.first < 0;
Nit: The comments don't quite follow the code logic. It might make more sense to return early if the times are not the same:
// 12 - Sort the tasks in events in ascending ...
if ((a.first != b.first)
return a.first - b.first < 0;
// Further sort tasks in events ...
int aCueIndex = a.second->track()->cues()->getCueIndex(a.second);
> Source/WebCore/html/HTMLMediaElement.cpp:984
> + Vector<CueIntervalTree::IntervalType> affectedCues;
> + Vector<CueIntervalTree::IntervalType> previouslyVisibleCues;
> + Vector<CueIntervalTree::IntervalType> missedCues;
Nit: an early return when m_cueTree is empty would prevent a lot of unnecessary setup churn.
> Source/WebCore/html/HTMLMediaElement.cpp:986
> + // 2 - other cues
Why is the first comment #2?
> Source/WebCore/html/HTMLMediaElement.cpp:990
> + // 1 - current cues
> m_currentlyActiveCues = m_cueTree.allOverlaps(m_cueTree.createInterval(movieTime, movieTime));
When I suggested including comments from the spec, I meant to literally include spec text as we have in other parts of this file. Eg:
// 4.8.10.8 Playing the media resource
// 1. Let current cues be a list of cues, initialized to contain all the cues of all the hidden, showing,
// or showing by default text tracks of the media element (not the disabled ones) whose start times are
// less than or equal to the current playback position and whose end times are greater than the current
// playback position.
m_currentlyActiveCues = m_cueTree.allOverlaps(m_cueTree.createInterval(movieTime, movieTime));
etc.
> Source/WebCore/html/HTMLMediaElement.cpp:995
> + if (m_lastTextTrackUpdate && !m_seekingBeforeTextTrackUpdate) {
> + Vector<CueIntervalTree::IntervalType> potentiallySkippedCues =
> + m_cueTree.allOverlaps(m_cueTree.createInterval(m_lastTextTrackUpdate, movieTime));
Nit: I had to read this several times before I figured out what it did, so I think "m_lastTextTrackUpdate" would be better as "m_lastTextTrackUpdateTime".
> Source/WebCore/html/HTMLMediaElement.cpp:999
> + if (potentiallySkippedCues[i].low() > m_lastTextTrackUpdate
> + && potentiallySkippedCues[i].high() < movieTime)
Nit: I think the line break makes this much harder to read.
> Source/WebCore/html/HTMLMediaElement.cpp:1017
> + bool activeSetChanged = false;
If any cues were skipped you know that the active set must be updated, so initialize activeSetChanged to missedCues.size() so you won't check previous and currently visible cues.
> Source/WebCore/html/HTMLMediaElement.cpp:1018
> + for (size_t i = 0; i < previouslyVisibleCues.size() && !activeSetChanged; ++i) {
Nit: 1) If !activeSetChanged is checked first, you won't have to check the queue size once it is true. 2) The queue size can't change in this loop so you should put the size into a local variable instead of fetching it from the queue every time.
> Source/WebCore/html/HTMLMediaElement.cpp:1020
> + if (!m_currentlyActiveCues.contains(previouslyVisibleCues[i])
> + && previouslyVisibleCues[i].data()->isActive())
Nit: I think the line break makes this much harder to read.
> Source/WebCore/html/HTMLMediaElement.cpp:1024
> + for (size_t i = 0; i < m_currentlyActiveCues.size() && !activeSetChanged; ++i) {
Nit: As above.
> Source/WebCore/html/HTMLMediaElement.cpp:1077
> + // 12 - Sort the events before dispatching.
12. Sort the tasks in events in ascending time order (tasks with earlier times first).
> Source/WebCore/html/HTMLMediaElement.cpp:1088
> + // 13 - Queue each task in events, in list order (we can already dispatch).
> + if (eventTasks[i].first == eventTasks[i].second->startTime())
> + eventTasks[i].second->setIsActive(true);
> + else
> + eventTasks[i].second->setIsActive(false);
"Queue each task in events ..." means the events should be fired asynchronously, but TextTrackCue::setIsActive fires the event immediately.
> Source/WebCore/html/HTMLMediaElement.cpp:1100
> + for (size_t i = 0; i < affectedTracks.size(); ++i)
> + affectedTracks[i]->fireCueChangeEvent();
"queue a task to fire a simple event" means the event is fired asynchronously, but TextTrack:: fireCueChangeEvent fires the event immediately.
> Source/WebCore/html/HTMLMediaElement.cpp:1717
> + m_seekingBeforeTextTrackUpdate = true;
Nit: This might be clearer if named something like "m_seekedSinceLastTextTrackUpdate".
> Source/WebCore/html/HTMLMediaElement.h:564
> + typedef std::pair <double, TextTrackCue*> EventTimeCuePair;
This typedef isn't used in the header so you should define in the .cpp file where it is used.
> Source/WebCore/html/HTMLMediaElement.h:566
> + typedef PODIntervalTree <double, TextTrackCue*> CueIntervalTree;
Either this diff is missing something, or this is a re-definition of the CueIntervalTree typedef.
> Source/WebCore/html/HTMLMediaElement.h:568
> - CueList m_currentlyActiveCues;
> + Vector<CueIntervalTree::IntervalType> m_currentlyActiveCues;
Is there a reason to not use the CueList type?
> Source/WebCore/html/TextTrackCueList.cpp:51
> +unsigned long TextTrackCueList::getCueIndex(TextTrackCue* cue) const
> +{
> + for (unsigned long index = 0; index < m_list.size(); ++index) {
> + if (m_list[index].get() == cue)
> + return index;
> + }
> +
> + return 0;
> +}
Could this use Vector::find?
Actually, a cue's index will change *much* less infrequently than this function will be called so I wonder if it would be better to have a cue store its index. You could invalidate a cue's stored index whenever a new cue is inserted into the list, and have a cue look up the index only when stored index is invalid.
> Source/WebCore/html/track/TextTrackList.cpp:66
> +unsigned TextTrackList::getTrackIndex(TextTrack *textTrack)
> +{
> + for (unsigned index = 0; index < m_elementTracks.size(); ++index) {
> + if (m_elementTracks[index].get() == textTrack)
> + return index;
> + }
> +
> + return 0;
> +}
Could this use Vector::find?
> LayoutTests/media/track/track-cues-missed-expected.txt:13
> +Tests that events are triggered for missed (skipped) cues during normal playback.
> +
> +EVENT(canplaythrough)
> +EXPECTED (testTrack.track.cues.length == '3') OK
> +RUN(video.play())
> +EVENT(enter)
> +EXPECTED ([object TextTrackCue] == '[object TextTrackCue]') OK
> +EXPECTED (currentCue.id == '3') OK
> +EVENT(exit)
> +EXPECTED ([object TextTrackCue] == '[object TextTrackCue]') OK
> +EXPECTED (currentCue.id == '3') OK
> +EVENT(ended)
> +END OF TEST
I would rather that this check more than one cue because it is possible, if unlikely, that a single cue will not be skipped.
> LayoutTests/media/track/track-cues-missed.html:16
> + function runTests() {
> + if (!trackLoaded || !videoCanPlayThrough)
WebKit style is for a function's opening brace to be on a new line.
> LayoutTests/media/track/track-cues-missed.html:47
> + waitForEvent('ended', function() {
> + endTest();
> + });
Nit: Why define a new function to call a single function? "waitForEvent('ended', endTest);"
> LayoutTests/media/track/track-cues-sorted-before-dispatch-expected.txt:9
> +Cue event: enter id: 1 time: 5.1
> +Cue event: enter id: 3 time: 5.1
> +Cue event: enter id: 2 time: 5.1
Shouldn't these file in ID order because of "Further sort tasks in events that have the same time by the relative text track cue order of the text track cues associated with these tasks"?
> LayoutTests/media/track/track-cues-sorted-before-dispatch.html:17
> + function runTests() {
As above, the opening brace goes on a new line.
> LayoutTests/media/track/track-cues-sorted-before-dispatch.html:27
> + for (var i = 0; i < testTrack.track.cues.length; ++i) {
> + testTrack.track.cues[i].addEventListener('enter', cueEntered);
> + testTrack.track.cues[i].addEventListener('exit', cueExited);
> + }
Is there an advantage to registering a listeners on each cue instead of on the <track>?
> LayoutTests/media/track/track-cues-sorted-before-dispatch.html:52
> + function cueEntered() {
> + currentCue = event.target;
> +
> + var eventObj = {};
> + eventObj.time = currentCue.startTime;
> + eventObj.type = "enter";
> + eventObj.cue = currentCue;
> +
> + dispatchedEvents.push(eventObj);
> + }
> +
> + function cueExited() {
> + currentCue = event.target;
> +
> + var eventObj = {};
> + eventObj.time = currentCue.endTime;
> + eventObj.type = "exit";
> + eventObj.cue = currentCue;
> +
> + dispatchedEvents.push(eventObj);
> + }
Nit: These functions are identical except for "eventObj.type = 'XXX'", but you could use "eventObj.type = event.type" and have a single function.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list