GNOME Bugzilla – Bug 765418
gst_buffer_unmap() should reset GstMapInfo state
Last modified: 2016-04-23 15:04:06 UTC
It's very minor thing. I think, that when user calls gst_buffer_unmap (), he should no longer be able to use GstMapInfo object, since using this object after mapping might lead to unexpected behavior (e.g. memory can be removed). However, GstMapInfo object is really needed after unmapping, user can always make a copy of this object.
Created attachment 326534 [details] [review] 0001-buffer-reset-GstMapInfo-state-in-gst_buffer_unmap.patch
Comment on attachment 326534 [details] [review] 0001-buffer-reset-GstMapInfo-state-in-gst_buffer_unmap.patch While this makes sense, I wouldn't be surprised if there is code out there using map.size after unmap... which would now fail even if that's arguably invalid. This needs some serious testing to understand the impact.
When I first time saw the unmap() method, I was pretty sure that GstMapInfo is reset to initial state - this behavior seems to be natural for me. Moreover, I think it's better to explicitly copy some values before unmap (e.g. size) if the value is needed afterwards, instead of letting user to accidentally use invalid object.
I wonder if we've tried this before but had to revert it. I don't think we can change this now, even if I agree that it'd been cleaner to do so. GstMapInfo is a stack-allocated helper struct, I don't think there's any reason to believe it's in any particular state after unmapping. Arguably it's also desirable from a performance point of view not to clear it unnecessarily.
It's basically in the same state as the memory behind a pointer after free(). Nobody knows ;) I think it would nonetheless be beneficial to check if and where we use something from the map info after unmap, and fix those cases.
Sounds a very tedious thing to check without compiler help (like marking a stack-variable as 'warn if accessed again after here without assignment'), are you planning on doing that? I think this is WONTFIX otherwise :)
No, it's a bit time consuming, and I don't expect to find enough time for fixing it. Moreover, it doesn't solve my initial problem. I was thinking about reseting only memory field, but it would be completely inconsistent (why memory, and not the other fields), so I consider it as a stupid idea :) If GStreamer somewhere access to memory field from GstMapInfo struct, after unmapping, when those memory is already deleted, I think sooner or later someone report this bug. So for now, I agree with WONTFIX. Thanks for discussion