GNOME Bugzilla – Bug 797143
vaapih265dec: support 422 chroma format hevc decode.
Last modified: 2018-09-29 01:09:38 UTC
support 422 chroma format hevc decode.
Created attachment 373653 [details] [review] hevc 422 support
Review of attachment 373653 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapicontext.c @@ +141,3 @@ + else + surface = gst_vaapi_surface_new (GST_VAAPI_OBJECT_DISPLAY (context), + cip->chroma_type, cip->width, cip->height); This changeset looks very hackish to me. The proper way to handle this is in gstvaapidecoder_h265.c:ensure_context() by setting the "proper" chroma type if YUV422 is detected. But why cannot be created surfaces with that chroma type? @@ +257,3 @@ if (!context_get_attribute (context, attrib->type, &value)) goto cleanup; + if (!(value && va_chroma_format)) { No. You cannot convert a bitwise operation into a boolean operation. ::: gst-libs/gst/vaapi/gstvaapiprofile.h @@ +132,3 @@ * @GST_VAAPI_PROFILE_H265_MAIN_STILL_PICTURE: * H.265 main still picture profile [A.3.4] + * @GST_VAAPI_PROFILE_H265_MAIN_422_10: bad code style @@ +178,3 @@ GST_VAAPI_PROFILE_H265_MAIN_STILL_PICTURE = GST_VAAPI_MAKE_PROFILE(H265,3), + GST_VAAPI_PROFILE_H265_MAIN_422_10 = GST_VAAPI_MAKE_PROFILE(H265,4), ditto. ::: gst/vaapi/gstvaapidecode.c @@ +88,3 @@ GST_VAAPI_MAKE_GLTEXUPLOAD_CAPS ";" #endif + GST_VIDEO_CAPS_MAKE("{ NV12, I420, YV12, YUY2, UYVY, P010_10LE }") ";" As far as I understand this changeset only applies to surfaces formats, not to mapped images. ::: gst/vaapi/gstvaapipluginbase.c @@ +1300,3 @@ + if (out_formats->len == 0) { + GST_DEBUG ("No invalid image format found for surface!"); I guess the message has some typo or grammatical error. Perhaps it should be "No valid image format found for surface" (no exclamation mark) And perhaps it should be GST_WARNING. But, the question is, do we really need that log message?
(In reply to Víctor Manuel Jáquez Leal from comment #2) > Review of attachment 373653 [details] [review] [review]: > > ::: gst-libs/gst/vaapi/gstvaapicontext.c > @@ +141,3 @@ > + else > + surface = gst_vaapi_surface_new (GST_VAAPI_OBJECT_DISPLAY (context), > + cip->chroma_type, cip->width, cip->height); > > This changeset looks very hackish to me. > > The proper way to handle this is in gstvaapidecoder_h265.c:ensure_context() > by setting the "proper" chroma type if YUV422 is detected. > > But why cannot be created surfaces with that chroma type? It's the fourcc format which is need when create surface with chroma type 422. In ensure_context, even we know the chroma type is 422, we cann't do anything until create surface side. > > @@ +257,3 @@ > if (!context_get_attribute (context, attrib->type, &value)) > goto cleanup; > + if (!(value && va_chroma_format)) { > > No. You cannot convert a bitwise operation into a boolean operation. Will return this back. But still a question why this need bitwise operation? Why not use (value == va_chroma_format) here? How about value=0x01, va_chroma_format=0x03? In my understanding, this case also should be unsupported chroma format, but It can pass the bitwise operation. > > ::: gst-libs/gst/vaapi/gstvaapiprofile.h > @@ +132,3 @@ > * @GST_VAAPI_PROFILE_H265_MAIN_STILL_PICTURE: > * H.265 main still picture profile [A.3.4] > + * @GST_VAAPI_PROFILE_H265_MAIN_422_10: > > bad code style Will fix this. > > @@ +178,3 @@ > GST_VAAPI_PROFILE_H265_MAIN_STILL_PICTURE = > > GST_VAAPI_MAKE_PROFILE(H265,3), > + GST_VAAPI_PROFILE_H265_MAIN_422_10 = GST_VAAPI_MAKE_PROFILE(H265,4), > > ditto. Will fix this. > > ::: gst/vaapi/gstvaapidecode.c > @@ +88,3 @@ > GST_VAAPI_MAKE_GLTEXUPLOAD_CAPS ";" > #endif > + GST_VIDEO_CAPS_MAKE("{ NV12, I420, YV12, YUY2, UYVY, P010_10LE }") ";" > > As far as I understand this changeset only applies to surfaces formats, not > to mapped images. It should be image formats. The formats will be used to create image one by one, and then the image will be tried to put into surface(without the forcc-format, only chroma format.). And find all the supported image format in list. > > ::: gst/vaapi/gstvaapipluginbase.c > @@ +1300,3 @@ > > + if (out_formats->len == 0) { > + GST_DEBUG ("No invalid image format found for surface!"); > > I guess the message has some typo or grammatical error. Perhaps it should be > "No valid image format found for surface" (no exclamation mark) > > And perhaps it should be GST_WARNING. > > But, the question is, do we really need that log message? It will be useful when the output format can not be used to create into image. Then the format list len will be 0, which mean no suit format that can be supported by driver. The supported formats can also be found in update decode src pads that need look at the log carefully. Anyway, to keep less output log, will prefer to move it.
Created attachment 373669 [details] [review] hevc 422 support
Review of attachment 373669 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapicontext.c @@ +141,3 @@ + else + surface = gst_vaapi_surface_new (GST_VAAPI_OBJECT_DISPLAY (context), + cip->chroma_type, cip->width, cip->height); Why do you need to use gst_vaapi_surface_new_with_format(), why gst_vaapi_surface_new() doesn't work for you? In other words, why do you need to convert from GST_VIDEO_FORMAT_YUY2 to GST_VAAPI_CHROMA_TYPE_YUV422, and then to VA_RT_FORMAT_YUV422 again? ::: gst-libs/gst/vaapi/gstvaapiutils_h265.c @@ +138,3 @@ + switch (sps->profile_tier_level.profile_idc) { + case GST_H265_PROFILE_IDC_MAIN: all the changes from GST_H265_PROFILE_ to GST_H265_PROFILE_IDC_ belongs to a different patch because it is a different fix. Please, split it, because it would need to be backported to 1.14 and 1.12 Good catch!
(In reply to Víctor Manuel Jáquez Leal from comment #5) > Review of attachment 373669 [details] [review] [review]: > > ::: gst-libs/gst/vaapi/gstvaapicontext.c > @@ +141,3 @@ > + else > + surface = gst_vaapi_surface_new (GST_VAAPI_OBJECT_DISPLAY (context), > + cip->chroma_type, cip->width, cip->height); > > Why do you need to use gst_vaapi_surface_new_with_format(), why > gst_vaapi_surface_new() doesn't work for you? I guess it is because you need to use a new version of vaCreateSurfaces with attributes. It that case I think it would be better to change gst_vaapi_surface_new() to use gst_vaapi_surface_create_full() if libva is >=0.34 and use the current code as fallback. Also, all you new code of setting YUY2 should be guarded too. > > In other words, why do you need to convert from GST_VIDEO_FORMAT_YUY2 to > GST_VAAPI_CHROMA_TYPE_YUV422, and then to VA_RT_FORMAT_YUV422 again? > > ::: gst-libs/gst/vaapi/gstvaapiutils_h265.c > @@ +138,3 @@ > > + switch (sps->profile_tier_level.profile_idc) { > + case GST_H265_PROFILE_IDC_MAIN: > > all the changes from GST_H265_PROFILE_ to GST_H265_PROFILE_IDC_ belongs to a > different patch because it is a different fix. Please, split it, because it > would need to be backported to 1.14 and 1.12 > > Good catch!
Created attachment 373678 [details] [review] hevc 422 support
(In reply to Víctor Manuel Jáquez Leal from comment #6) > (In reply to Víctor Manuel Jáquez Leal from comment #5) > > Review of attachment 373669 [details] [review] [review] [review]: > > > > ::: gst-libs/gst/vaapi/gstvaapicontext.c > > @@ +141,3 @@ > > + else > > + surface = gst_vaapi_surface_new (GST_VAAPI_OBJECT_DISPLAY (context), > > + cip->chroma_type, cip->width, cip->height); > > > > Why do you need to use gst_vaapi_surface_new_with_format(), why > > gst_vaapi_surface_new() doesn't work for you? > > I guess it is because you need to use a new version of vaCreateSurfaces with > attributes. > > It that case I think it would be better to change gst_vaapi_surface_new() to > use > gst_vaapi_surface_create_full() if libva is >=0.34 and use the current code > as fallback. > > Also, all you new code of setting YUY2 should be guarded too. Modified, please have a review attach. > > > > > In other words, why do you need to convert from GST_VIDEO_FORMAT_YUY2 to > > GST_VAAPI_CHROMA_TYPE_YUV422, and then to VA_RT_FORMAT_YUV422 again? > > > > ::: gst-libs/gst/vaapi/gstvaapiutils_h265.c > > @@ +138,3 @@ > > > > + switch (sps->profile_tier_level.profile_idc) { > > + case GST_H265_PROFILE_IDC_MAIN: > > > > all the changes from GST_H265_PROFILE_ to GST_H265_PROFILE_IDC_ belongs to a > > different patch because it is a different fix. Please, split it, because it > > would need to be backported to 1.14 and 1.12 > > > > Good catch! Patch submitted to https://bugzilla.gnome.org/show_bug.cgi?id=797160. And 422 format support patch is depend on 797160, so if possible, please review and merge 797160 first.
Review of attachment 373678 [details] [review]: Also this change would need changes here: https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst/vaapi/gstvaapipluginbase.c#n1257 if I understand correctly ::: gst-libs/gst/vaapi/gstvaapicontext.c @@ +140,3 @@ + else if (cip->chroma_type == GST_VAAPI_CHROMA_TYPE_YUV420) + gst_video_info_set_format (&vi, GST_VIDEO_FORMAT_NV12, cip->width, + cip->height); This snippet has a couple problems, for example, if libva<0.34, vi is not used, so this computation is useless. Also, not all the possible chroma types are considered, it assumes, by hard-code, that the only possible context's chroma types are YUV422 and YUV420. @@ +150,3 @@ cip->chroma_type, cip->width, cip->height); +#endif + This code also is a bit problematic: we would have to change all the calls to gst_vaapi_surface_new() with this. Isn't practical, in my opinion. I guess this feature request needs deeper changes in gstvaapisurface, a refactoring of gst_vaapi_surface_new() and gst_vaapi_surface_new_full(), in order to use the newer version depending on the used libva.
(In reply to Víctor Manuel Jáquez Leal from comment #10) > Review of attachment 373678 [details] [review] [review]: > > Also this change would need changes here: > > https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst/vaapi/ > gstvaapipluginbase.c#n1257 > > if I understand correctly > > ::: gst-libs/gst/vaapi/gstvaapicontext.c > @@ +140,3 @@ > + else if (cip->chroma_type == GST_VAAPI_CHROMA_TYPE_YUV420) > + gst_video_info_set_format (&vi, GST_VIDEO_FORMAT_NV12, cip->width, > + cip->height); > > This snippet has a couple problems, for example, if libva<0.34, vi is not > used, so this computation is useless. Also, not all the possible chroma > types are considered, it assumes, by hard-code, that the only possible > context's chroma types are YUV422 and YUV420. Will fix this. > > @@ +150,3 @@ > cip->chroma_type, cip->width, cip->height); > +#endif > + > > This code also is a bit problematic: we would have to change all the calls > to gst_vaapi_surface_new() with this. Isn't practical, in my opinion. > > I guess this feature request needs deeper changes in gstvaapisurface, a > refactoring of gst_vaapi_surface_new() and gst_vaapi_surface_new_full(), in > order to use the newer version depending on the used libva. Yes, it seems better to refactor gst_vaapi_surface_new, so that don't need to modify all positions which calls for gst_vaapi_surface_new.
Created attachment 373695 [details] [review] hevc 422 support When get supported image format, only 420 chroma-format surface is created. But 420 chroma-format surface will not support 422 format(eg. yuy2/uyvy) image. So in order to get 422 format supported image, another 422 chorma-format surface need to create.
Review of attachment 373695 [details] [review]: This is getting a better shape, but still needs some love :) ::: gst-libs/gst/vaapi/gstvaapisurface.c @@ +106,3 @@ + + width = GST_VIDEO_INFO_WIDTH (vip); + height = GST_VIDEO_INFO_HEIGHT (vip); why do you need the whole GstVideoInfo structure to only keep using the same width and height? I don't understand. @@ +347,3 @@ + gst_video_info_set_format (&vi, GST_VIDEO_FORMAT_YUY2, width, height); + else + gst_video_info_set_format (&vi, GST_VIDEO_FORMAT_NV12, width, height); I guess, the best approach to this is too add a new helper function in video-format.c called, gst_vaapi_video_format_from_chroma() instead of this branches. Also this block should be guarded by #if VA_CHECK_VERSION(0,34,0) ::: gst/vaapi/gstvaapipluginbase.c @@ +1317,3 @@ + gst_vaapi_object_unref (image); + } + This is code duplication, and it is bad pattern. It is better to refactor into an internal helper function.
(In reply to Víctor Manuel Jáquez Leal from comment #13) > Review of attachment 373695 [details] [review] [review]: > > This is getting a better shape, but still needs some love :) > > ::: gst-libs/gst/vaapi/gstvaapisurface.c > @@ +106,3 @@ > + > + width = GST_VIDEO_INFO_WIDTH (vip); > + height = GST_VIDEO_INFO_HEIGHT (vip); > > why do you need the whole GstVideoInfo structure to only keep using the same > width and height? > > I don't understand. This change is for fix of your previous comment: >>This snippet has a couple problems, for example, if libva<0.34, vi is not >>used, so this computation is useless. This make sure vi can be used in libva>=0.34 and libva<0.34 in gst_vaapi_surface_new. > > @@ +347,3 @@ > + gst_video_info_set_format (&vi, GST_VIDEO_FORMAT_YUY2, width, height); > + else > + gst_video_info_set_format (&vi, GST_VIDEO_FORMAT_NV12, width, height); > > I guess, the best approach to this is too add a new helper function in > video-format.c called, gst_vaapi_video_format_from_chroma() instead of this > branches. > > Also this block should be guarded by #if VA_CHECK_VERSION(0,34,0) Will fix this. > > ::: gst/vaapi/gstvaapipluginbase.c > @@ +1317,3 @@ > + gst_vaapi_object_unref (image); > + } > + > > This is code duplication, and it is bad pattern. It is better to refactor > into an internal helper function. Will fix this.
Created attachment 373709 [details] [review] hevc 422 support
Review of attachment 373709 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapisurface.c @@ +94,3 @@ } +#if !VA_CHECK_VERSION(0,34,0) That dates from 2013. I think we can bump the required API version and drop all the ifdef.
(In reply to Nicolas Dufresne (ndufresne) from comment #16) > Review of attachment 373709 [details] [review] [review]: > > ::: gst-libs/gst/vaapi/gstvaapisurface.c > @@ +94,3 @@ > } > > +#if !VA_CHECK_VERSION(0,34,0) > > That dates from 2013. I think we can bump the required API version and drop > all the ifdef. I would love to do that. Perhaps it is a good idea to create bug issue for that.
Review of attachment 373709 [details] [review]: almost there! ::: gst-libs/gst/vaapi/video-format.c @@ +320,3 @@ +{ +#if VA_CHECK_VERSION(0,34,0) + switch (chroma_type) { I guess we could refactor the switch in gst_vaapi_video_format_get_best_native() and re-use it, avoiding code duplication.
Created attachment 373752 [details] [review] hevc 422 support
(In reply to Víctor Manuel Jáquez Leal from comment #18) > Review of attachment 373709 [details] [review] [review]: > > almost there! > > ::: gst-libs/gst/vaapi/video-format.c > @@ +320,3 @@ > +{ > +#if VA_CHECK_VERSION(0,34,0) > + switch (chroma_type) { > > I guess we could refactor the switch in > gst_vaapi_video_format_get_best_native() and re-use it, avoiding code > duplication. Fixed.
Review of attachment 373752 [details] [review]: ::: gst-libs/gst/vaapi/video-format.c @@ +287,3 @@ **/ GstVideoFormat +gst_vaapi_video_format_get_best_native (GstVideoFormat format, guint chroma) ouch! this is not what I had in mind. There's no reason to have the same function for two different tasks. My idea was to add the function gst_vaapi_video_format_from_chroma() and this function would be used too by gst_vaapi_video_fromat_get_best_native() If I get time today, I will change it and push your patch
@Fei, Do you have a media sample with this format to test the code?
Review of attachment 373752 [details] [review]: ::: gst/vaapi/gstvaapipluginbase.c @@ +1269,3 @@ + if (chroma_type == 0) + continue; + if (format == GST_VIDEO_FORMAT_UNKNOWN) you should check for format sanity before use it
Created attachment 373754 [details] [review] libs: dec: h265: add 422 chroma format support. Add main-422-10 profile which support 422 chroma format straem.
@Fei, could you do a code review of my iteration of the patch, and verify if the changes are OK for you?
Review of attachment 373754 [details] [review]: Thanks Victor, it looks good for me. Please merge it.
@Fei, do yo have a sample file to test this feature?
(In reply to Víctor Manuel Jáquez Leal from comment #28) > @Fei, do yo have a sample file to test this feature? Yes, https://drive.google.com/file/d/0BxP5-S1t9VEEaVJBVDQtTmxWVFU/view?usp=sharing. I used this stream and it can be decode correctly(need a patch from https://github.com/intel/media-driver/pull/323).
So, summing up, this new feature only works, right now, for media-driver in icelake (gen11), right? To add that in the commit log.
Created attachment 373775 [details] [review] hevc 422 support
(In reply to Víctor Manuel Jáquez Leal from comment #30) > So, summing up, this new feature only works, right now, for media-driver in > icelake (gen11), right? > > To add that in the commit log. Yes, now media-driver only support hevc 422-main-10 profile on Icelake. I submitted another patch which added the information. Thanks.
pushed attachment 373775 [details] [review] as commit 619abbde - libs: dec: h265: add 422 chroma format support.
This totally broke JPEG encode/decode! https://bugzilla.gnome.org/show_bug.cgi?id=797222
ok, only decode broke
Quick question, is it handling both 422 and 422(10) bit ? IIRC, HEVC 10bit 422 profile can have any chroma format idc (0,1 and 2). Which means surface format can be 16bit 422 (eg: UYVY) or 10bit varient. I'm not sure whether it is the issue, but you have to make sure formats like UYVY not end up with 422-10bit formats.
(In reply to sreerenj from comment #36) > Quick question, is it handling both 422 and 422(10) bit ? > IIRC, HEVC 10bit 422 profile can have any chroma format idc (0,1 and 2). > Which means surface format can be 16bit 422 (eg: UYVY) or 10bit varient. > > I'm not sure whether it is the issue, but you have to make sure formats like > UYVY not end up with 422-10bit formats. Yes, main-422-10 should support both 422 8bit and 10bit. 422 10bit support is my next plan.