Bug 200922 – [GStreamer] Add support to copy YUV video textures into images

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Chris Lord]

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

[Chris Lord]

Created attachment 376772 [details]
Patch

[Philippe Normand]

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?

[Chris Lord]

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

[Miguel Gomez]

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

[Philippe Normand]

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

[Chris Lord]

Created attachment 376863 [details]
Patch

[Chris Lord]

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.

[Philippe Normand]

Comment on attachment 376863 [details]
Patch

lgtm :)

[Chris Lord]

Created attachment 377335 [details]
Patch

[Xabier Rodríguez Calvar]

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?

[Chris Lord]

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

[Xabier Rodríguez Calvar]

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.

[Chris Lord]

Created attachment 377884 [details]
Patch

[Chris Lord]

Created attachment 377892 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377892 [details]
Patch

Clearing flags on attachment: 377892

Committed r249429: <https://trac.webkit.org/changeset/249429>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.