Bug 162872 – [GStreamer] Drain query support

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Enrique Ocaña]

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.

[Enrique Ocaña]

Created attachment 290497 [details]
Patch

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

We don't need to wait for other MSE patches to land this one, do we?

[Zan Dobersek]

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.

[Xabier Rodríguez Calvar]

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

[Philippe Normand]

Comment on attachment 290497 [details]
Patch

Adding the signal without any handler connected from the player makes little sense

[Enrique Ocaña]

Created attachment 290713 [details]
Patch

[Enrique Ocaña]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Comment on attachment 290713 [details]
Patch

Address the comments please.

[Enrique Ocaña]

Created attachment 291755 [details]
Patch

[Enrique Ocaña]

Created attachment 292887 [details]
Patch

[Enrique Ocaña]

Comment on attachment 292887 [details]
Patch

Clearing flags on attachment: 292887

Committed r207874: <http://trac.webkit.org/changeset/207874>

[Enrique Ocaña]

All reviewed patches have been landed.  Closing bug.