After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740222 - libav: Video decoder push buffer mapped in READ/WRITE
libav: Video decoder push buffer mapped in READ/WRITE
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-libav
1.4.4
Other Linux
: Normal critical
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 774983 793463 (view as bug list)
Depends on: 754826
Blocks:
 
 
Reported: 2014-11-16 21:16 UTC by Nicolas Dufresne (ndufresne)
Modified: 2018-11-03 12:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] videdec: Only map buffer for writing (1.41 KB, patch)
2014-11-16 21:17 UTC, Nicolas Dufresne (ndufresne)
rejected Details | Review
GL Side workaround (2.66 KB, patch)
2014-11-22 16:37 UTC, Nicolas Dufresne (ndufresne)
none Details | Review

Description Nicolas Dufresne (ndufresne) 2014-11-16 21:16:14 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.
Comment 1 Nicolas Dufresne (ndufresne) 2014-11-16 21:17:57 UTC
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(-)
Comment 2 Olivier Crête 2014-11-19 23:50:03 UTC
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 ***
Comment 3 Sebastian Dröge (slomo) 2014-11-20 08:53:51 UTC
Also aviddec is potentially using the buffers for reference frames, which means that they must be readable. Just WRITE would be wrong here.
Comment 4 Nicolas Dufresne (ndufresne) 2014-11-20 14:34:42 UTC
(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.
Comment 5 Sebastian Dröge (slomo) 2014-11-20 14:37:33 UTC
The problem is that we can't know if libav is reading from the memory or really just writing to it.
Comment 6 Nicolas Dufresne (ndufresne) 2014-11-20 14:41:05 UTC
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.
Comment 7 Wim Taymans 2014-11-20 15:02:10 UTC
> 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.
Comment 8 Nicolas Dufresne (ndufresne) 2014-11-22 15:44:32 UTC
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.
Comment 9 Nicolas Dufresne (ndufresne) 2014-11-22 16:08:03 UTC
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.
Comment 10 Nicolas Dufresne (ndufresne) 2014-11-22 16:37:00 UTC
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).
Comment 11 Sebastian Dröge (slomo) 2014-11-24 11:00:47 UTC
Why can't we just demote the READ|WRITE map to a READ map and then forward that downstream?
Comment 12 Wim Taymans 2014-11-24 11:41:46 UTC
(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.
Comment 13 Wim Taymans 2014-11-24 11:43:48 UTC
> map(read)
> map(readwrite);

This will fail, recursive map should have equal or less permissions.
Comment 14 Wim Taymans 2014-11-24 12:26:06 UTC
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 15 Nicolas Dufresne (ndufresne) 2014-11-25 14:41:00 UTC
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
Comment 16 Sebastian Dröge (slomo) 2015-03-15 15:18:56 UTC
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!).
Comment 17 Sebastian Dröge (slomo) 2016-11-26 09:48:25 UTC
*** Bug 774983 has been marked as a duplicate of this bug. ***
Comment 18 Nicolas Dufresne (ndufresne) 2018-02-14 18:43:06 UTC
*** Bug 793463 has been marked as a duplicate of this bug. ***
Comment 19 GStreamer system administrator 2018-11-03 12:56:55 UTC
-- 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.