GNOME Bugzilla – Bug 704083
Improve GstBuffer/GstVideoFrame mappings for HW surfaces
Last modified: 2018-05-06 15:20:45 UTC
Hi, in context of hardware surfaces, a GstBuffer map would imply a surface read (download) or write (upload) only. So, implementing GST_MAP_READWRITE might be sub-optimal. Indeed, for correctness, RW mappings should be implemented as surface download on map, and surface upload/commit on unmap. For most decoders, that don't care of the initial contents since we are only going to make the whole frame dirty, this would be suboptimal because the initial surface download on map would not be necessary. I would suggest two things. GstBuffer layer. Add a flag to notify the GstMemory handlers that on GST_MAP_READWRITE, we are going to effectively read from the buffer, prior to writing to it. e.g. GST_MAP_READ_FIRST. In this case, GST_MAP_READWRITE could mean read for "intermediate" operations to the caller and next write/commit on un-map. GstVideoFrame layer. Add an argument to specify the expected dirty rectangle when we call gst_video_frame_map(). That way, the implementation could know whether downloading from the HW surface is necessary or not. By dirty, I mean that region is going to be trashed and we don't care of reading from it initially. WDYT?
Of course, the other solution could also be to fix gst-ffmpeg/libav to to call gst_video_frame_map() with GST_MAP_WRITE only, instead of GST_MAP_READWRITE. That probably could be simpler than updating the GstVideoFrame API. :)
(In reply to comment #0) > GstBuffer layer. Add a flag to notify the GstMemory handlers that on > GST_MAP_READWRITE, we are going to effectively read from the buffer, prior to > writing to it. e.g. GST_MAP_READ_FIRST. In this case, GST_MAP_READWRITE could > mean read for "intermediate" operations to the caller and next write/commit on > un-map. Sounds like a good idea IMHO :) > GstVideoFrame layer. Add an argument to specify the expected dirty rectangle > when we call gst_video_frame_map(). That way, the implementation could know > whether downloading from the HW surface is necessary or not. By dirty, I mean > that region is going to be trashed and we don't care of reading from it > initially. This seems a bit complicated, not sure if it's worth it? Also what would the API look like? What if you want multiple dirty rectangles? (In reply to comment #1) > Of course, the other solution could also be to fix gst-ffmpeg/libav to to call > gst_video_frame_map() with GST_MAP_WRITE only, instead of GST_MAP_READWRITE. > That probably could be simpler than updating the GstVideoFrame API. :) That won't be correct as libav is using our buffers also for reading (e.g. for reference frames).
(In reply to comment #2) > (In reply to comment #0) > > > GstBuffer layer. Add a flag to notify the GstMemory handlers that on > > GST_MAP_READWRITE, we are going to effectively read from the buffer, prior to > > writing to it. e.g. GST_MAP_READ_FIRST. In this case, GST_MAP_READWRITE could > > mean read for "intermediate" operations to the caller and next write/commit on > > un-map. > > Sounds like a good idea IMHO :) I think READ_FIRST should be the default, I think elements that map RW expect to be able to read valid data before writing. So, I would make WRITE_FIRST as an option and then I would go and add that flag to where it matters.
Erm, that's actually what I read into that flag. So yes, something that allows you to specify that you will write to the data first before reading anything. Which is often what happens, but still can't be made the default behaviour because of backwards compatibility and also could cause interesting bugs if it's not explicitly and conciously selected. This would btw also help with gst-plugins-gl quite a bit.
Yes, the idea is to notify the GstMemory implementation that we are going to write over the mapped buffer, and we don't care about the existing contents. So, the flag probably could be named GST_MAP_REWRITE or something more meaningful you may find. Though, GST_MAP_WRITE_FIRST looks good too. :) The other idea with the dirty rectangle thing that could be useful is if we have a local storage/cache in the local buffer (VDPAU). That way, we probably could download only the useful region we care about. On the other hand, for decoding purposes, we only care about modifying the whole frame IMHO.
(In reply to comment #2) > > Of course, the other solution could also be to fix gst-ffmpeg/libav to to call > > gst_video_frame_map() with GST_MAP_WRITE only, instead of GST_MAP_READWRITE. > > That probably could be simpler than updating the GstVideoFrame API. :) > > That won't be correct as libav is using our buffers also for reading (e.g. for > reference frames). You are right. In this case, this also means other decoders don't appropriately set GST_MAP_READ flag when needed. :)
(In reply to comment #5) > Yes, the idea is to notify the GstMemory implementation that we are going to > write over the mapped buffer, and we don't care about the existing contents. > So, the flag probably could be named GST_MAP_REWRITE or something more > meaningful you may find. Though, GST_MAP_WRITE_FIRST looks good too. :) Ok, so let's get that part done? :) Patches anybody?
I though Wim was the volunteer (In reply to comment #7) > (In reply to comment #5) > > Yes, the idea is to notify the GstMemory implementation that we are going to > > write over the mapped buffer, and we don't care about the existing contents. > > So, the flag probably could be named GST_MAP_REWRITE or something more > > meaningful you may find. Though, GST_MAP_WRITE_FIRST looks good too. :) > > Ok, so let's get that part done? :) Patches anybody? I thought Wim was the volunteer per comment #3. I misread, we was just suggested then. :)
Created attachment 250091 [details] [review] 0001-memory-Add-GST_MAP_WRITE_FIRST.patch
Review of attachment 250091 [details] [review]: Not completely sure about this. Should we only add non-lock flags after the GST_LOCK_FLAG_LAST? And as this flag only makes sense in combination with READWRITE, should it still get a separate value in the enum?
Wim, what do you think?
Review of attachment 250091 [details] [review]: ::: gst/gstmemory.h @@ +193,3 @@ + * Since: 1.2 + */ +#define GST_MAP_WRITE_FIRST (GST_MAP_READ | GST_MAP_WRITE | GST_LOCK_FLAG_LAST) Please define it in the enum so that it works correctly with GObject-Introspection. We should do the same for GST_MAP_READWRITE...
Yes, will do that after the general way of how to solve that is clear :) See comment 10
*** Bug 740222 has been marked as a duplicate of this bug. ***
The decoder should probably first map in write-only then remap in read-only when using as reference frames. Otherwise if you give away a handle to the reference frame (in rw-mode), it's possible that some downstream element can map again in write mode and modify the data.
Fir mapping write-only, then unmapping and then mapping read-only or read-write will potentially cause uploading of the data to the hardware and then downloading it again. That does not seem ideal either.
On the HW codec element side, I was thinking about an additional mechanism of caching/locality info. i.e. keep current video frame in VA image as long as possible, then commit back to VA surface only when needed. However, this incurs additional tracking to think about, which I am not too sure about yet.
In GLMemory, only mapping can cause an upload/download, and initial map will never do anything. So they added a special map flags to control the direction. Note that my patch on 740222 was slightly wrong, in the sense that the driver do need to read these buffer as they are the observation for the next buffers (but there was no useless download as I thought). So I think for libav (in direct rendering mode only), mapping READWRITE was right. Write only and unmapping would be better for the copy mode of course.
The decoder is multi-threaded, so we probably need a way to downgrade the map from readwrite to read-only.
(In reply to comment #0) > Hi, in context of hardware surfaces, a GstBuffer map would imply a surface read > (download) or write (upload) only. So, implementing GST_MAP_READWRITE might be > sub-optimal. Indeed, for correctness, RW mappings should be implemented as > surface download on map, and surface upload/commit on unmap. I think the GLMemory design is smarter then this and that it should serve as inspiration. Basically, I think unmap() should never do anything other then setting flags or state.
(In reply to comment #20) > (In reply to comment #0) > > Hi, in context of hardware surfaces, a GstBuffer map would imply a surface read > > (download) or write (upload) only. So, implementing GST_MAP_READWRITE might be > > sub-optimal. Indeed, for correctness, RW mappings should be implemented as > > surface download on map, and surface upload/commit on unmap. > > I think the GLMemory design is smarter then this and that it should serve as > inspiration. Basically, I think unmap() should never do anything other then > setting flags or state. That's indeed similar to the "locality" flags I had in mind, e.g. IN_SURFACE, IN_IMAGE, as in "where does the up-to-date frame sit?". However, how could we handle pipelines like swdec ! hwpostproc ! swpostproc ! hwsink? are we guaranteed to get called through some ::mem_copy() hook where we could sink states?
(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #0) > > > Hi, in context of hardware surfaces, a GstBuffer map would imply a surface read > > > (download) or write (upload) only. So, implementing GST_MAP_READWRITE might be > > > sub-optimal. Indeed, for correctness, RW mappings should be implemented as > > > surface download on map, and surface upload/commit on unmap. > > > > I think the GLMemory design is smarter then this and that it should serve as > > inspiration. Basically, I think unmap() should never do anything other then > > setting flags or state. > > That's indeed similar to the "locality" flags I had in mind, e.g. IN_SURFACE, > IN_IMAGE, as in "where does the up-to-date frame sit?". However, how could we > handle pipelines like swdec ! hwpostproc ! swpostproc ! hwsink? are we > guaranteed to get called through some ::mem_copy() hook where we could sink > states? s/sink/sync/ :) Nevermind, I found an easy place for it, where "it" = sync to VA surface if element wants the VA surface id.
(In reply to comment #20) > (In reply to comment #0) > > Hi, in context of hardware surfaces, a GstBuffer map would imply a surface read > > (download) or write (upload) only. So, implementing GST_MAP_READWRITE might be > > sub-optimal. Indeed, for correctness, RW mappings should be implemented as > > surface download on map, and surface upload/commit on unmap. > > I think the GLMemory design is smarter then this and that it should serve as > inspiration. Basically, I think unmap() should never do anything other then > setting flags or state. I think ::unmap() should hw unmap if the companion ::map() hook performed a hw map for consistency, symmetrical behaviourness and actually catching up bugs. e.g. appsink or other elements that use data mapped from gst buffer after they are normally gone, i.e. after unmap. This occurred in EFL context in the past.
Yes, if map allocated some virtual memory space, it would be nice to free it. But it's never the right moment to run cache flushing or in the extreme GLMemory case on PC, upload. Instead these costy operation should be post-poned. On driver side, most V4L2 driver do that right already.
Let's close this one then as discussed with Nicolas. This should be solved at a different layer and once there's an actual use-case that can't be fixed elsewhere we can revisit this.