Bug 48530 – Seeking by very small increment doesn't generate 'seeked' event

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 72200 [details]
Proposed patch

[Adam Roben (:aroben)]

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.

[Eric Carlson]

(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!

[Eric Carlson]

Created attachment 72230 [details]
Updated patch

[Eric Carlson]

http://trac.webkit.org/changeset/70814

[WebKit Review Bot]

http://trac.webkit.org/changeset/70814 might have broken Qt Linux Release

[James Robinson]

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

[Eric Carlson]

Reopened because the changes were rolled out in http://trac.webkit.org/changeset/70837.

[Eric Carlson]

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.

[Eric Carlson]

Committed in http://trac.webkit.org/changeset/71039

[Ilya Sherman]

Created attachment 72529 [details]
webkit-patch upload fail

[Ilya Sherman]

Comment on attachment 72529 [details]
webkit-patch upload fail

Sorry, webkit-patch upload attached to the wrong bug...