GNOME Bugzilla – Bug 701837
Add Video Post Processing in gst-vaapi
Last modified: 2013-12-13 06:41:16 UTC
add more features supported in gst-vaapi: denoise, deinterlace, surface fromat conversion, scaling and color balance
Created attachment 246294 [details] [review] add fourcc for GstVaapiSurface/GstVaapiSurfacePool
Created attachment 246295 [details] [review] Add new object GstVaapiPostProcess for post-processing
Created attachment 246296 [details] [review] check vpp support in configure.ac
Created attachment 246297 [details] [review] add vpp function for: denoise, deinterlace, surface fromat conversion, scaling and color balance
Created attachment 247415 [details] [review] remove gobject from gstvaapipostprocess remove gobject from gstvaapipostprocess
Created attachment 247416 [details] [review] improve deinterlace feature improve deinterlace feature
Created attachment 247944 [details] [review] v2: [PATCH 1/8] add fourcc for GstVaapiSurface/GstVaapiSurfacePool
Created attachment 247945 [details] [review] v2: [PATCH 2/8] vaapipostproc: Add new object GstVaapiPostProcess for post-processing
Created attachment 247946 [details] [review] v2: [PATCH 3/8] check vpp support in configure.ac
Created attachment 247947 [details] [review] v2: [PATCH 4/8] vaapipostproc: add vpp process in gstvaapipostprocess
Created attachment 247948 [details] [review] v2: [PATCH 5/8] vaapipostproc: remove gobject from gstvaapipostprocess
Created attachment 247949 [details] [review] v2: [PATCH 6/8] vaapipostproc deinterlace improvement
Created attachment 247950 [details] [review] v2: PATCH 7/8] vaapipostproc: remove auto saturation/brightness/contrast
Created attachment 247951 [details] [review] v2: [PATCH 8/8] vaapipostproc: support variable number of ref buffer
Review of attachment 247946 [details] [review]: Since the automake var is USE_VA_VPP, please also use USE_VA_VPP as the variable name. Besides, you can also derive the checking code from the VA/JPEG decoding API check just above. i.e. use AC_CACHE_CHECK(), AC_COMPILE_IFELSE() with a meaningful VA/VPP sample test with the mandatory <va/va_vpp.h> include + vaQueryVideoProcFilters() function check for example. In the future, we could therefore improve the check with more functions that we could also require. In any case, checking for includes implies CPPFLAGS, not CFLAGS. If you still see CFLAGS overrides in gst-vaapi configure checks, then that's an error and report them. That's important to correctly check for includes in non-standard install dirs. Thanks.
Review of attachment 247944 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapisurface.c @@ +190,3 @@ +#else + // I don't add VA_CHECK_VERSION for each place where format is mentioned, but leave + // a warning here if it is used by mistake. If think, for this internal function, having both chroma-type and pixel format set would be a programming error, that you could g_assert() at the beginning. Then, you can drop that warning. ::: gst-libs/gst/vaapi/gstvaapisurface.h @@ +103,3 @@ + GST_VAAPI_SURFACE_FORMAT_XBGR = GST_MAKE_FOURCC('X','B','G','R'), + GST_VAAPI_SURFACE_FORMAT_BGRX = GST_MAKE_FOURCC('B','G','R','X'), +} GstVaapiSurfaceFormat; I am wondering if it wouldn't be simpler / be less code to just ignore any GstVaapiSurfaceFormat type and just use GstVideoFormat + a map table in gstvaapisurface.c to VA pixel format. That's because gst_video_format_to_fourcc() won't return "valid" (in VA-API formats) fourcc for RGB and grayscale formats. So, that's why I tend to think about GstVideoFormat as the better compromise, even for VA image format (but I will look into this one). @@ +170,3 @@ +gst_vaapi_surface_new_with_fourcc( + GstVaapiDisplay *display, + guint32 fourcc, So, if we use GstVideoFormat, then that would be that type here + gst_vaapi_surface_new_with_format() as the name. ::: gst-libs/gst/vaapi/gstvaapisurfacepool.c @@ +44,2 @@ GstVaapiChromaType chroma_type; + guint fourcc; guint32 fourcc; is the usual type for explicit fourcc but I don't mind. @@ +53,3 @@ GstVaapiSurfacePool * const pool = GST_VAAPI_SURFACE_POOL(base_pool); GstVideoInfo vi; + gint width = 0, height = 0, format = 0, fourcc = 0; Keeping guint format; is enough. Others are not used. @@ +60,3 @@ pool->width = GST_VIDEO_INFO_WIDTH(&vi); pool->height = GST_VIDEO_INFO_HEIGHT(&vi); + format = GST_VIDEO_INFO_FORMAT(&vi); if (format == GST_VIDEO_FORMAT_UNKNOWN) return FALSE; @@ +83,3 @@ + surface = gst_vaapi_surface_new_with_fourcc(base_pool->display, + pool->fourcc, pool->width, pool->height); + } else [coding style] you could omit all braces for single statements, or at least align them as follows: if (xxx) { ... } else { ... }
Review of attachment 247947 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapipostprocess.c @@ +67,3 @@ + return 0; +} + Why not use vaapi_check_status()? @@ +103,3 @@ + } + } + vaapi_destroy_buffer(), it will also reset the buffer id to VA_INVALID_ID. @@ +128,3 @@ + va_status = vaQuerySurfaceAttributes(va_dpy, priv->config_id, + NULL, &num_surface_attribs); + if (va_status != VA_STATUS_SUCCESS) { if (va_status != VA_STATUS_ERROR_MAX_NUM_EXCEEDED)... this will mean a legitimate error occurred; or if status == 0 and num_surface_attribs remains to zero, this means we are not going to support any surface attributes. @@ +150,3 @@ + surface_attribs, &num_surface_attribs); + } + You don't need that loop. The VA driver shall return the exact number of expected elements to allocate. Otherwise, this is a driver bug. i.e. a single vaQuerySurfaceAttributes() here is enough. + if (!vaapi_check_status(va_status, "vaQuerySurfaceAttributes()")) // error out
Review of attachment 247948 [details] [review]: Squash into patch 4.
Review of attachment 247950 [details] [review]: Squash into patch 4.
Review of attachment 247951 [details] [review]: Squash into patch 6.
I'd like a clear separation between libgstvaapi (gst-libs/) and the plugin elements (gst/). The first series of patches would also contain tests/ as in test-filters. Likewise, a clear separation of purposes for each filter to be supported. Patch01: add support for GstVaapiSurface creation with specific format Patch02: add infrastructure for video processing Patch03: vpp: add support for advanced deinterlacing Patch04: vpp: add support for color balance Patch05: vaapipostproc: add support for scaling/color conversion Patch06: vaapipostproc: add support for advanced deinterlacing Note: only vaapisink element will support color balance, as of now.
(In reply to comment #22) > Likewise, a clear separation of purposes for each filter to be supported. > > Patch01 [details]: add support for GstVaapiSurface creation with specific format > Patch02 [details]: add infrastructure for video processing > Patch03 [details]: vpp: add support for advanced deinterlacing > Patch04 [details]: vpp: add support for color balance > Patch05 [details]: vaapipostproc: add support for scaling/color conversion > Patch06 [details]: vaapipostproc: add support for advanced deinterlacing I forgot about noise reduction and sharpening. Since those are simple filters, please add those before advanced deinterlacing, as patches 03 and 04, thus shifting all others.
Details for Patch02: add infrastructure for video processing. The current naming is not very consistent. Since video processing can be considered as a black box since multiple algorithms could be applied at once, let's keep the keep the model of "black box" as is and find better names and APIs. The suggested object to handle video processing is: GstVaapiFilter. Then, multiple operations could be attached to it.
Created attachment 248332 [details] gst-vaapi video processing API Here is the video processing API I want to see in gstreamer-vaapi (kind of). By default, as generated by gst_vaapi_filter_new(), the identity operation is performed, i.e. only scaling is performed. This means that the target surface format shall match the source surface format. Additional notes: - gst_vaapi_filter_get_operations() is useful to generate vaapipostproc element properties. - gst_vaapi_filter_set_operation() is the generic entry-point to set the various operations. It will call into each individual set_XXX() operation helper. - gst_vaapi_filter_process() will need to validate the dst_surface format as specified. - gst_vaapi_filter_set_xxx() is where you can create the VA filter buf id if not existing already, or update the associated value.
Created attachment 248585 [details] [review] v3: 0001-gstvaapisurface-add-GstVideoFormat-in-gstvaapisurfac
Created attachment 248586 [details] [review] v3: 0002-vaapipostproc-add-infrastructure-for-video-processin
Created attachment 248587 [details] [review] v3: 0003-vaapipostproc-add-test-filter
Created attachment 248588 [details] [review] v3: 0004-vaapipostproc-add-deinterlace-support-from-vaapi-vpp
Created attachment 248589 [details] [review] v3: 0005-vaapipostproc-add-denoise-support-from-va_vpp-api
Created attachment 248590 [details] [review] v3: 0006-vaapipostproc-add-color-balance-filters
Created attachment 248591 [details] [review] v3: 0007-vaapipostproc-improve-deinterlace-support-for-motion
(In reply to comment #24) > Details for Patch02 [details]: add infrastructure for video processing. > > The current naming is not very consistent. > sorry, I didn't notice this header file before. let's me have a look to see any update is required or not.
Review of attachment 248585 [details] [review]: I pushed the correct series of patches to support surface attributes and that work in both 0.10 and 1.0 modes, and also keep the older behaviour intact for older libva versions.
Created attachment 249373 [details] [review] v4: 0001-vaapipostproc-add-infrastructure-for-video-processin
Created attachment 249374 [details] [review] v4: 0002-vaapipostproc-add-test-filter
Created attachment 249375 [details] [review] v4: 0001-vaapipostproc-add-infrastructure-for-video-processin
Created attachment 249376 [details] [review] v4: 0002-vaapipostproc-add-test-filter
Created attachment 249377 [details] [review] v4: 0003-vaapipostproc-add-deinterlace-support-from-vaapi-vpp
Created attachment 249378 [details] [review] v4: 0004-vaapipostproc-add-denoise-support-from-va_vpp-api
Created attachment 249379 [details] [review] v4: 0005-vaapipostproc-add-sharpen-support
Created attachment 249380 [details] [review] v4: 0006-vaapipostproc-add-color-balance-filters
Created attachment 249381 [details] [review] v4: 0007-vaapipostproc-improve-deinterlace-support-for-motion
Merged in git master branch, except patches 0006 and 0007. - 0006: GstColorBalanceInterface ought to be supported first ; - 0007: could you please update the discontinuity check? You could add more info to GstVaapiDeinterlaceState (like last buffer PTS) and next determine if we are still in limits. If not -> ds_reset() should be enough. Thanks.
for GstColorBalanceInterface, do you expect it implement for vaapisink or vaapipostproc? I prefer to do it in vaapipostproc: 1. it benefits usage besides video rendering: transcoding, opencl usage etc 2. it is simpler to do it in vaapipostproc. otherwise, we'd introduce gstvaapifilter and another videopool in vaapisink.
(In reply to comment #45) > for GstColorBalanceInterface, > do you expect it implement for vaapisink or vaapipostproc? The expectation is to handle the appropriate VA display attributes in vaapisink. No need for extra video filters. On some platforms, we can do that in a single pass in vaPutSurface(), and without any additional temporary storage / surface. Of course, the other usages you describe are also useful and will be implemented. We just need to align vaapisink with other sink elements. BTW, I think we could close this bug and open two separate new bugs for (i) GstColorBalance support in vaapipostproc, and (ii) fix support for deinterlacing while seeking for instance, if needed.
create two new bugs respectively for advance deinterlace (fix) and GstColorBalance interface support. https://bugzilla.gnome.org/show_bug.cgi?id=720375 https://bugzilla.gnome.org/show_bug.cgi?id=720376 close this one.