Seeking by an increment smaller than a movie's time scale can fail to generate a 'seeked' event. This can happen because seeking to "currentTime + <value>" when <value> is smaller than the movie's time scale asks the engine to seek to the current time, which some engines consider a noop so the time changed callback to HTMLMediaElement is never fired, and the 'seeked' event is never fired.
Created attachment 72200 [details] Proposed patch
Comment on attachment 72200 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=72200&action=review What about MediaPlayerPrivateQuickTimeWin? > WebCore/html/HTMLMediaElement.cpp:1116 > + // Ask the media engine for the time value in the movie's time scale before comparing with current time. This Shouldn't we be converting to the movie's time scale before doing any comparisons at all, not just before comparing with the current time? > WebCore/platform/graphics/MediaPlayerPrivate.h:130 > + // Time value in the movie's time scale. It is only necessary to override this if the media > + // engine does not use real numbers to represent media time. > + virtual float mediaTimeForTimeValue(float timeValue) const { return timeValue; } What does "real numbers" mean? I assume you don't mean it in the mathematical sense? > WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:1063 > + long mediaTimeValue = static_cast<long>(timeValue * timeScale); > + return static_cast<float>(mediaTimeValue) / timeScale; I assume the multiplication, truncation, and division is intentional here? (It's probably the whole point of this function!) > WebCore/platform/graphics/win/QTMovie.cpp:744 > + if (!m_private->m_movie) > + 0; This won't compile.
(In reply to comment #2) > (From update of attachment 72200 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72200&action=review > > What about MediaPlayerPrivateQuickTimeWin? > It isn't used any more and should be removed from the build - https://bugs.webkit.org/show_bug.cgi?id=48556 > > WebCore/html/HTMLMediaElement.cpp:1116 > > + // Ask the media engine for the time value in the movie's time scale before comparing with current time. This > > Shouldn't we be converting to the movie's time scale before doing any comparisons at all, not just before comparing with the current time? > This is the only function that passes time values to the media engine. It wouldn't hurt to do the conversion earlier in this function but I don't think it is necessary because we only compare to clamp the value to the movie's min and max times. IOW, we only care that it is somewhere within the movie's timeline. > > WebCore/platform/graphics/MediaPlayerPrivate.h:130 > > + // Time value in the movie's time scale. It is only necessary to override this if the media > > + // engine does not use real numbers to represent media time. > > + virtual float mediaTimeForTimeValue(float timeValue) const { return timeValue; } > > What does "real numbers" mean? I assume you don't mean it in the mathematical sense? > I meant non-rational numbers. I will update the comment. > > WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:1063 > > + long mediaTimeValue = static_cast<long>(timeValue * timeScale); > > + return static_cast<float>(mediaTimeValue) / timeScale; > > I assume the multiplication, truncation, and division is intentional here? (It's probably the whole point of this function!) > Indeed the point is that some media engines us rational numbers for time values, so we need to convert the value into the one can be exactly represented in the movie's time scale. > > WebCore/platform/graphics/win/QTMovie.cpp:744 > > + if (!m_private->m_movie) > > + 0; > > This won't compile. > Hmm, it does compile though it clearly doesn't so what I intended!
Created attachment 72230 [details] Updated patch
http://trac.webkit.org/changeset/70814
http://trac.webkit.org/changeset/70814 might have broken Qt Linux Release
It appears this may have broken media/controls-drag-timebar.html on leopard. diff: --- /Volumes/Big/WebKit-BuildSlave/leopard-intel-release-tests/build/layout-test-results/media/controls-drag-timebar-expected.txt 2010-10-28 15:14:44.000000000 -0700 +++ /Volumes/Big/WebKit-BuildSlave/leopard-intel-release-tests/build/layout-test-results/media/controls-drag-timebar-actual.txt 2010-10-28 15:14:44.000000000 -0700 @@ -6,8 +6,8 @@ EVENT(seeked) Time: 0.3 EVENT(seeked) -Time: 1.1 +Time: 0.3 EVENT(seeked) -Time: 1.9 +Time: 1.1 END OF TEST
Reopened because the changes were rolled out in http://trac.webkit.org/changeset/70837.
Created attachment 72522 [details] Updated patch The test that failed last time should succeed now because it was updated so its results are not timing dependent, https://bugs.webkit.org/show_bug.cgi?id=48662. The only change in this patch is that the new test has been updated to not use setTimeout, it is not needed because media events are fired asynchronously.
Committed in http://trac.webkit.org/changeset/71039
Created attachment 72529 [details] webkit-patch upload fail
Comment on attachment 72529 [details] webkit-patch upload fail Sorry, webkit-patch upload attached to the wrong bug...