GNOME Bugzilla – Bug 756686
fix caps negotiation for meta:GstVideoGLTextureUploadMeta
Last modified: 2015-10-20 10:42:12 UTC
From bug 754680 > Jan Schmidt [developer] 2015-10-01 11:07:08 UTC > > I'm not sure if GstVideoGLTextureUploadMeta is not just more generally broken. > I can't get GstVideoGLTextureUploadMeta even with glx using git master. > > filesrc location=test-h264.mov ! qtdemux ! vaapidecode ! 'video/x-raw(meta:GstVideoGLTextureUploadMeta)' ! glimagesink > > yields not-negotiated. > > It seems that gst_vaapidecode_update_src_caps() relies on > decode->has_texture_upload_meta == TRUE, which is only set > gst_vaapidecode_decide_allocation. gst_vaapidecode_decide_allocation() doesn't > happen until after the decoder has already tried to set the wrong caps and > gotten not-negotiated. > > I am sure this worked last time I tried, so it's a regression since sometime in > July. > > Dolevo 2015-10-07 11:40:51 UTC > > I have also tried above pipeline that Jan provided and it doesn't work. format > is selected as I420 in glimagesinkbin and it goes over CPU memory. Can anybody > check that?
Created attachment 313449 [details] [review] vaapidecode: set format before decide allocation There is a regression from commit 3d8e5e. It was expected the buffer pool allocation occur before the caps negotiation, but it is not. This patch fixes this regression: the caps negotiation is done regardless the allocation query from downstream. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313450 [details] [review] vaapidecode: use caps to check the features Instead of calling gst_vaapi_find_preferred_caps_feature(), which is expensive, we check the caps from the allocation query, to check the negotiated feature. In order to do this verification a new utility function has been implemented: gst_vaapi_caps_has_feature() Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313451 [details] [review] vaapidecode: decide allocation doesn't update srccaps The received caps query will bring the already negotiated caps, so they are not expected to change. This patch removes this verification which is dead code path. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313480 [details] [review] vaapidecode: set format before decide allocation There is a regression from commit 3d8e5e. It was expected the buffer pool allocation occur before the caps negotiation, but it is not. This patch fixes this regression: the caps negotiation is done regardless the allocation query from downstream. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313481 [details] [review] vaapidecode: use caps to check the features Instead of calling gst_vaapi_find_preferred_caps_feature(), which is expensive, we check the caps from the allocation query, to check the negotiated feature. In order to do this verification a new utility function has been implemented: gst_vaapi_caps_has_feature() Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313482 [details] [review] vaapidecode: decide allocation doesn't update srccaps The received caps query will bring the already negotiated caps, so they are not expected to change. This patch removes this verification which is dead code path. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313483 [details] [review] vaapidecode: unguard memory:VASurface capsfeature Caps features are supported since GStreamer 1.2, and gstreamer-vaapi supports GStreamer since that version. So, first the guards where wrong and nowadays there is no need to guard it. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313484 [details] [review] vaapidecode: set format before decide allocation There is a regression from commit 3d8e5e. It was expected the buffer pool allocation occur before the caps negotiation, but it is not. This patch fixes this regression: the caps negotiation is done regardless the allocation query from downstream. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313485 [details] [review] vaapidecode: use caps to check the features Instead of calling gst_vaapi_find_preferred_caps_feature(), which is expensive, we check the caps from the allocation query, to check the negotiated feature. In order to do this verification a new utility function has been implemented: gst_vaapi_caps_feature_contains() Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313486 [details] [review] vaapidecode: decide allocation doesn't update srccaps The received caps query will bring the already negotiated caps, so they are not expected to change. This patch removes this verification which is dead code path. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313487 [details] [review] vaapidecode: unguard memory:VASurface capsfeature Caps features are supported since GStreamer 1.2, and gstreamer-vaapi supports GStreamer since that version. So, first the guards where wrong and nowadays there is no need to guard it. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Review of attachment 313484 [details] [review]: lgtm
Review of attachment 313485 [details] [review]: lgtm, But honestly I am not 100% sure whether the current implementation has been done like this to fix some corner cases... Anyway as log as you can test with glimagesink/cluttersink without any issue, I am fine with this patch :) BTW, IMHO, We should add a comment in source file itself for each corner case that we fix (not just in commit message).Otherwise we will forget about this later,,This reminds me to add some comment block for 1a3e884...
Review of attachment 313486 [details] [review]: lgtm
Review of attachment 313487 [details] [review]: lgtm
(In reply to sreerenj from comment #13) > Review of attachment 313485 [details] [review] [review]: > > lgtm, But honestly I am not 100% sure whether the current implementation has > been done like this to fix some corner cases... > Anyway as log as you can test with glimagesink/cluttersink without any > issue, I am fine with this patch :) > > BTW, IMHO, We should add a comment in source file itself for each corner > case that we fix (not just in commit message). I mean, for each corner cases that fix in future :) >Otherwise we will forget > about this later,,This reminds me to add some comment block for 1a3e884...
Created attachment 313670 [details] [review] vaapidecode: use caps to check the features Instead of calling gst_vaapi_find_preferred_caps_feature(), which is expensive, we check the caps from the allocation query, to check the negotiated feature. In order to do this verification a new utility function has been implemented: gst_vaapi_caps_feature_contains(). As this new function shared its logic with gst_caps_has_vaapi_surface(), both have been refactorized. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 313730 [details] [review] vaapidecode: relax guards for memory:VASurface capsfeature Though caps features are supported since GStreamer 1.2, there are some issues with the features caps negotiation in that version. Nonetheless, those issues are fixed in GStreamer 1.4. So, the memoy:VASurface caps feature negotiation is relaxed for GStreamer 1.4. The guard is the same as in vaapisink's caps template. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
After some testing, now the patches look correct to me. I'll push them now.
Attachment 313484 [details] pushed as 45487fe - vaapidecode: set format before decide allocation Attachment 313486 [details] pushed as 361f55b - vaapidecode: decide allocation doesn't update srccaps Attachment 313670 [details] pushed as 6d9f31e - vaapidecode: use caps to check the features Attachment 313730 [details] pushed as b76f482 - vaapidecode: relax guards for memory:VASurface capsfeature