Bug 201422 - REGRESSION(r249428): [GStreamer] VP9 video rendered green
Summary: REGRESSION(r249428): [GStreamer] VP9 video rendered green
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
: 203459 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-09-03 08:26 PDT by Philippe Normand
Modified: 2019-11-19 01:00 PST (History)
21 users (show)

See Also:


Attachments
Patch (2.00 KB, patch)
2019-09-06 04:07 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (5.88 KB, patch)
2019-10-03 06:08 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (5.52 KB, patch)
2019-10-06 06:30 PDT, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch (2.47 KB, patch)
2019-10-28 02:52 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (2.75 KB, patch)
2019-10-30 06:16 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2019-11-05 09:39 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-01 for mac-highsierra (3.64 MB, application/zip)
2019-11-06 01:20 PST, WebKit Commit Bot
no flags Details
Patch (4.31 KB, patch)
2019-11-18 10:28 PST, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2019-11-18 10:43 PST, Thibault Saunier
tsaunier: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2019-09-03 08:26:48 PDT
VP9 videos available on Youtube TV are rendered in opaque green. vp9dec is the video decoder used. H.264 playback is OK though.
Comment 1 Chris Lord 2019-09-06 04:07:17 PDT
Created attachment 378181 [details]
Patch
Comment 2 Philippe Normand 2019-09-06 08:23:37 PDT
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.
Comment 3 Chris Lord 2019-09-09 03:32:55 PDT
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 4 Víctor M. Jáquez L. 2019-09-17 05:32:23 PDT
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.
Comment 5 Chris Lord 2019-09-17 06:59:39 PDT
(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.
Comment 6 Philippe Normand 2019-09-18 08:27:38 PDT
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.
Comment 7 Chris Lord 2019-09-18 08:33:11 PDT
(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 :)
Comment 8 Philippe Normand 2019-09-18 18:11:42 PDT
Yes I meant this is a pure-GStreamer issue, no workaround should be needed in WebKit, IMHO :)
Comment 9 Philippe Normand 2019-10-03 04:56:36 PDT
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?
Comment 10 Thibault Saunier 2019-10-03 06:08:24 PDT
Created attachment 380103 [details]
Patch
Comment 11 Philippe Normand 2019-10-03 06:15:51 PDT
Comment on attachment 380103 [details]
Patch

Thanks :)
Comment 12 WebKit Commit Bot 2019-10-03 07:00:53 PDT
Comment on attachment 380103 [details]
Patch

Clearing flags on attachment: 380103

Committed r250652: <https://trac.webkit.org/changeset/250652>
Comment 13 WebKit Commit Bot 2019-10-03 07:00:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-10-03 07:02:18 PDT
<rdar://problem/55945741>
Comment 15 Alicia Boya García 2019-10-06 06:30:45 PDT
Reopening to attach new patch.
Comment 16 Alicia Boya García 2019-10-06 06:30:48 PDT
Created attachment 380296 [details]
Patch
Comment 17 Alicia Boya García 2019-10-06 06:34:25 PDT
Oops. Collision. :)
Comment 18 Charlie Turner 2019-10-26 18:11:58 PDT
*** Bug 203459 has been marked as a duplicate of this bug. ***
Comment 19 Charlie Turner 2019-10-26 18:12:40 PDT
This should be backported to our release branches, since it's affecting users of the ephy tech preview.
Comment 20 Michael Catanzaro 2019-10-27 06:15:03 PDT
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.
Comment 21 Michael Catanzaro 2019-10-27 06:20:43 PDT
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.
Comment 22 Charlie Turner 2019-10-27 23:38:19 PDT
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!
Comment 23 Philippe Normand 2019-10-28 02:52:39 PDT
Reopening to attach new patch.
Comment 24 Philippe Normand 2019-10-28 02:52:42 PDT
Created attachment 382056 [details]
Patch
Comment 25 Thibault Saunier 2019-10-28 03:45:33 PDT
Comment on attachment 382056 [details]
Patch

Informal r+
Comment 26 Carlos Garcia Campos 2019-10-28 05:01:33 PDT
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?
Comment 27 Philippe Normand 2019-10-30 06:16:26 PDT
Created attachment 382300 [details]
Patch
Comment 28 WebKit Commit Bot 2019-10-30 07:14:18 PDT
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 29 WebKit Commit Bot 2019-10-30 07:15:06 PDT
Comment on attachment 382300 [details]
Patch

Clearing flags on attachment: 382300

Committed r251772: <https://trac.webkit.org/changeset/251772>
Comment 30 WebKit Commit Bot 2019-10-30 07:15:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Miguel Gomez 2019-10-31 05:01:20 PDT
Reverted r251772 for reason:

Caused lots of media related tests to timeout

Committed r251838: <https://trac.webkit.org/changeset/251838>
Comment 32 Carlos Garcia Campos 2019-10-31 07:39:48 PDT
Maybe we also need to remove the gst patch?
Comment 33 Philippe Normand 2019-11-05 08:19:36 PST
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...
Comment 34 Philippe Normand 2019-11-05 09:39:03 PST
Created attachment 382829 [details]
Patch
Comment 35 WebKit Commit Bot 2019-11-06 01:20:47 PST
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
Comment 36 WebKit Commit Bot 2019-11-06 01:20:50 PST
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
Comment 37 Carlos Garcia Campos 2019-11-06 01:39:08 PST
Committed r252134: <https://trac.webkit.org/changeset/252134>
Comment 38 Thibault Saunier 2019-11-18 10:28:04 PST
Reopening to attach new patch.
Comment 39 Thibault Saunier 2019-11-18 10:28:07 PST
Created attachment 383758 [details]
Patch
Comment 40 Thibault Saunier 2019-11-18 10:43:19 PST
Created attachment 383761 [details]
Patch
Comment 41 Carlos Garcia Campos 2019-11-19 01:00:08 PST
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.