Created attachment 248997 [details] [review]
plugins: enable memory map for read & write

Hence vaapisink can display buffers decoded by gst-libav.

Hi, that's indeed an issue but I have not found any solution for the broader one: if we allow R/W mappings, we have to detect cases when the user maps the GstBuffer and expects to read data from it. So, we would have to download the VA surface contents first.

That's why, supporting read-only or write-only mappings are easy and efficient. read/write mappings are too generic. This could mean:
- reading, so downloading will be necessary ;
- writing, so only upload will be needed ;
- or read-modify-write, which means VA surface download first would be needed.

IMHO, it would be best if we had an additional flag that would tell "hey, I asked RW mapping but this is because I guarantee you that I will be writing to the whole buffer and possible read from it for intermediate operations".

What about GST_MAP_RESET, thus telling that the caller would be touching to the whole GstBuffer during the mapping? Or, alternatively, a flag that would mean "hey, I really expect data to be valid and the first operation to the buffer would be a read". e.g. GST_MAP_READ_FIRST?

That way, we would have the following cases:
- GST_MAP_READ: map with VA surface download first ;
- GST_MAP_WRITE: map, and VA surface upload on un-map ;
- GST_MAP_READWRITE: map, and VA surface upload on un-map;
- GST_MAP_READWRITE|GST_MAP_READ_FIRST: map, download VA surface, and commit it back on un-map;

(In reply to comment #2)
> Hi, that's indeed an issue but I have not found any solution for the broader
> one: if we allow R/W mappings, we have to detect cases when the user maps the
> GstBuffer and expects to read data from it. So, we would have to download the
> VA surface contents first.

Damn! I knew it wasn't that simple :/

IIUC, gst-libav maps the buffer to read some metadata on it and then it is used for write the frame data, just like GST_MAP_READ_FIRST that you propose.

Even if #704083 is fixed, we cannot expect people to upgrade to a newer GStreamer stack. So, I will allow read/write mappings only if direct-rendering mode is supported. That's the minimum thing to do. Though, this limits the usage to VA drivers that support vaDeriveImage() [Intel].

commit 77b2d884520f8d5de7f5d77545f2373da002fd30
Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
Date:   Wed Oct 9 10:33:55 2013 +0200

    plugins: enable memory maps for read & write with direct-rendering.
    
    Enable read and write mappings only if direct-rendering is supported.
    Otherwise, this means that we may need to download data from the VA
    surface first for correctness, even if the VA surface doesn't need to
    be read at all. i.e. sometimes, READWRITE mappings are meant for
    surfaces that are written to first, and read afterwards for further
    processing.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=704078

Victor, do you still have issues with the mentioned patch that was pushed to git master branch, now released as 0.5.7? At least, it works for me on Intel platforms.

The general case (vaGetImage() / vaPutImage()) looks complex, in terms of efforts needed to upgrade all the existing plugins, to get the expected performance level on platforms that do not support vaDeriveImage().

Hey!

I finally sat on my pants to test this.

With videotestsrc it works great, but if using gst-libav, as the decoder wants to use the sink's buffer-pool, when ask to write in them, the pipeline fails. But placing a videoconvert between the decoder and the sink (I guess because of the bugs when handling the features) the pipeline works.

I thinks the fix should be in gst-libav, the decode should create its own buffer-pool if the sink doesn't provide a proper one.

*** Bug 733242 has been marked as a duplicate of this bug. ***

(In reply to comment #7)
> Hey!
> 
> I finally sat on my pants to test this.
> 
> With videotestsrc it works great, but if using gst-libav, as the decoder wants
> to use the sink's buffer-pool, when ask to write in them, the pipeline fails.
> But placing a videoconvert between the decoder and the sink (I guess because of
> the bugs when handling the features) the pipeline works.
> 
> I thinks the fix should be in gst-libav, the decode should create its own
> buffer-pool if the sink doesn't provide a proper one.

The other issue with avdec I believe is that in some use cases, the video frame is not going to be unmaped, thus not committed back to the underlying VA surface. So, we will get jerkiness.

commit 0e87abc574427c6f1d702492fd71d7723a06ff4e
Author: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
Date:   Fri Jul 12 06:34:15 2013 -0400

    plugins: enable memory maps for read & write.
    
    Hence vaapisink can display buffers decoded by gst-libav, or HW decoded
    buffers can be further processed in-place, e.g. with a textoverlay.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=704078
    
    [ported to current git master branch, amended commit message]
    Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>

*** Bug 751711 has been marked as a duplicate of this bug. ***