Only decode is broken.

Created attachment 373800 [details]
actual result

Created attachment 373801 [details]
expected result

gst-launch-1.0 -vf videotestsrc num-buffers=1 ! video/x-raw,width=1280,height=720 ! vaapijpegenc ! jpegparse ! filesink location=test1280x720.jpeg

gst-launch-1.0 -vf filesrc location=test1280x720.jpeg ! jpegparse ! vaapijpegdec ! vaapisink

i965 driver

Thanks for reporting. I will take a look at this issue.

What I can see from the images is that so we get a 4:2:0 buffer, but we think it's 4:2:2. As a result, luma is fine, because it's the same, but the chroma is off. I assume we allocated to 4:2:2 buffer since we didn't crash on a buffer overrun.

(In reply to Nicolas Dufresne (ndufresne) from comment #7)
> What I can see from the images is that so we get a 4:2:0 buffer, but we
> think it's 4:2:2. As a result, luma is fine, because it's the same, but the
> chroma is off. I assume we allocated to 4:2:2 buffer since we didn't crash
> on a buffer overrun.

Emm, I guess not. If in this case, all decode will have same problem. But now only vaapijpegdec fails but all other decode codecs looks good. This issue only happens on intel-vaapi-driver, but works fine with media-driver.

In our previous solution, when create vasurface with gst_vaapi_surface_new(gst-libs/gst/vaapi/gstvaapisurface.c +333), VASurfaceAttrib will be set as NULL, and now VASurfaceAttrib is filled with VASurfaceAttribPixelFormat of NV12.
With this change, vaapijpegdec fails.

If I understand correctly, the fix should be in intel-vaapi-driver,rather than workaround it in gstreamer-vaapi.

(In reply to Víctor Manuel Jáquez Leal from comment #9)
> If I understand correctly, the fix should be in intel-vaapi-driver,rather
> than workaround it in gstreamer-vaapi.

Talked with xiang,haihao, HW may not support NV12 surface format. One solution is that we can set surface format as NULL when create vasurface once detect it is jpeg decode. Not sure if this solution make sense. Also, add haihao in CC list.

(In reply to Fei from comment #10)
> (In reply to Víctor Manuel Jáquez Leal from comment #9)
> > If I understand correctly, the fix should be in intel-vaapi-driver,rather
> > than workaround it in gstreamer-vaapi.
> 
> Talked with xiang,haihao, HW may not support NV12 surface format. One
> solution is that we can set surface format as NULL when create vasurface
> once detect it is jpeg decode. Not sure if this solution make sense. Also,
> add haihao in CC list.

TYPO:
> Talked with xiang,haihao, HW may not support NV12 format surface for jpeg decode.

Created attachment 373812 [details] [review]
patch for this issue.

Review of attachment 373812 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapicontext.c
@@ +134,3 @@
   guint i;
 
+  gst_vaapi_context_get_surface_formats (context);

memleak: this function returns a new reference to context->formats, but you are ignoring it, thus a memory leak is produced.

Please use the internal ensure_formats()

::: gst-libs/gst/vaapi/gstvaapisurface.c
@@ +323,3 @@
+ * @width: the requested surface width
+ * @height: the requested surface height
+ * @formats: the limited format list

add a new line here

@@ +325,3 @@
+ * @formats: the limited format list
+ * Creates a new #GstVaapiSurface with the specified chroma format and
+ * dimensions.

please explain here why this function is different from the rest.

@@ +337,3 @@
+  guint i;
+
+  surface = gst_vaapi_object_new (gst_vaapi_surface_class (), display);

this surface is only used in gst_vaapi_surface_create(), if your code-path  enables ensure_format branch, another memory leak is produced.

::: gst-libs/gst/vaapi/gstvaapisurface.h
@@ +174,3 @@
 GstVaapiSurface *
+gst_vaapi_surface_new_from_formats (GstVaapiDisplay * display,
+    GstVaapiChromaType chroma_type, guint width, guint height, GArray * formts);

I'm not very fond to add new internal API. Are we sure that we couldn't add this new behavior within in the functions we already have?

(In reply to Víctor Manuel Jáquez Leal from comment #13)
> Review of attachment 373812 [details] [review] [review]:
> 
> ::: gst-libs/gst/vaapi/gstvaapicontext.c
> @@ +134,3 @@
>    guint i;
>  
> +  gst_vaapi_context_get_surface_formats (context);
> 
> memleak: this function returns a new reference to context->formats, but you
> are ignoring it, thus a memory leak is produced.
> 
> Please use the internal ensure_formats()
will fix this.
> 
> ::: gst-libs/gst/vaapi/gstvaapisurface.c
> @@ +323,3 @@
> + * @width: the requested surface width
> + * @height: the requested surface height
> + * @formats: the limited format list
> 
> add a new line here
will fix this.
> 
> @@ +325,3 @@
> + * @formats: the limited format list
> + * Creates a new #GstVaapiSurface with the specified chroma format and
> + * dimensions.
> 
> please explain here why this function is different from the rest.
will fix this.
> 
> @@ +337,3 @@
> +  guint i;
> +
> +  surface = gst_vaapi_object_new (gst_vaapi_surface_class (), display);
> 
> this surface is only used in gst_vaapi_surface_create(), if your code-path 
> enables ensure_format branch, another memory leak is produced.
will fix this.
> 
> ::: gst-libs/gst/vaapi/gstvaapisurface.h
> @@ +174,3 @@
>  GstVaapiSurface *
> +gst_vaapi_surface_new_from_formats (GstVaapiDisplay * display,
> +    GstVaapiChromaType chroma_type, guint width, guint height, GArray *
> formts);
> 
> I'm not very fond to add new internal API. Are we sure that we couldn't add
> this new behavior within in the functions we already have?
I didn't find any create surface api which can pass through GstVaapiContext or GArray formats which can be used to get appropriate format that will used to create surafce.

Created attachment 373867 [details] [review]
patch for this issue.

Review of attachment 373867 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapisurface.c
@@ +325,3 @@
+ * @formats: the limited format list
+ *
+ * Creates a new #GstVaapiSurface by using the appropriate format in #formats

If I understand correctly I would rephrase this line as:

"Creates a new #GstVaapiSurface with a @chroma_type valid for any format in @formats; if there aren't any, the returned surface is created forcing the passed @chroma_type."

@@ +340,3 @@
+    GstVideoFormat format = g_array_index (formats, GstVideoFormat, i);
+    if (format == gst_vaapi_video_format_from_chroma (chroma_type))
+      ensure_format = TRUE;

I guess you will need a 'break' instruction here to avoid keep evaluating the format list.

Also you can do this

if (format == gst_vaapi_video_format_from_chroma (chroma_type))
  return gst_vaapi_surface_new (display, chroma_type, width, height);
}

/* Fallback: if there's no format valid for the chroma type let's just used the passed chroma */
...

(In reply to Víctor Manuel Jáquez Leal from comment #16)
> Review of attachment 373867 [details] [review] [review]:
> 
> ::: gst-libs/gst/vaapi/gstvaapisurface.c
> @@ +325,3 @@
> + * @formats: the limited format list
> + *
> + * Creates a new #GstVaapiSurface by using the appropriate format in
> #formats
> 
> If I understand correctly I would rephrase this line as:
> 
> "Creates a new #GstVaapiSurface with a @chroma_type valid for any format in
> @formats; if there aren't any, the returned surface is created forcing the
> passed @chroma_type."
> 
> @@ +340,3 @@
> +    GstVideoFormat format = g_array_index (formats, GstVideoFormat, i);
> +    if (format == gst_vaapi_video_format_from_chroma (chroma_type))
> +      ensure_format = TRUE;
> 
> I guess you will need a 'break' instruction here to avoid keep evaluating
> the format list.
> 
> Also you can do this
> 
> if (format == gst_vaapi_video_format_from_chroma (chroma_type))
>   return gst_vaapi_surface_new (display, chroma_type, width, height);
> }
> 
> /* Fallback: if there's no format valid for the chroma type let's just used
> the passed chroma */
> ...

Will apply this.Thanks.

Created attachment 373879 [details] [review]
patch for this issue.

attachment 373879 [details] [review] pushed as commit 82872f42 - libs: context: query surface format before context to create surface.