Bug 41340 – [GStreamer] Subtle race condition during seeks

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Sebastian Dröge (slomo)]

Hi,
attached patch fixes a race condition in WebKitWebSourceGStreamer.

The problem here is, that the callback from g_timeout_add() could be called before g_timeout_add() returns. This causes the seekID timeout ID to be set by 0 by the callback and afterwards to the timeout ID of the already called callback. This problem looks rather obscure but can really easy happen on multicore systems and causes the playback to stop.

The other, more minor problem, that is fixed by this patch is, that the source *must* not emit EOS if the HTTP stream finishes during a seek. Due to the multiple threads involved here this confuses GStreamer's appsink and again causes the playback to stop.

[Sebastian Dröge (slomo)]

The same can in theory happen for the other GTimeouts

[Sebastian Dröge (slomo)]

Created attachment 60002 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch

[WebKit Review Bot]

Attachment 60002 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:19:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:20:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

This looks good to me. I will test the patch later on.

For the ChangeLog you can remove the "No new test" line. Also please add a link to the bug. I usually prefill the entry with prepare-ChangeLog --bug 1234 [--git-commit 0123a] :)

[Philippe Normand]

Tested the patch. Without I could see the video pause just after the video "ad"... With the patch playback is smooth, no pausing

[Philippe Normand]

(In reply to comment #5)
> Tested the patch. Without I could see the video pause just after the video "ad"... With the patch playback is smooth, no pausing

Also tried seeking :)

[Sebastian Dröge (slomo)]

Created attachment 60100 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch

Better? :)

[Philippe Normand]

Yes, style bot is green, CCing Gustavo :)

[Gustavo Noronha (kov)]

Comment on attachment 60100 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch

WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:347
 +      priv->paused = FALSE;
The change makes sense to me, but this one seems to have been left out by mistake?

[Sebastian Dröge (slomo)]

Created attachment 60922 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch

[Gustavo Noronha (kov)]

Comment on attachment 60922 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch 

Looks fine now, thanks!

[WebKit Commit Bot]

Comment on attachment 60922 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch 

Clearing flags on attachment: 60922

Committed r62955: <http://trac.webkit.org/changeset/62955>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.