[Bug 797143] vaapih265dec: support 422 chroma format hevc decode.

[GStreamer (GNOME Bugzilla)]

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

--- Comment #3 from Fei <fei.w.wang at intel.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #2)
> 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?
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.

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