[Bug 754826] memory: add remap operation

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=754826

--- Comment #5 from Matthew Waters <ystreet00 at gmail.com> ---
(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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.