Bug 87575 - [GStreamer] http/tests/media/video-buffered-range-contains-currentTime.html is failing
Summary: [GStreamer] http/tests/media/video-buffered-range-contains-currentTime.html i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-26 00:09 PDT by Zan Dobersek
Modified: 2012-06-12 09:15 PDT (History)
10 users (show)

See Also:


Attachments
Patch (37.98 KB, patch)
2012-06-12 05:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.04 KB, patch)
2012-06-12 06:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2012-05-26 00:09:16 PDT
http/tests/media/video-buffered-range-contains-currentTime.html is failing since it has been introduced in r118577.
http://trac.webkit.org/changeset/118577/

Failures occur on all the Gtk builders, the diff looking like this:
--- /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/http/tests/media/video-buffered-range-contains-currentTime-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/http/tests/media/video-buffered-range-contains-currentTime-actual.txt 
@@ -1,7 +1,6 @@
+FAIL: Timed out waiting for notifyDone to be called
 Test that the painted buffered range contains currentTime.
 
 EVENT(loadedmetadata)
 EVENT(seeked)
-EVENT(ended)
-END OF TEST
Comment 1 Chris Dumez 2012-05-27 23:04:23 PDT
The test also fails on EFL port with the same diff.
Comment 2 Ami Fischman 2012-05-28 09:27:33 PDT
What the test does is:
- video.load()
- onloadedmetadata: seek to 0.5s short of the end of the video
- onseeked: start video.play()
- onended: finish the test

The diff posted shows 'seeked' is fired (so video.play() is called) but it times out before 'ended' is fired.  Some possibilities for why this can happen are:
1) The ports never fire 'ended'.  I'd expect that to break lots of media tests but glancing through efl/Skipped I see lots of media tests skipped.  Can someone who knows the port confirm that ended *is* fired for some (green) test that relies on it?
2) Seeking takes too long, eating most of the timeout interval for the overall test, so playing another 0.5s of video times out.  This can happen if there are no keyframes/index in the media format chosen for the port or if the ports' media engines can't take advantage of fast seeking in the media.  If this is the case the test should pass reliably when run with a longer timeout.

Can someone who knows one or both of these ports try these out?
Comment 3 Philippe Normand 2012-05-28 10:29:15 PDT
(In reply to comment #2)
> What the test does is:
> - video.load()
> - onloadedmetadata: seek to 0.5s short of the end of the video
> - onseeked: start video.play()
> - onended: finish the test
> 
> The diff posted shows 'seeked' is fired (so video.play() is called) but it times out before 'ended' is fired.  Some possibilities for why this can happen are:
> 1) The ports never fire 'ended'.  I'd expect that to break lots of media tests but glancing through efl/Skipped I see lots of media tests skipped.  Can someone who knows the port confirm that ended *is* fired for some (green) test that relies on it?

GTK and EFL both use the same MediaPlayerPrivateGStreamer engine. Not sure about EFL but GTK passes a fair amount of media tests :)
And yes the 'ended' event is fired, though there might issues in some cases.

> 2) Seeking takes too long, eating most of the timeout interval for the overall test, so playing another 0.5s of video times out.  This can happen if there are no keyframes/index in the media format chosen for the port or if the ports' media engines can't take advantage of fast seeking in the media.  If this is the case the test should pass reliably when run with a longer timeout.
> 
> Can someone who knows one or both of these ports try these out?

Well in our case I guess the test.wav file is used. Is there any reason why the test loads an audio file BTW?

var mediaFile = findMediaFile("audio", "../../../media/content/test");
Comment 4 Ami Fischman 2012-05-28 11:23:44 PDT
(In reply to comment #3)
> Well in our case I guess the test.wav file is used. Is there any reason why the test loads an audio file BTW?

Yes.  For the test to be effective in testing the new functionality introduced in its CL it must be able to set up the condition that the buffered range containing currentTime is not a singleton range starting from 0 and extending past the currentTime.  It's quite fiddly to set that up that condition with video media because by the time metadata is considered loaded quite a lot of the video data is buffered as well (because of how the videos are encoded, and because the video duration must be quite small to keep file size in svn down).
With audio, on the other hand, metadataloaded is fired quite early, while most of the file is not yet buffered, and it's much easier to trigger the condition that separate ranges are reported buffered.
Comment 5 Chris Dumez 2012-06-12 04:28:38 PDT
In this particular case, the "ended" event is not fired because the EOS detection fails. In HTMLMediaElement::mediaPlayerTimeChanged), it detects EOS by doing the following comparison: "currentTime() >= duration()". Unfortunately, due to some precision problem, this check fails for us.

My proposal is to tweak MediaPlayerPrivateGStreamer::currentTime() so that it returns duration() if m_isEndReached is true and if m_playbackRate > 1.
There is already such tweak for playbackRate < 0 so I'm merely extending it.
Comment 6 Chris Dumez 2012-06-12 05:20:21 PDT
Created attachment 147062 [details]
Patch
Comment 7 Chris Dumez 2012-06-12 05:41:56 PDT
Comment on attachment 147062 [details]
Patch

Clearing flags for now as this seems to cause some other tests to fail.
Comment 8 Chris Dumez 2012-06-12 06:09:10 PDT
Created attachment 147068 [details]
Patch

Synchronize the position and the duration in MediaPlayerPrivateGStreamer::didEnd() instead. This no longers causes any regressions.
Comment 9 Philippe Normand 2012-06-12 08:12:38 PDT
Comment on attachment 147068 [details]
Patch

Merci Christophe!
Comment 10 WebKit Review Bot 2012-06-12 09:15:19 PDT
Comment on attachment 147068 [details]
Patch

Clearing flags on attachment: 147068

Committed r120082: <http://trac.webkit.org/changeset/120082>
Comment 11 WebKit Review Bot 2012-06-12 09:15:25 PDT
All reviewed patches have been landed.  Closing bug.