Bug 123441 – [GStreamer] Updated support for gst-vaapi 1.2

• Status: RESOLVED
• Resolution: INVALID
• Component: Platform

[Philippe Normand]

Our video sink now needs to support the NV12 format to get the GLTextureUploadMeta support working.

[Philippe Normand]

Created attachment 215376 [details]
patch

[WebKit Commit Bot]

Attachment 215376 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp', u'Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:47:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:48:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:49:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:50:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:51:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:53:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:54:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:55:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:56:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:57:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:59:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:60:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:61:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:62:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:63:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:64:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:67:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:68:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:69:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:70:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:71:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 21 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 215377 [details]
patch

[Philippe Normand]

Sebastian, does this change makes sense to you? :)

[Sebastian Dröge (slomo)]

Comment on attachment 215377 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:355
> +#if GST_CHECK_VERSION(1, 3, 0)

Why 1.3.0? All this should be supported since GStreamer 1.2.0.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:64
>  #define WEBKIT_VIDEO_SINK_PAD_CAPS GST_FEATURED_CAPS GST_VIDEO_CAPS_MAKE(GST_CAPS_FORMAT)

This now means that you also have video/x-raw,format=NV12 here (i.e. without the caps feature). However without the GL texture upload meta NV12 is not supported by the sink.

[Philippe Normand]

Comment on attachment 215377 [details]
patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:355
>> +#if GST_CHECK_VERSION(1, 3, 0)
> 
> Why 1.3.0? All this should be supported since GStreamer 1.2.0.

But there's no release of gst-vaapi with that support yet and they moved all their code-checking to 1.3 already... So I thought I'd do the same :)

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:64
>>  #define WEBKIT_VIDEO_SINK_PAD_CAPS GST_FEATURED_CAPS GST_VIDEO_CAPS_MAKE(GST_CAPS_FORMAT)
> 
> This now means that you also have video/x-raw,format=NV12 here (i.e. without the caps feature). However without the GL texture upload meta NV12 is not supported by the sink.

Hmm, right... Ok I'll revise the patch. Thanks for looking!

[Philippe Normand]

Created attachment 216010 [details]
Patch

[Sebastian Dröge (slomo)]

When using the NV12 caps, are there two textures created from the GstGLVideoTextureUploadMeta? Or only a single one that behaves like (A)RGB?

[Philippe Normand]

(In reply to comment #8)
> When using the NV12 caps, are there two textures created from the GstGLVideoTextureUploadMeta? Or only a single one that behaves like (A)RGB?

Only one texture in GL_RGBA format. The code is in gst/vaapi/gstvaapivideometa_texture.c

In the end this patch only fixes caps negotiation :)

[Sebastian Dröge (slomo)]

(In reply to comment #9)
> (In reply to comment #8)
> > When using the NV12 caps, are there two textures created from the GstGLVideoTextureUploadMeta? Or only a single one that behaves like (A)RGB?
> 
> Only one texture in GL_RGBA format. The code is in gst/vaapi/gstvaapivideometa_texture.c
> 
> In the end this patch only fixes caps negotiation :)

That's a bug on their side then, it should use RGBA in the caps instead of NV12.

[Philippe Normand]

Sree thinks otherwise in https://bugzilla.gnome.org/show_bug.cgi?id=687183
It'd be nice to reach a consensus about this at some point.

[Philippe Normand]

Comment on attachment 216010 [details]
Patch

Pulling this out of review since it's a gst-vaapi bug