Bug 189349

Summary: [GStreamer] Several media related tests timing out around the same revision
Product: WebKit Reporter: Miguel Gomez <magomez>
Component: WebKitGTKAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, cgarcia, pnormand
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch cgarcia: review+

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>