Bug 162872

Summary: [GStreamer] Drain query support
Product: WebKit Reporter: Enrique Ocaña <eocanha>
Component: PlatformAssignee: Enrique Ocaña <eocanha>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, pnormand
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 157314    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

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 1 Enrique Ocaña 2016-10-03 11:28:28 PDT
Created attachment 290497 [details]
Patch
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 7 Enrique Ocaña 2016-10-05 09:21:21 PDT
Created attachment 290713 [details]
Patch
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 11 Enrique Ocaña 2016-10-16 11:26:04 PDT
Created attachment 291755 [details]
Patch
Comment 12 Enrique Ocaña 2016-10-26 01:18:01 PDT
Created attachment 292887 [details]
Patch
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.