[GStreamer] usec rounding is wrong during accurate seeking

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

The current approach is dummy :( We should switch to a floor()-based rounding solution.

[Philippe Normand]

With the current implementation 6.027200 seconds rounds to 6.030000. Kind of far fetched rounding.

[Philippe Normand]

Created attachment 151154 [details]
Patch

[Martin Robinson]

Can you give an example of how this affects rounding? Before, 6.0272 was rounded to 6.03. What is it rounded to now? Does this affect tests at all?

[Philippe Normand]

(In reply to comment #3)
> Can you give an example of how this affects rounding? Before, 6.0272 was rounded to 6.03.

Yes. Example when running media/video-seek-past-end-playing.html

Seek to 6.027200: 6.000000 secs 30000 usecs
Seek: 0:00:06.030000000

> What is it rounded to now?

Now, same test with the patch:

Seek to 6.027200: 6.000000 secs 27200 usecs
Seek: 0:00:06.027200000

> Does this affect tests at all?

Yes, well with the patch media/video-frame-accurate-seek.html still passes.
The issue with the previous rounding "strategy" is that some values were correctly rounded and some not, as you can see above. Maybe I can update the layout test!

[Philippe Normand]

Created attachment 153007 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153007 [details]
Patch

Attachment 153007 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13273552

New failing tests:
media/video-frame-accurate-seek.html

[WebKit Review Bot]

Created attachment 153014 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Philippe Normand]

Created attachment 153019 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153019 [details]
Patch

Attachment 153019 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13282548

New failing tests:
media/video-frame-accurate-seek.html

[WebKit Review Bot]

Created attachment 153023 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Philippe Normand]

Comment on attachment 153019 [details]
Patch

I need to rework/rebase this patch

[Philippe Normand]

I think there might be some small bugs in the GStreamer decoders, making sure currentTime() is as accurate as the seek method I can still see the frame displayed is either the one preceding or following the one requested. The video-frame-accurate-seek test actually shows the issue too, 3rd video should show frame #14 but shows frame #15 instead.

[Philippe Normand]

Created attachment 198003 [details]
Patch

[Build Bot]

Comment on attachment 198003 [details]
Patch

Attachment 198003 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/47297

New failing tests:
http/tests/ssl/ping-with-unsafe-redirect.html
media/video-frame-accurate-seek.html

[Build Bot]

Created attachment 198030 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 198003 [details]
Patch

Attachment 198003 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/139189

New failing tests:
http/tests/ssl/ping-with-unsafe-redirect.html
media/video-frame-accurate-seek.html

[Build Bot]

Created attachment 198037 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 198003 [details]
Patch

Attachment 198003 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/30262

New failing tests:
media/video-frame-accurate-seek.html

[Build Bot]

Created attachment 198040 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Philippe Normand]

Created attachment 200245 [details]
Patch

[Philippe Normand]

Created attachment 238351 [details]
Patch

Rebased.

[Philippe Normand]

Comment on attachment 238351 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238351&action=review

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:158
> +    timeValue.tv_usec = static_cast<glong>(floor(microSeconds = 0.5));

oops.

[Philippe Normand]

Created attachment 238352 [details]
Patch

[Michael Catanzaro]

Comment on attachment 238352 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238352&action=review

The code changes in this patch look fine, but I don't understand the test.

> LayoutTests/media/video-frame-accurate-seek.html:8
> +        <p>Test that setting currentTime is frame-accurate. Four videos below should be showing up, The first three at frames 12, 13, and 14. The last video should display at 6.0272 seconds.</p>

The -> the

> LayoutTests/platform/gtk/media/video-frame-accurate-seek-expected.txt:9
> +          text run at (0,17) width 356: "and 14. The last video should display at 6.0272 seconds."

It seems like something is wrong with this test. The expected result for the first three videos shows 12, 13, and 15, but the test says it's expected to show 12, 13, and 14. The last video displays 6.01 seconds, not 6.0272 or 6.03.

> LayoutTests/platform/mac/TestExpectations:1392
> +webkit.org/b/90734 media/video-frame-accurate-seek.html [ Failure ]

I think you should file new bugs for the EFL and Mac rebaselines, else they probably won't happen if you add an expectation to hide the failures.

[Philippe Normand]

(In reply to comment #24)
> Comment on attachment 238352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=238352&action=review
> 
> The code changes in this patch look fine, but I don't understand the test.
> 
> > LayoutTests/media/video-frame-accurate-seek.html:8
> > +        <p>Test that setting currentTime is frame-accurate. Four videos below should be showing up, The first three at frames 12, 13, and 14. The last video should display at 6.0272 seconds.</p>
> 
> The -> the
> 
> > LayoutTests/platform/gtk/media/video-frame-accurate-seek-expected.txt:9
> > +          text run at (0,17) width 356: "and 14. The last video should display at 6.0272 seconds."
> 
> It seems like something is wrong with this test. The expected result for the
> first three videos shows 12, 13, and 15, but the test says it's expected to
> show 12, 13, and 14. The last video displays 6.01 seconds, not 6.0272 or
> 6.03.
> 

See comment 12.

[Philippe Normand]

(In reply to comment #24)
> Comment on attachment 238352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=238352&action=review
> 
> The code changes in this patch look fine, but I don't understand the test.
> 
> > LayoutTests/media/video-frame-accurate-seek.html:8
> > +        <p>Test that setting currentTime is frame-accurate. Four videos below should be showing up, The first three at frames 12, 13, and 14. The last video should display at 6.0272 seconds.</p>
> 
> The -> the
> 
> > LayoutTests/platform/gtk/media/video-frame-accurate-seek-expected.txt:9
> > +          text run at (0,17) width 356: "and 14. The last video should display at 6.0272 seconds."
> 
> It seems like something is wrong with this test. The expected result for the
> first three videos shows 12, 13, and 15, but the test says it's expected to
> show 12, 13, and 14. 

This now works as expected \o/

> The last video displays 6.01 seconds, not 6.0272 or
> 6.03.
> 

That's because 6.01 means (6 * 25) + 1 frames = 151. And not 0.01 seconds.

So we do a seek to 6.0272 seconds which means 6 seconds and one half-frame (0.02 = 1/50) but the video is at 25fps so the demuxer choses instead to jump to the next full frame, if I understand correctly.

[Philippe Normand]

Created attachment 282241 [details]
patch

[Philippe Normand]

Committed r202558: <http://trac.webkit.org/changeset/202558>