[Bug 751876] vaapipostproc: enable passthrough on same caps

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=751876

Víctor Manuel Jáquez Leal <vjaquez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #329536|none                        |needs-work
             status|                            |

--- Comment #13 from Víctor Manuel Jáquez Leal <vjaquez at igalia.com> ---
Review of attachment 329536:
 --> (https://bugzilla.gnome.org/review?bug=751876&attachment=329536)

@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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.