[Bug 722686] omxvideodec: integrate resize component

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=722686
  GStreamer | gst-omx | git

--- Comment #14 from Josep Torra Valles <n770galaxy at gmail.com> 2014-03-05 14:04:02 UTC ---
Review of attachment 268926:
 --> (https://bugzilla.gnome.org/review?bug=722686&attachment=268926)

What I've seen are mostly structure or cosmetic issues that could be improved
in order to keep the code more readable.

It seems to me that tunneling related code is correct.

::: omx/gstomxvideodec.c
@@ +315,3 @@
+  use_resizer = GST_OMX_VIDEO_DEC (pool->element)->use_resizer;
+#else
+  use_resizer = FALSE;

This #else case seems not necessary if you already initialize use_resizer to
FALSE.

@@ +334,2 @@
   }
   GST_OBJECT_UNLOCK (pool);

Maybe would be better add a gboolean is_raw and just have a single
GST_OBJECT_UNLOCK; return (is_raw ? raw_video_options : options);

@@ +350,3 @@
+  use_resizer = GST_OMX_VIDEO_DEC (pool->element)->use_resizer;
+#else
+  use_resizer = FALSE;

This #else case seems not necessary if you already initialize use_resizer to
FALSE.

@@ +915,3 @@
+      out_port_index = param.nStartPortNumber + 1;
+    }
+    GST_OMX_INIT_STRUCT (&param);

Regarding the previous block, maybe would be better introduce a helper function
to be used in both use cases egl_render/resize.

@@ +1468,3 @@
 #else
   port = self->dec_out_port;
 #endif

Maybe would be better to initialize port = self->dec_out_port; first and remove
all else cases.

@@ +1852,3 @@
+            gst_omx_error_to_string (err), err);
+        goto done;
+            gst_omx_error_to_string (err), err);

Previous error message refer to "resize output port" but I've the feeling
there's a missing condition or that this code could be triggered for non resie
output port.

Maybe it should be refactored into a helper function.

@@ +1956,3 @@
+#else
+    bufsize = self->dec_out_port->port_def.nBufferSize;
+#endif

maybe better to initialize bufsize = self->dec_out_port->port_def.nBufferSize;
and simplify this conditional block to:

#if defined (USE_OMX_TARGET_RPI)
if (self->use_resizer)
  bufsize = self->res_out_port->port_def.nBufferSize;
#endif

@@ +2024,1 @@
 #endif

Maybe better first initialize to port = self->dec_out_port; and remove the else
cases.

@@ +2470,3 @@

+static gboolean gst_omx_video_dec_negotiate (GstOMXVideoDec * self);
+

Maybe better move the forward declaration to the top.

@@ +2494,3 @@
 #else
   port = self->dec_out_port;
 #endif

Simplify this like previous ones mentioned.

@@ +2903,1 @@
 #endif

Maybe add an intermediate sec_in_port/sec_out_port/sec_component (sec as
secondary component) and keep a single #if block and avoid duplicate code and
add conditionals.

@@ +2920,1 @@
 #endif

Avoid conditionals if intermediate variable is added for component above.

@@ +2940,1 @@
 #endif

Avoid conditionals if intermediate variable is added for component above.

@@ +3410,3 @@
 #else
     GstOMXPort *out_port = self->dec_out_port;
 #endif

Initialize first to GstOMXPort *out_port = self->dec_out_port; and simplify the
conditional here.

@@ +3530,3 @@
+
+          gst_omx_component_set_state (self->dec, OMX_StateExecuting);
+            gst_omx_component_get_state (self->dec, 1 * GST_SECOND);

Maybe better add a helper function for this block instead of duplicate it.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.