Bug 189349 - [GStreamer] Several media related tests timing out around the same revision
Summary: [GStreamer] Several media related tests timing out around the same revision
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
: 189340 189341 189342 189346 189348 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-09-06 07:46 PDT by Miguel Gomez
Modified: 2018-09-10 02:10 PDT (History)
4 users (show)

See Also:

Attachments
Patch (6.05 KB, patch)
2018-09-07 11:05 PDT, Philippe Normand 		cgarcia: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Miguel Gomez 2018-09-06 07:46:14 PDT 
media/media-ended.html [ Timeout ]
  media/media-fragments/TC0004.html [ Timeout ]
  media/media-fragments/TC0017.html [ Timeout ]
  media/video-currentTime-delay.html [ Timeout ]
  media/video-currentTime-set.html [ Timeout ]

I already reported bugs for the flakiness of these tests, but I suspect there might be something common to all of them that could be related to their timeouts. All the timeouts started to happen around r234983 (at least one at that revision exactly), which makes me think that it could be related.
Checking that revision I see that it's precisely media related: [GStreamer] reduce position queries frequency, so that could have something to do with the problem.
Comment 1 Miguel Gomez 2018-09-06 07:47:07 PDT 
Phil, could you give it a look and check whether your change could be causing the timeouts?

If not, just call me crazy and close the bug ;)
Comment 2 Philippe Normand 2018-09-07 05:01:45 PDT 
It's possible yes :) I'll try to check this, soon...
Comment 3 Philippe Normand 2018-09-07 10:50:45 PDT 
I almost have a patch.
Comment 4 Philippe Normand 2018-09-07 10:51:30 PDT 
*** Bug 189340 has been marked as a duplicate of this bug. ***
Comment 5 Philippe Normand 2018-09-07 10:51:51 PDT 
*** Bug 189341 has been marked as a duplicate of this bug. ***
Comment 6 Philippe Normand 2018-09-07 10:52:11 PDT 
*** Bug 189342 has been marked as a duplicate of this bug. ***
Comment 7 Philippe Normand 2018-09-07 10:52:39 PDT 
*** Bug 189346 has been marked as a duplicate of this bug. ***
Comment 8 Philippe Normand 2018-09-07 10:52:58 PDT 
*** Bug 189348 has been marked as a duplicate of this bug. ***
Comment 9 Philippe Normand 2018-09-07 11:05:00 PDT 
Created attachment 349163 [details]
Patch
Comment 10 Carlos Garcia Campos 2018-09-09 23:43:55 PDT 
Comment on attachment 349163 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [GTK] Several media related tests timing out around the same revision

Why is this GTK specific?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:348
> -    if (m_lastQuery > -1 && ((now - m_lastQuery) < 300) && m_cachedPosition.isValid())
> +    if (m_lastQuery > -1 && ((now - m_lastQuery) < 200) && m_cachedPosition.isValid())

This would be easier to read if m_lastQuery was a std::optional<Seconds> (and probably renamed to m_lastQuertTime) and now was Seconds too. I would also avoid the magic number, giving 200 a name:

static const Seconds positionCacheThreshold = 200_ms;
Seconds now = WTF::WallTime::now().secondsSinceEpoch();
if (m_lastQueryTime && (now - m_lastQueryTime.value()) < positionCacheThreshold && m_cachedPosition.isValid())

something like that
Comment 11 Philippe Normand 2018-09-10 02:10:50 PDT 
Committed r235846: <https://trac.webkit.org/changeset/235846>