GNOME Bugzilla – Bug 740222
libav: Video decoder push buffer mapped in READ/WRITE
Last modified: 2018-11-03 12:56:55 UTC
While looking into adding features in glbufferpool to allow liav direct rendering to work, I notice that avviddec was actually mapping buffers in READ/WRITE. When memory a GL textures, this would cause a download to occur.
Created attachment 290815 [details] [review] [PATCH] videdec: Only map buffer for writing Mapping buffer for read/write instead of just writing will cost a buffer download with texture backed GL memory. https://bugzilla.gnome.org/show_bug.cgi?id=74022 --- ext/libav/gstavviddec.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
As discussed offline, it will only cause a download if the buffer had some previous data. *** This bug has been marked as a duplicate of bug 704083 ***
Also aviddec is potentially using the buffers for reference frames, which means that they must be readable. Just WRITE would be wrong here.
(In reply to comment #3) > Also aviddec is potentially using the buffers for reference frames, which means > that they must be readable. Just WRITE would be wrong here. Though, it does not seem to keep it after the frame has been produces. Hence why this large latency.
The problem is that we can't know if libav is reading from the memory or really just writing to it.
We can read the code, it is clearly doing both in direct rendering mode. After it has let the frame go, it can't ever think of writing to it again, not without an API to let it know when it's rendered. It could still be readind, I'd like to test this. Right now we do the silliest thing ever, we push a buffer mapped in READ/WRITE. It only worked because XImage does not call map.
> Right now we do the silliest thing ever, we push a buffer mapped in READ/WRITE. > It only worked because XImage does not call map. It works because you can map a READWRITE mapped memory in READ and WRITE without problems. Mapping in write mode is problematic because downstream could then modify the reference frame.
Ok, reopening, as the other generic bug is more of a discussion, hence isn't a solution for this problem. Just to remind, I'm trying to enable direct rendering when using glimagesink and libav together (to avoid an extra copy). I have reverted the libav patch, as like Wim said the map failure was due to having only WRITE mapped the buffer. Now it maps of course, but the texture is never uploaded, like if the GLMemory map implementation was never called. I have experimented with simply unmapping the buffer, and then mapping it read-only. This fixed it, but if libav is still reading from that it's could be racy. So indeed, having a way to atomicly remap could be a plausible option. The other idea that was proposed was to: map(read) map(readwrite); ... unmap(readwrite); push() Not sure yet if that make sense.
Ok, so indeed this last part does not make sense: if (G_UNLIKELY (object->flags & GST_MINI_OBJECT_FLAG_LOCK_READONLY && flags & GST_LOCK_FLAG_WRITE)) return FALSE; I've looked a bit deeper in GLMemory, the NEED_UPLOAD flags is set on unmap(). So that explains why it won't upload we never unmapped from any kind of write map.
Created attachment 291241 [details] [review] GL Side workaround So with this kind of patch, I can get this to work without patching gst-libav. It still seem quite wrong to me to push a buffer in write. It's like saying, I'm done but not really done, sorry if I break your rendering. I think I'd prefer if we could downgrade a READWRITE map to READ only. After what the only way to write in it again would be to unmap/map it. Note that this patch assume that mapping in write only will require a full write. This may or may not be correct. Someone could get fooled with this, thinking MAP_WRITE is find to overlay an opaque rectangle, which in fact may result if an opaque rectangle with random data surrounding it (or instead of random, some data from previous run).
Why can't we just demote the READ|WRITE map to a READ map and then forward that downstream?
(In reply to comment #11) > Why can't we just demote the READ|WRITE map to a READ map and then forward that > downstream? We can unmap READ|WRITE and then map READ. The only concern is that in a multithreaded decoder, this can't be safe. Therefore we need an atomic remap call.
> map(read) > map(readwrite); This will fail, recursive map should have equal or less permissions.
We could add a remap function to memory and buffer with a relock function in miniobject or we could add a new GST_LOCK_RELOCK flag. I don't think there is much advantage to a flag, adding api seems more in line with mremap(2). At the minobject level, it would probably atomically flip some bits when the shared count is 1 (otherwise it would act like map, and fail when asking to relock something that you don't own, with less privileges) After relocking, the default implementation of the new remap function would unmap and map. You would have to implement something in the allocators that can do something smarter.
Comment on attachment 291241 [details] [review] GL Side workaround I'm making this patch obsolete, no worry, it will come back in another bug ;-P
This is a quite bad bug btw. The buffer is mapped writable, but multiple places have a reference to it (and thus it's not really writable anymore!).
*** Bug 774983 has been marked as a duplicate of this bug. ***
*** Bug 793463 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-libav/issues/18.