Bug 41340 - [GStreamer] Subtle race condition during seeks
Summary: [GStreamer] Subtle race condition during seeks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-29 03:56 PDT by Sebastian Dröge (slomo)
Modified: 2010-07-09 08:26 PDT (History)
5 users (show)

See Also:


Attachments
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch (5.40 KB, patch)
2010-06-29 04:01 PDT, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch (5.47 KB, patch)
2010-06-30 01:49 PDT, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch (5.47 KB, patch)
2010-07-08 11:58 PDT, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Dröge (slomo) 2010-06-29 03:56:48 PDT
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.
Comment 1 Sebastian Dröge (slomo) 2010-06-29 03:57:52 PDT
The same can in theory happen for the other GTimeouts
Comment 2 Sebastian Dröge (slomo) 2010-06-29 04:01:25 PDT
Created attachment 60002 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch
Comment 3 WebKit Review Bot 2010-06-29 04:03:21 PDT
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.
Comment 4 Philippe Normand 2010-06-29 05:04:50 PDT
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] :)
Comment 5 Philippe Normand 2010-06-29 06:15:45 PDT
Tested the patch. Without I could see the video pause just after the video "ad"... With the patch playback is smooth, no pausing
Comment 6 Philippe Normand 2010-06-29 06:20:33 PDT
(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 :)
Comment 7 Sebastian Dröge (slomo) 2010-06-30 01:49:40 PDT
Created attachment 60100 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch

Better? :)
Comment 8 Philippe Normand 2010-06-30 01:59:35 PDT
Yes, style bot is green, CCing Gustavo :)
Comment 9 Gustavo Noronha (kov) 2010-07-08 09:49:50 PDT
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?
Comment 10 Sebastian Dröge (slomo) 2010-07-08 11:58:52 PDT
Created attachment 60922 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch
Comment 11 Gustavo Noronha (kov) 2010-07-08 12:03:55 PDT
Comment on attachment 60922 [details]
0001-Bug-41340-GStreamer-Subtle-race-condition-during-see.patch 

Looks fine now, thanks!
Comment 12 WebKit Commit Bot 2010-07-09 08:26:26 PDT
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>
Comment 13 WebKit Commit Bot 2010-07-09 08:26:31 PDT
All reviewed patches have been landed.  Closing bug.