Summary: | Seeking by very small increment doesn't generate 'seeked' event | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, aroben, eric, jamesr, jer.noble, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 48612 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Eric Carlson
2010-10-28 09:55:20 PDT
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 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...
|