Bug 170003 - [GStreamer] Deadlock in MediaPlayerPrivateGStreamer::changePipelineState, web process often locks up on seeking in a youtube video that has already fully buffered
Summary: [GStreamer] Deadlock in MediaPlayerPrivateGStreamer::changePipelineState, web...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 172902 (view as bug list)
Depends on: 172918
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-23 05:27 PDT by Hussam Al-Tayeb
Modified: 2017-06-05 09:27 PDT (History)
6 users (show)

See Also:


Attachments
backtrace (120.86 KB, text/plain)
2017-03-23 05:27 PDT, Hussam Al-Tayeb
no flags Details
backtrace (this one covers all gst installed plugins) (123.77 KB, text/plain)
2017-03-23 05:29 PDT, Hussam Al-Tayeb
no flags Details
gdb output. (135.44 KB, text/plain)
2017-05-23 07:47 PDT, Hussam Al-Tayeb
no flags Details
Patch (6.80 KB, patch)
2017-06-05 01:53 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (7.29 KB, patch)
2017-06-05 04:47 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hussam Al-Tayeb 2017-03-23 05:27:42 PDT
Created attachment 305186 [details]
backtrace

Webkitwebprocess often locks up on seeking in a youtube video that has already fully buffered.

I ran:
gdb --batch --ex "thread apply all bt full" -pid=4550 &> /tmp/bt.txt where 4550 is the PID of WebKitWebProcess.


I am attaching the resultant backtrace.
Comment 1 Hussam Al-Tayeb 2017-03-23 05:29:58 PDT
Created attachment 305188 [details]
backtrace (this one covers all gst installed plugins)
Comment 2 Hussam Al-Tayeb 2017-05-04 11:33:42 PDT
Same issue with gstreamer 1.12.0
I'm not sure if this helps but youtube streams videos in segments if I understand correctly.
Comment 3 Michael Catanzaro 2017-05-04 12:15:45 PDT
Well the GStreamer portion of the backtrace is good: it's deadlocked on a mutex in gst_base_sink_change_state.

But there are no debug symbols for WebKit. Could you install those please? It would be good to get another backtrace with GStreamer 1.12 at any rate.
Comment 4 Hussam Al-Tayeb 2017-05-20 10:28:28 PDT
I managed to finish a debug build of webkit by unsetting the distribution DEBUG_CFLAGS, LDFLAGS, etc...
GDB isn't coopering (some weird warning) but I'll figure it out and hopefully post a new backtrace tonight.
Comment 5 Hussam Al-Tayeb 2017-05-23 07:47:08 PDT
Created attachment 311010 [details]
gdb output.

I don't know how good this is because gdb kept complaining. But this backtrace is with debug webkit.
Comment 6 Michael Catanzaro 2017-05-23 08:35:54 PDT
OK, that's good enough I suppose. It's a shame we can't see local variables inside WebKit (did you use -g1 or some low debugging level?), but we have function names and line numbers and that should be good enough.
Comment 7 Michael Catanzaro 2017-05-23 08:43:35 PDT
To be clear: don't spend any more time on this, it's good. ;)
Comment 8 Hussam Al-Tayeb 2017-05-26 09:13:59 PDT
Additionally, live youtube streams won't even start playing https://www.youtube.com/watch?v=7y7CLWArdFY
Comment 9 Michael Catanzaro 2017-06-04 08:05:06 PDT
*** Bug 172902 has been marked as a duplicate of this bug. ***
Comment 10 Carlos Garcia Campos 2017-06-05 01:22:41 PDT
I can't reproduce it either, but I think I know what's going on.

 - Thread 41. Takes preroll mutex in gst_base_sink_chain_main before calling ::render. In WebKit code, the render schedules a repaint in the main thread, and keeps waiting on draw mutex.

 - Main thread. pause() is requested causing the state change from PLAYING to PAUSE. That ends up changing the state of the video sink to in gst_base_sink_change_state(). It tries to take the preroll lock too, but Thread 41 hasn't released it yet, and it's waiting for us to do the repaint.

GStreamer actually handles this case, by calling ::unlock on the sink before trying to take the preroll lock. The problem is that WebKit is not canceling the draw timer and release the draw mutes in that case. The code path not using coordinated graphics does that indeed with its paint timer and mutex.

I'll prepare a patch, but I can't reproduce it.
Comment 11 Carlos Garcia Campos 2017-06-05 01:53:13 PDT
Created attachment 311998 [details]
Patch
Comment 12 Miguel Gomez 2017-06-05 02:17:47 PDT
Comment on attachment 311998 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:836
> +#if USE(GSTREAMER_GL) || USE(COORDINATED_GRAPHICS_THREADED)
> +    m_drawTimer.stop();
> +    LockHolder locker(m_drawMutex);
> +    m_drawCondition.notifyOne();
> +#endif

This is not going to work when GSTREAMER_GL is enabled. In that case neither drawTimer nor drawMutex or drawCondition are used, as we just push the video frame to the compositor and don't stop the gstreamer thread until the drawing is finished.
I would put this as #if !USE(GSTREAMER_GL) && USE(COORDINATED_GRAPHICS_THREADED)
Comment 13 Miguel Gomez 2017-06-05 03:12:17 PDT
(In reply to Miguel Gomez from comment #12)
> Comment on attachment 311998 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311998&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:836
> > +#if USE(GSTREAMER_GL) || USE(COORDINATED_GRAPHICS_THREADED)
> > +    m_drawTimer.stop();
> > +    LockHolder locker(m_drawMutex);
> > +    m_drawCondition.notifyOne();
> > +#endif
> 
> This is not going to work when GSTREAMER_GL is enabled. In that case neither
> drawTimer nor drawMutex or drawCondition are used, as we just push the video
> frame to the compositor and don't stop the gstreamer thread until the
> drawing is finished.
> I would put this as #if !USE(GSTREAMER_GL) &&
> USE(COORDINATED_GRAPHICS_THREADED)

Ah, forgot the case when GSTREAMER_GL is enabled and accelerated compositing disabled. In that case we do use the timer, mutex and condition. So we should keep the USE(GSTREAMER_GL) but check that the rendering is not being accelerated in the GSTREAMER_GL case.
Comment 14 Carlos Garcia Campos 2017-06-05 04:12:09 PDT
(In reply to Miguel Gomez from comment #13)
> (In reply to Miguel Gomez from comment #12)
> > Comment on attachment 311998 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=311998&action=review
> > 
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:836
> > > +#if USE(GSTREAMER_GL) || USE(COORDINATED_GRAPHICS_THREADED)
> > > +    m_drawTimer.stop();
> > > +    LockHolder locker(m_drawMutex);
> > > +    m_drawCondition.notifyOne();
> > > +#endif
> > 
> > This is not going to work when GSTREAMER_GL is enabled. In that case neither
> > drawTimer nor drawMutex or drawCondition are used, as we just push the video
> > frame to the compositor and don't stop the gstreamer thread until the
> > drawing is finished.
> > I would put this as #if !USE(GSTREAMER_GL) &&
> > USE(COORDINATED_GRAPHICS_THREADED)
> 
> Ah, forgot the case when GSTREAMER_GL is enabled and accelerated compositing
> disabled. In that case we do use the timer, mutex and condition. So we
> should keep the USE(GSTREAMER_GL) but check that the rendering is not being
> accelerated in the GSTREAMER_GL case.

I think it's harmless to stop the timer and notify the mutex in that case. That's what we do in the destructor. I'm going to update the patch to call cancelRepaint in the destructor indeed, to avoid the duplicated code.
Comment 15 Carlos Garcia Campos 2017-06-05 04:47:17 PDT
Created attachment 312005 [details]
Updated patch
Comment 16 Michael Catanzaro 2017-06-05 05:46:11 PDT
Comment on attachment 312005 [details]
Updated patch

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

I don't understand this code, but Miguel does and I'll trust his review, so rs=me.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:418
> +        0, // Only one parameter

You need to change this comment since it's zero parameters
Comment 17 Carlos Garcia Campos 2017-06-05 09:27:30 PDT
Committed r217786: <http://trac.webkit.org/changeset/217786>