GNOME Bugzilla – Bug 719372
vaapidecode: reports caps correctly if downstream supports GLTextureUploadMeta
Last modified: 2014-05-28 09:48:23 UTC
Vaapidecode does report its caps correctly when the downstream element only support the GLTextureUploadMeta API and always set its caps to NV12 (or another format supported by the decoder). This occurs in the auto-plugged and non-auto-plugged cases. The following patch tries to fix the non-auto-plugged case by asking the downstream element if it supports the GLTextureUploadMeta API. At the moment, i can't find a way to solve the auto-plugged case. Would you have an idea on how to solve it ?
Created attachment 262883 [details] [review] vaapidecode: check if downstream supports GLTextureUploadMeta
Created attachment 263936 [details] [review] vaapidecode: check if downstream supports GLTextureUploadMeta Patch rebased on master + minor fix preventing the source caps to be updated if incoming caps are NV12 and video decoder output state format is ENCODED.
Created attachment 264143 [details] [review] vaapidecode: check if downstream supports GLTextureUploadMeta vaapidecode does not query downstream caps anymore (which was wrong) but use the query allocation instead to decide whenever to use GLTextureUploadMeta and set the correct caps accordingly. Note that the GLTextureUploadMeta is set on the caps preventing downstream elements which don't support it to try to map the frames (ie: deinterlace, videobalance ). Related playbin, deinterlace and videobalance patches: https://bugzilla.gnome.org/show_bug.cgi?id=720205 https://bugzilla.gnome.org/show_bug.cgi?id=719636 https://bugzilla.gnome.org/show_bug.cgi?id=720388 https://bugzilla.gnome.org/show_bug.cgi?id=720345
Hi, I also thought about adding (ANY) to videobalance et al., and did so locally actually to help auto-plugging, but it didn't look right in the end. Is the proposed patch OK for GStreamer >= 1.3 only, or do we plan to backport the core GStreamer patches to 1.2 as well? Thanks. Meanwhile, I am also working on factoring out the code so that we readily have the same functionality for vaapipostproc too, instead of copying the same amount of code over to it.
Created attachment 264497 [details] [review] vaapidecode: check if downstream supports GLTextureUploadMeta Patch updated. Vaapidecode queries downstream and looks what format are prefered (according to caps order) and try to find something that is compatible with, set its caps to this format and check in decide_allocation if the feature (GLTextureUpload) is supported downstream via the query allocation. With this patch and the related patch to deinterlace/colorbalance/playbin, vaapidecode works with playbin minus the fact that interlaced content is not deinterlaced.
(In reply to comment #5) > Patch updated. Vaapidecode queries downstream and looks what format are > prefered (according to caps order) and try to find something that is compatible > with, set its caps to this format and check in decide_allocation if the feature > (GLTextureUpload) is supported downstream via the query allocation. Looks reasonable. Thanks. I wonder if there is any chance to drop the dependency on GstVaapiDecode, and let GstVaapiPluginBase do the job, e.g. through gst_vaapi_plugin_base_set_caps() + gst_vaapi_plugin_base_decide_allocation(). Though, I'd rather work on that myself since I don't express it well. :) > With this patch and the related patch to deinterlace/colorbalance/playbin, > vaapidecode works with playbin minus the fact that interlaced content is not > deinterlaced. My next subsidiary question is: does it still work with an older GStreamer 1.2 installation that does not provide those patched plugins? Concerning deinterlace: my idea was to let the autoplug magic try to resolve vaapipostproc if vaapisink has interlace-mode=progressive. i.e. we could imagine the resolve finding out that vaapidecode emits memory:VASurface + interlace-mode=mixed for instance, vaapisink requiring memory:VASurface + interlace-mode=progressive and vaapipostproc can fit the requirements. Alternative: add video-processing caps features. Advantage this can work for deinterlace but for other filters to be combined in. i.e. vaapipostproc could handle deinterlace + videobalance at once.
Created attachment 264532 [details] [review] vaapidecode: check if downstream supports GLTextureUploadMeta Fix typo /st_vaapidecode_update_src_caps/gst_vaapidecode_update_src_caps/
(In reply to comment #6) > (In reply to comment #5) > > Patch updated. Vaapidecode queries downstream and looks what format are > > prefered (according to caps order) and try to find something that is compatible > > with, set its caps to this format and check in decide_allocation if the feature > > (GLTextureUpload) is supported downstream via the query allocation. > > Looks reasonable. Thanks. I wonder if there is any chance to drop the > dependency on GstVaapiDecode, and let GstVaapiPluginBase do the job, e.g. > through gst_vaapi_plugin_base_set_caps() + > gst_vaapi_plugin_base_decide_allocation(). Though, I'd rather work on that > myself since I don't express it well. :) I'll have a look at GstVaapiPluginBase. > > > With this patch and the related patch to deinterlace/colorbalance/playbin, > > vaapidecode works with playbin minus the fact that interlaced content is not > > deinterlaced. > > My next subsidiary question is: does it still work with an older GStreamer 1.2 > installation that does not provide those patched plugins? It *shoudl* work if you don't use it with playbin. With playbin you'll need at least (considering you use the native video flags to disable deinterlace/colorbalance/...) the playbin patch to allow it to insert decoder which set theirs caps with a feature (other than sysmem). If you don't set the native video flags, you'll need a patched version of deinterlace and colorbalance. > > Concerning deinterlace: my idea was to let the autoplug magic try to resolve > vaapipostproc if vaapisink has interlace-mode=progressive. i.e. we could > imagine the resolve finding out that vaapidecode emits memory:VASurface + > interlace-mode=mixed for instance, vaapisink requiring memory:VASurface + > interlace-mode=progressive and vaapipostproc can fit the requirements. > > Alternative: add video-processing caps features. Advantage this can work for > deinterlace but for other filters to be combined in. i.e. vaapipostproc could > handle deinterlace + videobalance at once. I agree, a long term solution needs to be found to make playbin plugs the right "filter" elements such as deinterlace/colorbalance/... In the ideal case, playbin would also be able to know that a filter is also able to process it at once.
(In reply to comment #8) > > > With this patch and the related patch to deinterlace/colorbalance/playbin, > > > vaapidecode works with playbin minus the fact that interlaced content is not > > > deinterlaced. > > > > My next subsidiary question is: does it still work with an older GStreamer 1.2 > > installation that does not provide those patched plugins? > > It *shoudl* work if you don't use it with playbin. > > With playbin you'll need at least (considering you use the native video flags > to disable deinterlace/colorbalance/...) the playbin patch to allow it to > insert decoder which set theirs caps with a feature (other than sysmem). > If you don't set the native video flags, you'll need a patched version of > deinterlace and colorbalance. Ah, then we probably should make the patch for GStreamer >= 1.3 only then for now. That's because I cannot expect all OS vendors to upgrade their GStreamer 1.2 stack in existing releases, in a timely manner. My concern is that gst-launch-1.0 playbin [...] still needs to work on existing GStreamer 1.2 installations. > > Concerning deinterlace: my idea was to let the autoplug magic try to resolve > > vaapipostproc if vaapisink has interlace-mode=progressive. i.e. we could > > imagine the resolve finding out that vaapidecode emits memory:VASurface + > > interlace-mode=mixed for instance, vaapisink requiring memory:VASurface + > > interlace-mode=progressive and vaapipostproc can fit the requirements. > > > > Alternative: add video-processing caps features. Advantage this can work for > > deinterlace but for other filters to be combined in. i.e. vaapipostproc could > > handle deinterlace + videobalance at once. > > I agree, a long term solution needs to be found to make playbin plugs the right > "filter" elements such as deinterlace/colorbalance/... > In the ideal case, playbin would also be able to know that a filter is also > able to process it at once. I will try to think about opening an appropriate bug report for that, unless there is already one? i.e. what about your deinterlacebin proposal?
(In reply to comment #9) > (In reply to comment #8) > > > > With this patch and the related patch to deinterlace/colorbalance/playbin, > > > > vaapidecode works with playbin minus the fact that interlaced content is not > > > > deinterlaced. > > > > > > My next subsidiary question is: does it still work with an older GStreamer 1.2 > > > installation that does not provide those patched plugins? > > > > It *shoudl* work if you don't use it with playbin. > > > > With playbin you'll need at least (considering you use the native video flags > > to disable deinterlace/colorbalance/...) the playbin patch to allow it to > > insert decoder which set theirs caps with a feature (other than sysmem). > > If you don't set the native video flags, you'll need a patched version of > > deinterlace and colorbalance. > > Ah, then we probably should make the patch for GStreamer >= 1.3 only then for > now. That's because I cannot expect all OS vendors to upgrade their GStreamer > 1.2 stack in existing releases, in a timely manner. My concern is that > gst-launch-1.0 playbin [...] still needs to work on existing GStreamer 1.2 > installations. I can probably make it work with unpatched gstreamer 1.2 but i'm not sure as it will increase complexity. > > > > Concerning deinterlace: my idea was to let the autoplug magic try to resolve > > > vaapipostproc if vaapisink has interlace-mode=progressive. i.e. we could > > > imagine the resolve finding out that vaapidecode emits memory:VASurface + > > > interlace-mode=mixed for instance, vaapisink requiring memory:VASurface + > > > interlace-mode=progressive and vaapipostproc can fit the requirements. > > > > > > Alternative: add video-processing caps features. Advantage this can work for > > > deinterlace but for other filters to be combined in. i.e. vaapipostproc could > > > handle deinterlace + videobalance at once. > > > > I agree, a long term solution needs to be found to make playbin plugs the right > > "filter" elements such as deinterlace/colorbalance/... > > In the ideal case, playbin would also be able to know that a filter is also > > able to process it at once. > > I will try to think about opening an appropriate bug report for that, unless > there is already one? i.e. what about your deinterlacebin proposal? Here is a related bug: https://bugzilla.gnome.org/show_bug.cgi?id=687182 I have not yet created a bug related to the deinterlacebin feature. It came up from a discussion with slomo on IRC. I'll open a bug related to this when I have a better view over playbin and how this feature can be implemented. One question, there is something missing in the current patch when it resets the caps if downstream does not support the GLTextureUploadMeta (in decide_allocation). The output codec state should be reset to something safe before update_src_caps is called. Is GST_VIDEO_FORMAT_ENCODED acceptable ? Or should the get_prefered_caps_feature should look for every supported format NV12, YUV* ? If so the function should be renamed to get_prefered_caps and update_src_caps should not base its format on a previous/current codec state. What do you think ?
Additional questions, is possible to know what kind of format would vaapi would prefer ? Does it handle 422 content ?
(In reply to comment #10) > One question, there is something missing in the current patch when it resets > the caps if downstream does not support the GLTextureUploadMeta (in > decide_allocation). The output codec state should be reset to something safe > before update_src_caps is called. Is GST_VIDEO_FORMAT_ENCODED acceptable ? IIRC, GstVideoDecoder doesn't understand output format == GST_VIDEO_FORMAT_ENCODED. That's why we have this hack to replace "encoded" format with plain "nv12", spread all around in the plugins. > Or should the get_prefered_caps_feature should look for every supported format > NV12, YUV* ? If so the function should be renamed to get_prefered_caps and > update_src_caps should not base its format on a previous/current codec state. > > What do you think ? Every supported format for what purpose? As a way to expose what formats are available should the downstream element want to map/read-back VA surface pixels?
(In reply to comment #11) > Additional questions, is possible to know what kind of format would vaapi would > prefer ? Does it handle 422 content ? Prefer format for what purpose? Decode? No, there is no API for that. We could overcome this limitation, but you would need (i) to wait for the very first frame to be decoded, and (ii) have a VA driver that supports vaDeriveImage(). Read-back formats, i.e. vaGetImage() compatible formats? It should be possible to do some guess work similar to what is done in GstVaapiUploader. I actually have an older GstVaapiDownloader companion, but I never pushed it yet. VA-API can handle 4:2:2 contents, but it all depends on the HW implementation actually. i.e. API: OK, implementation: it depends. :) In practice, we can support JPEG decode in 4:2:2 on Intel hardware. I won't comment for other codecs.
Created attachment 264551 [details] [review] vaapidecode: check if downstream supports GLTextureUploadMeta The patch doesn't modify the output codec state anymore if GLTextureUploadMeta is to be used and only modify the state caps. This allows to keep the codec state format as is and default to it when for example GLTextureUploadMeta has been choosen but the query allocation does not have the proper meta and we need to update the src caps.
I reworked the patch to move gst_vaapi_get_preferred_caps_feature() to a common place (gstvaapipluginutil) and actually fix the "preferred-feature" lookup algorithm. In particular, caps features are enumerated in order sysmem < gltextureupload < vaapi-surface. Then, the best one is chosen according to the srcpad caps. Also made the VASurface caps features addition to the srcpad caps specific to GStreamer >= 1.3.0 at this time since this breaks auto-plugging for me on 1.2.0.
commit a674d9eff2665327fa534c2edbcb2cb8ed71d815 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Wed Dec 11 18:08:26 2013 +0000 vaapidecode: query downstream caps features like GLTextureUploadMeta. Fix vaapidecode to correctly report caps features downstream, when a custom pipeline is built manually. https://bugzilla.gnome.org/show_bug.cgi?id=719372 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit ef9819ecf4cf50bd030129d66d7a02d6bb39f0ae Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue Jan 14 19:08:36 2014 +0100 plugins: add helpers to create video caps with features. Add gst_vaapi_video_format_new_template_caps_with_features() helper function to add the supplied caps feature string on GStreamer >= 1.2. Add gst_vaapi_find_preferred_caps_feature() helper function to discover the "best" caps feature to use for the supplied pad. In practice, we will always favor memory:VASurface first, then meta:GLTextureUploadMeta, and finally the system memory caps. https://bugzilla.gnome.org/show_bug.cgi?id=719372
(In reply to comment #15) > I reworked the patch to move gst_vaapi_get_preferred_caps_feature() to a common > place (gstvaapipluginutil) and actually fix the "preferred-feature" lookup > algorithm. In particular, caps features are enumerated in order sysmem < > gltextureupload < vaapi-surface. Then, the best one is chosen according to the > srcpad caps. First of all thanks for applying the patch. The new logic of the preferred caps feature does not feel right to me since the downstream caps you get is already by order of preference and upstream should take it into account. For example, if GLTextureUpload comes first, vaapidecode should choose GLTextureUpload, not VASurface.
Created attachment 266370 [details] [review] plugins: fix gst_vaapi_find_preferred_caps_feature function logic
In my initial patch, I had used the cleanup label for that, i.e.feature = XXX + goto, thus removing the in-loop gst_caps_replace(). However, then in initial testing, it turned out that the downstream element (glimagesink IIRC) exposed raw YUV first, instead of the GLTextureUpload one. Are the elements supposed to sort their caps by preference first? i.e. is this a guaranteed thing? Thanks.
Besides, since those are capabilities, the downstream element should accept what the upstream elements thinks is best to be used, isn't it? i.e. even if downstream would like the first kind of caps, but still leaves the door open for others, this would mean its judgement could be overriden by upstream.
(In reply to comment #19) > In my initial patch, I had used the cleanup label for that, i.e.feature = XXX + > goto, thus removing the in-loop gst_caps_replace(). However, then in initial > testing, it turned out that the downstream element (glimagesink IIRC) exposed > raw YUV first, instead of the GLTextureUpload one. > > Are the elements supposed to sort their caps by preference first? i.e. is this > a guaranteed thing? Thanks. Yes the elements are supposed to sort their caps by preference order.
(In reply to comment #20) > Besides, since those are capabilities, the downstream element should accept > what the upstream elements thinks is best to be used, isn't it? i.e. even if > downstream would like the first kind of caps, but still leaves the door open > for others, this would mean its judgement could be overriden by upstream. I guess I was wrong on the caps negotiation. After a discussion with Sebastian on irc to clear things up, upstream is supposed to choose what it prefers from what downstream supports.
(In reply to comment #19) > In my initial patch, I had used the cleanup label for that, i.e.feature = XXX + > goto, thus removing the in-loop gst_caps_replace(). However, then in initial > testing, it turned out that the downstream element (glimagesink IIRC) exposed > raw YUV first, instead of the GLTextureUpload one. > > Are the elements supposed to sort their caps by preference first? i.e. is this > a guaranteed thing? Thanks. Upstream decides, downstream is supposed to keep upstream caps first when doing intersection, file a bug if you find an error.