GNOME Bugzilla – Bug 744042
encoder: jpeg: Support more input video formats
Last modified: 2015-08-13 15:25:38 UTC
The vaapi-intel-driver is supporting input video formats UYVY, YUY2, and Y8 for JPEG Encoding. But gstvaapiencdoer_jpeg is only supporting YUV420 formats(nv12, i420 and yv12) and the user is supposed to explicitly use the vaapipostproc before the encoder to encode other formats.
Shall we distinguish the used VA-API backend in order to expose or not those formats?
(In reply to Víctor Manuel Jáquez Leal from comment #1) > Shall we distinguish the used VA-API backend in order to expose or not those > formats? The list of supported formats can be retrieved through two ways: 1. Use vaGetConfigAttribute() with a profile/entrypoint pair, and the VAConfigAttribRTFormat attribute type ; 2. Use vaQuerySurfaceAttributes() with an appropriate config built from a profile/entrypoint pair. The former allows you to determine the colorspace, e.g. planar YUV 4:2:0, packed YUV 4:2:2, while the latter allows you to determine the underlying set of pixel formats. However, in practice, there are chances that (1) is more correctly implemented than (2), VA driver-wise.
Created attachment 306109 [details] [review] gstvaapiencoder: framerate 0/1 is valid too Framerate 0/1 is valid, and it is particularly useful for picture encoding, such as jpeg. This patch makes the encoder to admit that framerate.
Created attachment 306110 [details] [review] gstvaapiencoder: validate chroma according to the VA's RT format Before, only YUV420 color space where supported. With this patch, the encoder is queried to know the supported formats and admits YUV422 color space if its available.
Created attachment 306111 [details] [review] plugins: reduce the noise of warnings Those messagse should be attached to the object, also the lack of caps is not an error, in particular in the case of JPEG encoding.
Created attachment 306112 [details] [review] gstvaapicontext: fix the JPEG encoder attribs value When we query for the VAConfigAttribEncJPEG, we get a value which packs the VAConfigAttribValEncJPEG structure, but we did not assign it. This patch assigns the returned value to the attribute.
Created attachment 306113 [details] [review] vaapivideomemory: allocator use the requested format THIS PATCH IS A WORK IN PROGRESS Instead of hard code the use of NV12 format for the surface pool, it is used the requested one. With it, the JPEG encoder can encode directly the UYVY images. Nevertheless, it breaks the direct-rendering mode for raw buffer uploads (commit 3f15a682).
As this feature was moved to 0.6.1 I'll work in 0.6 tasks. Nevertheless, I think attachment 306109 [details] [review] and attachment 306112 [details] [review] are worth to included in 0.6 Meanwhile, attachment 306113 [details] [review] is the core for supporting more input formats for jpeg encoder, but it breaks the direct-rendering mode. We need to rethink how to handle this if we want to support this feature, that only exists in jpeg encoder, so far.
By the way, I tested it with these samples http://www.sunrayimage.com/examples.html
Review of attachment 306109 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder.c @@ +453,3 @@ if (!vip->width || !vip->height) goto error_invalid_resolution; + if (!(!vip->fps_n && vip->fps_d == 1) && (!vip->fps_n || !vip->fps_d)) Reminders: - !(A && B) == !A || !B - !(A || B) == !A && !B Anyway, more logically, what you want is error out if denominator is zero or if numerator is negative. So, if (vip->fps_n < 0 || !vip->fps_d). Besides, if you accept 0/1, you will much very likely accept 0/n with n > 0 as well. :)
Review of attachment 306110 [details] [review]: OK with cosmetic changes. ::: gst-libs/gst/vaapi/gstvaapiencoder.c @@ +609,3 @@ + if (format != GST_VIDEO_FORMAT_ENCODED && + !is_chroma_type_supported (encoder)) { + GST_ERROR ("We are only supporting YUV420 and YUV422 for encoding," YUV 4:2:0 and YUV 4:2:2
Review of attachment 306113 [details] [review]: As mentioned in the changelog, that's not really nice and heavy regression testing would be needed for implicit conversion to the various downstream sinks available. :) You'd better supply a function, possibly a helper function in libgstvaapi, that returns the best "native" pixel format that matches a particular colorspace. e.g. YUV 4:2:0 -> NV12, YUV 4:2:2 -> YUY2, YUV 4:0:0 -> Y800, etc.
Review of attachment 306111 [details] [review]: If we don't have caps, how do we determine the pixel format and surface size then?
(In reply to Gwenole Beauchesne from comment #13) > Review of attachment 306111 [details] [review] [review]: > > If we don't have caps, how do we determine the pixel format and surface size > then? I checked the implementation in base video encoder class and it seems that it is something that doesn't need a message: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/video/gstvideoencoder.c#n748 And doing jpeg encoding, always it shows up.
Created attachment 308016 [details] [review] gstvaapiencoder: framerate 0/1 is valid too Framerate 0/1 is valid, and it is particularly useful for picture encoding, such as jpeg. This patch makes the encoder to admit that framerate. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308017 [details] [review] gstvaapiencoder: validate chroma according to the VA's RT format Before, only YUV420 color space where supported. With this patch, the encoder is queried to know the supported formats and admits YUV422 color space if its available. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308018 [details] [review] plugins: reduce the noise of warnings Those messagse should be attached to the object, also the lack of caps is not an error, in particular in the case of JPEG encoding. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308019 [details] [review] gstvaapicontext: fix the JPEG encoder attribs value When we query for the VAConfigAttribEncJPEG, we get a value which packs the VAConfigAttribValEncJPEG structure, but we did not assign it. This patch assigns the returned value to the attribute. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308020 [details] [review] gstvaapivideomemory: refactor gst_vaapi_video_allocator_new() Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308021 [details] [review] surface pool config based on video info First added the function gst_vaapi_video_format_get_best_native(), which returns the best native format that matches a particular chroma type: YUV 4:2:0 -> NV12, YUV 4:2:2 -> YUY2, YUV 4:0:0 -> Y800 That format is used to configure the surface's pool for the allocator. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308073 [details] [review] surface pool config based on video info First added the function gst_vaapi_video_format_get_best_native(), which returns the best native format that matches a particular chroma type: YUV 4:2:0 -> NV12, YUV 4:2:2 -> YUY2, YUV 4:0:0 -> Y800 That format is used to configure the surface's pool for the allocator. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Review of attachment 308016 [details] [review]: lgtm
Review of attachment 308017 [details] [review]: lgtm
Review of attachment 308018 [details] [review]: lgtm
Review of attachment 308019 [details] [review]: I think we need to do some rework here. First of all, VAConfigAttribEncJPEG is read-only. Also the VAConfigAttribEncJPEG.value == VAConfigAttribValEncJPEG fields packed in bits. IMHO, This is only useful when we have more user configurations in the input, for eg, if we have a property to set coding mode(whether arithamatic or huffman) then we have to query whether the hw is supporting this..For now, we can just keep only the following code: attrib->type = VAConfigAttribEncJPEG; if (!context_get_attribute (context, attrib->type, &value)) goto cleanup; attrib->value = value; attrib++;
Review of attachment 308020 [details] [review]: lgtm..
Review of attachment 308073 [details] [review]: lgtm ,except the commented section :) ::: gst/vaapi/gstvaapivideomemory.c @@ +670,3 @@ /* nothing to configure */ + if (GST_VIDEO_INFO_FORMAT (vinfo) == GST_VIDEO_FORMAT_ENCODED || + fmt != GST_VIDEO_FORMAT_NV12) Sorry, I didn't get the meaning/requirement of "fmt != GST_VIDEO_FORMAT_NV12"
Created attachment 308223 [details] [review] gstvaapiencoder: framerate 0/1 is valid too Framerate 0/1 is valid, and it is particularly useful for picture encoding, such as jpeg. This patch makes the encoder to admit that framerate. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308224 [details] [review] gstvaapiencoder: validate chroma according to the VA's RT format Before, only YUV420 color space where supported. With this patch, the encoder is queried to know the supported formats and admits YUV422 color space if its available. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308225 [details] [review] plugins: reduce the noise of warnings Those messagse should be attached to the object, also the lack of caps is not an error, in particular in the case of JPEG encoding. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308226 [details] [review] gstvaapicontext: fix the JPEG encoder attribs value When we query for the VAConfigAttribEncJPEG, we get a value which packs the VAConfigAttribValEncJPEG structure, but we did not assign it. This patch assigns the returned value to the attribute. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308227 [details] [review] gstvaapivideomemory: refactor gst_vaapi_video_allocator_new() Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308228 [details] [review] surface pool config based on video info First added the function gst_vaapi_video_format_get_best_native(), which returns the best native format that matches a particular chroma type: YUV 4:2:0 -> NV12, YUV 4:2:2 -> YUY2, YUV 4:0:0 -> Y800 RGB32 chroma and encoded format map to NV12 too. That format is used to configure the surface's pool for the allocator. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
ouch... I uploaded all the patches again and lost the reviews. I'll do it, but left the ones with work-needed.
Review of attachment 308226 [details] [review]: fine :)
Review of attachment 308228 [details] [review]: ::: gst/vaapi/gstvaapivideomemory.c @@ +671,3 @@ + * update its parameters from internal VaapiImage configuration. */ + if (GST_VIDEO_INFO_FORMAT (vinfo) == GST_VIDEO_FORMAT_ENCODED || + fmt != GST_VIDEO_FORMAT_NV12) How about the code path to find "allocator->has_direct_rendering" ? Suppose the driver has direct rendering support for YUY2 (YUV422), this code path will just return with out checking the direct rendering capability...., am I missing something?
(In reply to sreerenj from comment #36) > Review of attachment 308228 [details] [review] [review]: > > ::: gst/vaapi/gstvaapivideomemory.c > @@ +671,3 @@ > + * update its parameters from internal VaapiImage configuration. */ > + if (GST_VIDEO_INFO_FORMAT (vinfo) == GST_VIDEO_FORMAT_ENCODED || > + fmt != GST_VIDEO_FORMAT_NV12) > > How about the code path to find "allocator->has_direct_rendering" ? > Suppose the driver has direct rendering support for YUY2 (YUV422), this code > path will just return with out checking the direct rendering capability...., > am I missing something? If I understand correctly the code, it is more complex than that. The direct rendering flag is just a side calculation. The main complexity is that this code allocates memory for buffers that will go either upstream (encoders/vpp) and downstream (decoders/vpp), and right now there is not a mechanism to know for which purpose is going to be used. Than snippet after that conditional configures the surface's info, from which the surface pool will be configured. That configuration is taken from a dummy derive image, which format is always (???) NV12. That snippet is useful, *AFAIK*, in the case of decoders (buffers that are going to flow downstream) and if these buffers are going to be downloaded into the system memory. But it is not in the case of encoders, where the buffer is going to be uploaded into a VA-API surface. I used this heuristics, which looks OK for me. But perhaps the real solution will be to add a mechanism to tell the allocator if the buffer is going to be used upstream or downstream. It tried to figure out one, but I couldn't find a clean solution.
(In reply to Víctor Manuel Jáquez Leal from comment #37) > (In reply to sreerenj from comment #36) > > Review of attachment 308228 [details] [review] [review] [review]: > > > > ::: gst/vaapi/gstvaapivideomemory.c > > @@ +671,3 @@ > > + * update its parameters from internal VaapiImage configuration. */ > > + if (GST_VIDEO_INFO_FORMAT (vinfo) == GST_VIDEO_FORMAT_ENCODED || > > + fmt != GST_VIDEO_FORMAT_NV12) > > > > How about the code path to find "allocator->has_direct_rendering" ? > > Suppose the driver has direct rendering support for YUY2 (YUV422), this code > > path will just return with out checking the direct rendering capability...., > > am I missing something? > > If I understand correctly the code, it is more complex than that. The direct > rendering flag is just a side calculation. > > The main complexity is that this code allocates memory for buffers that will > go either upstream (encoders/vpp) and downstream (decoders/vpp), and right > now there is not a mechanism to know for which purpose is going to be used. > > Than snippet after that conditional configures the surface's info, from > which the surface pool will be configured. That configuration is taken from > a dummy derive image, which format is always (???) NV12. No, I think :) > > That snippet is useful, *AFAIK*, in the case of decoders (buffers that are > going to flow downstream) and if these buffers are going to be downloaded > into the system memory. But it is not in the case of encoders, where the > buffer is going to be uploaded into a VA-API surface. > > I used this heuristics, which looks OK for me. But perhaps the real solution > will be to add a mechanism to tell the allocator if the buffer is going to > be used upstream or downstream. It tried to figure out one, but I couldn't > find a clean solution.
(In reply to sreerenj from comment #38) > (In reply to Víctor Manuel Jáquez Leal from comment #37) > > (In reply to sreerenj from comment #36) > > > Review of attachment 308228 [details] [review] [review] [review] [review]: > > > > > > ::: gst/vaapi/gstvaapivideomemory.c > > > @@ +671,3 @@ > > > + * update its parameters from internal VaapiImage configuration. */ > > > + if (GST_VIDEO_INFO_FORMAT (vinfo) == GST_VIDEO_FORMAT_ENCODED || > > > + fmt != GST_VIDEO_FORMAT_NV12) > > > > > > How about the code path to find "allocator->has_direct_rendering" ? > > > Suppose the driver has direct rendering support for YUY2 (YUV422), this code > > > path will just return with out checking the direct rendering capability...., > > > am I missing something? > > > > If I understand correctly the code, it is more complex than that. The direct > > rendering flag is just a side calculation. > > > > The main complexity is that this code allocates memory for buffers that will > > go either upstream (encoders/vpp) and downstream (decoders/vpp), and right > > now there is not a mechanism to know for which purpose is going to be used. upstream/downstream usage: please see the code in gst_video_meta_map_vaapi_memory(), ensure surface will create a new surface for upstream element use case, or use the existing surface (which could be set by decoder in meta) for downstream use case... > > Than snippet after that conditional configures the surface's info, from > > which the surface pool will be configured. That configuration is taken from > > a dummy derive image, which format is always (???) NV12. > > No, I think :) > > > > > That snippet is useful, *AFAIK*, in the case of decoders (buffers that are > > going to flow downstream) and if these buffers are going to be downloaded > > into the system memory. But it is not in the case of encoders, where the > > buffer is going to be uploaded into a VA-API surface. > > > > I used this heuristics, which looks OK for me. But perhaps the real solution > > will be to add a mechanism to tell the allocator if the buffer is going to > > be used upstream or downstream. It tried to figure out one, but I couldn't > > find a clean solution.
Created attachment 308407 [details] [review] surface pool config based on video info First added the function gst_vaapi_video_format_get_best_native(), which returns the best native format that matches a particular chroma type: YUV 4:2:0 -> NV12, YUV 4:2:2 -> YUY2, YUV 4:0:0 -> Y800 RGB32 chroma and encoded format map to NV12 too. That format is used to configure the surface's pool for the allocator, but only if the derived image has the same format of the native format. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Review of attachment 308407 [details] [review]: ::: gst/vaapi/gstvaapivideomemory.c @@ +684,3 @@ + * native one */ + updated = (GST_VAAPI_IMAGE_FORMAT (image) == fmt) && + gst_video_info_update_from_image (&allocator->surface_info, image); I don't think it is right :) If the driver is capable of doing derive_image, it is the actual zero copy (even though we are disabling the direct rendering in gstremaer-vaapi), and the format we have to use for surface is the format that we get from derived image.. For eg: in my HSW, it is YV12...
This format juggling is hard. Let's go shopping.... (In reply to sreerenj from comment #41) > Review of attachment 308407 [details] [review] [review]: > > ::: gst/vaapi/gstvaapivideomemory.c > @@ +684,3 @@ > + * native one */ > + updated = (GST_VAAPI_IMAGE_FORMAT (image) == fmt) && > + gst_video_info_update_from_image (&allocator->surface_info, image); > > I don't think it is right :) > If the driver is capable of doing derive_image, it is the actual zero copy > (even though we are disabling the direct rendering in gstremaer-vaapi), and > the format we have to use for surface is the format that we get from derived > image.. > For eg: in my HSW, it is YV12... Zero copy is interesting for decoders (buffer downstream). In the case of encoders (buffer upstream) this format change corrupts the output image.
(In reply to sreerenj from comment #39) > upstream/downstream usage: please see the code in > gst_video_meta_map_vaapi_memory(), ensure surface will create a new surface > for upstream element use case, or use the existing surface (which could be > set by decoder in meta) for downstream use case... It looks to me that this heuristics is valid only for memory allocation because gst_vaapi_video_memory_new() passes the meta. But when creating the allocator the code flow is different. As far as I can see, we need to add another flag to pass to the sink buffer pool configuration in the case of encoders, so we can bail out this zero copy configuration.
Created attachment 308573 [details] [review] surface pool config based on video info First added the function gst_vaapi_video_format_get_best_native(), which returns the best native format that matches a particular chroma type: YUV 4:2:0 -> NV12, YUV 4:2:2 -> YUY2, YUV 4:0:0 -> Y800 RGB32 chroma and encoded format map to NV12 too. That format is used to configure, initially, the surface's pool for the allocator. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308574 [details] [review] allocator is aware of buffer type The allocator only needs to configure its surface (according with the derive image) if the buffer is going to be rendered. Otherwise it is not needed to configure it with the derive image format because is not going to be used. Even more, it may corrupt the input image. This patch, using the GstBufferPoolConfig notices to the allocator if the surface info needs to be configured or not. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
(In reply to Víctor Manuel Jáquez Leal from comment #42) > This format juggling is hard. Let's go shopping.... > > (In reply to sreerenj from comment #41) > > Review of attachment 308407 [details] [review] [review] [review]: > > > > ::: gst/vaapi/gstvaapivideomemory.c > > @@ +684,3 @@ > > + * native one */ > > + updated = (GST_VAAPI_IMAGE_FORMAT (image) == fmt) && > > + gst_video_info_update_from_image (&allocator->surface_info, image); > > > > I don't think it is right :) > > If the driver is capable of doing derive_image, it is the actual zero copy > > (even though we are disabling the direct rendering in gstremaer-vaapi), and > > the format we have to use for surface is the format that we get from derived > > image.. > > For eg: in my HSW, it is YV12... > > Zero copy is interesting for decoders (buffer downstream). In the case of > encoders (buffer upstream) this format change corrupts the output image. Corrupt??, Could you please provide some examples??
(In reply to Víctor Manuel Jáquez Leal from comment #43) > (In reply to sreerenj from comment #39) > > upstream/downstream usage: please see the code in > > gst_video_meta_map_vaapi_memory(), ensure surface will create a new surface > > for upstream element use case, or use the existing surface (which could be > > set by decoder in meta) for downstream use case... > > It looks to me that this heuristics is valid only for memory allocation > because gst_vaapi_video_memory_new() passes the meta. But when creating the > allocator the code flow is different. We always use gst_vaapi_video_memroy_new(), isn't it?? > > As far as I can see, we need to add another flag to pass to the sink buffer > pool configuration in the case of encoders, so we can bail out this zero > copy configuration. Anyway we always use vaGetImage() , not derive_image in gstremaer-vaapi :)..So there is no "actual-zero-copy"..
Review of attachment 308573 [details] [review]: lgtm,
Created attachment 308661 [details] corrupted jpeg because of wrong surface info
(In reply to sreerenj from comment #46) > (In reply to Víctor Manuel Jáquez Leal from comment #42) > > This format juggling is hard. Let's go shopping.... > > > > (In reply to sreerenj from comment #41) > > > Review of attachment 308407 [details] [review] [review] [review] [review]: > > > > > > ::: gst/vaapi/gstvaapivideomemory.c > > > @@ +684,3 @@ > > > + * native one */ > > > + updated = (GST_VAAPI_IMAGE_FORMAT (image) == fmt) && > > > + gst_video_info_update_from_image (&allocator->surface_info, image); > > > > > > I don't think it is right :) > > > If the driver is capable of doing derive_image, it is the actual zero copy > > > (even though we are disabling the direct rendering in gstremaer-vaapi), and > > > the format we have to use for surface is the format that we get from derived > > > image.. > > > For eg: in my HSW, it is YV12... > > > > Zero copy is interesting for decoders (buffer downstream). In the case of > > encoders (buffer upstream) this format change corrupts the output image. > > Corrupt??, Could you please provide some examples?? attachment 308661 [details] It is get from tulips_uyvy422_prog_packed_qcif.yuv downloaded from http://www.sunrayimage.com/examples.html and using this pipeline gst-launch-1.0 -e filesrc location=~/tulips_uyvy422_prog_packed_qcif.yuv ! videoparse format=uyvy width=176 height=144 ! vaapiencode_jpeg ! filesink location=~/tulips-uyvy.jpg With all the patches above except the last one (attachment 308574 [details] [review])
(In reply to sreerenj from comment #47) > (In reply to Víctor Manuel Jáquez Leal from comment #43) > > (In reply to sreerenj from comment #39) > > > upstream/downstream usage: please see the code in > > > gst_video_meta_map_vaapi_memory(), ensure surface will create a new surface > > > for upstream element use case, or use the existing surface (which could be > > > set by decoder in meta) for downstream use case... > > > > It looks to me that this heuristics is valid only for memory allocation > > because gst_vaapi_video_memory_new() passes the meta. But when creating the > > allocator the code flow is different. > > We always use gst_vaapi_video_memroy_new(), isn't it?? Yes, but it uses the allocator's information. > > As far as I can see, we need to add another flag to pass to the sink buffer > > pool configuration in the case of encoders, so we can bail out this zero > > copy configuration. > > Anyway we always use vaGetImage() , not derive_image in gstremaer-vaapi > :)..So there is no "actual-zero-copy".. Now I utterly confused. Can I remove the "derive image video information update" code??
(In reply to Víctor Manuel Jáquez Leal from comment #51) > (In reply to sreerenj from comment #47) > > (In reply to Víctor Manuel Jáquez Leal from comment #43) > > > (In reply to sreerenj from comment #39) > > > > upstream/downstream usage: please see the code in > > > > gst_video_meta_map_vaapi_memory(), ensure surface will create a new surface > > > > for upstream element use case, or use the existing surface (which could be > > > > set by decoder in meta) for downstream use case... > > > > > > It looks to me that this heuristics is valid only for memory allocation > > > because gst_vaapi_video_memory_new() passes the meta. But when creating the > > > allocator the code flow is different. > > > > We always use gst_vaapi_video_memroy_new(), isn't it?? > > Yes, but it uses the allocator's information. > > > > As far as I can see, we need to add another flag to pass to the sink buffer > > > pool configuration in the case of encoders, so we can bail out this zero > > > copy configuration. > > > > Anyway we always use vaGetImage() , not derive_image in gstremaer-vaapi > > :)..So there is no "actual-zero-copy".. > > Now I utterly confused. Can I remove the "derive image video information > update" code?? We were using derive image before, but it was causing issues here and there..So, Gwenole has enabled the vaGetImage/vaPutImage (no direct-rendering) as the default code path (IIRC)... I would prefer to keep all codes related with vaDeriveImage , we can think about using derive image as default code path in future,,not now for sure..
Created attachment 308680 [details] [review] gstvaapivideomemory: native format with no derived image If USE_NATIVE_FORMATS is defined we bail out before configuring the surface info based on the derived image configuration. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 308681 [details] [review] gstvaapivideomemory: remove direct rendering path Since the direct rendering code flow path is not used, this commit removes it completely. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
(In reply to Víctor Manuel Jáquez Leal from comment #54) > Created attachment 308681 [details] [review] [review] > gstvaapivideomemory: remove direct rendering path I added this commit just for completeness. If we want to remove this code path. Otherwise, my best option (right now) is attachment 308680 [details] [review] > > Since the direct rendering code flow path is not used, this commit removes it > completely. > > Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
(In reply to Víctor Manuel Jáquez Leal from comment #53) > Created attachment 308680 [details] [review] [review] > gstvaapivideomemory: native format with no derived image > > If USE_NATIVE_FORMATS is defined we bail out before configuring the surface > info based on the derived image configuration. > > Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> I have a feeling that we are hiding the actual issue here... Let me try to explain what I understood: with out this patch: ----------------------- SurfaceFormat = NV12 ImageForamt = UYVY outcome: corrupted image with this patch ------------------------ SurfaceFormat = YUY2 ImageFormat = UYVY outcome: correctly encoded jpeg image So the issue is when doing vaPutimage of YUV422 VAImage to YUV420 VASurface, isn't it?
Attachment 308223 [details] pushed as 351496a - gstvaapiencoder: framerate 0/1 is valid too Attachment 308225 [details] pushed as 9bdf43a - plugins: reduce the noise of warnings Attachment 308226 [details] pushed as 1e8610f - gstvaapicontext: fix the JPEG encoder attribs value
(In reply to sreerenj from comment #56) > (In reply to Víctor Manuel Jáquez Leal from comment #53) > > Created attachment 308680 [details] [review] [review] [review] > > gstvaapivideomemory: native format with no derived image > > > > If USE_NATIVE_FORMATS is defined we bail out before configuring the surface > > info based on the derived image configuration. > > > > Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> > > I have a feeling that we are hiding the actual issue here... > Let me try to explain what I understood: > > with out this patch: > ----------------------- > SurfaceFormat = NV12 > ImageForamt = UYVY > outcome: corrupted image > > with this patch > ------------------------ > SurfaceFormat = YUY2 > ImageFormat = UYVY > outcome: correctly encoded jpeg image > > So the issue is when doing vaPutimage of YUV422 VAImage to YUV420 VASurface, > isn't it? Yes that's the problem. I did this experiment: I patched libva-intel-driver: diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c index 4ba87f8..14342a7 100644 --- a/src/i965_drv_video.c +++ b/src/i965_drv_video.c @@ -4434,10 +4434,10 @@ i965_PutImage(VADriverContextP ctx, dst_rect.width = dest_width; dst_rect.height = dest_height; - if (HAS_ACCELERATED_PUTIMAGE(i965)) - va_status = i965_hw_putimage(ctx, obj_surface, obj_image, - &src_rect, &dst_rect); - else + //if (HAS_ACCELERATED_PUTIMAGE(i965)) + // va_status = i965_hw_putimage(ctx, obj_surface, obj_image, + // &src_rect, &dst_rect); + //else va_status = i965_sw_putimage(ctx, obj_surface, obj_image, &src_rect, &dst_rect); To force the software path in putimage, and the pipeline returned this error: 0:00:17.803299636 26615 0x6654a0 DEBUG vaapi gstvaapiutils.c:53:vaapi_check_status: vaPutImage(): invalid VAImageFormat 0:00:17.803358758 26615 0x6654a0 ERROR vaapiencode gstvaapiencode.c:522:gst_vaapiencode_handle_frame: failed to get VA surface proxy This is, by software, the image format must be the same of the surface format. By hardware, in the specific case of SKL, the post-processor is used, it is suppose to handle the color format conversion, but it doesn't do it correctly. I'm writing a small app to prove this error, and file a bug in libva-intel-driver. As far as I understand attachment 308680 [details] [review] is a valid solution if we do not support direct-rendering (as is now), and we want to support putimage's software path. In case we want to enable direct-rendering but no direct-uploading (*), in my opinion, attachment 308574 [details] [review] may be in the right direction. I we want to enable "direct-uploading" that would demand some redesign, since that code path was not contemplated as far as I see. * direct-rendering is not semantically correct in the case of encoding, though vaDeriveImage can be used, and we should call it direct-uploading, perhaps.
(In reply to Víctor Manuel Jáquez Leal from comment #58) > (In reply to sreerenj from comment #56) > > (In reply to Víctor Manuel Jáquez Leal from comment #53) > > > Created attachment 308680 [details] [review] [review] [review] [review] > > > gstvaapivideomemory: native format with no derived image > > > > > > If USE_NATIVE_FORMATS is defined we bail out before configuring the surface > > > info based on the derived image configuration. > > > > > > Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> > > > > I have a feeling that we are hiding the actual issue here... > > Let me try to explain what I understood: > > > > with out this patch: > > ----------------------- > > SurfaceFormat = NV12 > > ImageForamt = UYVY > > outcome: corrupted image > > > > with this patch > > ------------------------ > > SurfaceFormat = YUY2 > > ImageFormat = UYVY > > outcome: correctly encoded jpeg image > > > > So the issue is when doing vaPutimage of YUV422 VAImage to YUV420 VASurface, > > isn't it? > > Yes that's the problem. > > I did this experiment: I patched libva-intel-driver: > > diff --git a/src/i965_drv_video.c b/src/i965_drv_video.c > index 4ba87f8..14342a7 100644 > --- a/src/i965_drv_video.c > +++ b/src/i965_drv_video.c > @@ -4434,10 +4434,10 @@ i965_PutImage(VADriverContextP ctx, > dst_rect.width = dest_width; > dst_rect.height = dest_height; > > - if (HAS_ACCELERATED_PUTIMAGE(i965)) > - va_status = i965_hw_putimage(ctx, obj_surface, obj_image, > - &src_rect, &dst_rect); > - else > + //if (HAS_ACCELERATED_PUTIMAGE(i965)) > + // va_status = i965_hw_putimage(ctx, obj_surface, obj_image, > + // &src_rect, &dst_rect); > + //else > va_status = i965_sw_putimage(ctx, obj_surface, obj_image, > &src_rect, &dst_rect); > > To force the software path in putimage, and the pipeline returned this error: > > 0:00:17.803299636 26615 0x6654a0 DEBUG vaapi > gstvaapiutils.c:53:vaapi_check_status: vaPutImage(): invalid VAImageFormat > 0:00:17.803358758 26615 0x6654a0 ERROR vaapiencode > gstvaapiencode.c:522:gst_vaapiencode_handle_frame: failed to get VA surface > proxy > > This is, by software, the image format must be the same of the surface > format. By hardware, in the specific case of SKL, the post-processor is > used, it is suppose to handle the color format conversion, but it doesn't do > it correctly. > > I'm writing a small app to prove this error, and file a bug in > libva-intel-driver. Good work ! thanks..Please file a bug against driver... > > As far as I understand attachment 308680 [details] [review] [review] is a valid > solution if we do not support direct-rendering (as is now), and we want to > support putimage's software path. This is fine for me...please push So thats it for jpeg encoder , right? > > > In case we want to enable direct-rendering but no direct-uploading (*), in > my opinion, attachment 308574 [details] [review] [review] may be in the right > direction. > > I we want to enable "direct-uploading" that would demand some redesign, > since that code path was not contemplated as far as I see. > > * direct-rendering is not semantically correct in the case of encoding, > though vaDeriveImage can be used, and we should call it direct-uploading, > perhaps. May we can track all these in a separate bug, wdt?
(In reply to sreerenj from comment #59) > > I'm writing a small app to prove this error, and file a bug in > > libva-intel-driver. > > Good work ! thanks..Please file a bug against driver... https://bugs.freedesktop.org/show_bug.cgi?id=91624 > > > > > As far as I understand attachment 308680 [details] [review] [review] [review] is a valid > > solution if we do not support direct-rendering (as is now), and we want to > > support putimage's software path. > > This is fine for me...please push > So thats it for jpeg encoder , right? I'll push it now. > > I we want to enable "direct-uploading" that would demand some redesign, > > since that code path was not contemplated as far as I see. > > > > * direct-rendering is not semantically correct in the case of encoding, > > though vaDeriveImage can be used, and we should call it direct-uploading, > > perhaps. > > May we can track all these in a separate bug, wdt? bug #753591
Attachment 308224 [details] pushed as 002ac0b - gstvaapiencoder: validate chroma according to the VA's RT format Attachment 308227 [details] pushed as 940dc1a - gstvaapivideomemory: refactor gst_vaapi_video_allocator_new() Attachment 308573 [details] pushed as ceb70c3 - surface pool config based on video info Attachment 308680 [details] pushed as f0d6b0a - gstvaapivideomemory: native format with no derived image