GNOME Bugzilla – Bug 744618
lazy src caps negotiation
Last modified: 2015-02-26 15:06:14 UTC
Currently, the src caps are set inmediatly after the sink caps are set, but in that moment the pipeline might not constructed and the imagesink has not announced its supported caps features, leading to cases where a buffer has a conflict with its caps and its features (e.g. I40 chroma with feature GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META) and the renderer would not know which method use to display it (bug #744039). This patch delays the src caps setting until the first frame arrives to the decoder, assuming that until that very moment the whole pipeline is already negotiated.
Created attachment 296970 [details] [review] lazy src caps negotiation
Hi Few questions: --is it fixing the issue in #744039 ? --did you test against 1.2 and 1.4 with multiple combinations(we have to make sure that it is not causing issue to the autoplugging) 1)playbin should autoplug hw_decoder+hw_sink. 2) playbin should switch to software decoder if hw_decoder is not supporting a pariticular profile of a video (you can find a sample like that in this bug https://bugzilla.gnome.org/show_bug.cgi?id=730997) --make sure that it is not causing issues in other stable clutter-gst releases
I think there are issue, reproducible with gst-launch playbin uri=... video-sink=glimagesink
(In reply to sreerenj from comment #3) > I think there are issue, reproducible with > gst-launch playbin uri=... video-sink=glimagesink If this is glimagesink from master, then I believe this is another issue. For 1.4, I get correct caps negotiated with GLTextureUploadMeta memory. For master, I indeed get failures to negotiate with GL meta, and raw I420 are exposed instead. However, the meta still get installed and working though. I had another patch here: https://github.com/gbeauchesne/gstreamer-vaapi/tree/egl (to be merged when the other client bits are complete & integrated, e.g. EFL) :)
Created attachment 297391 [details] [review] lazy src caps negotiation Currently, the src caps are set inmediatly after the sink caps are set, but in that moment the pipeline might not constructed and the imagesink has not announced its supported caps features, leading to cases where a buffer has a conflict with its caps and its features (e.g. I40 chroma with feature GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META) and the renderer would not know which method use to display it (bug #744039). This patch delays the src caps setting until the first frame arrives to the decoder, assuming that until that very moment the whole pipeline is already negotiated.
(In reply to Víctor Manuel Jáquez Leal from comment #5) > Created attachment 297391 [details] [review] [review] > lazy src caps negotiation V2 updates: 1\ Tested against gst-1.2 2\ Checks if the src pad needs to be reconfigured 3\ Use gst_pad_get_allowed_caps() when finding the preferred feature 4\ Update the src caps if decide_allocation() is called (not sure if this is required) > > Currently, the src caps are set inmediatly after the sink caps are > set, but in that moment the pipeline might not constructed and > the imagesink has not announced its supported caps features, > leading to cases where a buffer has a conflict with its caps > and its features (e.g. I40 chroma with feature > GST_CAPS_FEATURE_META_GST_VIDEO_GL_TEXTURE_UPLOAD_META) and > the renderer would not know which method use to display it > (bug #744039). > > This patch delays the src caps setting until the first frame > arrives to the decoder, assuming that until that very moment the > whole pipeline is already negotiated.
Created attachment 297746 [details] [review] plugins: upload meta only if feature and allocation Working on bug #743687, I realized that vaapidecode always adds to its buffer pool the config option GST_BUFFER_POOL_OPTION_VIDEO_GL_TEXTURE_UPLOAD_META if the decide_allocation()'s query has GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE. Nevertheless, there are occasions where the query has the API type, but the last negotiated caps don't have the feature meta:GstVideoGLTextureUploadMeta. Under this contradiction, vaapidecode adds the GLTextureUploadMeta API to its buffer pool configuration, and adds its buffer's meta to each output buffer, even if the negotiated caps feature is memory:SystemMemory with I420 color format. This kind of output buffers chokes ClutterAutoVideosSink, since it uses a map that relates caps <-> GL upload method. If it receives a buffer with color format I420, it assumes that it doesn't have a texture upload meta, because only those with RGB color format has it. Our buffers, with I420 format, say that they have the upload meta too. In that case the mapped method is a dummy one which does nothing. I reported this issue in bug #744039 (the patch, obviously, was rejected). This patch workarounds the problem: the buffer pool's configuration option GST_BUFFER_POOL_OPTION_VIDEO_GL_TEXTURE_UPLOAD_META is set if and only if the query has the GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE *and* the negotiated caps feature is meta:GstVideoGLTextureUploadMeta. I have tested these patches with gst-master (1.5), gst-1.4 and gst-1.2 and in all they seem to work correctly.
Review of attachment 297746 [details] [review]: That patch is probably a workaround to the issue reported here, but nonetheless this looks like a valid patch itself as is. So, I will likely push it with the EGL bits later on. Thanks.
Hi Sree, Sorry for not answering promptly your comm (In reply to sreerenj from comment #2) > Hi Few questions: > --is it fixing the issue in #744039 ? Both patches does! :) The first patch fixes a problem in gstreamer-vaapi: it should attend the caps renegotiation events from downstream. The second patch actually workarounds a problem in caps negotiation during auto-plugin when playback. > --did you test against 1.2 and 1.4 with multiple combinations(we have to > make sure that it is not causing issue to the autoplugging) > 1)playbin should autoplug hw_decoder+hw_sink. > 2) playbin should switch to software decoder if hw_decoder is not > supporting a pariticular profile of a video (you can find a sample like that > in this bug https://bugzilla.gnome.org/show_bug.cgi?id=730997) With both patches works fine in gst-1.2, but in all my test (using gst-play-1.0) the negotiated caps are I420. Let me check with gst-1.4. > --make sure that it is not causing issues in other stable clutter-gst > releases In gst-1.2 clutter-gst-2 works fine too.
(In reply to Víctor Manuel Jáquez Leal from comment #9) > > --did you test against 1.2 and 1.4 with multiple combinations(we have to > > make sure that it is not causing issue to the autoplugging) > > 1)playbin should autoplug hw_decoder+hw_sink. > > 2) playbin should switch to software decoder if hw_decoder is not > > supporting a pariticular profile of a video (you can find a sample like that > > in this bug https://bugzilla.gnome.org/show_bug.cgi?id=730997) > Both patches work in gst-1.4 too. Most of my tests negotiate correctly meta:GstVideoGLTextureUploadMeta with clutterautovideosink. Two test files fallback to memory:SystemMemory/I420 > > --make sure that it is not causing issues in other stable clutter-gst > > releases > In gst-1.4, clutter-gst3 works fine :)
Created attachment 297812 [details] [review] vaapidecode: delayed src caps negotiation Currently the src caps are set immediately after the sink caps are set, but in that moment the pipeline might not fully constructed and the video sink has not negotiated its supported caps and features. As a consequence, in many cases of playback, the least optimized caps feature is forced. This is partially the responsible of bug #744039. Also, vaapidecode doesn't attend the reconfigure events from downstream, which is a problem too, since the video sink can be changed with different caps features. This patch delays the src caps, setting them until the first frame arrives to the decoder, assuming until that very moment the whole pipeline is already negotiated. Particularly, it checks if the src pad needs to be reconfigured, as a consequence of a reconfiguration event from downstream. A key part of this patch is the new GstVaapiCapsFeature GST_VAAPI_CAPS_FEATURE_NOT_NEGOTIATED, which is returned when the src pad doesn't have a peer yet. Also, for a better report of the caps allowed through the src pad and its peer, this patch uses gst_pad_get_allowed_caps() instead of gst_pad_peer_query_caps() when looking for the preferred feature.
Created attachment 297813 [details] [review] vaapidecode: upload meta only if feature and allocation When vaapidecode finishes the decoding of a frame and pushes it, if, in the decide_allocation() method, it is determined if the next element supports the GL texture upload meta feature, the decoder adds the buffer's meta. Nonetheless, in the same spirit of the commit 71d3ce4d, the determination if the next element supports the GL texture upload meta needs to check both the preferred caps feature *and* if the allocation query request the API type. This patch, first removes the unused variable need_pool, and determines the attribute has_texture_upload_meta using the preferred caps feature *and* the allocation query. Also, the feature passed to GstVaapPluginBase is not longer determined by has_texture_upload_meta, but by the computed preferred one.
Created attachment 297814 [details] [review] vaapidecode: refactor gst_vaapidecode_update_src_caps() This patch improves the readability of the function gst_vaapidecode_update_src_caps() and simplify its logic. But also, the patch validates if the buffer pool has the configuration for the GL texture upload meta, in order to set the meta:GLTextureUpload. If the buffer pools doesn't have it, the I420 format is set back.
How do I obsolete a patch here? How do I mark a patch as committed? :( OK, let me manifest it here: --- lazy src caps negotiation (attachment 297391 [details] [review]) is obsolete. plugins: upload meta only if feature and allocation (attachment 297746 [details] [review]) is already committed ---- ---- vaapidecode: delayed src caps negotiation (attachment 297812 [details] [review]) is waiting for review vaapidecode: upload meta only if feature and allocation (attachment 297813 [details] [review]) is waiting for review vaapidecode: refactor gst_vaapidecode_update_src_caps() (attachment 297814 [details] [review]) is waiting for review --- These three new patches fixes bug 743687 and bug 744039. I've tested them in GStreamer 1.2, GStreamer 1.4 and GStreamer 1.5 (master) and they work well with clutterautovideosink/cluttersink. With vaapisink in master, I found a media[1] that doesn't work, but it does with other sinks, so I guess is a problem with vaapisink. With glimagesink in master, all the media negotiated with meta:GLTextureUpdateMeta fail because some error with OpenGL (a bug to report in glimagesink). In all the GStreamer versions, playbin switches to software decoder if vaapidecode doesn't supporte a pariticular profile. 1. http://samples.mplayerhq.hu/MPEG2/interlaced/
Review of attachment 297746 [details] [review]: Pushed to git master. Thanks.
(In reply to Víctor Manuel Jáquez Leal from comment #14) > How do I obsolete a patch here? How do I mark a patch as committed? :( I indeed had to look around too, the tick boxes changed places and there is another indirection to reach them. :) Basically, in "edit details".
Created attachment 297863 [details] [review] vaapidecode: delayed src caps negotiation Currently the src caps are set immediately after the sink caps are set, but in that moment the pipeline might not fully constructed and the video sink has not negotiated its supported caps and features. As a consequence, in many cases of playback, the least optimized caps feature is forced. This is partially the responsible of bug #744039. Also, vaapidecode doesn't attend the reconfigure events from downstream, which is a problem too, since the video sink can be changed with different caps features. This patch delays the src caps, setting them until the first frame arrives to the decoder, assuming until that very moment the whole pipeline is already negotiated. Particularly, it checks if the src pad needs to be reconfigured, as a consequence of a reconfiguration event from downstream. A key part of this patch is the new GstVaapiCapsFeature GST_VAAPI_CAPS_FEATURE_NOT_NEGOTIATED, which is returned when the src pad doesn't have a peer yet. Also, for a better report of the caps allowed through the src pad and its peer, this patch uses gst_pad_get_allowed_caps() instead of gst_pad_peer_query_caps() when looking for the preferred feature. v3: move the input_state unref to close(), since videodecoder resets at some events such as navigation.
Created attachment 297879 [details] [review] vaapidecode: delayed src caps negotiation Currently the src caps are set immediately after the sink caps are set, but in that moment the pipeline might not fully constructed and the video sink has not negotiated its supported caps and features. As a consequence, in many cases of playback, the least optimized caps feature is forced. This is partially the responsible of bug #744039. Also, vaapidecode doesn't attend the reconfigure events from downstream, which is a problem too, since the video sink can be changed with different caps features. This patch delays the src caps, setting them until the first frame arrives to the decoder, assuming until that very moment the whole pipeline is already negotiated. Particularly, it checks if the src pad needs to be reconfigured, as a consequence of a reconfiguration event from downstream. A key part of this patch is the new GstVaapiCapsFeature GST_VAAPI_CAPS_FEATURE_NOT_NEGOTIATED, which is returned when the src pad doesn't have a peer yet. Also, for a better report of the caps allowed through the src pad and its peer, this patch uses gst_pad_get_allowed_caps() instead of gst_pad_peer_query_caps() when looking for the preferred feature. v3: move the input_state unref to close(), since videodecoder resets at some events such as navigation. v4: a) the state_changed() callback replaces the input_state if the media changed, so this case is also handled. b) since the parameter ref_state in gst_vaapidecode_update_src_caps() is always the input_state, the parameter were removed. c) there were a lot of repeated code handling the input_state, so I refactored it with the function gst_vaapi_decode_input_state_replace().
Created attachment 297880 [details] [review] vaapidecode: upload meta only if feature and allocation When vaapidecode finishes the decoding of a frame and pushes it, if, in the decide_allocation() method, it is determined if the next element supports the GL texture upload meta feature, the decoder adds the buffer's meta. Nonetheless, in the same spirit of the commit 71d3ce4d, the determination if the next element supports the GL texture upload meta needs to check both the preferred caps feature *and* if the allocation query request the API type. This patch, first removes the unused variable need_pool, and determines the attribute has_texture_upload_meta using the preferred caps feature *and* the allocation query. Also, the feature passed to GstVaapPluginBase is not longer determined by has_texture_upload_meta, but by the computed preferred one.
Created attachment 297881 [details] [review] vaapidecode: keep src caps and output state in sync vaapidecode keeps an output state that use the format GST_VIDEO_FORMAT_ENCODED, while it crafts a different src caps for a correct negotiation. I don't see the rational behind this decoupling, it looks like unnecessary complexity. This patch simplify this logic keeping in sync the output state and the src caps. This patch improves the readability of the function gst_vaapidecode_update_src_caps() and simplify its logic. Also, the patch validates if the buffer pool has the configuration for the GL texture upload meta, in order to set the caps feature meta:GLTextureUpload. Otherwise, the I420 format is set back.
With this patchset: - bug 743687 and bug 744039 are fixed. - faster caps negotiation when using playbin. - tested in gstreamer-1.2, gstreamer-1.4 and gstreamer-1.5 (master) - navseek works - sofware-based decoder is switched correctly if vaapidecode fails - works with clutter-based sinks, vaapsink and ximagesink. In the case of glimagesink, when meta:GLTextureUploadMeta is negotitated, it fails because some issue in the GL context.
(In reply to Víctor Manuel Jáquez Leal from comment #21) > With this patchset: > > - bug 743687 and bug 744039 are fixed. > - faster caps negotiation when using playbin. > - tested in gstreamer-1.2, gstreamer-1.4 and gstreamer-1.5 (master) > - navseek works > - sofware-based decoder is switched correctly if vaapidecode fails > - works with clutter-based sinks, vaapsink and ximagesink. In the case of > glimagesink, when meta:GLTextureUploadMeta is negotitated, it fails because > some issue in the GL context. glimagesink definitely works with either GLX or EGL in either 1.4 or 1.5 (master). Please make sure libgstgl is compiled in, i.e. check the logs. With recent Mesa, there is usually a mismatch with GLsync definitions. If the patches cause glimagesink to no longer work, I'd consider that as a regression. :)
(In reply to Gwenole Beauchesne from comment #22) > > If the patches cause glimagesink to no longer work, I'd consider that as a > regression. :) Not quite exactly since now, playbin truly uses meta:GLTextureUploadMeta. Without the patches always fallback to memory:SystemMemory. meta:GLTextureUploadMeta is also used by clutterautovideosink and now goes wonderful, meanwhile without the patches it doesn't work. Nevertheless, I'll narrow the problem in glimagesink.
Review of attachment 297863 [details] [review]: LGTM.. Will push it if no other comments
(In reply to sreerenj from comment #24) > Review of attachment 297863 [details] [review] [review]: > > LGTM.. > Will push it if no other comments Aha wrong click, sorry.
Review of attachment 297879 [details] [review]: LGTM, Will push it if there is no other comments.
Review of attachment 297880 [details] [review]: LGTM, Will push it if there is no other comments.
Review of attachment 297881 [details] [review]: Just for your info: The ENCODED format is basically for handling encrypted formats. There might be cases where we can't figure out the exact format of the surface and only vaapidecode->vaapisink pipeline is supposed to work in that case. Anyway the patch LGTM and it simplifies the code nicely, thanks! Will push it if there is no other comment.
(In reply to Gwenole Beauchesne from comment #22) > If the patches cause glimagesink to no longer work, I'd consider that as a > regression. :) Just filed bug 745206 for glimagesink, now it "just works" :)
Pushed, Lets comeback to this if QA find any regression :) Thanks for the patch. commit 3d8e5e59a72aafc5d06917a3b22006278697a7d7 Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Thu Feb 26 12:28:02 2015 +0200 vaapidecode: keep src caps and output state in sync commit e4f8d14979c4cdf18e5af5594ee02d59f3b3250b Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Thu Feb 26 12:26:54 2015 +0200 vaapidecode: upload meta only if feature and allocation commit 9799875df41c263a2614cae37cb7405a032928db Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Thu Feb 26 12:24:55 2015 +0200 vaapidecode: delayed src caps negotiation