Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP]: dmabuf support #48

Closed
wants to merge 42 commits into from
Closed

Conversation

columbarius
Copy link
Collaborator

@columbarius columbarius commented Aug 23, 2020

This Draft is going to implement screen sharing via wlr-export-dmabuf-unstable-v1 #9.
The first step will be to move from the current shm implementation to a protocol agnostic one with different choosable protocols.
After that the above mentioned protocol will be added.

This Draft is currently to display and discuss the current progress and will not be compilable at every time.

It contains 3 or 4 steps marked by dependent branches:

@columbarius columbarius changed the title [WIP]: support dmabuf [WIP]: dmabuf support Aug 23, 2020
@columbarius
Copy link
Collaborator Author

Currently I use the strlcpy function from bsd as it is used in wayvnc. I'm not a huge fan, of using a string function not provided by the stdlibs of the os. If you have a preferred function to copy strings, I'm happy to use that.

@columbarius
Copy link
Collaborator Author

@emersion This build probably needs at least libdrm to build on alpine. Would it be possible to add that?

@columbarius columbarius force-pushed the dmabuf branch 2 times, most recently from 5dd8a34 to 9fc7fe7 Compare September 13, 2020 10:46
@danshick
Copy link
Collaborator

danshick commented Sep 13, 2020

You can use strdup, which is nice and safe, and is available everywhere.

As for libdrm, we had that dependency once upon a time and it should be fine to add. Just update the meson config and the .builds/*.yml files as needed.

@columbarius
Copy link
Collaborator Author

@danshick All right, thanks.

@@ -25,6 +25,7 @@ void xdpw_screencast_instance_init(struct xdpw_screencast_context *ctx,
cast->framerate = out->framerate;
cast->with_cursor = with_cursor;
cast->refcount = 1;
cast->type = XDPW_INSTANCE_SCP_SHM;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this declaration is used to change between shm screencopy and dmabuf screencopy.

@columbarius columbarius force-pushed the dmabuf branch 6 times, most recently from 20ca6a1 to 09292f0 Compare September 15, 2020 18:30
@columbarius
Copy link
Collaborator Author

columbarius commented Sep 17, 2020

I found in the wlroots screencop-dmabuf.c example the option to create a linear dmabuf... which didn't work with pipewire (on intel).
So I added inspired from the same example an instance type, which prepares the data for pipewire in ram ... I guess that's not the type of dmabuf copy we want to achieve, but now we have a way to test the exported dmabuf.

@columbarius
Copy link
Collaborator Author

I realised, we messed up the difference between the modifier and the format.
The format describes the colour layout and the modifier describes the specific hardware modifications for this layout.

https://github.com/wayland-project/wayland-protocols/blob/master/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml#L144
https://github.com/wayland-project/wayland-protocols/blob/master/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml#L202

@danshick
Copy link
Collaborator

So I added inspired from the same example an instance type, which prepares the data for pipewire in ram ... I guess that's not the type of dmabuf copy we want to achieve, but now we have a way to test the exported dmabuf.

Does this offer any advantage over screencopy alone? If this is just an experiment, that's fine, but I don't think it'll be merged.

I realised, we messed up the difference between the modifier and the format.
The format describes the colour layout and the modifier describes the specific hardware modifications for this layout.

Where did we mess this up? At the moment, the color format is being handled properly AFAIK.

@columbarius
Copy link
Collaborator Author

Does this offer any advantage over screencopy alone? If this is just an experiment, that's fine, but I don't think it'll be merged.

I guess not really. I don't know how to benchmark it to be sure, but it's more like a workaround for testing. I probably move it to another branch I guess?

Where did we mess this up? At the moment, the color format is being handled properly AFAIK.

The color format yes, but the modifier is not handled properly.

@danshick
Copy link
Collaborator

The modifier is meaningless for traditional screencopy. It is always a linear frame.

@columbarius
Copy link
Collaborator Author

All this commit was missing was the Flag
PW_STREAM_FLAG_ALLOC_BUFFERS

Will rebase this pr and add that flag ...

@columbarius
Copy link
Collaborator Author

One possible option to solve temporarily afaik would be to use the modifier format parameter to determine if a consumer uses mmap for dmabufs. If a client or pipewire (PW_FLAG_MAP_BUFFER) uses mmap, they should set the modifier to 0 (This has to be proposed to pipewire). In that case, we could just deliver MemPtr (if supported, but that should be the case every time), mapped by us.

@danshick
Copy link
Collaborator

danshick commented Nov 4, 2020

In that case, we could just deliver MemPtr (if supported, but that should be the case every time), mapped by us.

I think I need to recap. On the wlroots side, we have screencopy, screencopy-dmabuf, and export-dmabuf. In the memptr case (which I believe is what we do right now), we would be forced to continue using screencopy? Or am I misunderstanding memptr?

Of course, if we see support for modifiers, we can supply a dmabuf fd from export-dmabuf (or screencopy-dmabuf, if we're concerned about a race condition).

I guess what I'd like to understand is how the buffer types correspond with the various flags that can be set, and what the corresponding mapping behavior should be.

@columbarius
Copy link
Collaborator Author

columbarius commented Nov 4, 2020

Sure.
Wlroots:
On the wlroots side we can "export" the screenbuffer in 3 different ways:

  • screencopy: Create an annonymous file descriptor in ram and let the compositor copy the buffer to it.
  • screencopy-dmabuf: Create an dmabuf linked via an file descriptor on the gpu and let the compositor copy the buffer to it.
  • export-dmabuf: Let the compositor just export the dmabuf file descriptor of the output buffer.

Pipewire:
Pipewire supports for us 3 relevant buffer types: MemPtr, MemFd and DmaBuf (all declared by SPA_DATA_*).

  • MemPtr: If you use the PW_STREAM_FLAG_MAP_BUFFERS Flag as a producer, pipewire will allocate internally memory via an file descriptor and maps it to pw_buffer buffer->buffer->datas[].data. You just get a buffer with mapped memory, ready to copy the content.

If you want another buffer type PW_STREAM_FLAG_MAP_BUFFERS is not an option, you have to use PW_STREAM_FLAG_ALLOC_BUFFERS and do the buffer allocation on your own, that's where we need:

  • MemFd: Create a filedescriptor to shared memory via create_memfd, or the trick from emersion with the wl_shm. We can map this buffer and copy our buffer information into it as usual, or just send the file descriptor where screencopy put it's content in (just thought about it, should try it later, could be racy). On the client side, you receive either a MemFd or a MemPtr, second if you let pipewire do the mapping for you (PW_STREAM_FLAG_MAP_BUFFERS).

  • DmaBuf: This just let's us send the file descriptor to the underlying dmabuf. Client will receive the same file descriptor as with MemFd, but knows, it should treat it as an dmabuf (should ... sigh). If the client uses PW_STREAM_FLAG_MAP_BUFFERS, pipewire will mmap the file descriptor to the data pointer and the client will receive an MemPtr buffer.

So long story short:
MemPtr and MemFd are interchangeable, since both are mmaped/mmapable areas of memory in ram to transport buffers.
DmaBuf: Tells the client, the file descriptor they receive should not be treated as an shared memory file descriptor, but as memory on the graphics card.

Even if we use screencopy dmabuf, we can either create a linear buffer or one with a modifier. For now i only have seen clients either support full modifiers (obs, either wlrobs or obs-xdg-portal (currently merging a patch)), or none (which means mmaping dmabuf). In the none case i would guess, we would get better results, if we do the transition from dmabuf to shm and map the dmabuf with gbm and copy the content to shared memory.
We can't just switch to the screencopy backend in that case, since the screencopy buffer might have another format and we would need to do the whole negotiation again from start, hoping it might work (If you have a good idea on how to switch the wlroots protocol I'm interested).

Tangent about changing wroots protocol on the fly:
We need to export one buffer from wlroots, to get the format and size information to start a pipewire stream, so wich backend should we use for this? The most common one (probably screencopy), or the one we would prefer (screencopy-dmabuf or export-dmabuf)?
Let's start with one using export-dmabuf: We do the negotiation and realize, the client only supports modifer 0. This is not possible, so we move to screencopy-dmabuf with an linear buffer. We update the params with the new format and tell pipewire to renegotiate. First problem: The gstreamer plugin doesn't support renegotiation and just stops (this was the result when i tested it last, maybe it got an update). Hopefully we get an match an we can start streaming. If not switch to screencast and do, what we are doing now.
If we start with screencast, we would try it and if the negotiation works, we could remember that, switch to the next backend, do the negotiation again and see, if it works, else fall back. What happens, if another client joins while negotiation and suddenly the requirements changes.
Maybe you have a better idea then i, but for now, i would just let the user set a backend (could add a start flag, or this could be usable in the config).

@danshick
Copy link
Collaborator

danshick commented Nov 4, 2020

* DmaBuf: This just let's us send the file descriptor to the underlying dmabuf. Client will receive the same file descriptor as with MemFd, but knows, it should treat it as an dmabuf (should ... sigh). If the client uses PW_STREAM_FLAG_MAP_BUFFERS, pipewire will mmap the file descriptor to the data pointer and the client will receive an MemPtr buffer.

Do we know why pipewire is still offering to mmap dmabufs? Is there any case in which this is desirable? Also, doesn't the client have to opt-in to dmabuf buffers for negotiation purposes? Shouldn't the MAP_BUFFERS flag preclude the client from negotiating a dmabuf format successfully? (I'm only asking here because I know you've had more discussions with Wim on this than me)

MemPtr and MemFd are interchangeable, since both are mmaped/mmapable areas of memory in ram to transport buffers.

Okay, this is the part I was most confused about. So a producer can claim MemFd with the ALLOC_BUFFERS flag, but there's no reason that a consumer couldn't then request MemPtr on its end and let pipewire do the mapping.

Even if we use screencopy dmabuf, we can either create a linear buffer or one with a modifier. For now i only have seen clients either support full modifiers (obs, either wlrobs or obs-xdg-portal (currently merging a patch)), or none (which means mmaping dmabuf). In the none case i would guess, we would get better results, if we do the transition from dmabuf to shm and map the dmabuf with gbm and copy the content to shared memory.

So if we were to use screencopy-dmabuf exclusively and used gbm to map it linearly, we would still have to call this a dmabuf buffer on the pipewire side, because even in linear, it still isn't kosher to mmap a dma-buf (without fencing?, I remember daniels mentioning this sowewhere).

Tangent about changing wroots protocol on the fly...

I really think we need to discuss with Wim about some way to allow negotiation to involve a round trip. Right now, a producer cannot change much about what it offers if the client doesn't support renegotiation. It'd be great to be able to signal to pipewire (maybe inside of the param_changed callback) that we would like it to hint to the consumer to try a different pipewire node. We could set up a node for each protocol, start with best possible, and continually fall back until we can negotiate properly. This should be easy for consumers to implement too, it's just a retry, and consumers that don't listen to this hint could still fail like they do today, so it should also be backwards compatible.

@columbarius
Copy link
Collaborator Author

columbarius commented Nov 12, 2020

Because of #65 I looked into chromium screensharing, which didn't work for me. Still doesn't work with this patch. Reason seems to be https://github.com/emersion/xdg-desktop-portal-wlr/pull/48/files#diff-25e29cec459c41df3c682e4e3ed522d43aeda1c932c4b8e96d3910925e50fbf4R361 which stops the negotiation.

@columbarius
Copy link
Collaborator Author

Some discussion i had with danshick

19:36:40 columbarius | I probably have to redo my whole dmabuf work since we have to use shm and dmabuf copy in tandem enabling and disabling them if needed.
19:37:57    danshick | There's got to be something better we can do in pipewire (or even xdg-desktop-portal) to improve that wha
19:38:02    danshick | *whole situation
19:38:53 columbarius | I thought about creating a inbetween struct screencast_session or something, which gets created for each combination of output and cursor_mode and inherits the pipewire node.
19:40:02    danshick | What would that struct hold?
19:40:41 columbarius | Then it has two screencast_intances, one for shm and one for dmabuf and two corresponding pipewire ports. We can declare format and buffer type at the start and listen if a client connects to one
                     | port and then start the corresponding screencast instance
19:42:22 columbarius | Via dbus we could return the id of the node and pipewire has to connect the consumer to the correct port. I don't know, if it's able to do so at the moment, but i think that would be the best
                     | solution
19:45:40 columbarius | the screencast_session would hold the montor information, the pipewire information and all the two screencast_instances can share. It's more like moving the wlr_screencopy part into it's own struct
                     | and adding the port information
19:47:13    danshick | Ah, gotcha.
19:47:28 columbarius | I havn't tried to do it yet, but if it works, it would avoid the whole negotiation issue. We would offer all formats, we support and if a client is not compatible with any port, there is not much we
                     | can do   
19:47:41    danshick | I definitely don't know enough about pipewire nodes vs. ports to say what can be done with that.
19:47:55    danshick | Makes sense.
19:55:56 columbarius | I also don't know enough, but i looked into the api for nodes and you can create arbitrary nodes, so i think it's worth to look into it.
19:56:25 columbarius | I think, it will also help to formulate what kind of patches we need from / to add to pipewire

@columbarius
Copy link
Collaborator Author

This is obsolete by #152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants