Bug 201422 – REGRESSION(r249428): [GStreamer] VP9 video rendered green

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

VP9 videos available on Youtube TV are rendered in opaque green. vp9dec is the video decoder used. H.264 playback is OK though.

[Chris Lord]

Created attachment 378181 [details]
Patch

[Philippe Normand]

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.

[Chris Lord]

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.

[Víctor M. Jáquez L.]

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.

[Chris Lord]

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

[Philippe Normand]

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.

[Chris Lord]

(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 :)

[Philippe Normand]

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

[Philippe Normand]

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?

[Thibault Saunier]

Created attachment 380103 [details]
Patch

[Philippe Normand]

Comment on attachment 380103 [details]
Patch

Thanks :)

[WebKit Commit Bot]

Comment on attachment 380103 [details]
Patch

Clearing flags on attachment: 380103

Committed r250652: <https://trac.webkit.org/changeset/250652>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55945741>

[Alicia Boya García]

Reopening to attach new patch.

[Alicia Boya García]

Created attachment 380296 [details]
Patch

[Alicia Boya García]

Oops. Collision. :)

[Charlie Turner]

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

[Charlie Turner]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Charlie Turner]

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!

[Philippe Normand]

Reopening to attach new patch.

[Philippe Normand]

Created attachment 382056 [details]
Patch

[Thibault Saunier]

Comment on attachment 382056 [details]
Patch

Informal r+

[Carlos Garcia Campos]

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?

[Philippe Normand]

Created attachment 382300 [details]
Patch

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 382300 [details]
Patch

Clearing flags on attachment: 382300

Committed r251772: <https://trac.webkit.org/changeset/251772>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Miguel Gomez]

Reverted r251772 for reason:

Caused lots of media related tests to timeout

Committed r251838: <https://trac.webkit.org/changeset/251838>

[Carlos Garcia Campos]

Maybe we also need to remove the gst patch?

[Philippe Normand]

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

[Philippe Normand]

Created attachment 382829 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Carlos Garcia Campos]

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

[Thibault Saunier]

Reopening to attach new patch.

[Thibault Saunier]

Created attachment 383758 [details]
Patch

[Thibault Saunier]

Created attachment 383761 [details]
Patch

[Carlos Garcia Campos]

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.

[Philippe Normand]

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.

[Thibault Saunier]

Created attachment 383857 [details]
Patch for landing

[WebKit Commit Bot]

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

[Thibault Saunier]

Created attachment 383859 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 383859 [details]
Patch for landing

Clearing flags on attachment: 383859

Committed r252624: <https://trac.webkit.org/changeset/252624>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.