Bug 184925 - [GStreamer] Video freezes when GStreamerGL is not installed
Summary: [GStreamer] Video freezes when GStreamerGL is not installed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: All Linux
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
: 185360 185576 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-04-24 11:59 PDT by bugzilla-ok
Modified: 2018-06-13 06:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (11.90 KB, patch)
2018-06-12 08:28 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.75 MB, application/zip)
2018-06-12 09:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews205 for win-future (12.82 MB, application/zip)
2018-06-12 23:16 PDT, EWS Watchlist
no flags Details
Patch (12.24 KB, patch)
2018-06-13 02:55 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (12.33 KB, patch)
2018-06-13 04:22 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (12.40 KB, patch)
2018-06-13 04:56 PDT, Philippe Normand
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description bugzilla-ok 2018-04-24 11:59:48 PDT
Video at https://vimeo.com/266002800 freezes part way. Works fine in other browsers.

WebKitGTK+ 2.18.6
Comment 1 bugzilla-ok 2018-04-24 16:23:52 PDT
Audio unaffected.
Comment 2 Philippe Normand 2018-04-25 01:37:06 PDT
Can you attach a debug log please? GST_DEBUG_FILE=gst.log GST_DEBUG=3,webkit*:6
Which GStreamer version do you use?
Comment 3 Philippe Normand 2018-04-25 01:37:38 PDT
Please also try the 2.20 release.
Comment 4 Charlie Turner 2018-05-01 03:50:50 PDT
I couldn't reproduce any freeze in trunk.
Comment 5 bugzilla-ok 2018-05-09 13:20:42 PDT
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
Comment 6 Philippe Normand 2018-05-09 14:08:01 PDT
(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 ?
Comment 7 bugzilla-ok 2018-05-13 15:22:55 PDT
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 :-)
Comment 8 Michael Catanzaro 2018-05-14 06:29:00 PDT
Reopening because this needs to work without GStreamerGL installed.
Comment 9 Michael Catanzaro 2018-05-14 06:30:38 PDT
Well, it's clear these are all duplicates, let's use just this one.
Comment 10 Michael Catanzaro 2018-05-14 06:31:04 PDT
*** Bug 185576 has been marked as a duplicate of this bug. ***
Comment 11 Michael Catanzaro 2018-05-14 06:31:22 PDT
*** Bug 185360 has been marked as a duplicate of this bug. ***
Comment 12 Michael Catanzaro 2018-05-14 06:32:44 PDT
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.
Comment 13 Michael Catanzaro 2018-05-14 06:50:07 PDT
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.
Comment 14 Philippe Normand 2018-05-14 06:51:15 PDT
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.
Comment 15 Michael Catanzaro 2018-05-14 07:45:56 PDT
Jeremy, could you comment please?
Comment 16 Jeremy Bicha 2018-06-05 13:54:35 PDT
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?
Comment 17 Michael Catanzaro 2018-06-06 15:23:39 PDT
I'm going to close this since it seems we actually do not expect video playback to work without GStreamerGL installed.
Comment 18 Philippe Normand 2018-06-12 05:30:37 PDT
(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.
Comment 19 Philippe Normand 2018-06-12 08:28:26 PDT
Created attachment 342538 [details]
Patch
Comment 20 Philippe Normand 2018-06-12 08:29:09 PDT
Miguel, Zan, please have a look!
Comment 21 EWS Watchlist 2018-06-12 09:40:32 PDT
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
Comment 22 EWS Watchlist 2018-06-12 09:40:34 PDT
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
Comment 23 EWS Watchlist 2018-06-12 23:15:54 PDT
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
Comment 24 EWS Watchlist 2018-06-12 23:16:06 PDT
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
Comment 25 Philippe Normand 2018-06-13 02:55:54 PDT
Created attachment 342643 [details]
Patch
Comment 26 Philippe Normand 2018-06-13 02:56:51 PDT
Same patch with a g_warning().
EWS errors are false-positive.
Please review ;)
Comment 27 Zan Dobersek 2018-06-13 03:59:41 PDT
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?
Comment 28 Philippe Normand 2018-06-13 04:21:36 PDT
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.
Comment 29 Philippe Normand 2018-06-13 04:22:57 PDT
Created attachment 342644 [details]
Patch
Comment 30 Philippe Normand 2018-06-13 04:56:58 PDT
Created attachment 342647 [details]
Patch
Comment 31 Philippe Normand 2018-06-13 06:39:44 PDT
Committed r232790: <https://trac.webkit.org/changeset/232790>