|Summary:||[GStreamer] Drain query support|
|Product:||WebKit||Reporter:||Enrique Ocaña <eocanha>|
|Component:||Platform||Assignee:||Enrique Ocaña <eocanha>|
|Version:||WebKit Local Build|
|Bug Depends on:|
Description Enrique Ocaña 2016-10-03 11:22:57 PDT
Under some circumstances, GStreamer deadlocks completely during MSE seeks on OpenGL ES systems. This is because the video sink still holds samples. Proper DRAIN support would fix this issue.
Comment 2 Xabier Rodríguez Calvar 2016-10-04 06:32:59 PDT
Comment on attachment 290497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290497&action=review These things can be fixed at landing time. No need to upload the patch again. > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:374 > + WebKitVideoSinkPrivate* priv = sink->priv; > + priv->scheduler.drain(); Since you are using priv just once, you can collapse these two lines. > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:432 > + 0, // Class offset > + 0, // Accumulator > + 0, // Accumulator data > + g_cclosure_marshal_generic, > + G_TYPE_NONE, // Return type > + 0); // No parameters Add period at the end of the comments.
Comment 3 Xabier Rodríguez Calvar 2016-10-04 06:34:18 PDT
We don't need to wait for other MSE patches to land this one, do we?
Comment 4 Zan Dobersek 2016-10-04 06:55:48 PDT
Comment on attachment 290497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290497&action=review > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:376 > + LOG_MEDIA_MESSAGE("Flush-start, emitting DRAIN signal and releasing EGL samples"); This shouldn't build -- LOG_MEDIA_MESSAGE stuff was removed.
Comment 5 Xabier Rodríguez Calvar 2016-10-04 07:25:34 PDT
(In reply to comment #4) > > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:376 > > + LOG_MEDIA_MESSAGE("Flush-start, emitting DRAIN signal and releasing EGL samples"); > > This shouldn't build -- LOG_MEDIA_MESSAGE stuff was removed. Yes, I missed that, sorry. Still, this can be fixed at landing time.
Comment 6 Philippe Normand 2016-10-05 05:28:58 PDT
Comment on attachment 290497 [details] Patch Adding the signal without any handler connected from the player makes little sense
Comment 8 Enrique Ocaña 2016-10-05 09:25:50 PDT
Addressed all the suggestions, added VideoRenderRequestScheduler::drain() to release the internal m_sample and subscribed to the drain signal from the base player to release the other m_sample held by the player. Now it really does a proper drain, particularly on flushes. The changes are relevant enough for a review.
Comment 9 Zan Dobersek 2016-10-05 09:46:30 PDT
Comment on attachment 290713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290713&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:615 > + m_videoSize.setWidth(0); > + m_videoSize.setHeight(0); Just assign a new FloatSize() to m_videoSize, essentially resetting it to (0, 0). > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:389 > + break; Unnecessary.
Comment 10 Zan Dobersek 2016-10-06 06:50:15 PDT
Comment on attachment 290713 [details] Patch Address the comments please.
Comment 13 Enrique Ocaña 2016-10-26 01:40:02 PDT
Comment on attachment 292887 [details] Patch Clearing flags on attachment: 292887 Committed r207874: <http://trac.webkit.org/changeset/207874>
Comment 14 Enrique Ocaña 2016-10-26 01:40:10 PDT
All reviewed patches have been landed. Closing bug.