Bug 170003 – [GStreamer] Deadlock in MediaPlayerPrivateGStreamer::changePipelineState, web process often locks up on seeking in a youtube video that has already fully buffered

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

[Hussam Al-Tayeb]

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.

[Hussam Al-Tayeb]

Created attachment 305188 [details]
backtrace (this one covers all gst installed plugins)

[Hussam Al-Tayeb]

Same issue with gstreamer 1.12.0
I'm not sure if this helps but youtube streams videos in segments if I understand correctly.

[Michael Catanzaro]

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.

[Hussam Al-Tayeb]

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.

[Hussam Al-Tayeb]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

To be clear: don't spend any more time on this, it's good. ;)

[Hussam Al-Tayeb]

Additionally, live youtube streams won't even start playing https://www.youtube.com/watch?v=7y7CLWArdFY

[Michael Catanzaro]

*** Bug 172902 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 311998 [details]
Patch

[Miguel Gomez]

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)

[Miguel Gomez]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 312005 [details]
Updated patch

[Michael Catanzaro]

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

[Carlos Garcia Campos]

Committed r217786: <http://trac.webkit.org/changeset/217786>