GNOME Bugzilla – Bug 754826
memory: add remap operation
Last modified: 2018-11-03 12:29:37 UTC
See commit messages
Created attachment 311061 [details] [review] miniobject: add lock modification functions
Created attachment 311062 [details] [review] memory: add remap operation
Review of attachment 311061 [details] [review]: The whole undo operation feels fishy to me, I'm not sure how a undo is different from modifying the flags again? Although locking downgrades seem find, locking upgrades seem like a risky idea. Downgrading and then re-upgrading seems downright wrong for example as someone else could have mapped it for reading while that happens and then you won't be able to re-upgrade! I'd have a simple gst_mini_object_downgrade_lock(GstMiniObject, gint flags_to_drop);
Review of attachment 311062 [details] [review]: Is this for the problem of gst-ffmpeg pushing down RW mapped buffers or do you have some other usecase? ::: gst/gstbuffer.c @@ +1705,3 @@ +gboolean +gst_buffer_remap (GstBuffer * buffer, GstMapInfo * info, gboolean add, + GstMapFlags flags) Why do you need the buffer here? wouldn't gst_memory_remap(GstMapInfo, add, flags) be enough? Is any remapping function but adding or removing the GST_MAP_WRITE ever going to be useful? Do we need something so generic? ::: gst/gstmemory.c @@ +348,3 @@ +gboolean +gst_memory_remap (GstMemory * mem, GstMapInfo * info, gboolean add, + GstMapFlags flags) Why not get the memory from info->memory?
(In reply to Olivier Crête from comment #3) > The whole undo operation feels fishy to me, I'm not sure how a undo is > different from modifying the flags again? Although locking downgrades seem > find, locking upgrades seem like a risky idea. Downgrading and then > re-upgrading seems downright wrong for example as someone else could have > mapped it for reading while that happens and then you won't be able to > re-upgrade! Actually, you will be able to reupgrade in that case. The more dangerous case is first adding a write map and then undoing with a potential map with the write bit in between. > I'd have a simple > gst_mini_object_downgrade_lock(GstMiniObject, gint flags_to_drop); That's what I started with :) however videometa has its own map()/unmap() and now remap() vfuncs which may also need/want to undo a remap operation unless we attempt something else. The question is what does an API user do if the memory remap operation fails. As far as I see it, we have a two of options. 1. do nothing (easy) 2. attempt to rollback (hard) With just downgrade_lock, it's effectively forcing (1) everywhere. My attempt at 2 was this. If a remap operation fails, rollback by remapping each previously mapped memory with !add. I don't believe for a second that it's thread safe at all. (In reply to Olivier Crête from comment #4) > Is this for the problem of gst-ffmpeg pushing down RW mapped buffers or do > you have some other usecase? For the moment yes, ffmpeg pushing RW buffers is the primary use case. > ::: gst/gstbuffer.c > @@ +1705,3 @@ > +gboolean > +gst_buffer_remap (GstBuffer * buffer, GstMapInfo * info, gboolean add, > + GstMapFlags flags) > > Why do you need the buffer here? wouldn't gst_memory_remap(GstMapInfo, add, > flags) be enough? Consistency in the API. gst_buffer_ functions take a GstBuffer paramater. Plus possible inconsistency in class based languages with this/self pointers through gi. > Is any remapping function but adding or removing the GST_MAP_WRITE ever > going to be useful? Do we need something so generic? See (2) above. > ::: gst/gstmemory.c > @@ +348,3 @@ > +gboolean > +gst_memory_remap (GstMemory * mem, GstMapInfo * info, gboolean add, > + GstMapFlags flags) > > Why not get the memory from info->memory? Could but see the consistency in the API point above.
(In reply to Matthew Waters from comment #5) > (In reply to Olivier Crête from comment #3) > > The whole undo operation feels fishy to me, I'm not sure how a undo is > > different from modifying the flags again? Although locking downgrades seem > > find, locking upgrades seem like a risky idea. Downgrading and then > > re-upgrading seems downright wrong for example as someone else could have > > mapped it for reading while that happens and then you won't be able to > > re-upgrade! > > Actually, you will be able to reupgrade in that case. The more dangerous > case is first adding a write map and then undoing with a potential map with > the write bit in between. I think if you only downgrade, remapping can never fail, in the worse case, the buffer will still be mapped as RW internally even though the elements think it'S RO, no ? I would never allow adding write, only removing it. > > I'd have a simple > > gst_mini_object_downgrade_lock(GstMiniObject, gint flags_to_drop); > > That's what I started with :) however videometa has its own map()/unmap() > and now remap() vfuncs which may also need/want to undo a remap operation > unless we attempt something else. I'm not sure why those would want to undo if we saw that we only allow downgrading? > The question is what does an API user do if the memory remap operation > fails. As far as I see it, we have a two of options. > 1. do nothing (easy) > 2. attempt to rollback (hard) How can a downgrad remap fail?
(In reply to Olivier Crête from comment #6) > (In reply to Matthew Waters from comment #5) > > (In reply to Olivier Crête from comment #3) > > > The whole undo operation feels fishy to me, I'm not sure how a undo is > > > different from modifying the flags again? Although locking downgrades seem > > > find, locking upgrades seem like a risky idea. Downgrading and then > > > re-upgrading seems downright wrong for example as someone else could have > > > mapped it for reading while that happens and then you won't be able to > > > re-upgrade! > > > > Actually, you will be able to reupgrade in that case. The more dangerous > > case is first adding a write map and then undoing with a potential map with > > the write bit in between. > > I think if you only downgrade, remapping can never fail, in the worse case, > the buffer will still be mapped as RW internally even though the elements > think it'S RO, no ? I would never allow adding write, only removing it. Sure but what if you have a case where you have 1 memory per YUV plane and one of those planes fails the remap operation. You now have mappings that are in an inconsistent state. There are also other flags that would possible be removed (or added?) from/to a mapping. e.g. in GL there's a coherent bit that allows mapping on both the GPU and the CPU concurrently. Without that, we have to memcpy for coherency like we currently do. This of course this is all an optimization to avoid transfers on every map related to tracking map flag state inside the memory subclass for memories that have more than one memory domain that they are dealing with. > > > I'd have a simple > > > gst_mini_object_downgrade_lock(GstMiniObject, gint flags_to_drop); > > > > That's what I started with :) however videometa has its own map()/unmap() > > and now remap() vfuncs which may also need/want to undo a remap operation > > unless we attempt something else. > > I'm not sure why those would want to undo if we saw that we only allow > downgrading? Inconsistent map state as above. > > The question is what does an API user do if the memory remap operation > > fails. As far as I see it, we have a two of options. > > 1. do nothing (easy) > > 2. attempt to rollback (hard) > > How can a downgrad remap fail? Pretty much in exactly the same ways the the lock downgrade can fail. Subclasses may have their own requirements for performing a remap that they need to check. e.g. attempting to lose the GL bit in a GL memory mapping which would result in no mapping at all. map(GL | RW) downgrade(GL) -> error, memory domain change invalidates mapped pointer to a texture handle
What's the status of this now? Is there a plan how to go forward or more discussion needed?
Basically there are issues with inconsistent map states between the memory and the map info when mapping a memory multiple times. My original attempt at remapping only allowed there to be one mapping outstanding. That fails with GstVideoFrame in the normal sysmem case where a single memory holds multiple planes. gst_video_frame_map will gst_video_meta_map that same memory multiple times (one for each plane). If one lifts the 'only one outstanding mapping' restriction one encounters other issues such as, attempting to remap the video frame will succeed with the first memory but fail with any subsequent remaps (because the map flag/s has already been removed). If we don't revert the remap, then the memory will fail unmapping because again, the flags in the map info aren't on the memory anymore. I essentially came to the conclusion that remapping video frames was an impossible endeavour with the current design using GstMemory, GstVideoFrame and GstVideoMeta. Remapping individual memories is however very much a possibility.
Could we add a special case in VideoFrame to only map once ?
(In reply to Nicolas Dufresne (stormer) from comment #10) > Could we add a special case in VideoFrame to only map once ? The only way I can see that being implemented is for GstVideoFrame to not map with any GstVideoMeta on the buffer however that is not a generic solution.
-- 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/gstreamer/issues/128.