[Bug 785434] omx: add H265 support on Zynq UltraScale+

[GStreamer (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #358675|none                        |reviewed
             status|                            |

--- Comment #7 from Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> ---
Review of attachment 358675:
 --> (https://bugzilla.gnome.org/review?bug=785434&attachment=358675)

Looks fine overall, I have few question and nitpick though.

::: omx/gstomx.c
@@ +2308,3 @@
       , gst_omx_theora_dec_get_type
 #endif
+#ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS

I think this one should be HAVE_HEVC instead.

::: omx/gstomxh265enc.c
@@ +90,3 @@
+#ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS
+  g_object_class_install_property (gobject_class, PROP_PERIODICITYOFIDRFRAMES,
+      g_param_spec_uint ("periodicty-idr", "Target Bitrate",

That one is specific to Xilinx ? I thought it was part of the spec.

@@ +98,3 @@
+
+  g_object_class_install_property (gobject_class, PROP_B_FRAMES,
+      g_param_spec_uint ("b-frames", "Number of B-frames",

There is nothing to configure b-frame in the spec ? Only in Xilinx extension ?

@@ +268,3 @@
+
+#ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS
+  /* GOP pattern */

This part looks like some HW specific quirks, but I can't understand what bug
it's working around. Can you explain in a comment ?

@@ +300,3 @@
+  err =
+      gst_omx_component_set_parameter (GST_OMX_VIDEO_ENC (self)->enc,
+      (OMX_INDEXTYPE) OMX_ALG_IndexParamVideoHevc, &param);

This is identical, why do ALG needed to fork this part ? Can we use a #define
instead ?

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