GNOME Bugzilla – Bug 779524
buffer with video meta information do not support memory split buffers
Last modified: 2018-11-03 11:55:05 UTC
Try to avoid some clipping of images and pass DRM primes to GStreamer I tried to use gst_buffer_add_video_meta_full functions. This worked well on textures (DRM primes) and frames composed by a single memory chunk but with frames (that is GstBuffer) having multiple memory chunks (GstMemory) you get memory errors. This is due to the fact that gst_video_frame_map_id (called internally by GStreamer) assume that if there are video meta information attached there is a single chunk of memory. This behaviour is not documented for gst_buffer_add_video_meta_full or GstVideoMeta and having multiple GstMemory is supported normally so for me is an API or documentation bug. Looking at git master code there are no changes (either implementation or documentation) in this respect.
Multiple memories inside a GstBuffer are supported for video frames iff the memories are aligned to plane boundaries. Otherwise, no, multiple memories per plane aren't currently supported if that's what you're doing. For the first case, an example/test case might help with finding the cause of your problem.
For the second case, there's a FIXME in the code in default_map of gstvideometa.c
What's the first/second case? Why multiple memory is supported even on no plane alignment if gst_buffer_add_video_meta_full is not used? Is just something that works but is not expected to?
(In reply to Frediano Ziglio from comment #3) > What's the first/second case? 1. Plane-aligned memories. 2. non Plane-aligned memories. > Why multiple memory is supported even on no plane alignment if > gst_buffer_add_video_meta_full is not used? Is just something that works but > is not expected to? Which case (above) are we talking about?
(In reply to Matthew Waters (ystreet00) from comment #4) > (In reply to Frediano Ziglio from comment #3) > > What's the first/second case? > > 1. Plane-aligned memories. > 2. non Plane-aligned memories. > > > Why multiple memory is supported even on no plane alignment if > > gst_buffer_add_video_meta_full is not used? Is just something that works but > > is not expected to? > > Which case (above) are we talking about? Case 2, if you don't call gst_buffer_add_video_meta_full works. We are working with a single plane so basically the difference is plane split in multiple chunks or not.
Proposed a documentation change at https://lists.freedesktop.org/archives/gstreamer-devel/2017-March/062932.html.
Patches are handled in Bugzilla, not on the mailing list. Please attach it here in "git format-patch" format (or use git-bz).
Created attachment 347147 [details] [review] Proposed patch. This patch add user documentation for the issue of no-Plane alignment.
Review of attachment 347147 [details] [review]: Documenting a fixme is king of soso. To be honest, you probably got your offsets wrong. We could validate and warn if the remaining memory space is too small. This is simple, and more helpful.
Offsets are fine. If I made the same buffer contiguous with the same metadata it works perfectly. Basically what Matthew said is true, plane must be aligned (I would use the word contiguous but I'm not native English, maybe is the same). From gst_video_frame_map documentation: "The purpose of this function is to make it easy for you to get to the video pixels in a generic way, without you having to worry too much about details such as whether the video data is allocated in one contiguous memory chunk or multiple memory chunks (e.g. one for each plane); or if custom strides and custom plane offsets are used or not (as signalled by GstVideoMeta on each buffer). This function will just fill the GstVideoFrame structure with the right values and if you use the accessor macros everything will just work and you can access the data easily. It also maps the underlying memory chunks for you." So the documentation state that not contiguous memory chunks are supported. Yes, there is the plane example but it's just an example, not a constraint. Also note that the memory problem happens way further from the called functions and not contiguous supported happens depending on the state of the buffer. Would be great if an error would be detected in default_map (testing and notified from the code. default_map calls gst_buffer_map_range which is supposed to handle not contiguous however don't pass the plane length. What's the range of the plane ? [offset, offset + meta->height * meta->stride) if stride is positive. Can stride be negative? Would the range be [offset + meta->height * meta->stride, offset) or [offset + (meta->height-1) * meta->stride, offset - meta->stride) ? Looks like that no contiguous memory cause extra memory allocation and copies. Would be worth documenting this so people can decide to do this by themselves if more appropriate.
Just to make things clear (they weren't for me when reading this bug report). What Frediano is trying to use is a GstBuffer containing several chunks, each chunk contains N * stride bytes (N is potentially different between each chunk), with offset/stride metada attached to that buffer. What then happens is some display corruption as GStreamer only try to use the first chunk, and does some out of bounds reads on that chunk (as it assumes it's aligned on a plane boundary?) Is your « We could validate and warn if the remaining memory space is too small » suggestion aimed at fixing this buffer overflow?
As it is now, sending "chunks" or plane in multiple allocation is simply not supported by GStreamer. As it is now, this is a bug from the element producing this data. Now, that being said, is there an actual use case in which we should consider adding support for that ? If so, we'll have to copy the data and replace it when map is being called. This also requires calculating the required size of a plane. This is a lot of work, which only make sense if there is a real use case. Meanwhile, we can help developer avoid this trap, a) with documentation, which I believe is a good idea and b) with run-time checks.
Review of attachment 347147 [details] [review]: ::: gst-libs/gst/video/gstvideometa.c @@ +298,3 @@ + * Attaching metadata to @buffer require that the memories attached to + * the buffer are Plane-alignment. Failing to do so will possible cause + * memory errors. I'd try another phrasing. Maybe "Each plane must fit within a single #GstMemory. That means you may have a single memory for all planes, or one memory per plane. The start of the memory does not need to align with the start of the plane. Merging #GstMemory on map is currently not supported by this API." Or something along these lines, I use the term #GstMemory as this is what effectively wraps one chunk of data in GStreamer and can be cross-referenced. Note that most of the time the GstMemory will align with the start of the plane, that because you can hide padding using GstMemory::offset. That also makes it easier to calculate the offset, as you just have to add the GstMemory size to get the next offset. ::: gst-libs/gst/video/video-frame.c @@ +66,3 @@ * + * If video frame has GstVideoMeta the buffer requires to have Plane-aligned + * memories. Same here, something like "A single GstMemory or a GstMemory for each plane" @@ +235,3 @@ * + * If video frame has GstVideoMeta the buffer requires to have Plane-aligned + * memories. Same.
Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment. Thanks!
I must forget the previous comment. Fine with the changes.
Frediano, will you update the patch soonish ? I believe Vivia just assumed you didn't care anymore, hence closing this bug. We are cleaning up the abandoned bugs now in preparation to migration.
-- 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-plugins-base/issues/341.