Bug 201422

Created attachment 378181 [details] Patch

Comment on attachment 378181 [details] Patch I confirm this approach works but I would prefer, if possible, to make glupload behave the same for all video formats. For vp9 the "Raw Data" uploader is selected, I suspect this is related with the issue at hand. For h264 the GLMemory uploader is selected and that works well. So I would like to understand why the glupload behavior differs.

Good call on r-'ing this, it seems to cause problems while seeking videos. Philippe's investigation shows that the failing vp9 videos are hitting the rawdata upload path rather than the glupload path, due to a buffer number mismatch. My theory was that this sync would be necessary anyway, as what's possibly happening is that we're missing a sync point when videos hit the passthrough path on glcolorconvert and the only reason it seems to be working is that because the glupload path uses a buffer pool, we get to see old frames without syncing. While I'm still not uncertain about the sync being necessary, testing shows that seeking videos with this patch can (easily) cause deadlock. Without this patch, the video display goes blank (as I suspected it might), so there may be an issue here. It looks like the synchronisation is necessary, but our current method isn't quite right.

Comment on attachment 378181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378181&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1215 > + gst_caps_set_features(caps.get(), 0, gst_caps_features_new(GST_CAPS_FEATURE_MEMORY_GL_MEMORY, GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META, nullptr)); It is better to avoid the use of GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META since it is going to be deprecated soon.

(In reply to Víctor M. Jáquez L. from comment #4) > Comment on attachment 378181 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378181&action=review > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1215 > > + gst_caps_set_features(caps.get(), 0, gst_caps_features_new(GST_CAPS_FEATURE_MEMORY_GL_MEMORY, GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META, nullptr)); > > It is better to avoid the use of > GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META since it is going to > be deprecated soon. Yes, I realise it doesn't really do what I thought it did. We essentially debugged far enough to settle on the idea that this bug is caused by a quirk of behaviour in the vpx decoder that means a different upload path is taken, one which doesn't provide sync meta. I've put it on hold to work on OffscreenCanvas for now, but my last thought for a work-around was to detect when there's no sync meta and use a locally allocated GStreamer buffer pool with the sync meta flag set and copy the buffer over. This is essentially what makes things work when they go through glcolorconvert (and even copying it, though not ideal, is still marginally less work than forcing an unnecessary colour conversion). I have most of the code lying around to try this out and once OffscreenCanvas work has gotten to a better stopping point, I'll try to write a new patch to fix this. Meanwhile, using gst-plugins-libav for vp9 decoding doesn't present this issue.

I think this would need to be solved: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/209 With a GL buffer pool in the "Raw Data" uploader it would be trivial to handle the GL sync meta.

(In reply to Philippe Normand from comment #6) > I think this would need to be solved: > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/209 > > With a GL buffer pool in the "Raw Data" uploader it would be trivial to > handle the GL sync meta. Agreed (on the GStreamer side though, I assume you mean). I'm happy to pass the buck to GStreamer on this one and not have any unnecessary work-around code in WebKit :)

Yes I meant this is a pure-GStreamer issue, no workaround should be needed in WebKit, IMHO :)

This was fixed in gst-plugins-base: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/commit/8d32de090554cf29fe359f83aa46000ba658a693 Can we have that patch in our JHBuild?

Created attachment 380103 [details] Patch

Comment on attachment 380103 [details] Patch Thanks :)

Comment on attachment 380103 [details] Patch Clearing flags on attachment: 380103 Committed r250652: <https://trac.webkit.org/changeset/250652>

All reviewed patches have been landed. Closing bug.

<rdar://problem/55945741>

Reopening to attach new patch.

Created attachment 380296 [details] Patch

Oops. Collision. :)

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

This should be backported to our release branches, since it's affecting users of the ephy tech preview.

No, the bug doesn't occur in 2.26.1. It only occurs with 2.27.2. Reopening since 2.27.2 is much newer than this fix.

Ah, I see the patch only touches the JHBuild moduleset (and so will have no effect on releases). Looks like the code in 2.27.2 depends on GStreamer 1.16.2 to function properly? I will hold Tech Preview to 2.26 until that is released, then. It's unfortunate to effectively add a new dependency on unreleased GStreamer, especially for LTS distros. Without knowing the technical details behind whatever changed in WebKit since 2.26, I assume you would avoid that if possible.

Yes, by release branches I was of course referring to GStreamer, whether we can get it back into the LTS verions of GStreamer remains to be seen. You assume correctly!

Reopening to attach new patch.

Created attachment 382056 [details] Patch

Comment on attachment 382056 [details] Patch Informal r+

Comment on attachment 382056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382056&action=review Currently building with the patch, I'll let you know when it finishes. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1208 > + // Workaround until GStreamer 1.16.2 shipping > + // https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/commit/8d32de090554cf29fe359f83aa46000ba658a693 > + // is released. See also https://bugs.webkit.org/show_bug.cgi?id=201422 We won't be able to remove this even when 1.16.2 is released, because we don't depend on gst 1.16.2. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1210 > + gst_caps_features_add(features, GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META); Could you explain in the comment why adding GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META?

Created attachment 382300 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 382300 [details]: imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3.html bug 203609 (author: cdumez@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 382300 [details] Patch Clearing flags on attachment: 382300 Committed r251772: <https://trac.webkit.org/changeset/251772>

All reviewed patches have been landed. Closing bug.

Reverted r251772 for reason: Caused lots of media related tests to timeout Committed r251838: <https://trac.webkit.org/changeset/251838>

Maybe we also need to remove the gst patch?

Comment on attachment 382300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382300&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1214 > + gst_caps_features_add(features, GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META); Actually this doesn't work because glupload doesn't accept the texture upload and the gl-mem caps features simultaneously. So caps negotiation is failure prone here. I have another patch proposal...

Created attachment 382829 [details] Patch

Comment on attachment 382829 [details] Patch Rejecting attachment 382829 [details] from commit-queue. New failing tests: webgl/1.0.3/conformance/extensions/webgl-draw-buffers.html Full output: https://webkit-queues.webkit.org/results/13218193

Created attachment 382904 [details] Archive of layout-test-results from webkit-cq-01 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-01 Port: mac-highsierra Platform: Mac OS X 10.13.6

Committed r252134: <https://trac.webkit.org/changeset/252134>

Reopening to attach new patch.

Created attachment 383758 [details] Patch

Created attachment 383761 [details] Patch

Comment on attachment 383761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383761&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1215 > + GRefPtr<GstCaps> caps = nullptr; No need to initialize to nullptr, the constructor already initializes the pointer.

Comment on attachment 383761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383761&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1218 > if (webkitGstCheckVersion(1, 16, 2) || getenv("WEBKIT_GST_NO_RGBA_CONVERSION")) { > - GRefPtr<GstPad> pad = adoptGRef(gst_element_get_static_pad(upload, "sink")); > - gst_element_add_pad(videoSink, gst_ghost_pad_new("sink", pad.get())); > + caps = adoptGRef(gst_caps_from_string("video/x-raw, format = (string) " GST_GL_CAPS_FORMAT)); > } else { {} can be removed because this is a one-line block now.

Created attachment 383857 [details] Patch for landing

Comment on attachment 383857 [details] Patch for landing Rejecting attachment 383857 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 383857, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/13266584

Created attachment 383859 [details] Patch for landing

Comment on attachment 383859 [details] Patch for landing Clearing flags on attachment: 383859 Committed r252624: <https://trac.webkit.org/changeset/252624>

All reviewed patches have been landed. Closing bug.