Bug 200922 - [GStreamer] Add support to copy YUV video textures into images
Summary: [GStreamer] Add support to copy YUV video textures into images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords:
Depends on: 132869
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-20 08:42 PDT by Chris Lord
Modified: 2019-09-03 07:58 PDT (History)
12 users (show)

See Also:


Attachments
Patch (3.41 KB, patch)
2019-08-20 08:47 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (4.40 KB, patch)
2019-08-21 04:54 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (4.40 KB, patch)
2019-08-27 03:51 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (4.43 KB, patch)
2019-09-03 03:42 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (4.42 KB, patch)
2019-09-03 05:36 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2019-08-20 08:42:48 PDT
Bug 132869 adds shaders to render YUV video textures to avoid having to render to an extra off-screen surface. The patch on that bug only implements the playback portion and YUV videos become broken/non-working on accelerated and unaccelerated canvases. This bug tracks the work to enable YUV video textures in unaccelerated canvases (so unaccelerated 2D canvas).
Comment 1 Chris Lord 2019-08-20 08:47:47 PDT
Created attachment 376772 [details]
Patch
Comment 2 Philippe Normand 2019-08-20 09:28:00 PDT
Comment on attachment 376772 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:68
> +    GstGLContext* context = ((GstGLBaseMemory*)mem)->context;
> +    GRefPtr<GstGLColorConvert> colorConvert = adoptGRef(gst_gl_color_convert_new(context));

Would it make sense to keep the colorConvert in the player somehow? So that it doesn't need to be re-created for every video sample?
Or is the performance impact negligible here?
Comment 3 Chris Lord 2019-08-20 09:41:24 PDT
(In reply to Philippe Normand from comment #2)
> Comment on attachment 376772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376772&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:68
> > +    GstGLContext* context = ((GstGLBaseMemory*)mem)->context;
> > +    GRefPtr<GstGLColorConvert> colorConvert = adoptGRef(gst_gl_color_convert_new(context));
> 
> Would it make sense to keep the colorConvert in the player somehow? So that
> it doesn't need to be re-created for every video sample?
> Or is the performance impact negligible here?

Looking at the source, I think the bulk of the work in terms of allocation is done when setting the caps - so it would be ideal, assuming the caps don't change, for the object to live in MediaPlayerPrivateGStreamerBase.

I wasn't sure what the relationships are with regards to lifetimes though - is there a one-to-one mapping between one video stream and MediaPlayerPrivateGStreamerBase? Are the caps during playback guaranteed to stay the same? (I guess this isn't a big deal, could just take a reference on the last caps used)

I note that MediaPlayerPrivateGStreamerBase reuses a single VideoTextureCopier, but recreates the ImageGStreamer each time - would it make sense for it to keep an ImageGStreamer around to re-use so these objects could live there (which I think would be much nicer than an API kludge)?
Comment 4 Miguel Gomez 2019-08-21 01:19:46 PDT
(In reply to Chris Lord from comment #3)
> (In reply to Philippe Normand from comment #2)
> > Comment on attachment 376772 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=376772&action=review
> > 
> > > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:68
> > > +    GstGLContext* context = ((GstGLBaseMemory*)mem)->context;
> > > +    GRefPtr<GstGLColorConvert> colorConvert = adoptGRef(gst_gl_color_convert_new(context));
> > 
> > Would it make sense to keep the colorConvert in the player somehow? So that
> > it doesn't need to be re-created for every video sample?
> > Or is the performance impact negligible here?
> 
> Looking at the source, I think the bulk of the work in terms of allocation
> is done when setting the caps - so it would be ideal, assuming the caps
> don't change, for the object to live in MediaPlayerPrivateGStreamerBase.

Potentially several ImageGStreamer objects can be created for the same buffer, so it would be better to keep the colorConvert around.

> I wasn't sure what the relationships are with regards to lifetimes though -
> is there a one-to-one mapping between one video stream and
> MediaPlayerPrivateGStreamerBase? Are the caps during playback guaranteed to
> stay the same? (I guess this isn't a big deal, could just take a reference
> on the last caps used)

The player cannot play more than a single stream at the same time. It can be reused to play one stream after another, but it won't play more than that. During playback some caps can change (I'm thinking of adaptive streaming), but the color format caps should stay the same for each video (Phil, correct me if I'm wrong)

> I note that MediaPlayerPrivateGStreamerBase reuses a single
> VideoTextureCopier, but recreates the ImageGStreamer each time - would it
> make sense for it to keep an ImageGStreamer around to re-use so these
> objects could live there (which I think would be much nicer than an API
> kludge)?

VideoTextureCopier is supposed to be just a tool to copy a texture into another without keeping any internal state, so it can be easily reused. I'm not sure that's the case for ImageGStreamer. That's meant to hold a buffer with the video frame that can be kept around, and we are not interested in changing it when there might be other components still using it. Also, the current implementation of the createImageBitmap feature causes that ImageGStreamer objects can be created in secondary threads (to initialize the bitmap element from a video), so we need to be able to have several instances of ImageGStreamer for the same video (or properly handle multithread access to them).
Comment 5 Philippe Normand 2019-08-21 01:35:39 PDT
> 
> The player cannot play more than a single stream at the same time. It can be
> reused to play one stream after another, but it won't play more than that.
> During playback some caps can change (I'm thinking of adaptive streaming),
> but the color format caps should stay the same for each video (Phil, correct
> me if I'm wrong)
> 

Correct, Miguel :)
Comment 6 Chris Lord 2019-08-21 04:54:07 PDT
Created attachment 376863 [details]
Patch
Comment 7 Chris Lord 2019-08-21 04:56:23 PDT
This moves the changes into MediaPlayerPrivateGStreamerBase, which I think makes more sense. This lets us cache the colour-conversion object and also make sure the conversion only happens once if paint is called multiple times.

I also fixed a leak (whoops) and added support to omit the alpha channel if it doesn't exist so that the buffer can be directly reused without having to pre-multiply alpha unnecessarily.
Comment 8 Philippe Normand 2019-08-21 04:59:46 PDT
Comment on attachment 376863 [details]
Patch

lgtm :)
Comment 9 Chris Lord 2019-08-27 03:51:05 PDT
Created attachment 377335 [details]
Patch
Comment 10 Xabier Rodríguez Calvar 2019-08-29 08:26:13 PDT
Comment on attachment 377335 [details]
Patch

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

Wouldn't it be easier/possible to ensure we have a glcolorconvert element in the pipeline with the caps set to RGB and letting GStreamer do the job for us?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:979
> +            m_colorConvertLastCaps = caps;
> +            m_colorConvertCaps = adoptGRef(gst_caps_copy(caps));

If I understand this correctly, I think a better name for these would be:
m_colorConvertLastCaps -> m_colorConvertInputCaps
m_colorConvertCaps -> colorConvertOutputCaps

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:984
> +#if G_BYTE_ORDER == G_LITTLE_ENDIAN
> +            const gchar* formatString = GST_VIDEO_INFO_HAS_ALPHA(&videoInfo) ? "RGBA" : "BGRx";
> +#else
> +            const gchar* formatString = GST_VIDEO_INFO_HAS_ALPHA(&videoInfo) ? "RGBA" : "RGBx";
> +#endif

Are these format strings right? I have no clue, just asking because my pattern recognition does not yield a match here...

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:300
> +    GRefPtr<GstCaps> m_colorConvertCaps;

We don't need to hold this here since gst_gl_color_convert_set_caps will perform a gst_caps_replace which will ref them internally. We don't need them anymore once they are passed down to the element, right?
Comment 11 Chris Lord 2019-08-29 08:38:20 PDT
(In reply to Xabier Rodríguez Calvar from comment #10)
> Comment on attachment 377335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377335&action=review
> 
> Wouldn't it be easier/possible to ensure we have a glcolorconvert element in
> the pipeline with the caps set to RGB and letting GStreamer do the job for
> us?

This needs to be read in conjunction with the patch on bug #132869 - which changes the supported colour formats of the glcolorconvert that is in the pipeline to avoid having to do an unnecessary copy.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:979
> > +            m_colorConvertLastCaps = caps;
> > +            m_colorConvertCaps = adoptGRef(gst_caps_copy(caps));
> 
> If I understand this correctly, I think a better name for these would be:
> m_colorConvertLastCaps -> m_colorConvertInputCaps
> m_colorConvertCaps -> colorConvertOutputCaps

This sounds good.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:984
> > +#if G_BYTE_ORDER == G_LITTLE_ENDIAN
> > +            const gchar* formatString = GST_VIDEO_INFO_HAS_ALPHA(&videoInfo) ? "RGBA" : "BGRx";
> > +#else
> > +            const gchar* formatString = GST_VIDEO_INFO_HAS_ALPHA(&videoInfo) ? "RGBA" : "RGBx";
> > +#endif
> 
> Are these format strings right? I have no clue, just asking because my
> pattern recognition does not yield a match here...

In fairness, I've only tested the little-endian path, but I believe this to be correct - I think this is maintaining old behaviour, but would certainly appreciate testing if you have a big-endian device handy.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:300
> > +    GRefPtr<GstCaps> m_colorConvertCaps;
> 
> We don't need to hold this here since gst_gl_color_convert_set_caps will
> perform a gst_caps_replace which will ref them internally. We don't need
> them anymore once they are passed down to the element, right?

We hold onto it to be able to create a new sample - the colour conversion object provides no accessor for it (at least, no documented way that I saw, and it didn't look like it was possible reading the source either)
Comment 12 Xabier Rodríguez Calvar 2019-08-30 04:02:36 PDT
Comment on attachment 377335 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:300
>>> +    GRefPtr<GstCaps> m_colorConvertCaps;
>> 
>> We don't need to hold this here since gst_gl_color_convert_set_caps will perform a gst_caps_replace which will ref them internally. We don't need them anymore once they are passed down to the element, right?
> 
> We hold onto it to be able to create a new sample - the colour conversion object provides no accessor for it (at least, no documented way that I saw, and it didn't look like it was possible reading the source either)

Right.
Comment 13 Chris Lord 2019-09-03 03:42:08 PDT
Created attachment 377884 [details]
Patch
Comment 14 Chris Lord 2019-09-03 05:36:54 PDT
Created attachment 377892 [details]
Patch
Comment 15 WebKit Commit Bot 2019-09-03 07:58:44 PDT
Comment on attachment 377892 [details]
Patch

Clearing flags on attachment: 377892

Committed r249429: <https://trac.webkit.org/changeset/249429>
Comment 16 WebKit Commit Bot 2019-09-03 07:58:47 PDT
All reviewed patches have been landed.  Closing bug.