GNOME Bugzilla – Bug 747352
applemedia: texture cache negotiation doesn't work
Last modified: 2015-04-23 22:31:25 UTC
For the following pipeline: avfvideosrc device-index=0 ! video/x-raw(memory:GLMemory),width=1280,height=720,framerate=30/1 ! glimagesink enable-last-sample=0 sync=false the texture cache path doesn't work, i.e. textureCache == NULL and no optimisation takes place. PRELIMINARY ANALYSIS In avfvideosrc, we have the following code in decideAllocations: gst_query_find_allocation_meta (query, GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE, &idx) This meta is only added by _upload_meta_upload_propose_allocation, which is called through: gst_gl_upload_propose_allocation _gst_gl_upload_element_propose_allocation GST_BASE_TRANSFORM (bt)->propose_allocation However, gst_gl_base_filter_query contains: switch (GST_QUERY_TYPE (query)) { case GST_QUERY_ALLOCATION: { if (direction == GST_PAD_SINK && gst_base_transform_is_passthrough (trans)) { _find_local_gl_context (filter); return gst_pad_peer_query (GST_BASE_TRANSFORM_SRC_PAD (trans), query); } break; } Since the direction of the GstUpload "filter" (why is it a filter?) is indeed -> sink, the query is forwarded onward to gstglimagesink, which lacks the special handling.
I've realized it's not glupload's job, so I've been barking up the wrong tree. However, c06715bde6b261d3f93381a0bdb57cab0e35c647 from 2015-02-03 seems interesting: glupload/download/convert: provide transform_caps functions Allows finer grain decisions about formats and features at each stage of the pipeline. Also provide propose_allocation for glupload besed on the supported methods. This commit removed the GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE meta addition from gst_glimage_sink_propose_allocation.
... but this commit added to gst_glimage_sink_propose_allocation: + gst_gl_upload_propose_allocation (glimage_sink->upload, NULL, query); But then 8a0017e21d5f9a8507f0593c6b24f723aa415258 from 2015-02-20 removed it: glimagesink: implement as a bin glupload ! glcolorconvert ! sink Some properties are manually forwarded. The rest are available using GstChildProxy. The two signals are forwarded as well.
I think we should reconsider some things here, as I mentioned in bug #743974 But nonetheless the caps transformation functions should be fixed up. While glimagesink (currently) only seems to support GL memory, GLUpload can convert everything to GL memory... so it shouldn't just proxy the caps query, but also add all the things to the result it sends upstream that it can handle (raw memory, textureuploadmeta, eglimage, ...)
Created attachment 300964 [details] [review] gl: Fix query_allocate for GLMemory Commit 8a0017e21d5f9a8507f0593c6b24f723aa415258 meant for GstGLUploadElement to propose allocation, but as a result of a condition in GstGLBaseFilterClass, it didn't do so. As a side effect, upstream elements couldn't enable their GL texture support. Note: This motivation for this patch is to fix glimagesink.
Created attachment 300965 [details] [review] applemedia: Core Video RGBA textures on iOS We were configuring RGBA textures; no wonder Core Video refused to give us NV12 textures. Luckily for us, RGBA is what we wanted.
Created attachment 300966 [details] [review] glimagesink: Fix drawing on iOS (GLES) I'm not an OpenGL expert, just following the example from: https://developer.apple.com/library/ios/samplecode/RosyWriter/Listings/Classes_RosyWriterOpenGLRenderer_m.html See also: http://stackoverflow.com/questions/21501510/trouble-rendering-camera-feed-as-opengl-texture-ios NOTE: Xcode's OpenGL ES Analysis instrument shows further issues: 1 x Uninitialized Texture Data 3 x Redundant Uniform Location Query 3 x Recommend Using Element Array Buffers 3 x Redundant Call
Created attachment 300967 [details] [review] glimagesink: Don't keep next_buffer after drawing SMALL PATCH for simplifying our already-confusing buffer refcount: next_buffer should be alive between prepare and show_frame. For keeping a stationary frame, we have aptly named named stored_buffer.
Created attachment 300968 [details] [review] avfvideosrc: Fix risky typo LAST BUT NOT LEAST: - GST_BUFFER_OFFSET_END (*buf) = GST_BUFFER_OFFSET (buf) + 1; + GST_BUFFER_OFFSET_END (*buf) = GST_BUFFER_OFFSET (*buf) + 1; How did this ever work?!
Created attachment 300969 [details] [review] applemedia: Core Video NV12 textures I couldn't test the NV12 patch since glimagesink doesn't accept NV12 over GLMemory. Any idea how I can test it?
Created attachment 300970 [details] [review] applemedia: Core Video RGBA textures on iOS Whoops! Should output GL_BGRA, so that we'll draw GL_RGBA... Otherwise, blue and red were swapped. Whatevah.
BEFORE I FORGET: With my patches, gst_glimage_sink_propose_allocation is only called for non-GLMemory cases. If so, should the following 2 lines be in it? if (glimage_sink->context->gl_vtable->FenceSync) gst_query_add_allocation_meta (query, GST_GL_SYNC_META_API_TYPE, 0);
Good work :) I just looked over everything shortly, will take a longer look tomorrow probably. Just some comments (In reply to Ilya Konstantinov from comment #9) > Created attachment 300969 [details] [review] [review] > applemedia: Core Video NV12 textures > > I couldn't test the NV12 patch since glimagesink doesn't accept NV12 over > GLMemory. Any idea how I can test it? glupload or glcolorconvert handle NV12 (unless broken) (In reply to Ilya Konstantinov from comment #8) > Created attachment 300968 [details] [review] [review] > avfvideosrc: Fix risky typo > > LAST BUT NOT LEAST: > > - GST_BUFFER_OFFSET_END (*buf) = GST_BUFFER_OFFSET (buf) + 1; > + GST_BUFFER_OFFSET_END (*buf) = GST_BUFFER_OFFSET (*buf) + 1; > > How did this ever work?! It was just reading some random value from the stack there :) Good that nothing really uses the offset/offset-end here ;) (In reply to Ilya Konstantinov from comment #7) > Created attachment 300967 [details] [review] [review] > glimagesink: Don't keep next_buffer after drawing > > SMALL PATCH for simplifying our already-confusing buffer refcount: > > next_buffer should be alive between prepare and show_frame. > For keeping a stationary frame, we have aptly named named stored_buffer. The reason for this is that we want to keep around the buffer until we draw the next one. IIRC it's not guaranteed that after glTexImage2D() or similar everything is done, and we can't safely unref/destroy/reuse the memory yet. (In reply to Ilya Konstantinov from comment #6) > NOTE: Xcode's OpenGL ES Analysis instrument shows further issues: > > 1 x Uninitialized Texture Data > 3 x Redundant Uniform Location Query > 3 x Recommend Using Element Array Buffers > 3 x Redundant Call Does it also tell you where? (In reply to Ilya Konstantinov from comment #11) > BEFORE I FORGET: > > With my patches, gst_glimage_sink_propose_allocation is only called for > non-GLMemory cases. If so, should the following 2 lines be in it? > > if (glimage_sink->context->gl_vtable->FenceSync) > gst_query_add_allocation_meta (query, GST_GL_SYNC_META_API_TYPE, 0); I think so. Someone should really review all that negotiation code, it seems a bit half-refactored :)
(In reply to Sebastian Dröge (slomo) from comment #12) > > I couldn't test the NV12 patch since glimagesink doesn't accept NV12 over > > GLMemory. Any idea how I can test it? > > glupload or glcolorconvert handle NV12 (unless broken) static GstCaps * gst_glimage_sink_get_caps (GstBaseSink * bsink, GstCaps * filter) { GstCaps *tmp = NULL; ... tmp = gst_caps_from_string ("video/x-raw(memory:GLMemory),format=RGBA"); Hmm. Could this be related? > (In reply to Ilya Konstantinov from comment #7) > > Created attachment 300967 [details] [review] [review] [review] > > glimagesink: Don't keep next_buffer after drawing > > > > SMALL PATCH for simplifying our already-confusing buffer refcount: > > > > next_buffer should be alive between prepare and show_frame. > > For keeping a stationary frame, we have aptly named named stored_buffer. > > The reason for this is that we want to keep around the buffer until we draw > the next one. IIRC it's not guaranteed that after glTexImage2D() or similar > everything is done, and we can't safely unref/destroy/reuse the memory yet. Yes, we still keep it -- it's called "stored_buffer". It's what we draw, and what we'll keep drawing until we get another buffer. I thought "next_buffer" is supposed to be only alive between prepare and show_frame. Before this patch, we ref the same buffer twice -- in 1) stored_buffer, and 2) next_buffer, and this is just confusing. > (In reply to Ilya Konstantinov from comment #6) > > NOTE: Xcode's OpenGL ES Analysis instrument shows further issues: > > ... > Does it also tell you where? It pinpoints the exact GL calls, so you could grep and figure it out. I don't think it pinpoints lines of code, but I'll snoop some more. > > if (glimage_sink->context->gl_vtable->FenceSync) > > gst_query_add_allocation_meta (query, GST_GL_SYNC_META_API_TYPE, 0); > > I think so. Someone should really review all that negotiation code, it seems > a bit half-refactored :) I think Matthew Waters can answer best.
Review of attachment 300967 [details] [review]: This will cause tearing. We have no control on exactly when the texture will actually be used.
(In reply to Nicolas Dufresne (stormer) from comment #14) > Review of attachment 300967 [details] [review] [review]: > > This will cause tearing. We have no control on exactly when the texture will > actually be used. The texture that's used in the drawing routine is glimage_sink->stored_buffer. I'm merely nullifying next_buffer earlier (and unrefing once), not changing any behavior.
(In reply to Ilya Konstantinov from comment #15) > (In reply to Nicolas Dufresne (stormer) from comment #14) > > Review of attachment 300967 [details] [review] [review] [review]: > > > > This will cause tearing. We have no control on exactly when the texture will > > actually be used. > > The texture that's used in the drawing routine is > glimage_sink->stored_buffer. I'm merely nullifying next_buffer earlier (and > unrefing once), not changing any behavior. Removing the ref will cause flickering and possible reading invalid textures. An extensive reason as to why it's needed is available in 5b8d7a443ead269d034401a6788f2d47f8fa4bc5 glupload: provide the output buffer that is rendered into.
(In reply to Matthew Waters from comment #16) > Removing the ref will cause flickering and possible reading invalid > textures. An extensive reason as to why it's needed is available in > 5b8d7a443ead269d034401a6788f2d47f8fa4bc5 glupload: provide the output buffer > that is rendered into. OK, I think I can see the problem. Sorry for the trouble -- this patch doesn't change much anyway, so I'm retracting this part.
For the original problem, it is not going to be guaranteed that the upload meta is present when you configure with the GLMemory caps features. Especially if we selectively enable/advertise meta's/allocators based on the caps features (which I have planned). avfvideosrc needs to check based on the GLMemory caps features if it needs to use GLMmeory or not rather than on the existence of an unrelated meta that will not be used.
Review of attachment 300964 [details] [review]: I don't think this is needed if you fix avfvideosrc to not detect GL downstream with the upload meta.
Review of attachment 300966 [details] [review]: This should be taken care of at texture creation time. Overriding the sampling parameters of the texture is not friendly to other texture types which may have different constraints. I'm also not sure if its necessary for the textures CoreVideo provides.
(In reply to Matthew Waters from comment #18) > For the original problem, it is not going to be guaranteed that the upload > meta is present when you configure with the GLMemory caps features. > Especially if we selectively enable/advertise meta's/allocators based on the > caps features (which I have planned). The one who broke compatibility should've done it. They could've grepped the code for GST_VIDEO_GL_TEXTURE_UPLOAD_META_API_TYPE to see what effect this removal had, at the very least on elements in the same repo. (In reply to Matthew Waters from comment #19) > I don't think this is needed if you fix avfvideosrc to not detect GL > downstream with the upload meta. Right now nothing calls the propose_allocation of gluploadelement, the propose_allocation that you and others have painstakingly written. (In reply to Matthew Waters from comment #20) > I'm also not sure if its necessary for the textures CoreVideo provides. Right now, there's black screen on iOS without it, and good smooth video with it. You're welcome to find another solution, but I probably wouldn't be putting more time into it.
> (In reply to Matthew Waters from comment #19) > > I don't think this is needed if you fix avfvideosrc to not detect GL > > downstream with the upload meta. > > Right now nothing calls the propose_allocation of gluploadelement, the > propose_allocation that you and others have painstakingly written. Which is correct as glupload is not doing anything as it's in passthrough mode. > (In reply to Matthew Waters from comment #20) > > I'm also not sure if its necessary for the textures CoreVideo provides. > > Right now, there's black screen on iOS without it, and good smooth video > with it. You're welcome to find another solution, but I probably wouldn't be > putting more time into it. i.e. don't do glTexParameters in glimagesink but when you create/wrap the texture from CoreVideo.
Alessandro, CC'ing you cause you're the last person to touch that. I've attached patches for RGBA texture export. In your code you've had NV12 textures hardcoded even in RGBA mode, which is a bug. I proposed a fix, but I'll let more experienced people than me take over this now.
(In reply to Matthew Waters from comment #18) > avfvideosrc needs to check based on the GLMemory caps features if it needs > to use GLMmeory or not rather than on the existence of an unrelated meta > that will not be used. So I assume something like this? https://github.com/ikonst/gst-plugins-bad/commit/953e89763af12ee9ced80eabdd9145cd7be5a636
Nicolas, I've looked again at Matthew's code and mine, and tried to analyse the ref-unrefs: MATTHEW'S CODE AFTER 5b8d7a443ead269d034401a6788f2d47f8fa4bc5 prepare: next = B1 stored = 0 B1+ show-frame-pre-redisplay: next = B1 stored = B1 B1++ show-frame-post-redisplay: next = B1 stored = B1 B1++ prepare: next = B2 stored = B1 B1+ B2+ show-frame-pre-redisplay: next = B2 stored = B2 B1+ B2++ show-frame-post-redisplay: next = B1 stored = B1 B1 <- unref'd B2++ etc. ILYA'S PROPOSED CHANGE: prepare: next = B1 stored = 0 B1+ show-frame-pre-redisplay: next = B1 stored = B1 B1++ [redisplay] show-frame-post-redisplay: next = 0 <-- DIFFERENCE stored = B1 B1+ <-- DIFFERENCE prepare: next = B2 stored = B1 B1+ B2+ show-frame-pre-redisplay: next = B2 stored = B2 B1+ B2++ [redisplay] show-frame-post-redisplay: next = 0 <-- DIFFERENCE stored = B1 B1 <- unref'd B2+
(In reply to Ilya Konstantinov from comment #23) > Alessandro, CC'ing you cause you're the last person to touch that. Hi, sorry it took me a while to reply. > I've attached patches for RGBA texture export. In your code you've had NV12 > textures hardcoded even in RGBA mode, which is a bug. > > I proposed a fix, but I'll let more experienced people than me take over > this now. It's actually not a bug. When you negotiate RGBA with AVF it internally produces NV12 and then converts to RGBA. The conversion is slower than ours according to my profiling, so we get NV12 and then convert to RGBA ourselves with GstGLColorConvert.
Alessandro's commit edf9035d02c947ed9c836ddafa01fa34681918b4 seems to fix this.