GNOME Bugzilla – Bug 785434
omx: add H265 support on Zynq UltraScale+
Last modified: 2017-09-06 18:53:21 UTC
The Zynq UltraScale+ OMX stack supports HEVC so we added elements for to use it.
Created attachment 356404 [details] [review] omxh265enc: add H265 encoder The OMX spec doesn't support HEVC but the OMX stack of the zynqultrascaleplus adds it as a custom extension. I used the H264 encoder code as a template.
Created attachment 356405 [details] [review] omxh265dec: add H265 decoder Add HEVC decoder for the zynqultrascaleplus platform. I used the H264 decoder code as a template.
I wonder how much that matches what Android does? https://android.googlesource.com/platform/frameworks/native/+/master/include/media/openmax/OMX_VideoExt.h Afaik, Xilinx should implement the Android API, as this is the effective upstream of OpenMAX IL now.
Created attachment 358675 [details] [review] omxh265enc: add H265 encoder The OMX spec doesn't support HEVC but the OMX stack of the zynqultrascaleplus adds it as a custom extension. It uses the same API as the one of Android's OMX stack. I used the H264 encoder code as a template.
Created attachment 358676 [details] [review] omxh265dec: add H265 decoder Add HEVC decoder for the zynqultrascaleplus platform. I used the H264 decoder code as a template.
(In reply to Olivier Crête from comment #3) > I wonder how much that matches what Android does? This new version now uses the same API as Android.
Review of attachment 358675 [details] [review]: 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, ¶m); This is identical, why do ALG needed to fork this part ? Can we use a #define instead ?
Review of attachment 358676 [details] [review]: That one is pretty small, looks good, just unsure about a ifdef. ::: omx/gstomx.c @@ +2309,3 @@ , gst_omx_theora_dec_get_type #endif #ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS HAVE_HEVC ?
Review of attachment 358675 [details] [review]: ::: omx/gstomx.c @@ +2308,3 @@ , gst_omx_theora_dec_get_type #endif +#ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS indeed. Fixed. ::: 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", The spec defines OMX_VIDEO_CONFIG_AVCINTRAPERIOD which is AVC specific so Allegro defined its own extension for it. @@ +98,3 @@ + + g_object_class_install_property (gobject_class, PROP_B_FRAMES, + g_param_spec_uint ("b-frames", "Number of B-frames", No, the spec API is AVC specific: OMX_VIDEO_PARAM_AVCTYPE. @@ +268,3 @@ + +#ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS + /* GOP pattern */ Limitation of the Android API. I added a comment. @@ +300,3 @@ + err = + gst_omx_component_set_parameter (GST_OMX_VIDEO_ENC (self)->enc, + (OMX_INDEXTYPE) OMX_ALG_IndexParamVideoHevc, ¶m); The param arg is different so they use another index to make this clearer. I added a comment and used a #define.
Review of attachment 358676 [details] [review]: ::: omx/gstomx.c @@ +2309,3 @@ , gst_omx_theora_dec_get_type #endif #ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS Yep fixed that as well.
Created attachment 358855 [details] [review] omxh265enc: add H265 encoder The OMX spec doesn't support HEVC but the OMX stack of the zynqultrascaleplus adds it as a custom extension. It uses the same API as the one of Android's OMX stack. I used the H264 encoder code as a template.
Created attachment 358856 [details] [review] omxh265dec: add H265 decoder Add HEVC decoder for the zynqultrascaleplus platform. I used the H264 decoder code as a template.
Attachment 358855 [details] pushed as cf9f190 - omxh265enc: add H265 encoder Attachment 358856 [details] pushed as 5751001 - omxh265dec: add H265 decoder Thanks