After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 732691 - vaapivideomemory: Fix allocator's surfacepool format caculation
vaapivideomemory: Fix allocator's surfacepool format caculation
Status: RESOLVED FIXED
Product: gstreamer-vaapi
Classification: Other
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: gstreamer-vaapi maintainer(s)
gstreamer-vaapi maintainer(s)
Depends on:
Blocks: 720305
 
 
Reported: 2014-07-03 14:08 UTC by sreerenj
Modified: 2014-07-23 16:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vaapivideomemory: Fix allocator's surfacepool format caculation (1.62 KB, patch)
2014-07-03 14:08 UTC, sreerenj
none Details | Review
vaapivideomemory: Fix allocator's surfacepool format caculation (1.55 KB, patch)
2014-07-03 15:46 UTC, sreerenj
committed Details | Review

Description sreerenj 2014-07-03 14:08:51 UTC
Created attachment 279844 [details] [review]
vaapivideomemory: Fix allocator's surfacepool format caculation

While creating the vaapi video allocator, make sure that the associated surface pool has correct format instead of defaulting to NV12 video format even though there is no direct rendering support.
Comment 1 sreerenj 2014-07-03 14:42:52 UTC
We could create a GstvaapiImage with desired format(format of the surface), then get the image data from the surface with surface_get_image, and then update the allocator->surface_info from this new image by gst_video_info_update_from_image().  This could be the right way since the surface might have created with a different plane/stride offset by the driver. right?
Comment 2 Gwenole Beauchesne 2014-07-03 14:50:17 UTC
Review of attachment 279844 [details] [review]:

Could you please indicate how is the fixed intended to be tested? :) Thanks. I would like QA to add a test for it.

::: gst/vaapi/gstvaapivideomemory.c
@@ +528,3 @@
+                    gst_video_info_set_format(&allocator->surface_info, format,
+                        width, height);
+            }

I think we could just move up guint width, height; and make them = GST_VIDEO_INFO_{WIDTH,HEIGHT}(vip).
Comment 3 sreerenj 2014-07-03 15:46:14 UTC
It is difficult to test this ;) 

This code path will be active only in the following scenario,
-- try to create a new surface with a specific videoformat.
-- driver failing to create surface in this format choosing a  different format in same chromatype. (in this case surface_format will be always == GST_VIDEO_FORMAT_ENCODED)
-- gst_vaapi_surface_derive_image() should be successful.
-- but it won't support direct rendering because of different i/p and o/p formats.

I think there is a better way to fix this. I will add the patch
But yes, since it won't occur in usual cases, itz up to you to commit or not :) 
This just came to my mind while reading the code.
Comment 4 sreerenj 2014-07-03 15:46:34 UTC
Created attachment 279850 [details] [review]
 vaapivideomemory: Fix allocator's surfacepool format caculation
Comment 5 Gwenole Beauchesne 2014-07-23 16:39:26 UTC
Review of attachment 279850 [details] [review]:

::: gst/vaapi/gstvaapivideomemory.c
@@ +513,3 @@
                 &allocator->surface_info, image);
+            if (GST_VAAPI_IMAGE_FORMAT(image) != GST_VIDEO_INFO_FORMAT(vip))
+               allocator->has_direct_rendering = FALSE;

has_direct_rendering = update_from_image() && image-format == video-format looks more intuitive.
Comment 6 Gwenole Beauchesne 2014-07-23 16:50:31 UTC
commit 9cb3acc813320aad072672296b5af168badf82a0
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Thu Jul 3 18:41:11 2014 +0300

    vaapivideomemory: fix determination of the surface pool format.
    
    While creating the vaapi video allocator, make sure the associated
    surface pool has correct format instead of defaulting to NV12 video
    format even though there is no direct rendering support.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=732691