Copy&Paste from 745216

Simon Farnsworth 2015-03-05 14:04:30 UTC :
One comment on the code (not been able to test yet); you don't expose any of the vaapipostproc properties to the user, nor are they controlled by other means (e.g. GstMeta).
We've found that the default deinterlace-mode looks bad in most situations, and it's useful to be able to change it from 'bob' to 'motion-compensated' or 'motion-adaptive'.

sreerenj [reporter] [gstreamer-vaapi developer] 2015-03-05 14:31:44 UTC :
I deliberately  did that :) ..
I would like to get some discussion on this topic. 
Whether to make it simple which is only useful for playbin and don't expose any properties OR to make it as a replaceable element for vaapipostproc too.


Simon Farnsworth 2015-03-05 14:34:36 UTC:
Maybe worth discussing a change to the default deinterlace-method? "bob" looks awful, whereas (assuming field order is right in the source material) "motion-compensated" looks good on HSW, and falls back nicely to "motion-adaptive" on SNB and "bob" on things that can't do advanced deinterlacing.

Two thoughts (on top of the need to fix the default deinterlace-method):

 1. Can vaapidecodebin and vaapipostproc implement http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstcolorbalance.html for controlling hue, saturation etc? That way, there's an alternative to just setting properties that's easy to machine-discover.

 2. Can vaapipostproc be enhanced to be fully caps-driven, so that a downstream capable of coping with interlaced content (e.g. a H.265 encoder) gets interlaced content, and scaling is done based on upstream and downstream caps?

IOW, can things be changed so that "src ! vaapidecodebin ! video/x-raw,width=720,height=480 ! x264enc ! mux ! sink" gives x264enc 720x480 video, same framerate and interlacing as the src, while "src ! vaapidecodebin ! vaapisink" uses vaapipostproc to deinterlace for display?

Created attachment 302571 [details] [review]
Expose deinterlace-method property from inner vaapipostproc

This patch manually exposes the deinterlace-method property from the inner vaapipostproc.

This might be considered a suboptimal way of resolving the bug, since the ~20 lines of code from this patch should be repeated with minimal variations for every property from vaapipostproc we want to expose through.

Another way to resolve this would be using a more general method to expose child properties like GstChildProxy. It would depend on the approach we want to take: would we want to expose all the properties from vaapipostproc as a general rule, or only some specific ones?

-more suggestions from Gwenole:

BTW, as a placeholder for vaapidecodebin, it might be interesting to
add an option to disable vaapipostproc insertion, or at least allow
for propagating deinterlacing flags to it, or allow for disabling
deinterlacing actually.

(In reply to Simon Farnsworth from comment #2)
> Two thoughts (on top of the need to fix the default deinterlace-method):
> 
>  1. Can vaapidecodebin and vaapipostproc implement
> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-
> libs/html/gst-plugins-base-libs-gstcolorbalance.html for controlling hue,
> saturation etc? That way, there's an alternative to just setting properties
> that's easy to machine-discover.


We have colorbalance interface support in vaapipostproc now.

> 
>  2. Can vaapipostproc be enhanced to be fully caps-driven, so that a
> downstream capable of coping with interlaced content (e.g. a H.265 encoder)
> gets interlaced content, 


A general solution could be problematic, we cannot rely on downstream capabilities all the time. For eg, playbin will autoplug a software deinterlacer in downstream which will do deinterlacing instead of the upstream vaapipostproc.

>and scaling is done based on upstream and
> downstream caps?
> 
> IOW, can things be changed so that "src ! vaapidecodebin !
> video/x-raw,width=720,height=480 ! x264enc ! mux ! sink" gives x264enc
> 720x480 video, same framerate and interlacing as the src, while "src !
> vaapidecodebin ! vaapisink" uses vaapipostproc to deinterlace for display?

(In reply to sreerenj from comment #4)
> -more suggestions from Gwenole:
> 
> BTW, as a placeholder for vaapidecodebin, it might be interesting to
> add an option to disable vaapipostproc insertion, or at least allow
> for propagating deinterlacing flags to it, or allow for disabling
> deinterlacing actually.

It seems, vaapidecodebin introduces issues with negotiation(#745660,#749554) even for 1.4...
How about a property to disable vaapipostproc insertion and keep the default value as FALSE for now. This will be helpful atleast for the next release. We can change the default value to TRUE in future :)

Suggestions?

vaapidecodebin was supposed to be particularly useful for 1.4 where autoplugging of SW/HW deinterlace elements are not available/possible. Without for 1.4, the usefulness of vaapidecodebin decreases dramatically IMHO :)

bug #745660 and bug #749554 aims to situations where VAEntrypointVideoProc is not available (vdpau backend and g45 chipset).

My naive approach would be a conditional registration of vaapipostproc, whether the entry is available or not.

(In reply to Víctor Manuel Jáquez Leal from comment #8)
> bug #745660 and bug #749554 aims to situations where VAEntrypointVideoProc
> is not available (vdpau backend and g45 chipset).
> 
> My naive approach would be a conditional registration of vaapipostproc,
> whether the entry is available or not.

That's desirable.

As a side note, and IIRC, thaytan also suggested to additionally register only the supported set of vaapiencode_* elements.

My point of divergence could be that run-time checks for those (encode, vpp, etc.) would incur additional costs, thus increased start-up latency of the application. Hard to tell exactly, needs to be measured.

(In reply to Gwenole Beauchesne from comment #9)
> My point of divergence could be that run-time checks for those (encode, vpp,
> etc.) would incur additional costs, thus increased start-up latency of the
> application. Hard to tell exactly, needs to be measured.

Yes, but, as far as I understand, it will only happen when the plugins' cache is created or updated.

(In reply to Víctor Manuel Jáquez Leal from comment #10)
> (In reply to Gwenole Beauchesne from comment #9)
> > My point of divergence could be that run-time checks for those (encode, vpp,
> > etc.) would incur additional costs, thus increased start-up latency of the
> > application. Hard to tell exactly, needs to be measured.
> 
> Yes, but, as far as I understand, it will only happen when the plugins'
> cache is created or updated.

Interesting. What are the criteria for cache updates?

I mean, imagine you have a some hardware that supports codecs C1 and C2. VA driver version n1 supports only C1 and further enablement causes C2 to be supported in version n2 only. Does this mean that, if this is cached, we wouldn't support C2 despite the fact that driver n2 is installed?

(In reply to Gwenole Beauchesne from comment #11)
>
> Interesting. What are the criteria for cache updates?

AFAIK: when 

1) the envvar GST_REGISTRY_UPDATE is _not_ set to "no" AND (
2) the user removes ~/.cache/gstreamer-1.0/ or wherever aims the envvar GST_REGISTRY.
3) OR the plugins path has a different mtime  -with new/removed/changed plugins-. )

> I mean, imagine you have a some hardware that supports codecs C1 and C2. VA
> driver version n1 supports only C1 and further enablement causes C2 to be
> supported in version n2 only. Does this mean that, if this is cached, we
> wouldn't support C2 despite the fact that driver n2 is installed?

That's an interesting situation: the user should "force" the cache update, otherwise the element won't appear.

Well, the user is presumed/supposed to not be aware of underlying driver changes. Anyway, for an interim solution, that's a good starter imho.

There should be a way for the registry cache engine to hash or do something else on the processed caps on either pad. This is another interesting topic that would require core gstreamer changes anyway.

May be we can check the VPP capabilities inside vaapipostproc when changing state from READY to PAUSED. I think playbin will automatically switch to vaapidecode instead of vaapidecodebin if it failed to change the state of vaapidecodebin...

(In reply to sreerenj from comment #14)
> May be we can check the VPP capabilities inside vaapipostproc when changing
> state from READY to PAUSED. I think playbin will automatically switch to
> vaapidecode instead of vaapidecodebin if it failed to change the state of
> vaapidecodebin...

Besides, specifically to vaapipostproc, there should be an easy and readily available way to act exclusively in "passthrough" mode. Well, unless this didn't get through my mind at this time. :)

(In reply to sreerenj from comment #14)
> May be we can check the VPP capabilities inside vaapipostproc when changing
> state from READY to PAUSED. I think playbin will automatically switch to
> vaapidecode instead of vaapidecodebin if it failed to change the state of
> vaapidecodebin...

This is happening in gstreamer 1.5, but not in gstreamer 1.4

(In reply to Víctor Manuel Jáquez Leal from comment #16)
> (In reply to sreerenj from comment #14)
> > May be we can check the VPP capabilities inside vaapipostproc when changing
> > state from READY to PAUSED. I think playbin will automatically switch to
> > vaapidecode instead of vaapidecodebin if it failed to change the state of
> > vaapidecodebin...
> 
> This is happening in gstreamer 1.5, but not in gstreamer 1.4

Aha, that could be a different bug then,,but only with nvidia right???

Regarding the property expose,
I don't want to duplicate all code in vaapipostproc to vaapidecodebin..
For convenience we can expose deinterlace-method , and another property for disable/enable vaapipostproc... Customer can utilize GstChildProxy interface to set other properties of child elements...

(In reply to sreerenj from comment #17)
> (In reply to Víctor Manuel Jáquez Leal from comment #16)
> > (In reply to sreerenj from comment #14)
> > > May be we can check the VPP capabilities inside vaapipostproc when changing
> > > state from READY to PAUSED. I think playbin will automatically switch to
> > > vaapidecode instead of vaapidecodebin if it failed to change the state of
> > > vaapidecodebin...
> > 
> > This is happening in gstreamer 1.5, but not in gstreamer 1.4
> 
> Aha, that could be a different bug then,,but only with nvidia right???

All those backends/chipsets that don't provide VAEntrypointVideoProc, such as nvidia and G45 chipset.

(In reply to Víctor Manuel Jáquez Leal from comment #19)
> (In reply to sreerenj from comment #17)
> > (In reply to Víctor Manuel Jáquez Leal from comment #16)
> > > (In reply to sreerenj from comment #14)
> > > > May be we can check the VPP capabilities inside vaapipostproc when changing
> > > > state from READY to PAUSED. I think playbin will automatically switch to
> > > > vaapidecode instead of vaapidecodebin if it failed to change the state of
> > > > vaapidecodebin...
> > > 
> > > This is happening in gstreamer 1.5, but not in gstreamer 1.4
> > 
> > Aha, that could be a different bug then,,but only with nvidia right???
> 
> All those backends/chipsets that don't provide VAEntrypointVideoProc, such
> as nvidia and G45 chipset.

I seriously think that there is some major change in playbin negotiation while moving from 1.4 to git master(1.5)..

A similar case but issue only with 1.5, https://bugzilla.gnome.org/show_bug.cgi?id=750944

Review of attachment 302571 [details] [review]:

lgtm

Created attachment 305829 [details] [review]
vaapidecodebin: Add property to disable VPP

Review of attachment 305829 [details] [review]:

::: gst/vaapi/gstvaapidecodebin.c
@@ +145,3 @@
+        gst_object_unref (pad);
+      }
+      break;

An what about when we want to disable the vpp in runtime? In that case, the code will be more complex, so I also think we should move it into a function.

Review of attachment 302571 [details] [review]:

::: gst/vaapi/gstvaapidecodebin.h
@@ +52,3 @@
   guint   max_size_bytes;
   guint64 max_size_time;
+  GstVaapiDeinterlaceMethod deinterlace_method;

Is it necessary to have this variable?

I guess we could just proxy the get/set to the postproc's get/set (if postproc does exist)

(In reply to Víctor Manuel Jáquez Leal from comment #23)
> Review of attachment 305829 [details] [review] [review]:
> 
> ::: gst/vaapi/gstvaapidecodebin.c
> @@ +145,3 @@
> +        gst_object_unref (pad);
> +      }
> +      break;
> 
> An what about when we want to disable the vpp in runtime? In that case, the
> code will be more complex, so I also think we should move it into a function.

Do we really need to support runtime-disabling features?? Any specific use cases?
Allowing runtime disabling might hit more issues i think :)...

(In reply to sreerenj from comment #25)
> (In reply to Víctor Manuel Jáquez Leal from comment #23)
> > Review of attachment 305829 [details] [review] [review] [review]:
> > 
> > ::: gst/vaapi/gstvaapidecodebin.c
> > @@ +145,3 @@
> > +        gst_object_unref (pad);
> > +      }
> > +      break;
> > 
> > An what about when we want to disable the vpp in runtime? In that case, the
> > code will be more complex, so I also think we should move it into a function.
> 
> Do we really need to support runtime-disabling features?? Any specific use
> cases?

I don't have a use-case in mind. Just the API doesn't refrain the user, neither inform they if their request is ignored.

> Allowing runtime disabling might hit more issues i think :)...

I agree.

Comment on attachment 302571 [details] [review]
Expose deinterlace-method property from inner vaapipostproc

commit 76060c9542847a93edc6912dc21a4ef8f162dd75
Author: Jacobo Aragunde Pérez <jaragunde@igalia.com>
Date:   Wed Apr 29 16:34:07 2015 +0200

    vaapidecodebin: expose deinterlace-method property from inner vaapipostproc
    
    https://bugzilla.gnome.org/show_bug.cgi?id=745901

Pushed.

commit f1bc4f8461cd6e5be9a22092f8c9c5ea042223e5
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Tue Jun 30 09:35:37 2015 +0300

    vaapidecodebin: Add property to disable VPP