Created attachment 361216 [details] [review]
vaapidecodebin: Register element if no VPP support is available too

VPP support is only needed for advanced deinterlacing, which is not
enabled by default either. Error out if it is selected but VPP is not
supported, and otherwise just work without VPP support.

Review of attachment 361216 [details] [review]:

::: gst/vaapi/gstvaapidecodebin.c
@@ +314,3 @@
+  } else {
+    GST_INFO_OBJECT (vaapidecbin, "Creating a dummy display to test for vpp");
+    display = gst_vaapi_create_test_display ();

I'm a bit worry of creating, again, a test display to check if vpp is supported. I foresee three options:

1\ pass has_vpp to the element at registration time
2\ query for display through the gst context mechanism
3\ ignore all that and just create a test display again 

The first option looks promising

The second would mean to get vaapidecodebin more complex than needed

The third option is simpler, cleaner in code terms, but expensive.

(In reply to Víctor Manuel Jáquez Leal from comment #2)
> Review of attachment 361216 [details] [review] [review]:
> 
> ::: gst/vaapi/gstvaapidecodebin.c
> @@ +314,3 @@
> +  } else {
> +    GST_INFO_OBJECT (vaapidecbin, "Creating a dummy display to test for
> vpp");
> +    display = gst_vaapi_create_test_display ();
> 
> I'm a bit worry of creating, again, a test display to check if vpp is
> supported. I foresee three options:
> 
> 1\ pass has_vpp to the element at registration time

From the code that registers vaapidecodebin you mean? There's no easy way to pass this information through to the element, other than a global variable.

We could check once in gstvaapi.c if VPP is supported with the test display, and then just remember that. Would that work for you? E.g. by allowing to pass NULL to gst_vaapi_display_has_video_processing() or adding a new function or global variable.

(In reply to Sebastian Dröge (slomo) from comment #3)
>
> We could check once in gstvaapi.c if VPP is supported with the test display,
> and then just remember that. Would that work for you? E.g. by allowing to
> pass NULL to gst_vaapi_display_has_video_processing() or adding a new
> function or global variable.

Yes, that looks promising.

Which option do you prefer? Personally I'd just add a global variable inside the plugin that is set in plugin_init(), it's simplest.

(In reply to Sebastian Dröge (slomo) from comment #5)
> Which option do you prefer? Personally I'd just add a global variable inside
> the plugin that is set in plugin_init(), it's simplest.

That's ok for me.

Created attachment 361882 [details] [review]
vaapidecodebin: Register element if no VPP support is available too

VPP support is only needed for advanced deinterlacing, which is not
enabled by default either. Error out if it is selected but VPP is not
supported, and otherwise just work without VPP support.

Review of attachment 361882 [details] [review]:

lgtm

::: gst/vaapi/gstvaapi.h
@@ +5,3 @@
+ *    Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
+ *  Copyright (C) 2011 Collabora Ltd.
+ *    Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>

Is copyright ok?

Attachment 361882 [details] pushed as a8f2309 - vaapidecodebin: Register element if no VPP support is available too