[webkit-reviews] review denied: [Bug 72171] <track>-related events cuechange, enter, and exit should be sorted and filtered before dispatching : [Attachment 120898] Updated code, tests and spec in comments
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 3 09:49:50 PST 2012
Eric Carlson <eric.carlson at apple.com> has denied Victor Carbune
<victor at rosedu.org>'s request for review:
Bug 72171: <track>-related events cuechange, enter, and exit should be sorted
and filtered before dispatching
https://bugs.webkit.org/show_bug.cgi?id=72171
Attachment 120898: Updated code, tests and spec in comments
https://bugs.webkit.org/attachment.cgi?id=120898&action=review
------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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.
More information about the webkit-reviews
mailing list