Bug 123441

Summary: [GStreamer] Updated support for gst-vaapi 1.2
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: commit-queue, eric.carlson, glenn, gns, jer.noble, menard, mrobinson, pnormand, slomo, vjaquez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
Patch none

Description Philippe Normand 2013-10-29 05:02:08 PDT
Our video sink now needs to support the NV12 format to get the GLTextureUploadMeta support working.
Comment 1 Philippe Normand 2013-10-29 05:07:07 PDT
Created attachment 215376 [details]
patch
Comment 2 WebKit Commit Bot 2013-10-29 05:08:14 PDT
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.
Comment 3 Philippe Normand 2013-10-29 05:10:50 PDT
Created attachment 215377 [details]
patch
Comment 4 Philippe Normand 2013-10-29 10:55:46 PDT
Sebastian, does this change makes sense to you? :)
Comment 5 Sebastian Dröge (slomo) 2013-10-30 09:41:23 PDT
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.
Comment 6 Philippe Normand 2013-10-30 09:50:38 PDT
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!
Comment 7 Philippe Normand 2013-11-05 00:14:14 PST
Created attachment 216010 [details]
Patch
Comment 8 Sebastian Dröge (slomo) 2013-11-05 00:39:47 PST
When using the NV12 caps, are there two textures created from the GstGLVideoTextureUploadMeta? Or only a single one that behaves like (A)RGB?
Comment 9 Philippe Normand 2013-11-06 08:39:59 PST
(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 :)
Comment 10 Sebastian Dröge (slomo) 2013-11-06 08:48:59 PST
(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.
Comment 11 Philippe Normand 2013-11-06 09:17:35 PST
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.
Comment 12 Philippe Normand 2013-11-15 03:48:54 PST
Comment on attachment 216010 [details]
Patch

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