Bug 184925 – [GStreamer] Video freezes when GStreamerGL is not installed

• Component: WebKitGTK
• Status: RESOLVED FIXED

Video at https://vimeo.com/266002800 freezes part way. Works fine in other browsers.

WebKitGTK+ 2.18.6

Audio unaffected.

[Philippe Normand]

Can you attach a debug log please? GST_DEBUG_FILE=gst.log GST_DEBUG=3,webkit*:6
Which GStreamer version do you use?

[Philippe Normand]

Please also try the 2.20 release.

[Charlie Turner]

I couldn't reproduce any freeze in trunk.

Problem has changed. . . .

https://vimeo.com/266002800

WebKitGTK+ 2.20.2

Most recent updates from Ubuntu 18.4 repository (main, universe, restricted, multiverse, partner; security, updates, backports)

Epiphany 3.28.1, Surf 2.0, MiniBrowser (supplied with WebKit)
Video plays audio with blank (white upper 1/8, black lower 7/8) screen.

Firefox
Video plays as expected.

Can you attach a debug log please? GST_DEBUG_FILE=gst.log GST_DEBUG=3,webkit*:6

Not familiar. Where does this go?

Which GStreamer version do you use?

GStreamer 1.0

[Philippe Normand]

(In reply to bugzilla-ok from comment #5)
> Problem has changed. . . .
> 
> https://vimeo.com/266002800
> 
> WebKitGTK+ 2.20.2
> 
> Most recent updates from Ubuntu 18.4 repository (main, universe, restricted,
> multiverse, partner; security, updates, backports)
> 
Did you install gstreamer1.0-gl too?

> Epiphany 3.28.1, Surf 2.0, MiniBrowser (supplied with WebKit)
> Video plays audio with blank (white upper 1/8, black lower 7/8) screen.
> 
> Firefox
> Video plays as expected.
> 
> Can you attach a debug log please? GST_DEBUG_FILE=gst.log
> GST_DEBUG=3,webkit*:6
> 
> Not familiar. Where does this go?
> 

Those are environment variables you need to set when running epiphany from a terminal:

$ GST_DEBUG=3,webkit*:6 GST_DEBUG_FILE=gst.log epiphany ....

> Which GStreamer version do you use?
> 
> GStreamer 1.0

This is not the full version. But if you use Ubuntu 18.04 I assume you have GStreamer 1.14.0 ?

Yes, version 1.14.0

Ah, got it. . . .

And yes, that was the problem. Video works as expected with gstreamer1.0-gl installed. A missing dependency. Thanks :-)

[Michael Catanzaro]

Reopening because this needs to work without GStreamerGL installed.

[Michael Catanzaro]

Well, it's clear these are all duplicates, let's use just this one.

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

I marked the YouTube and Vidible bugs as duplicates.

Seems something breaks badly when GStreamerGL plugin is not installed. We do have to support that case for another couple years, for Ubuntu, so let's keep this bug open.

[Michael Catanzaro]

I think the problem is that WebKit assumes that if GStreamerGL is present at build time, it will also be present at runtime. I don't think that's true in Debian/Ubuntu.

[Philippe Normand]

Ubuntu already (supposedly) switched to GstGL in their latest LTS release (AFAIK).
I suspect this is a downstream issue, I already mentioned to the Ubuntu wkgtk packager that gstreamer-gl-1.0 should be a dependency of their webkitgtk package. So I suppose he forgot to make that change.

[Michael Catanzaro]

Jeremy, could you comment please?

[Jeremy Bicha]

On Ubuntu 18.04 LTS (and in Debian Testing), libwebkit2gtk-4.0-37 Recommends: gstreamer1.0-gl.

When did you install Ubuntu 18.04 LTS?

Did you disable installing recommends?

[Michael Catanzaro]

I'm going to close this since it seems we actually do not expect video playback to work without GStreamerGL installed.

[Philippe Normand]

(In reply to Michael Catanzaro from comment #17)
> I'm going to close this since it seems we actually do not expect video
> playback to work without GStreamerGL installed.

Reopening. The player should be able to fallback if gstreamer-1.0-opengl isn't available at runtime.

[Philippe Normand]

Created attachment 342538 [details]
Patch

[Philippe Normand]

Miguel, Zan, please have a look!

[EWS Watchlist]

Comment on attachment 342538 [details]
Patch

Attachment 342538 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8148151

New failing tests:
svg/dom/animated-tearoff-list-remove-target.html

[EWS Watchlist]

Created attachment 342552 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 342538 [details]
Patch

Attachment 342538 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8157306

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 342637 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Philippe Normand]

Created attachment 342643 [details]
Patch

[Philippe Normand]

Same patch with a g_warning().
EWS errors are false-positive.
Please review ;)

[Zan Dobersek]

Comment on attachment 342643 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:212
> +        m_buffer = gst_sample_get_buffer(sample);

How is this GstBuffer ensured to live through the GstVideoFrameHolder lifetime? Do/Should we rely on the reference that gst_video_frame_map() takes?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:226
> +            m_flags = flags | (GST_VIDEO_INFO_HAS_ALPHA(&videoInfo) ? BitmapTexture::SupportsAlpha : BitmapTexture::NoFlag);

Not really in favor of mixing BitmapTexture and TextureMapperGL flags in one value.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-842
> -#if USE(GSTREAMER_GL)
> -    pushTextureToCompositor();
> -#else
>      {
>          LockHolder lock(m_drawMutex);
>          if (!m_platformLayerProxy->scheduleUpdateOnCompositorThread([this] { this->pushTextureToCompositor(); }))
>              return;
> +        m_drawTimer.startOneShot(0_s);
>          m_drawCondition.wait(m_drawMutex);
>      }
> -#endif

Why is this necessary for gstreamer-gl-enabled cases? Before, this was easily done in the gstreamer thread where the repaint callback was dispatched.

Why does now everything have to be managed on the composition thread unconditionally, and with the m_drawTimer being dispatched on top of that?

[Philippe Normand]

Comment on attachment 342643 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:212
>> +        m_buffer = gst_sample_get_buffer(sample);
> 
> How is this GstBuffer ensured to live through the GstVideoFrameHolder lifetime? Do/Should we rely on the reference that gst_video_frame_map() takes?

I think there's no need for an extra ref, as the video frame already keeps one indeed.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:226
>> +            m_flags = flags | (GST_VIDEO_INFO_HAS_ALPHA(&videoInfo) ? BitmapTexture::SupportsAlpha : BitmapTexture::NoFlag);
> 
> Not really in favor of mixing BitmapTexture and TextureMapperGL flags in one value.

It's not a mix in this line, it's a reassignment.
Or are you complaining about the enum type of m_flags?

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-842
>> -#endif
> 
> Why is this necessary for gstreamer-gl-enabled cases? Before, this was easily done in the gstreamer thread where the repaint callback was dispatched.
> 
> Why does now everything have to be managed on the composition thread unconditionally, and with the m_drawTimer being dispatched on top of that?

No good reason I suppose. I'll enable this code path only for the non-gl sink.

[Philippe Normand]

Created attachment 342644 [details]
Patch

[Philippe Normand]

Created attachment 342647 [details]
Patch

[Philippe Normand]

Committed r232790: <https://trac.webkit.org/changeset/232790>