GNOME Bugzilla – Bug 755072
vaapi: expose memory:DMABuf capsfeature
Last modified: 2017-07-24 10:14:17 UTC
If and once memory:dmabuf caps fetaure is defined in gst-plugins-base and these bugs fixes: #745459, #743345, vaapi should also use this feature. I went ahead and add this support here: https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_caps_feature So far I could do: GST_GL_PLATFORM=egl GST_GL_API=gles2 GST_GL_WINDOW=x11 GST_DEBUG=2 LIBVA_DRIVER_NAME=gallium gst-launch-1.0 filesrc location=video.mp4 ! qtdemux ! h264parse ! vaapidecode ! "video/x-raw(memory:VASurface), format=NV12" ! vaapipostproc ! "video/x-raw(memory:dmabuf), format=RGBA" ! glimagesink Though the video is still now showing correctly because the gallium vaapi backend I use is a work in progress.
Just for note, the work in progress in mesa/gallium vaapi backend is there: https://github.com/CapOM/mesa/commits/wip_export_import_and_vpp The 4/5 first patch are to enable vaapi with "nouveau" driver. Already submitted upstream and are under review. But there are some pending issues. The last patch (that I need to split) adds missing bit to enable vaapipostproc and also to allow exporting a vasurface to a dmabuf and to import a dmabuf into as a vasurface.
Thanks Julien!
As it was still showing crap with nouveau driver. I tried on a intel card but it shows black frames. Without any gl errors or vaapi errors. Tough inteldmabufupload work with glimagesink. So it is something related to vaapi-intel-driver and gstreamer-vaapi. But honestly everything looks fine, the surfaces are processed and correctly exported. Current status is that: videotestsrc ! "video/x-raw, format=NV12" ! vaapipostproc ! "video/x-raw(memory:dmabuf), format=RGBA" ! glimagesink is showing crap with nouveau driver. And shows black frames with intel card. (Though you would need to modify the gstreamer-vaapi patches I have on github a bit to add RGBx in the caps. And also this patch for vaapi-intel-driver) Any idea how I could turn on some kernel traces/logging related to drm kernel module driver and dmabuf ? Thx
(patch for vaapi-intel-driver I mentioned in comment #3 : https://bugs.freedesktop.org/show_bug.cgi?id=92088)
(In reply to Julien Isorce from comment #3) > > Any idea how I could turn on some kernel traces/logging related to drm > kernel module driver and dmabuf ? Thx May be you can ask about this in libva mailing list,,,Graphics kernel guys are there :)
(In reply to sreerenj from comment #5) > (In reply to Julien Isorce from comment #3) > May be you can ask about this in libva mailing list,,,Graphics kernel guys > are there :) All right I'll do that, in the mean time I found: cat /sys/class/drm/card0/error sudo cat /sys/kernel/debug/dma_buf/bufinfo (launch it while the gst pipeline is running to print info on the dmabufs involved, including size) echo 1 | sudo tee /sys/module/drm/parameters/debug (to enable drm logs in /var/log/kern.log) echo 1 | sudo tee /sys/class/drm/card0/device/driver/module/parameters/debug (to enable drm logs in /var/log/kern.log)
vaapidecode ! "video/x-raw(memory:VASurface), format=NV12" ! vaapipostproc ! "video/x-raw(memory:dmabuf), format=RGBA" ! glimagesink is now working. The problem was that postproc was creating an extra surface and replacing it in the output buffer :) The fix: https://github.com/CapOM/gstreamer-vaapi/commit/4d66db13e36d4d42d112f40182b290eca38a2504
So I suggest the following plan: First, have glimagesink supporting dmabuf caps feature + EXT_image_dma_buf_import, in upstream gstreamer. It is on going work by Lubosz see #743345 . Once it is merged lets review my gstreamer-vaapi branch for dmabuf. Though if you have already any remark do not hesitate to notify me. We need to do it this way because it is also still under discussion how to negotiate dmabuf in gstreamer in term of caps and allocation queries.
(In reply to Julien Isorce from comment #8) > So I suggest the following plan: Works for me!
mmmhh.. it seems that gstreamer "went forward without the memory:DMABuf feature" We need to figure out how to handle the dmabuf negotiation without the feature.
https://bugzilla.gnome.org/show_bug.cgi?id=759358(In reply to Víctor Manuel Jáquez Leal from comment #10) > mmmhh.. it seems that gstreamer "went forward without the memory:DMABuf > feature" > > We need to figure out how to handle the dmabuf negotiation without the > feature. Yes because we need further investigation. For now it is assumed to be mappable It is also assumed that the importer will succeed (no sure if it falls back to software). So basically in the logic I drafted here https://bugzilla.gnome.org/show_bug.cgi?id=759358#c7 it does not go further than first par of 1). But it is still a big step forward :) So for now, gstreamer-vaapi could do 1) without the feature part. (Any in any case it cannot be done by checking the allocation query, see comments "Only offer our custom allocator if that type of memory was negotiated." in GstGL)
Maybe we need to step back a bit. What do we need to negotiate ? As of now, only glupload implements DMABuf in a usable way. What happens is that glupload instrospect the incoming buffer and select method to "upload". If a active method fails, it fallback to another one. Nothing in this scenario is "negotiated", glupload simply react to the type of buffer that comes in. Often the importation of dmabuf will fail for various reason, but in GL, there is no way to check (hence negotiate) those before trying. This means, for the glupload path to work, an upstream element need to produce dmabuf (a bit like vaapi) and if thise dmabuf are not compatible, it will use a slow path. That's also why, memory:DMABuf was of no use there. I would like to know if VAAPI can achieve the very same thing. For upstream element, like v4l2src, my opinion is that it should always produce DMABuf when supported, end of the story. There was a bug with report, with patches, but the patches were broken, and was later not updated. I'll see if I can make this happen soon.
> I would like to know if VAAPI can achieve the very same thing. For upstream > element, like v4l2src, my opinion is that it should always produce DMABuf > when supported, end of the story. There was a bug with report, with patches, > but the patches were broken, and was later not updated. I'll see if I can > make this happen soon. In the case of the elements that receives dmabufs (encoders and vaapisink) It looks doable. In the case of the elements that might push dmabufs (decoder and postproc), it looks harder, since we don't have a way to figure out what would be "best". The short-circuit is to enable the io-mode property, which, I guess, we don't want.
For the other way aruund: ... ! vaapidecode ! glupload This seems to be a case where you would maybe need to make a decision about what is best. And the answer is probably quite arbitrary, why would GLUploadMeta be slower or faster then DMABuf ? This is an example of case where I would not know. Correct me if I'm wrong, here's what I believe the negotiation should be (I'm ignoring the VideoMeta aspect): Query downstream pool (allocation query) If downstream buffers from that pool can be zero-copy imported ? Use this method, (downstream pool could be giving DMABuf, or pool could be of a known type) Else, you have two choices a) Push VASurface b) Push DMABuf If I remember, VASurface are already negotiated using caps. So choice a) can be decided already. Normally the choice shall be evident, unless you have non-mappable DMABuf, which is a very unfortunate type of DMABuf imho. That being said, maybe memory:DMABuf is only needed when downstream can import DMABuf and upstream produce non-mappable DMABuf ? Is that an actual use cases introduced by VAAPI ? DMABuf in V4L2 are always mappable (at least for now, there is discussion to introduce a security mechanism, so permission to map could be controlled in a way one process (like your compositor) could be allowed to map the data, while another would not (for DRM purpose apparently). Does that seems like a plausible solution and a good description of what memory:DMABuf would be used for ? (announce that downstream can import DMABuf)
(In reply to Nicolas Dufresne (stormer) from comment #14) > maybe memory:DMABuf is only needed when downstream can import > DMABuf and upstream produce non-mappable DMABuf ?(for DRM purpose > apparently). > * fail to map: This case part of 1- in https://bugzilla.gnome.org/show_bug.cgi?id=759358#c7 "If producer fails to map then it adds the caps feature" Indeed that could be the case for secure dmabuf, i.e. when using "smaf_set_secure" see: https://git.linaro.org/people/benjamin.gaignard/libsmaf.git/blob/HEAD:/lib/libsmaf.h#l35 . Which could be used in conjunction to a TEE (Trusted Execution Environment (Also see https://www.phoronix.com/scan.php?page=news_item&px=SMAF-v5-DMA-BUF-SPD and https://wiki.linaro.org/WorkingGroups/Security/OP-TEE) In gstreamer-vaapi one would need to add VA_SURFACE_EXTBUF_DESC_PROTECTED when creating the surface that is going to be exported as dmabuf. This would tell the vaapi backend to call "smaf_set_secure". * choice between pushing buffers with meta:GstVideoGLTextureUploadMeta and pushing dmabuf: In gst_vaapi_plugin_base_decide_allocation, if has_texture_upload_meta is true (i.e. downstream supports meta:GstVideoGLTextureUploadMeta) then you would decide to go for dmabuf if the gstglcontext is of type GST_GL_CONTEXT_TYPE_EGL and gst_gl_check_extension ("EGL_EXT_image_dma_buf_import", ctx_egl->egl_exts) returns true; (then try to map is through vaMapBuffer, if it fails add DMABuf caps feature is supported by downstream) In short, if egl then prefer dmabuf over meta. The reason is because the later requires to keep a handle on the downstrean GstGLContext which is more error prone (see gst_vaapi_display_egl_set_gl_context (GstGLContext...); ). Other reason is that dmabuf allows to be transfered to another process (decoding in on process and rendering in another process like it is done in ChromiumGStreamerBackend). That would make pushing dmabuf only a very particular case but as Nicolas said: "As of now, only glupload implements DMABuf in a usable way". Also between 2 vaapi gst elements I would always do memory:VASurface so no dmabuf at all. Actually I can try to add that logic in my branch https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_caps_feature which currently does not supports to pushing dmabuf whithout caps feature.
Done here: https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_with_and_without_caps_feature Last-1 patch should just be split in 2: dmabuf without then with feature. You would need patch for gst-plugins-base: https://bug759358.bugzilla-attachments.gnome.org/attachment.cgi?id=317223 and for gstglupload: https://github.com/CapOM/gst-plugins-bad/commit/f274539bc47f300624dee52d304f65cafcd459bb Nicolas: mapping of dmabuf created by exporting a VASurface fails on desktop with: "gst_fd_mem_map: 0x7f39880051c0: fd 26: mmap failed: Permission denied". See l195 here https://github.com/CapOM/gstreamer-vaapi/commit/ffceae0c0cd099967f6bc3b1a27451151550e70a#diff-bec9172c05be682d331c790d8a50ba95R195. I did not expected it but it was actually good to test switch case where map fail and then switch to feature. See regression test list in that branch. The most important thing is to agree on how the following pipelines behave: https://github.com/CapOM/gstreamer-vaapi/commit/c450b7c7693b1e3417c96dbbc906c394416a6852. I am planning to convert them to some automatic tests. (gst-integration-testsuites ? gst-devtools/validate ?)
(In reply to Julien Isorce from comment #16) > You would need patch for gst-plugins-base: > https://bug759358.bugzilla-attachments.gnome.org/attachment.cgi?id=317223 > and for gstglupload: > https://github.com/CapOM/gst-plugins-bad/commit/ > f274539bc47f300624dee52d304f65cafcd459bb Assuming there will be documentation in the final version, that patch looks fine to me. Make sure to keep it separate. > > Nicolas: mapping of dmabuf created by exporting a VASurface fails on desktop > with: "gst_fd_mem_map: 0x7f39880051c0: fd 26: mmap failed: Permission > denied". See l195 here > https://github.com/CapOM/gstreamer-vaapi/commit/ > ffceae0c0cd099967f6bc3b1a27451151550e70a#diff- > bec9172c05be682d331c790d8a50ba95R195. > > I did not expected it but it was actually good to test switch case where map > fail and then switch to feature. See regression test list in that branch. That answers a question I asked a long time ago. I was never sure if the Intel driver would let us map or not the DMAbuf. Note, it could be laziness in the driver (as it's often the case for GFX, they implement the strict minimum). It could also be a very complex process, I don't know. My next question is, can we trust the format ? It's important, as this is for sharing between drivers. The DMAbuf objects are just a memory buffers, everything else need to be communicated to the other driver by userspace. > > The most important thing is to agree on how the following pipelines behave: > https://github.com/CapOM/gstreamer-vaapi/commit/ > c450b7c7693b1e3417c96dbbc906c394416a6852. I am planning to convert them to > some automatic tests. (gst-integration-testsuites ? gst-devtools/validate ?) I'd say create a DMABuf/Intel validation suite (for gst-validate). One that we can run on such HW. I don't know what is available within our test both at the moment, they are all virtual machines. We could start creating regression for VAAPI too, and maybe we can Approach Intel to get a dedicated HW sponsored.
I rebased my gstreamer-vaapi branch: https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_with_and_without_caps_feature_26april_2016 Done here: https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_with_and_without_caps_feature Again you would need patch for gst-plugins-base: https://bug759358.bugzilla-attachments.gnome.org/attachment.cgi?id=317223 and for gstglupload: https://github.com/CapOM/gst-plugins-bad/commit/f274539bc47f300624dee52d304f65cafcd459bb Victor, your recent changes in gstreamer-vaapi actually simplified my branch since some part were common like the fact that gst_vaapi_find_preferred_caps_feature no takes "allowed_caps" in parameter. So that was great news. Also I will need your help to go further. That would be great if you could go through the patches and let me know what you think at first glance. Feel free to review on github directly. Then we can setup a moment to improve the patches together since you know that code much better than me. Thx Nicolas you might be interested by this patch https://github.com/CapOM/gstreamer-vaapi/commit/603a62efdbe62b9db90f75e43a58dd280edfa1b1 . Thx about the gst-validate suggestion, I had a look but that would require to develop some custom plugins according to Thibault recommendations. Sofor now the regression tests will remain manual: https://github.com/CapOM/gstreamer-vaapi/commit/305d13dd521463b64f90cf46f7ccdbd7efb08c49 Also I do not have the permission denied with mesa gallium vaapi backend, so it succeeds to map. The fact that it fails to map with intel backend is actually good for testing all cases :), but I agree there is maybe a way to make it not fail.
Arf, "Done here: https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_with_and_without_caps_feature" should not have appear in previous comment, the last branch is really: https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_with_and_without_caps_feature_26april_2016 :)
commit 8e7ebaa505a4a6554549d47c454d17a2eb31269a Author: Julien Isorce <j.isorce@samsung.com> Date: Fri Nov 27 05:09:10 2015 +0000 gstvaapisurface: explicitely clear TILING flag if dmabuf https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit 1dbcc8a0e199f2da6a0ab8e949f13341916128a3 Author: Julien Isorce <j.isorce@samsung.com> Date: Sun Oct 4 23:44:16 2015 +0100 gstvaapisurface_drm: release image when done Otherwise intel-vaapi-driver will fail to process the exported surface because it will find it is currently derived, so considered as busy. https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit 910bacc55ccadf2ea4fa7ecf5c3e53c9eee7db3b Author: Julien Isorce <j.isorce@samsung.com> Date: Sat Sep 26 06:25:12 2015 +0100 vaapipostproc: already have a surface proxy if dmabuf https://bugzilla.gnome.org/show_bug.cgi?id=755072
commit 240fc0f726f8be5da29554332d0e50a78d4be238 Author: Julien Isorce <j.isorce@samsung.com> Date: Mon Sep 28 08:49:39 2015 +0100 vaapibufferpool: do not create texture upload meta if dmabuf https://bugzilla.gnome.org/show_bug.cgi?id=755072
What is the status here?, waiting for gst-plugins-base/bad patches to land first?
Hi, I need to rebase https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_with_and_without_caps_feature_26april_2016 against latest gstreamer-vaapi. Last time I tried their it was failing some of my regressions tests after rebase. But basically now this branch just contain https://github.com/CapOM/gstreamer-vaapi/commit/2a55a5fc774eb8bba68094ba231f06b8b07b3969 . I need to split it to help with review too. Once it is ready, then I would have to push the 2 patches gst-plugins-base/bad first. Also I understood that Victor has even more changes since the hackfest so I should maybe wait for it first.
(In reply to Julien Isorce from comment #23) > Once it is ready, then I would have to push the 2 patches > gst-plugins-base/bad first. Also I understood that Victor has even more > changes since the hackfest so I should maybe wait for it first. And my patches have changed the way the vaapivideobufferpool gets its allocator. It doesn't create them anymore using custom flags, it receive it through the bufferpool configuration. Thus, the element (gstvaapipluginbase), is the one which instanciates the allocator to use. Also, I have found out that our dmabuf handling doesn't support multiplanar color formats. It needs some API changes :/
Just as a side note, both import and export options of v4l2src is working with vaapi elements now. "v4l2src io-mode=dmabuf-import ! vaapipostrpoc" was the only working pipeline when I was testing last time. Now the "v4l2src io-mode=dmabuf " is also testable.
(In reply to sreerenj from comment #25) > "v4l2src io-mode=dmabuf-import ! vaapipostrpoc" was the only working > pipeline when I was testing last time. Now the "v4l2src io-mode=dmabuf " is > also testable. In my setup (HSW) only io-mode=dmabuf works. With dmabuf-import this error is shown: 0:00:03.311454792 9392 0x2494e30 ERROR v4l2allocator gstv4l2allocator.c:735:gst_v4l2_allocator_start:<v4l2src0:pool:src:allocator> error requesting 2 buffers: Invalid argument 0:00:03.311484207 9392 0x2494e30 ERROR v4l2bufferpool gstv4l2bufferpool.c:848:gst_v4l2_buffer_pool_start:<v4l2src0:pool:src> we received 0 buffer from device '/dev/video0', we want at least 2 0:00:03.311494158 9392 0x2494e30 ERROR bufferpool gstbufferpool.c:531:gboolean gst_buffer_pool_set_active(GstBufferPool *, gboolean):<v4l2src0:pool:src> start failed And this looks to me an error in v4l2, not vaapi.
Have you disabled libv4l2 ? It prevents using dmabuf-import and userptr. If already disable, can you track down the kernel trace that explain what the invalid argument is ?
(In reply to Víctor Manuel Jáquez Leal from comment #26) > (In reply to sreerenj from comment #25) > > > "v4l2src io-mode=dmabuf-import ! vaapipostrpoc" was the only working > > pipeline when I was testing last time. Now the "v4l2src io-mode=dmabuf " is > > also testable. > > In my setup (HSW) only io-mode=dmabuf works. With dmabuf-import this error > is shown: > > 0:00:03.311454792 9392 0x2494e30 ERROR v4l2allocator > gstv4l2allocator.c:735:gst_v4l2_allocator_start:<v4l2src0:pool:src: > allocator> error requesting 2 buffers: Invalid argument > 0:00:03.311484207 9392 0x2494e30 ERROR v4l2bufferpool > gstv4l2bufferpool.c:848:gst_v4l2_buffer_pool_start:<v4l2src0:pool:src> we > received 0 buffer from device '/dev/video0', we want at least 2 > 0:00:03.311494158 9392 0x2494e30 ERROR bufferpool > gstbufferpool.c:531:gboolean gst_buffer_pool_set_active(GstBufferPool *, > gboolean):<v4l2src0:pool:src> start failed > > And this looks to me an error in v4l2, not vaapi. See the commit 82fc406dfda4d477396c1396d0cf2d03b4417d6a
hehehe, --withou-libv4l2, that's the trick. Thanks!
(In reply to Víctor Manuel Jáquez Leal from comment #29) > hehehe, --withou-libv4l2, that's the trick. Thanks! This library need a serious bug-fix session to not be in the way, it also break CREATE_BUFS when supported. But that's another subject ;-P
Julien, I have pushed a branch in my repository with the dmabuf downstream support: https://github.com/ceyusa/gstreamer-vaapi/tree/755072 (branch name is 755072) Basically are the patches posted in bug 765435, plus a couple more which sets the dmabuf allocator if downstream supports EGL_EXT_image_dma_buf_import. Right now to test is is running a pipeline similar to this one: GST_GL_PLATFORM=egl gst-play-1.0 sample.mkv --videosink="capsfilter caps=\"video/x-raw, format=YUY2\" ! glimagesink" I need a patch to check the dowstream support of EGL_EXT_image_dma_buf_import before the caps negotiation, so system memory caps will have priority over GLTextureUpload Please note that, at least in intel platforms, the format needs to be one which can be a vaDeriveImage from the VASurface, but also needs to be a format with one plane, since the dmabuf support in vaapi onle delivers one fd. This is why I force YUY2 or I420. RGBA won't work :(
I have pushed a couple commits, now the dmabuf is negotiated automatically. It needs, in order to work without capsfilter, the merge of this patch: https://lists.freedesktop.org/archives/libva/2016-June/004014.html
Great ! I guess you are now just missing the try map. If you cannot do the try map in decide_allocation then do it much earlier by trying to create a vasurface for a fixed size, exporting it and map it for a fixed set of formats: {YUY2, I420, RGBx, BGRx, RGBA, BGRA}. It will give you a set of valid formats to filter caps negotiation or to adjust can_try_dmabuf flag. Better would be to try map the real vasurface but in the branch I did (see early comments) it was painful to implement (the part to fallback to meta) and hard to maintain for long term. (At least it was the case before your several great refactoring) Later I can add GST_VIDEO_CAPS_MAKE_WITH_FEATURES(GST_CAPS_FEATURE_MEMORY_DMABUF, "{ YUY2, I420, RGBx, BGRx, RGBA, BGRA }") and the try map routine will only keep formats that failed to map.
The problem I'm facing now is a fd exhaustion. Though the buffers looks to be freed, the buffer pool is not reusing them.
Hi Victor, are we only missing this now: https://bugzilla.gnome.org/show_bug.cgi?id=765600 ? I saw you implemented gst_vaapi_dmabuf_can_map in https://github.com/ceyusa/gstreamer-vaapi/commits/755072 and it seems some patches form this branch are already merged. Also I updated: https://github.com/CapOM/gstreamer-vaapi/tree/dmabuf_caps_feature_only_30june The new caps feature is in wrong place but it is just for testing. Your last refactoring are great ! (those who move allocator creation from pool to propose and decide)
Hi Victor, I just rebased your branch https://github.com/ceyusa/gstreamer-vaapi/commits/755072 and it works fine with both intel and gallium(nouveau) vaapi drivers. I mean the exported VA as dmabuf is mappable. Kind of good news because it did not make any sense that it failed before.
(In reply to Julien Isorce from comment #36) > Hi Victor, I just rebased your branch > https://github.com/ceyusa/gstreamer-vaapi/commits/755072 and it works fine > with both intel and gallium(nouveau) vaapi drivers. I mean the exported VA > as dmabuf is mappable. Kind of good news because it did not make any sense > that it failed before. That's good news!! But what's the magic there? Did you hit the fd exhaustion isse?
Finally I came back to this issue. Sorry for the delay. I started to look at the fd (file descriptor) leaking when pushing to downstream dmabuf-based buffers. It is because they are not released because of commit 1dbcc8a0 (gstvaapisurface_drm: release image when done) destroys the created derived image, thus, its fd is not marked as "allocated" by va-intel-driver, thus vaReleaseBufferHandler() returns error and the fd is never really released. This means currently there is fd leakage when the vaapipostproc or vaapisink connects to a v4l2src with io-mode dmabuf-import. IMO, we should revert that patch ASAP. What I realize now is current vaapi dmabuf allocator is designed for upstream use case, because it: 1\ creates a surface 2\ creates a derived image from the surface 3\ gets the fd of the image with vaAcquireBufferHandler 6\ The dmabuf-base buffer is used upstream by v4l2src 7\ As the buffer is associated with a vaapi buffer meta, the surface can be read (and read only, since no operation is allowed when Acquired) 8\ The vaReleaseBufferHandler is called with the buffer pool unrefs the allocated buffers But in the case of pushing dmabuf-based buffers to downstream, the logic is different: we cannot acquire the buffer before writing on its surface, only when all the processing is done and it is going to be pushed downstream. 1\ Process the normal VA Surface until is done (using vaapi surface allocator) 2\ call vaAcquireBufferHandler() on that Surface and get the fd 3\ Create a dmabuf-base gstbuffer and assing the fd 4\ Add a GstParentBufferMeta relation between the vasurface-based buffer with dmabuf-based buffer and push the dmabuf-based buffer
Thx Victor it makes sense to me and sorry if you got some regressions, I had not tested this patch with v4l2sc, only the downstream case. I think the fact that you identified that it requires a deign change to handle downstream case is great. Previously I was only trying to kind of workaround it. In your detailed downstream sequence (1, 2, 3, 4) for me it means that you will export every-time a buffer is pushed (instead of exporting once for all at pool configuration), so the same surface will be exported with a different fd and downstream element will not be able to do caching. What I am missing ? Is 4) to call vaReleaseBufferHandler at the proper time ? Thx!
(In reply to Julien Isorce from comment #39) > In your detailed downstream sequence (1, 2, 3, 4) for me it means that you > will export every-time a buffer is pushed (instead of exporting once for all > at pool configuration), so the same surface will be exported with a > different fd and downstream element will not be able to do caching. What I > am missing ? My first approach will be to assume that the surface's derive image will always export the same fd, and to cache the dmabuf-based exported buffer as a vaapi buffer qdata. > Is 4) to call vaReleaseBufferHandler at the proper time ? That's why we need to cache the dmabuf-based buffer as a vaapi buffer qdata, to release it before processing it.
I have pushed to ceyusa/755072 my new branch for dmabuf as downstream buffers with the algorithm explained above. It only works for glimgaesink with EGL GST_GL_PLATFORM=egl gst-play-1.0 sample.mkv --videosink=glimagesink 1\ There is a commit that disables vaapidecodebin because dmabuf is only negotiated by vaapidecode, but no by vaapipostproc. 2\ Only my Honey 4K media played correctly (with ~7% of cpu usage). The rest my lower resolution samples are rendered incorrectly, but still don't know why.
3\ If I force to get mapped (ximagesink, for example) I still get this error 0:00:04.023598272 9074 0x7ff2bc0aa280 ERROR fdmemory gstfdmemory.c:114:gpointer gst_fd_mem_map(GstMemory *, gsize, GstMapFlags): 0x7ff2c415f920: fd 31: mmap failed: Invalid argument 0:00:04.023605094 9074 0x7ff2bc0aa280 ERROR GST_MEMORY gstmemory.c:324:gboolean gst_memory_map(GstMemory *, GstMapInfo *, GstMapFlags): mem 0x7ff2c415f920: subclass map failed 0:00:04.023610860 9074 0x7ff2bc0aa280 ERROR default video-frame.c:169:gboolean gst_video_frame_map_id(GstVideoFrame *, GstVideoInfo *, GstBuffer *, gint, GstMapFlags): failed to map buffer
(In reply to Víctor Manuel Jáquez Leal from comment #42) > 3\ If I force to get mapped (ximagesink, for example) I still get this error > > 0:00:04.023598272 9074 0x7ff2bc0aa280 ERROR fdmemory > gstfdmemory.c:114:gpointer gst_fd_mem_map(GstMemory *, gsize, GstMapFlags): > 0x7ff2c415f920: fd 31: mmap failed: Invalid argument > 0:00:04.023605094 9074 0x7ff2bc0aa280 ERROR GST_MEMORY > gstmemory.c:324:gboolean gst_memory_map(GstMemory *, GstMapInfo *, > GstMapFlags): mem 0x7ff2c415f920: subclass map failed > 0:00:04.023610860 9074 0x7ff2bc0aa280 ERROR default > video-frame.c:169:gboolean gst_video_frame_map_id(GstVideoFrame *, > GstVideoInfo *, GstBuffer *, gint, GstMapFlags): failed to map buffer Err... wrong... It doesn't work with ximagesink, but it does with xvimagesink!!! But still, the color format is not managed correctly because I only see color stripes.
Hi Victor, with very recent intel va driver I tried your 755072 branch and I got similar results as: - #41 (only one of the 2 planes is renderer correctly) - #42 (though I get permission denied here for READWRITE, but it succeeds for READ. I am doing the try gst_memory_map just after creating it in gstvaapipluginbase). Same result when rebasing your branch with latest upstream gstreamer-vaapi.
Hi Julien, I haven't checked for a while the DMAbuf, since I decided to wait for 1.11 to merge it. Are you coming for the gstconf? We can work on it there.
For the rendering problem: I was surprised to see that it uses the same dmabuf for the 2 distinct EGLImage, so it could have been an issue. Having a second thought I guess it should not matter and should just work. So probably a bug in GstGL (or in mesa). I will have a closer look. But I am also concerned about the fact that GstGL fails to cache the EGLImages because it uses the GstMemory pointer as a key for the qdata based cache, not the dma buf number. Indeed with your new approach, gstvaapipluginbase creates a new GstMemory each time. 1: We could either make the GstGL_cache uses dma buffer numbers as key instead of the GstMemory pointer. 2: We could cache these GstMemories somewhere in gstreamer-vaapi, for example as a qdata of GstVaapiSurface. Or re-using the member "extbuf_proxy" of GstVaapiSurface, plus having that GstMemory a member of GstVaapiBufferProxy. So that from the surface we can get always the same proxy and from the proxy always the same GstMemory. With option 1 we still do not avoid the "ioctl" call that exports the surface as a dmabuf (even if it returns the same value each time). So I will experiment option 2 I think.
(In reply to Julien Isorce from comment #46) > For the rendering problem: I was surprised to see that it uses the same > dmabuf for the 2 distinct EGLImage, so it could have been an issue. Having a > second thought I guess it should not matter and should just work. So > probably a bug in GstGL (or in mesa). I will have a closer look. This is intentional in GstGL. Drivers must support DMABuf with offsets. > > But I am also concerned about the fact that GstGL fails to cache the > EGLImages because it uses the GstMemory pointer as a key for the qdata based > cache, not the dma buf number. Indeed with your new approach, > gstvaapipluginbase creates a new GstMemory each time. > > 1: We could either make the GstGL_cache uses dma buffer numbers as key > instead of the GstMemory pointer. The problem is that the cache would keep growing even if the dmabuf are released. Worst, the FD might get reused and then the cache becomes incoherent. > > 2: We could cache these GstMemories somewhere in gstreamer-vaapi, for > example as a qdata of GstVaapiSurface. Or re-using the member "extbuf_proxy" > of GstVaapiSurface, plus having that GstMemory a member of > GstVaapiBufferProxy. So that from the surface we can get always the same > proxy and from the proxy always the same GstMemory. > > With option 1 we still do not avoid the "ioctl" call that exports the > surface as a dmabuf (even if it returns the same value each time). > So I will experiment option 2 I think. ++
I implemented options 2, i.e. re-use GstVaapiSurface->extbuf_proxy to cache GstVaapiBufferProxy which now owns the GstMemory(FD). And seems to work fine, I can see that in GstGL it creates only 16 EGLImages (2 per frames and because 8 dmabuf because 8 vaapisurface on a 720p video) but then that is all, it is cached and properly re-used in GstGL. https://github.com/CapOM/gstreamer-vaapi/commit/7364ea196030ac56b5aaf75a0b8a485e611390a5 (branch "ceyusa_755072_plus_caching") I had to add the 3 new following API: GstMemory* gst_vaapi_buffer_proxy_get_memory (GstVaapiBufferProxy * proxy, gsize size); void gst_vaapi_surface_set_buffer_proxy (GstVaapiSurface * surface, GstVaapiBufferProxy * proxy); GstVaapiBufferProxy * gst_vaapi_surface_peek_buffer_proxy (GstVaapiSurface * surface); Let's discuss about it and then I can re-work it.
About the rendering problem, I could workaround it for a 720p video with the following hack for GstGL: attribs[atti++] = offset ? (offset - 16*720) : 0; So it seems that the vaapi-intel-driver adds 16 lines padding for the first plane (or offset the second plane by 16 additional lines, depending on how you interpret it). I tried to play with FLAG_LINEAR_STORAGE & FLAG_FIXED_STRIDES & FLAG_FIXED_OFFSETS, see https://github.com/CapOM/gstreamer-vaapi/commit/7364ea196030ac56b5aaf75a0b8a485e611390a5#diff-369e0f9df85ddc32a12842788712646fR142 but without any success. But with that hack above it renders properly. At this point I think it is a problem in vaapi-intel-driver (maybe not considering VASurfaceAttribExternalBuffers.offsets[N] properly) but not sure at all.
Sorry, should read "offset + 16*1280" above.
The other day I also found that tiling is not disabled by default anymore for the decoded VASurface: https://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c#n1245 Because since recently gstreamer-vaapi is now defaulting to NV12, and it is a good thing, but then I think in that link above one should add "expected_fourcc == VA_FOURCC_NV12" too.
Hey Julien, I just realized that my local branch for this bug is slightly updated than the one in my repository. I have updated it now. It has a missing commit that updates the metas in the exported buffer, with important data for VideoMeta and cropping. That fixes several use cases.
Thx Victor, I just tried and it does not change the result for videos I use for testing: http://www.h264info.com/clips.html Note that my comment #51 is wrong, I asked on libva mailing list. For #49, #50, the heuristic I have for now for the offset is: attribs[atti++] = offset ? (offset + w * (4096 / (w & ~(w - 1)))) : 0; This is the only blocking problem to go further at the moment. Could be also a bug in my hardware (Haswell). See you tomorrow :)
Hi Victor, I found that VAImage.data_size/offsets are not retrieved at all in gstreamer-vaapi. If I log them it prints the right values I was estimating in previous comments. So we just need to adjust the GstVideoMeta with these values. I will have a go.
Done here https://github.com/CapOM/gstreamer-vaapi/commits/ceyusa_755072_plus_caching_and_offsets with commit "WIP: dmabuf: retrieve and pass VAImage.offsets to meta"
I reworked it to re-use the existing allocator and pool: https://github.com/CapOM/gstreamer-vaapi/commits/ceyusa_755072_dmabuf_allocator_vpp_decode Victor I popped out the 4 last commit of you branch https://github.com/ceyusa/gstreamer-vaapi/commits/755072 . Then I added 16 commits on top of your branch. The 10 first commits are to add dmabuf for gstvaapipluginbase and vpp. The 6 last commits are to add dmabuf for the decoder. vpp: GST_GL_XINITTHREADS=1 GST_GL_API=gles2 GST_GL_PLATFORM=egl GST_GL_WINDOW=x11 gst-launch-1.0 filesrc location=serenity.mp4 ! qtdemux ! h264parse ! vaapih264dec ! vaapipostproc ! glimagesink decoder: GST_GL_XINITTHREADS=1 GST_GL_API=gles2 GST_GL_PLATFORM=egl GST_GL_WINDOW=x11 gst-launch-1.0 filesrc location=serenity.mp4 ! qtdemux ! h264parse ! vaapih264dec ! glimagesink
Cool! I've glanced the branch and looks nice. I even cherry-picked one commit and pushed it to master already ;) The thing is that I have as urgent bug 753591, and AFAIU, I'll have to modify heavily the gstvaapi allocator, so perhaps the rebase, of either branch, will be painful. We'll see.
(In reply to Víctor Manuel Jáquez Leal from comment #57) > Cool! > > I've glanced the branch and looks nice. I even cherry-picked one commit and > pushed it to master already ;) > > The thing is that I have as urgent bug 753591, and AFAIU, I'll have to > modify heavily the gstvaapi allocator, so perhaps the rebase, of either > branch, will be painful. We'll see. No worries. This can wait.
I found why we get EACCES "Permission denied" when trying to use mmap on the dmabuf fd of the exported vasurface. (gst_memory_map (gstfdmem, READWRITE)) It is because the ioctl call that export the surface has params read only. Similar as if it was doing open(..., "r"). The following hack allow the mmap RW to succeed: In git://anongit.freedesktop.org/mesa/drm: diff --git a/xf86drm.c b/xf86drm.c index 9cfca49..3dfecf1 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -2737,7 +2737,7 @@ int drmPrimeHandleToFD(int fd, uint32_t handle, uint32_t flags, int *prime_fd) memclear(args); args.fd = -1; args.handle = handle; - args.flags = flags; + args.flags = flags | DRM_RDWR; ret = drmIoctl(fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &args); if (ret) return ret; But in both intel va driver and gallium va driver, this function is not called directly but it calls another libdrm function that only set the flag DRM_CLOEXEC. So readonly by default. Also in vaAcquireBufferHandle's param there is no way to configure readonly or readwrite. So I think there is no intention to allow the user to have write access, only mmap READ is intended to work. But anyway I did had a go with the above hack and I could have the following pipeline working: vaapih264dec ! gamma gamma=5 ! glimagesink since gamma element is working in-place. Also be aware that with mmap readwrite on a dmabuf we are supposed to do: mmap struct drm_prime_handle args; args.flags = DMA_BUF_SYNC_START; ioctl(dma_buf_fd, DMA_BUF_IOCTL_SYNC, &args then you can change the data/pixels struct drm_prime_handle args; args.flags = DMA_BUF_SYNC_END; ioctl(dma_buf_fd, DMA_BUF_IOCTL_SYNC, &args munmap So I did added that in GstFdMemory::map/unmap but I could not noticed any different since I guess the pipeline is already taking case of the synchronization. Funny enough, I then talked to Tiago Vignatti and they are doing the same hack for ChromeOS, see https://chromium.googlesource.com/chromiumos/platform/minigbm/+/b96ffe1983295492bc4b4f28b22d6f0e78af69ef%5E!/#F0
(In reply to Víctor Manuel Jáquez Leal from comment #57) > The thing is that I have as urgent bug 753591, and AFAIU, I'll have to > modify heavily the gstvaapi allocator We could create a GstVaapiDmabufAllocator the same way it is done for GstIONAllocator: https://bug768794.bugzilla-attachments.gnome.org/attachment.cgi?id=333309
> We could create a GstVaapiDmabufAllocator the same way it is done for > GstIONAllocator: > https://bug768794.bugzilla-attachments.gnome.org/attachment.cgi?id=333309 It seems that @ndufresne will merge a patch (bug 768794) to expose GstDmaBufAllocator as subclassable. That will simplify a lot the work. But it's going to happen after freeze.
FYI clutter-gst is also adding support for dmabuf: bug 773453
About #59, instead of using gst_memory_map I realized that I should try to use fstat instead, to check the mode of the dmabuf fd. But it is always marked as writable (st_mode has S_IWUSR flag) for both case: mmap fail, mmap succeeds. So I confirm that we cannot use fstat.
I have pushed these commits commit b0016e336bba268e4eb9968ff7847932711e07db Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Sun Oct 16 01:04:09 2016 +0100 vaapidecode: don't GLTextureUpload if dmabuf Do not add the meta:GstVideoGLTextureUploadMeta feature if the render element can handle dmabuf-based buffers, avoiding its negotiation. commit 25e8309567167321b1a596235f1114bc4a9da960 Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 16:21:21 2016 +0100 vaapidecode: make pool to export decoder's surface Use new -base API gst_video_decoder_allocate_output_frame_full() to pass the current proxy/surface to the pool. The pool will will export thins given surface instead of exporting a brand new surface that will never be filled in with meaningfull data. https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit 50242eaaf7a14abfa4f62308a808b9d7e7950b1d Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Fri Feb 3 17:06:29 2017 +0100 plugins: decoder can negotiate dmabuf downstream commit 9ed73e76afba6b88ca50b0b8c94cfecf6995f35e Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 16:07:07 2016 +0100 vaapivideobufferpool: override acquire_buffer() Overriding the vmethod acquire_buffer() it is possible to attach the right GstMemory to the current acquired buffer. As a matter of fact, this acquired buffer may contain any instantiated GstFdmemory, since this buffer have been popped out from the buffer pool, which is a FIFO queue. So there is no garantee that this buffer matches with the current processed surface. Evenmore, the VA driver might not use a FIFO queue. Therefore, it is no way to guess on the ordering. In short, acquire_buffer on the VA driver and on the buffer pool return none matching data, we have to manually attach the right GstFdMemory to the acquired GstBuffer. The right GstMemory is the one associated with the current surface. https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit 6a0375d96eae0378d19f5c450a278516e5d16305 Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 16:05:04 2016 +0100 vaapivideomemory: export surface if it is provided gst_vaapi_dmabuf_memory_new() always exports a surface. Previously, it had to create that surface. Now it can also export an already provided surface. It is useful to export decoder's surfaces (from VA context). https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit 9132510ce090f1a6e7279f19c230fdea8aae0b4c Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 15:55:27 2016 +0100 vaapivideobufferpool: add GstVaapiVideoBufferPoolAcquireParams Useful to let the pool know the current surface proxy when calling gst_buffer_pool_alloc_buffer() / gst_buffer_pool_acquire_buffer() https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit 4f037a036bf138607df201e73ff3ff0f8fa449b4 Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 15:09:34 2016 +0100 libs: surface: add gst_vaapi_surface_{set,peek}_buffer_proxy() These functions are useful when a dmabuf-based memory is instantiated in order to relate the generated buffer @proxy with the processed @surface. https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit 7fc1b70ff65bc5a78181e1faa23b4e5e510f8c3b Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 15:07:31 2016 +0100 libs: bufferproxy: gst_vaapi_buffer_proxy_{set,peek}_mem() This patch adds a GstMemory as a variable member of the buffer proxy, because we will need to associate the buffer proxy with the memory which exposes it. Later, we will know which memory, in the video buffer pool, is attached to the processed surface. https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit bc97987ffb5a5d5699fcb53a9413f9e8b4e525f2 Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 15:33:41 2016 +0100 vaapipostproc: don't GLTextureUpload if dmabuf Do not add the meta:GstVideoGLTextureUploadMeta feature if the render element can handle dmabuf-based buffers, avoiding its negotiation. Similar as "vaapidecode: do not add meta:GstVideoGLTextureUploadMeta feature if can dmabuf" https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit aa20508bcfd1109b1fd522b0fba0f5f7f48a1d73 Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Fri Dec 16 14:12:30 2016 +0100 plugins: enable DMAbuf allocator to downstream If the negotiated caps are raw caps and downstream supports the EGL_EXT_image_dma_buf_import extension, then the created allocator is the DMAbuf, configured to downstream. At this moment, the only element which can push dmabuf-based buffers to downstream, is vaapipostproc. commit 37b774393413aec9c444bc45f08be2bf385e3be2 Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Thu Jun 2 22:13:51 2016 +0200 plugins: check if negotiate dmabuf with downstream In order to enable, in the future, dmabuf-based buffers, the vaapi base plugin needs to check if downstream can import dmabuf buffers. This patch checks if downstream can handle dmabuf, by introspecting the shared GL context. If the GL context is EGL/GLES2 and have the extension EGL_EXT_image_dma_buf_import, then dmabuf can be negotiated. Original-patch-by: Julien Isorce <j.isorce@samsung.com> commit 33af1fc5786a179a22ecb9c43021ddd24c382263 Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 15:37:04 2016 +0100 vaapivideomemory: release proxy's data if downstream The surface created for downstream is going to be filled by VAAPI elements. So, the driver needs write access on that surface. This patch releases the derived image held by the proxy, thus the surface is unmarked as busy. This is how it has to be done as discussed on libva mailing list. https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit 69a2406a20335ce045c4c67a9ff98e2c96d143c9 Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 15:01:04 2016 +0100 libs: bufferproxy: add gst_vaapi_buffer_proxy_release_data() Adds an API to request the user's data release in the buffer proxy. https://bugzilla.gnome.org/show_bug.cgi?id=755072 commit fbed3c3366da9a544069d2d472949d5f31b63e5d Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Oct 19 15:27:03 2016 +0100 vaapivideomemory: add direction to dmabuf allocator Add GstPadDirection param to gst_vaapi_dmabuf_allocator_new(), thus we later could do different thing when the allocated memory is for upstream or dowstream, as required by VA-API. https://bugzilla.gnome.org/show_bug.cgi?id=755072
These patches enable dmabuf downstream negotiation. Basically if you run this pipeline: GST_GL_PLATFORM=egl gst-play-1.0 video.mp4 --videosink=glimagesink You'll get dmabuf flowing between the vaapipostproc and the glimagesink Warning: some regressions may appear, please let us know to fix them.
Is this related ? 0:00:00.113822887 18615 0x7f1e080590a0 ERROR vaapipostproc gstvaapipostproc.c:806:gst_vaapipostproc_process_vpp:<vaapipostproc0> failed to apply VPP filters (error 2)
(In reply to Nicolas Dufresne (stormer) from comment #66) > Is this related ? > > 0:00:00.113822887 18615 0x7f1e080590a0 ERROR vaapipostproc > gstvaapipostproc.c:806:gst_vaapipostproc_process_vpp:<vaapipostproc0> failed > to apply VPP filters (error 2) I don't think so. What's your pipeline?
(In reply to Víctor Manuel Jáquez Leal from comment #67) > I don't think so. What's your pipeline? GST_GL_PLATFORM=egl gst-play-1.0 video.mp4 --videosink=glimagesink Same occure for anything that ends with: vaapipostproc ! glimagesink
Apparently that's bug #769253, let's track this from there.
Just a note. GST_GL_PLATFORM=egl gst-launch-1.0 filesrc location=300.mp4 ! decodebin ! glimagesink This pipeline is working as rawdata in glupload, not even as texture upload. Because it's negotiated with VASurface memory at first and creates an allocator(non dmabuf), and src caps got updated to raw caps afterward.
Another problem (Not sure if this should be fixed) : Since patches about creation of its own GLContext landed (in vaapi), srcpad_can_dmabuf is always TRUE if the env variable "GST_GL_PLATFORM" is set to egl. For example, GST_GL_PLATFORM=egl gst-play-1.0 300.mp4 --videosink=xvimagesink/ximagesink/waylandsink/clutterautovideosink this doesn't play fine. Probably, if plugins that can handle dmabuf start using memory:DMABuf, we can handle this scenario wisely.
Created attachment 354330 [details] [review] vaapivideomemory: add gst_vaapi_dmabuf_can_map() This new method checks the specified allocator can create GstMemory that can be mapped.
Created attachment 354331 [details] [review] vaapipluginbase: dmabuf memory map trial for raw caps Only push dmabuf-based buffers with raw caps if gst_memory_map() succeeds. Otherwise, use the the vaapi surfaces allocator. https://bugzilla.gnome.org/show_bug.cgi?id=755072 https://bugzilla.gnome.org/show_bug.cgi?id=774649 Original-patch-by: Julien Isorce <j.isorce@samsung.com>
Created attachment 354332 [details] [review] vaapipluginutil: add support for DMABuf caps feature https://bugzilla.gnome.org/show_bug.cgi?id=755072 Signed-off-by: Julien Isorce <j.isorce@samsung.com> Signed-off-by: Victor Jaquez <vjaquez@igalia.com> vaapipluginutil: add support for DMABuf caps feature
Created attachment 354333 [details] [review] vaapipluginbase: force dmabuf allocator if DMABuf caps feature Instantiate all dmabuf allocator for src pad buffer pool if the src caps ask for memory:DMABuf feature.
Created attachment 354334 [details] [review] vaapidecode: add support for DMABuf caps feature https://bugzilla.gnome.org/show_bug.cgi?id=755072 Original-patch-by: Julien Isorce <j.isorce@samsung.com>
Created attachment 354335 [details] [review] vaapipostproc: add support for DMABuf caps feature https://bugzilla.gnome.org/show_bug.cgi?id=755072 Signed-off-by: Julien Isorce <j.isorce@samsung.com>
Attachment 354330 [details] pushed as 5312923 - vaapivideomemory: add gst_vaapi_dmabuf_can_map() Attachment 354331 [details] pushed as f578515 - vaapipluginbase: dmabuf memory map trial for raw caps Attachment 354332 [details] pushed as 953afe9 - vaapipluginutil: add support for DMABuf caps feature Attachment 354333 [details] pushed as b6863e6 - vaapipluginbase: force dmabuf allocator if DMABuf caps feature Attachment 354334 [details] pushed as 332cfe5 - vaapidecode: add support for DMABuf caps feature Attachment 354335 [details] pushed as e7dd25f - vaapipostproc: add support for DMABuf caps feature
Fantastic! Thx. Just a question, if plugin->srcpad_can_dmabuf but map fails then does the plugin try to negotiate with the dmabuf caps feature ? (this is what was doing my wip branch). It looks to me that in the commits you pushed the caps feature is only added if someone forced it somewhere (for example in a caps filter).
(In reply to Julien Isorce from comment #79) > Fantastic! Thx. Just a question, if plugin->srcpad_can_dmabuf but map fails > then does the plugin try to negotiate with the dmabuf caps feature ? (this > is what was doing my wip branch). It looks to me that in the commits you > pushed the caps feature is only added if someone forced it somewhere (for > example in a caps filter). Right now if the buffer map fails, then the chosen allocator is vaapi one, which is well tested, and there is no need to renegotiate caps. I think that is more sensible than the (for me, obscure) caps renegotiation. But I'm open to further changes.
Sure, it is great anyway! So I tried and I have a few remarks. For the caps feature did you try with glimagesink patch from https://bugzilla.gnome.org/show_bug.cgi?id=774649 ? Because if I apply that patch, it seems that vaapi does not respect the gstglupload caps order/prefs. Indeed vaapih264dec ! glimagesink will negotiate the DMABuf caps feature, whereas being ordered after the SystemMemory. Can also be tested with: vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory); video/x-raw(memory:DMABuf)" ! glimagesink Second remark, here https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst/vaapi/gstvaapipluginbase.c#n611 I would expect to be plugin->srcpad_can_dmabuf instead of the !plugin->srcpad_can_dmabuf but actually shouldn't be passing TRUE always ? In order to match the commit message https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/commit/?id=f578515988ecf6c0ebaf920a94ea43b3fcf5c2a6 Can be tested with: vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory)" ! glimagesink Currently the decoder pushes dmabuf buffers despite being not mappable. ---- So without caps renegotiation support, which I am fine with it for now (again what you did is already a great step further and might be enough after all), I would expect that the pipeline: vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory); video/x-raw(memory:DMABuf)" ! glimagesink would just does not do any dmabuf at all. Because it will try dmabuf without caps feature first and map will fail. But since it will not try to renegotiate, it will not try to negotiate with the caps feature. So no dmabuf which is what I would expect with current commits upstream (+gstglupload patch that adds the caps feature) The only way to do dmabuf with current upstream (+ gstglupload patch that adds the caps feature) would be to add a capsfilter: vaapih264dec ! capsfilter caps="video/x-raw(memory:DMABuf)" ! glimagesink or vaapih264dec ! capsfilter caps="video/x-raw(memory:DMABuf); video/x-raw(memory:SystemMemory)" ! glimagesink (i.e. changing the order by using a caps filter for custom usage for example) Which would match what the commit message https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/commit/?id=b6863e64b550af8b472eeb35520071ab7cc0d6eb says :) Thx !
(In reply to Julien Isorce from comment #81) > Sure, it is great anyway! > > So I tried and I have a few remarks. For the caps feature did you try with > glimagesink patch from https://bugzilla.gnome.org/show_bug.cgi?id=774649 ? No. I did not :S > > Because if I apply that patch, it seems that vaapi does not respect the > gstglupload caps order/prefs. Indeed vaapih264dec ! glimagesink will > negotiate the DMABuf caps feature, whereas being ordered after the > SystemMemory. > > Can also be tested with: > > vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory); > video/x-raw(memory:DMABuf)" ! glimagesink > > Second remark, here > https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst/vaapi/ > gstvaapipluginbase.c#n611 I would expect to be plugin->srcpad_can_dmabuf > instead of the !plugin->srcpad_can_dmabuf but actually shouldn't be passing > TRUE always ? In order to match the commit message > https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/commit/ > ?id=f578515988ecf6c0ebaf920a94ea43b3fcf5c2a6 IIUC, the issue here is how do we understand plugin->srcpad_can_dmabuf As I understand it, it means that downstream can "import" dmabuf, so there is no need to map them. As a matter of fact, I'm planning to add a similar mechanism to kmssink to let know upstream that it can "import" dmabuf. We should rename that variable to make it clearer. With these patches, the logic is to try to use dmabuf as a SytemMemory if it is mappable, because it will be "more standard" rather than the vaapi mapping/unmapping. And yeah, I didn't review the commit log for that commit. My mistake. > > Can be tested with: > > vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory)" ! > glimagesink > > Currently the decoder pushes dmabuf buffers despite being not mappable. If your are under EGL, yes, it will. But if you are under GLX it won't, because dmabuf cannot be imported. > > ---- > > So without caps renegotiation support, which I am fine with it for now > (again what you did is already a great step further and might be enough > after all), I would expect that the pipeline: > > vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory); > video/x-raw(memory:DMABuf)" ! glimagesink > > would just does not do any dmabuf at all. Because it will try dmabuf without > caps feature first and map will fail. But since it will not try to > renegotiate, it will not try to negotiate with the caps feature. So no > dmabuf which is what I would expect with current commits upstream > (+gstglupload patch that adds the caps feature) IMHO that's the correct approach, since memory:SystemMemory works and it is preferred by the user. > > The only way to do dmabuf with current upstream (+ gstglupload patch that > adds the caps feature) would be to add a capsfilter: > > vaapih264dec ! capsfilter caps="video/x-raw(memory:DMABuf)" ! glimagesink > or > vaapih264dec ! capsfilter caps="video/x-raw(memory:DMABuf); > video/x-raw(memory:SystemMemory)" ! glimagesink (i.e. changing the order by > using a caps filter for custom usage for example) > > Which would match what the commit message > https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/commit/ > ?id=b6863e64b550af8b472eeb35520071ab7cc0d6eb says :) > > Thx !
(In reply to Víctor Manuel Jáquez Leal from comment #82) > (In reply to Julien Isorce from comment #81) > > here > > https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst/vaapi/ > > gstvaapipluginbase.c#n611 I would expect to be plugin->srcpad_can_dmabuf > > instead of the !plugin->srcpad_can_dmabuf but actually shouldn't be passing > > TRUE always ? In order to match the commit message > > https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/commit/ > > ?id=f578515988ecf6c0ebaf920a94ea43b3fcf5c2a6 > > > IIUC, the issue here is how do we understand plugin->srcpad_can_dmabuf > > As I understand it, it means that downstream can "import" dmabuf, so there > is no need to map them. > Ah ok I see but it still does not fit with: vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory)" ! videoscale ! "video/x-raw, width=320, height=240" ! glimagesink In general, it should just not push any dmabuf as memory:SystemMemory if it is not mappable. Your last week set of patches does not break anything compared to the state without them. It is just a note how it should be. Maybe we should add a TODO in the code for now. We might need to wait for Scott's meta to address this TODO properly. > > > > So without caps renegotiation support, which I am fine with it for now > > (again what you did is already a great step further and might be enough > > after all), I would expect that the pipeline: > > > > vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory); > > video/x-raw(memory:DMABuf)" ! glimagesink > > > > would just does not do any dmabuf at all. Because it will try dmabuf without > > caps feature first and map will fail. But since it will not try to > > renegotiate, it will not try to negotiate with the caps feature. So no > > dmabuf which is what I would expect with current commits upstream > > (+gstglupload patch that adds the caps feature) > > IMHO that's the correct approach, since memory:SystemMemory works and it is > preferred by the user. > Let me clarify, with current upstream gstreamer-vaapi vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory); video/x-raw(memory:DMABuf)" ! fakesink chooses the caps feature directly despite system being first. I think that one is a bug and should not be a TODO like the other problem mentioned on top.
My two cents, while testing with waylandsink, I found that it's much simpler to prefer the caps feature (regardless the order). A sw filter that do not support it will remove it from the caps, so it makes no difference in the end. Obviously non-mappable dmabuf as sysmem is clearly a bug here, but I guess thats working as the dmabuf info is redundant in the allocation query I suppose. Nice work btw.
(In reply to Julien Isorce from comment #83) > Ah ok I see but it still does not fit with: vaapih264dec ! capsfilter > caps="video/x-raw(memory:SystemMemory)" ! videoscale ! "video/x-raw, > width=320, height=240" ! glimagesink This is another bug: glimagesink requests more buffers than those that vaapidecode can output, so you're facing a deadlock. If you add a vaapipostproc after the decoder, it works. > > > So without caps renegotiation support, which I am fine with it for now > > > (again what you did is already a great step further and might be enough > > > after all), I would expect that the pipeline: > > > > > > vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory); > > > video/x-raw(memory:DMABuf)" ! glimagesink > > > > > > would just does not do any dmabuf at all. Because it will try dmabuf without > > > caps feature first and map will fail. But since it will not try to > > > renegotiate, it will not try to negotiate with the caps feature. So no > > > dmabuf which is what I would expect with current commits upstream > > > (+gstglupload patch that adds the caps feature) > > > > IMHO that's the correct approach, since memory:SystemMemory works and it is > > preferred by the user. > > > > Let me clarify, with current upstream gstreamer-vaapi vaapih264dec ! > capsfilter caps="video/x-raw(memory:SystemMemory); > video/x-raw(memory:DMABuf)" ! fakesink chooses the caps feature directly > despite system being first. > I think that one is a bug and should not be a TODO like the other problem > mentioned on top. We have to re-think how gst_vaapi_find_preferred_caps_feature() .. also, that's another bug :(
(In reply to Víctor Manuel Jáquez Leal from comment #85) > (In reply to Julien Isorce from comment #83) > > > Ah ok I see but it still does not fit with: vaapih264dec ! capsfilter > > caps="video/x-raw(memory:SystemMemory)" ! videoscale ! "video/x-raw, > > width=320, height=240" ! glimagesink > > This is another bug: glimagesink requests more buffers than those that > vaapidecode can output, so you're facing a deadlock. > > If you add a vaapipostproc after the decoder, it works. No deadlock here and I am trying with intel vaapi driver. If you use vaapipostproc in pipeline above it will make videoscale to switch to passthrough mode so you need this change: vaapih264dec ! vaapipostproc ! capsfilter caps="video/x-raw(memory:SystemMemory), width=640, height=480" ! videoscale ! "video/x-raw, width=320, height=240" ! glimagesink Sorry my bad all good, indeed it is not RW mappable but it is RO mappable so the videoscale succeeds to map and do its job. But since it is tiled, the output frames are wrong. Indeed GST_VAAPI_SURFACE_ALLOC_FLAG_LINEAR_STORAGE is not set. If I set it then output is correct. So no bug here. And again it will be improved with Scott's meta. > > > > > So without caps renegotiation support, which I am fine with it for now > > > > (again what you did is already a great step further and might be enough > > > > after all), I would expect that the pipeline: > > > > > > > > vaapih264dec ! capsfilter caps="video/x-raw(memory:SystemMemory); > > > > video/x-raw(memory:DMABuf)" ! glimagesink > > > > > > > > would just does not do any dmabuf at all. Because it will try dmabuf without > > > > caps feature first and map will fail. But since it will not try to > > > > renegotiate, it will not try to negotiate with the caps feature. So no > > > > dmabuf which is what I would expect with current commits upstream > > > > (+gstglupload patch that adds the caps feature) > > > > > > IMHO that's the correct approach, since memory:SystemMemory works and it is > > > preferred by the user. > > > > > > > Let me clarify, with current upstream gstreamer-vaapi vaapih264dec ! > > capsfilter caps="video/x-raw(memory:SystemMemory); > > video/x-raw(memory:DMABuf)" ! fakesink chooses the caps feature directly > > despite system being first. > > I think that one is a bug and should not be a TODO like the other problem > > mentioned on top. > > We have to re-think how gst_vaapi_find_preferred_caps_feature() .. also, > that's another bug :( Ack. Will also depends on Nicolas's replies. The current behavior might be alread y ok in fact. Depends on what we want. I was just confused with what commits messages say as opposed to what is effectively implemented.
After discussion with Nicolas it seems that things have changed and we now want to prefer the caps feature always. To be honest this simplify things a lot because the other way is too much a problem for a few benefit. So all good here regarding this subject. I just need to change caps order in https://bugzilla.gnome.org/show_bug.cgi?id=774649 .