Created attachment 306680 [details] [review]
vaapipostproc: enable passthrough on same caps

(In reply to Víctor Manuel Jáquez Leal from comment #0)
> Enable passthrough on same caps feature. With it, the negotiation with
> vaapisink is faster.

Any measurement to share? Thanks. :)

Review of attachment 306680 [details] [review]:

LGTM.

I haven't pushed this patch because I'm not sure about what if the caps are the same but some properties, such as denoise, sharpen, color-balance or ske, are enabled. It that case the passthrough shall not be enabled though the caps are the same.

Moving to Product:GStreamer, Component:gstreamer-vaapi

Created attachment 329245 [details] [review]
vaapipostproc: add prefer-passthrough property

One suggestion that
adding new property to enable passthrough for users who wants to passthrough,
if both caps are same.
This is referred to alpha element.
In this case, postproc ignores filtering values.

(In reply to Hyunjun Ko from comment #6)
> Created attachment 329245 [details] [review] [review]
> vaapipostproc: add prefer-passthrough property
> 
> One suggestion that
> adding new property to enable passthrough for users who wants to passthrough,
> if both caps are same.
> This is referred to alpha element.
> In this case, postproc ignores filtering values.

And a small code optimization.

(In reply to Hyunjun Ko from comment #6)
> Created attachment 329245 [details] [review] [review]
> vaapipostproc: add prefer-passthrough property
> 
> One suggestion that
> adding new property to enable passthrough for users who wants to passthrough,
> if both caps are same.
> This is referred to alpha element.
> In this case, postproc ignores filtering values.

I don't think that this is the way to go. The user should not think on these issues, it is only matter of the postprocessor if it enables the passthrough or not.

Perhaps, you should split the patch it two, so we could discuss the property separated.

Created attachment 329353 [details] [review]
vaapipostproc: Don't set filtering value always in transform operation

Patch splited.

I would suggest new suggestion for passthrough later, according to Victor's comment.

Probably, we can enable passthrough by below
- if both caps are same, passthrough mode is enabled basically.
- once filtering value is set, it turns to non-passthrough mode.
- if (all) filtering value is set to default again, it can turn to passthrough mode.

To do this, something necessary is below.
- Basically, if both caps are same and no filter value set, passthrough mode enabled.
- call gst_base_transform_reconfigure_src when filtering value is set
- During reconfigure, we can set passthrough mode with sink/src caps and filter flag.
- code to reset flags when it turns to default value.

Created attachment 329536 [details] [review]
vaapipostproc: make it enable/disable pass-through mode

See commit message.

Review of attachment 329536 [details] [review]:

@Hyunjun,

Sorry for the late review.

Overall, I like what a see. But two comments:

1\ I would split this patch in two: 1) improving the parameter settings, to avoid checking for them at every frame; and 2) the passthrough enable
2\ A couple comment on code style basically

::: gst/vaapi/gstvaapipostproc.c
@@ +1125,3 @@
+{
+  GstVaapiPostproc *const postproc = GST_VAAPIPOSTPROC (trans);
+  gboolean ret = FALSE;

I would rename 'ret' as filters_updated, to be clearer

@@ +1130,3 @@
+    ret = update_filter (postproc);
+
+  if (!postproc->same_caps || (ret && check_filter_update (postproc))) {

I'd move 'ret && check_filter_update (postproc)' up, so the comparison will look clearer

if (!postproc->same_caps || filters_updated)

or better

if (postproc->same_caps && !filters_updated)

or better... perhaps...

gst_base_transofrm_set_passthrough (trans, posptroc->same_caps && !filters_updated);

@@ +1319,3 @@
+  else
+    postproc->same_caps = FALSE;
+

It is better, sometimes, to avoid branching. I would put this as

postproc->same_caps = gst_caps_is_equal (caps, out_caps)

@@ +1822,3 @@
   gst_video_info_init (&postproc->filter_pool_info);
+
+  postproc->same_caps = FALSE;

This is not required since gobject structures are always initialized with zero.

Created attachment 331951 [details] [review]
[PATCH 1/2] vaapipostproc: checking and updating filter parameter only when it's set

patch splitted according to Victor's suggestion

Created attachment 331952 [details] [review]
[PATCH 2/2] vaapipostproc: make it enable/disable pass-through mode

In case that sink caps and src caps are same, and no filtering parameter set,
pass-through mode is enabled.
If new filtering parameter is set during playback, it makes it reconfiguring,
so that pass-through mode is changed
 
In addition, updating filter is performed during reconfiguration, if needed.

Review of attachment 331951 [details] [review]:

Looks good, but you forgot to filter_update() when the colorbalance interface change. I'll post a patch above yours for this.