GNOME Bugzilla – Bug 704321
Add complete support for GstContext
Last modified: 2013-12-30 10:12:43 UTC
See summary, currently working on that
Do you mean replacing all the GST_QUERY_CUSTOM/gst_structure_has_name (structure, "gstgldisplay") by the use of GST_QUERY_CONTEXT ? If yes, nice!
Yes, however that's a bit more complicated and related to the other bugs. Via GstContext only the GstGLDisplay should be exchanged. Independent of that the GstGLContext should be exchanged via the ALLOCATION query (because that's what it is about). (Also not really working on it anymore because larger refactoring was necessary and I don't have time for that right now, Matthew works on the refactoring though)
Created attachment 257936 [details] [review] use GstContext for GstGLDisplay propogation
Created attachment 257937 [details] [review] use GstContext for GstGLDisplay propogation This one includes gltestsrc and glmixer changes as well
Created attachment 258004 [details] [review] upload: clear our mutex
Created attachment 258005 [details] [review] filter: small code cleanup
Created attachment 258006 [details] [review] display: remove get,set_context
Created attachment 258131 [details] [review] gltestsrc: use context_get_gl_api
Created attachment 258132 [details] [review] mixer: don't unref the query in the default case
Review of attachment 257937 [details] [review]: ::: gst-libs/gst/gl/gstgldisplay.c @@ +156,2 @@ s = gst_context_writable_structure (context); gst_structure_set (s, GST_GL_DISPLAY_CONTEXT_TYPE, GST_TYPE_GL_DISPLAY, You might want to use "display" or a shorter string here. The long complex type string is used as the name of the context now. ::: gst-libs/gst/gl/gstgldisplay.h @@ +77,3 @@ gboolean gst_context_get_gl_display (GstContext * context, GstGLDisplay ** display); +gboolean gst_gl_display_replace (GstGLDisplay **oldobje, GstGLDisplay *newobj); We don't do such things usually, just cast in the code that uses it to GstObject** and GstObject* ::: gst-libs/gst/gl/gstglfilter.c @@ +219,3 @@ + gst_structure_has_name (structure, "gstglcontext")) { + gst_structure_set (structure, "gstglcontext", G_TYPE_POINTER, + filter->context, NULL); This custom query is not needed anymore? @@ +289,3 @@ + + structure = gst_structure_new_empty ("gstglcontext"); + context_query = gst_query_new_custom (GST_QUERY_CUSTOM, structure); This should be done with the context query ::: gst-libs/gst/gl/gstglmixer.c @@ +379,3 @@ + if (gst_structure_has_name (structure, "gstglcontext")) { + gst_structure_set (structure, "gstglcontext", G_TYPE_POINTER, + mix->context, NULL); Not needed anymore @@ +952,3 @@ + + structure = gst_structure_new_empty ("gstglcontext"); + context_query = gst_query_new_custom (GST_QUERY_CUSTOM, structure); Use context query ::: gst-libs/gst/gl/gstglutils.c @@ +474,3 @@ + } + + if (ctxt) { If the context is set as result of the need-context message, nothing will put it into ctxt and this function won't return the display ::: gst/gl/gstglimagesink.c @@ +408,3 @@ + if (gst_structure_has_name (structure, "gstglcontext")) { + gst_structure_set (structure, "gstglcontext", G_TYPE_POINTER, + glimage_sink->context, NULL); Not needed anymore ::: gst/gl/gstgltestsrc.c @@ +552,3 @@ + + structure = gst_structure_new_empty ("gstglcontext"); + context_query = gst_query_new_custom (GST_QUERY_CUSTOM, structure); Use context query
Comment on attachment 258005 [details] [review] filter: small code cleanup Maybe squash that into the main patch
(In reply to comment #10) > Review of attachment 257937 [details] [review]: > > ::: gst-libs/gst/gl/gstgldisplay.c > @@ +156,2 @@ > s = gst_context_writable_structure (context); > gst_structure_set (s, GST_GL_DISPLAY_CONTEXT_TYPE, GST_TYPE_GL_DISPLAY, > > You might want to use "display" or a shorter string here. The long complex type > string is used as the name of the context now. Ok > ::: gst-libs/gst/gl/gstgldisplay.h > @@ +77,3 @@ > gboolean gst_context_get_gl_display (GstContext * context, GstGLDisplay ** > display); > > +gboolean gst_gl_display_replace (GstGLDisplay **oldobje, GstGLDisplay > *newobj); > > We don't do such things usually, just cast in the code that uses it to > GstObject** and GstObject* Ok > ::: gst-libs/gst/gl/gstglfilter.c > @@ +219,3 @@ > + gst_structure_has_name (structure, "gstglcontext")) { > + gst_structure_set (structure, "gstglcontext", G_TYPE_POINTER, > + filter->context, NULL); > > This custom query is not needed anymore? Soon :). That's to be moved to the ALLOCATION query. Still needed currently because GstGLDisplay does not hold onto the GstGLContext. > @@ +289,3 @@ > + > + structure = gst_structure_new_empty ("gstglcontext"); > + context_query = gst_query_new_custom (GST_QUERY_CUSTOM, structure); > > This should be done with the context query See above > ::: gst-libs/gst/gl/gstglmixer.c > @@ +379,3 @@ > + if (gst_structure_has_name (structure, "gstglcontext")) { > + gst_structure_set (structure, "gstglcontext", G_TYPE_POINTER, > + mix->context, NULL); > > Not needed anymore > > @@ +952,3 @@ > + > + structure = gst_structure_new_empty ("gstglcontext"); > + context_query = gst_query_new_custom (GST_QUERY_CUSTOM, structure); > > Use context query > > ::: gst-libs/gst/gl/gstglutils.c > @@ +474,3 @@ > + } > + > + if (ctxt) { > > If the context is set as result of the need-context message, nothing will put > it into ctxt and this function won't return the display Hmm, this will probably require intervention from the specific element now that gst_element_get_context has disappeared. Just to make sure this is the correct order of proceedings for the need-context message. 1. Element posts need-context with a type 2. Some bin/pipeline containing the element can respond if it already has that context 3. Else the application is given a chance (via the sync bus) to respond 4. If not successful create our own and propogate via have-context The only way that I can find that the app/bin/pipeline can respond with is via gst_element_set_context (which we currently do not implement). However, we do not really care that ctxt has been set or not, only that *display_ptr has been set. Which, assuming that the element updates the pointer display_ptr points to in a gst_element_set_context, everything's fine. > ::: gst/gl/gstglimagesink.c > @@ +408,3 @@ > + if (gst_structure_has_name (structure, "gstglcontext")) { > + gst_structure_set (structure, "gstglcontext", G_TYPE_POINTER, > + glimage_sink->context, NULL); > > Not needed anymore > > ::: gst/gl/gstgltestsrc.c > @@ +552,3 @@ > + > + structure = gst_structure_new_empty ("gstglcontext"); > + context_query = gst_query_new_custom (GST_QUERY_CUSTOM, structure); > > Use context query P.S we don't handle the context query in elements correctly.
(In reply to comment #12) > > This custom query is not needed anymore? > > Soon :). That's to be moved to the ALLOCATION query. Still needed currently > because GstGLDisplay does not hold onto the GstGLContext. OK :) > > If the context is set as result of the need-context message, nothing will put > > it into ctxt and this function won't return the display > > Hmm, this will probably require intervention from the specific element now that > gst_element_get_context has disappeared. Just to make sure this is the correct > order of proceedings for the need-context message. > > 1. Element posts need-context with a type > 2. Some bin/pipeline containing the element can respond if it already has that > context > 3. Else the application is given a chance (via the sync bus) to respond > 4. If not successful create our own and propogate via have-context > > The only way that I can find that the app/bin/pipeline can respond with is via > gst_element_set_context (which we currently do not implement). > > However, we do not really care that ctxt has been set or not, only that > *display_ptr has been set. Which, assuming that the element updates the > pointer display_ptr points to in a gst_element_set_context, everything's fine. Yes that's correct, GstElement::set_context() needs to be implemented by the element (also handling of the CONTEXT query).
Created attachment 259520 [details] [review] use GstContext for GstGLDisplay propogation This addresses the previous issues
So after looking at this in more depth, GstGLDisplay will probably need to be morphed into a display connection abstract class (similar to GstVaapiDisplay) in order to integrate with other plugins/apis not knowing about gst-gl and/or OpenGL. Come to think of it, some defined types such as x11 display (handle or name), wayland display (handle or name) with opening and closing of the display connection would be nice.
(In reply to comment #15) > So after looking at this in more depth, GstGLDisplay will probably need to be > morphed into a display connection abstract class (similar to GstVaapiDisplay) > in order to integrate with other plugins/apis not knowing about gst-gl and/or > OpenGL. Come to think of it, some defined types such as x11 display (handle or > name), wayland display (handle or name) with opening and closing of the display > connection would be nice. Like the EGLDisplay stuff in gst-plugins-bad/gst-libs/gst/egl? Yes, indeed. I think we'll need that sooner or later. Otherwise, yes. GstGLDisplay should become exactly that in the end :)
Review of attachment 259520 [details] [review]: Almost ready to be merged :) Should we do the ALLOCATION query instead of custom query stuff in another bug or as part of this one here? ::: gst-libs/gst/gl/gstglutils.c @@ +474,3 @@ + } + + if (ctxt) { In case it was set from a message, you need to check *display_ptr again instead of ctxt
(In reply to comment #17) > Review of attachment 259520 [details] [review]: > > Almost ready to be merged :) > > Should we do the ALLOCATION query instead of custom query stuff in another bug > or as part of this one here? I don't really mind. I'll see if I can quickly whip something up for this. > ::: gst-libs/gst/gl/gstglutils.c > @@ +474,3 @@ > + } > + > + if (ctxt) { > > In case it was set from a message, you need to check *display_ptr again instead > of ctxt Which is currently performed by the calling function, gst_gl_ensure_display(). I wrote a small program to set the GstGLDisplay on the need-context message that works :)
Created attachment 259661 [details] [review] use the allocation query to propogate GstGLContext This uses the GstVideoGLTextureUploadMeta api type but doesn't fully implement the meta properly. I have wip patches for that as well. I suppose the question is whether it should stay on the upload meta or should we create our own GstGLMeta.
(In reply to comment #18) > > ::: gst-libs/gst/gl/gstglutils.c > > @@ +474,3 @@ > > + } > > + > > + if (ctxt) { > > > > In case it was set from a message, you need to check *display_ptr again instead > > of ctxt > > Which is currently performed by the calling function, gst_gl_ensure_display(). > > I wrote a small program to set the GstGLDisplay on the need-context message > that works :) So that's all done from the ::set_context() code instead of doing it there? A bit confusing, could you write a comment in the code and provide a new patch? :)
Review of attachment 259661 [details] [review]: I think it should become a GstGLMeta instead. But this patch looks good to merge, after that we can remove the custom queries already?
I'll merge all these patches once the remaining once got the comment btw
Created attachment 259713 [details] [review] use GstContext for GstGLDisplay propogation With comment
Created attachment 259714 [details] [review] use the allocation query to propogate GstGLContext This removes the custom query as well.
Created attachment 259715 [details] [review] mixer: unmap the video frames we map
Created attachment 259716 [details] [review] mixer: unmap the video frames we map Patches should compile :)
Created attachment 259779 [details] [review] fail in context query if display is NULL
Created attachment 260071 [details] [review] update .gitignore
Created attachment 260072 [details] [review] Bump gstreamer requirement to 1.2 Branch https://github.com/ystreet/gst-plugins-gl/tree/upload_meta
Review of attachment 259713 [details] [review]: Generally looks good :) ::: gst-libs/gst/gl/gstgldisplay.c @@ +128,3 @@ + g_return_val_if_fail (context != NULL, FALSE); + + GST_DEBUG_CATEGORY_GET (gst_context, "GST_CONTEXT"); Do this only once in a central place ::: gst-libs/gst/gl/gstglutils.c @@ +435,3 @@ + + if (!GST_CAT_CONTEXT) + GST_DEBUG_CATEGORY_GET (GST_CAT_CONTEXT, "GST_CONTEXT"); Not necessarily threadsafe, do it once in a central place
Review of attachment 259714 [details] [review]: Those comments apply to all files you changed, didn't repeat them for each ::: gst-libs/gst/gl/gstglfilter.c @@ +805,3 @@ + GST_GL_TYPE_CONTEXT, filter->context, NULL); + gst_query_add_allocation_meta (query, + GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE, gl_context); I think we should not abuse the GstVideoGLTextureUploadMeta for that but add our own meta @@ +869,3 @@ + if (gst_query_find_allocation_meta (query, + GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE, &idx)) { + GstContext *context; It's a GstGLContext, not a GstContext :) ::: gst/gl/gstglimagesink.c @@ +836,3 @@ gboolean need_pool; + GError *error = NULL; + const GstStructure *gl_context; Remove the const @@ +902,3 @@ + gst_query_add_allocation_meta (query, + GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE, gl_context); + gst_structure_free ((GstStructure *) gl_context); And here remove the cast
Created attachment 264957 [details] [review] use GstContext for GstGLDisplay propogation v2
Created attachment 264958 [details] [review] use GstContext for GstGLDisplay propogation Also, fail in the context query response if display is NULL
Created attachment 264959 [details] [review] use the allocation query to propogate GstGLContext Addresses all but the GstGLMeta issue. I agree that we should have it :)
All pushed