Improve pipewire buffertype handling #61

columbarius · 2020-10-23T00:28:35Z

Updated and rebased and probably rewritten large parts of this MR to work with #141. So there are some old and probably some wrong things in the discussions.

I looked into buffertype handling in pipewire and talked to @wtay at #pipewire about it.
This draft is to show the current state (of at least my understanding).

SPA_PARAM_BUFFERS_dataType
SPA_PARAM_BUFFERS_dataType should be set by a consumer not by a producer as a bitmap to indicate the supported buffer types.
{add,remove}_buffer callbacks
those callbacks are called, if pipewire adds or removes a new pw_buffer.
If no alloc flag is used to connect to pipewire the pw_buffer is read-only and buffer->datas[].type contains the supported buffer type bitmap from the client or SPA_DATA_Invalid if the client has not specified anything.
If PW_STREAM_FLAG_ALLOC_BUFFERS is used it's to producers duty to read the bitmap and set at least it's used buffer type and maybe other buffer parameters like size, stride, etc (not sure about that).
PW_STREAM_FLAG_MAP_BUFFERS
This flag tells pipewire to mmap in case of dmabuf or memfd the fd to the data pointer. Since we are doing our own fd handling i guess, we don't need that (Will clarify if it does anything in case of SPA_DATA_MemPtr).

danshick · 2020-10-23T00:57:47Z

Is there any proper documentation of those flag behaviors for pipewire? Other than reading from the examples/tutorials, I've never found a solid explanation. Anyway, I'd like to comment why we're using certain flags/metadata ao we can keep track of what we're intending. Something like this.

These comments are how we determined WebRTC was incorrectly specifying allowed framerates, rather than insisting on variable on purpose.

columbarius · 2020-10-23T01:16:20Z

I can look up the discussions at #pipewire, but afaik nothing official.
That's a good idea with those comments.

I just opened this Draft to discuss this with the ability to point at explicit code parts.
Reference add_buffer
https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/master/src/examples/video-src-alloc.c#L224
Reference Datatype bitmask
https://gitlab.freedesktop.org/pipewire/pipewire/-/commit/47ab07589593a3b13b4d2d79aaa4eedb242f1b86

danshick · 2020-10-23T01:38:41Z

I can look up the discussions at #pipewire, but afaik nothing official.

My bouncer follows #pipewire, I'll go back and take a look.

emersion · 2020-10-23T21:34:23Z

src/screencast/pipewire_screencast.c

@@ -214,8 +214,7 @@ void xdpw_pwr_stream_init(struct xdpw_screencast_instance *cast) {
 	pw_stream_connect(cast->stream,
 		PW_DIRECTION_OUTPUT,
 		PW_ID_ANY,
-		(PW_STREAM_FLAG_DRIVER |
-			PW_STREAM_FLAG_MAP_BUFFERS),
+		(PW_STREAM_FLAG_DRIVER),


Extra parens here

emersion · 2020-10-23T21:34:46Z

src/screencast/pipewire_screencast.c

+	logprint(TRACE, "pipewire: add buffer event handle");
+
+	d = buffer->buffer->datas;
+	logprint(TRACE, "pipewire: buffer type %08x",d[0].type);


Style: missing space after comma

emersion · 2020-10-23T21:34:51Z

src/screencast/pipewire_screencast.c

+	d = buffer->buffer->datas;
+	logprint(TRACE, "pipewire: buffer type %08x",d[0].type);
+	if (d[0].type == SPA_DATA_Invalid || ((d[0].type & (1<<SPA_DATA_MemPtr)) == 0)) {
+		logprint(ERROR, "pipewire: wrong buffer type %08x",d[0].type);


Style: missing space after comma

emersion · 2020-10-23T21:35:02Z

src/screencast/pipewire_screencast.c

+}
+
+static void pwr_handle_stream_remove_buffer(void *data, struct pw_buffer *buffer) {
+


Style: extra newline here

emersion · 2020-10-23T21:35:56Z

I think "Add {add,remove}_buffer handler to pipewire_screencast.c" and "Fix typecheck" can be squashed together?

It would be nice to add short explanations of the reason of the changes in each commit message.

columbarius · 2020-10-24T12:47:54Z

@emersion thanks for the review.
This draft is very early and i would rebase #48 first onto it, to test, how the add_buffer functions could be used best to check the negotiated parameters. Since we need to wait for pipewire to add the discussed defaults for dmabufs, for this to have a real advantage to the current state.
So I would keep small commits for now to have discrete markers for different features, which i would squash if it's ready.

emersion · 2020-10-24T13:19:20Z

No problem, let me know when you need a new review.

columbarius · 2020-10-29T13:23:22Z

@emersion @danshick
I currently can't figure out a good way to create some hierarchy on how to select which buffer-type and respectively backend (screencopy dmabuf/export dmabuf/screencopy shm) should be used. The current way is:

Choose a backend and request a frame for dimention and format information for this backend.
Offer this information to pipewire
pipewire negotiates the format
pipewire calls the param_changed handle, we can compare the result and set the buffer information
pipewire negotiates the buffer
pipewire calls add_buffer, we can inspect the negotiated buffer type and can either use one available buffer type of our backend, or we have to change our backend and do the whole negotiation again.

What about opening one port with its format and buffer type for every backend and let pipewire to the negotiation and selection of the correct port and we will just start copying frames if the pipewire stream is used.
Would that be possible without more overhead?

…_handle_stream_add_buffer

emersion · 2021-07-02T08:26:26Z

src/screencast/pipewire_screencast.c

+
+		if (d[0].fd == -1) {
+			logprint(ERROR, "pipewire: unable to create anonymous filedescriptor");
+			return;


Should we set cast->err for this error condition? (And the next 2 as well)

yeah sounds good. Don't know if we should check for that before registering the wlroots callback or somewhere else.

Hm, yeah we should probably check this as soon as we get the broken buffer from somewhere else, which is probably when sending the screencopy request to the compositor. It's unfortunate that add_buffer can't return an error.

src/screencast/pipewire_screencast.c

emersion · 2021-07-02T08:28:23Z

LGTM apart from the error handling.

emersion

LGTM, thanks!

columbarius · 2021-07-02T10:39:01Z

Thanks for merging!

emersion reviewed Oct 23, 2020

View reviewed changes

columbarius force-pushed the fix-pipewire-dataType branch 4 times, most recently from 231e270 to 1ea7928 Compare October 27, 2020 00:12

columbarius force-pushed the fix-pipewire-dataType branch from 1ea7928 to 99679e0 Compare November 6, 2020 17:41

columbarius mentioned this pull request May 26, 2021

use pipewire buffers #141

Merged

columbarius force-pushed the fix-pipewire-dataType branch from 084c1bb to 437d74c Compare June 13, 2021 20:49

columbarius force-pushed the fix-pipewire-dataType branch from 437d74c to 7835136 Compare June 22, 2021 12:18

columbarius changed the title ~~[WIP]: Improve pipewire buffertype handling~~ Improve pipewire buffertype handling Jun 22, 2021

columbarius force-pushed the fix-pipewire-dataType branch 2 times, most recently from 4772dad to e284a2f Compare July 2, 2021 07:34

columbarius marked this pull request as ready for review July 2, 2021 07:37

columbarius added 4 commits July 2, 2021 09:52

screencast: announce supported buffer type

05000b6

screencast: pipewire add {add,remove}_buffer handler

9ded917

screencast: pipewire move buffer information from pwr_on_event to pwr…

07ded59

…_handle_stream_add_buffer

screencast: pipewire remove uneeded PW_STREAM_FLAG_MAP_BUFFERS flag

cbaad39

columbarius force-pushed the fix-pipewire-dataType branch 3 times, most recently from 677ef53 to 15faf46 Compare July 2, 2021 08:04

emersion reviewed Jul 2, 2021

View reviewed changes

src/screencast/pipewire_screencast.c Outdated Show resolved Hide resolved

columbarius force-pushed the fix-pipewire-dataType branch 2 times, most recently from e4c117e to e98dc9f Compare July 2, 2021 10:00

screencast: pipewire change to self allocation of shm buffers

b7ee68a

columbarius force-pushed the fix-pipewire-dataType branch from e98dc9f to b7ee68a Compare July 2, 2021 10:01

emersion approved these changes Jul 2, 2021

View reviewed changes

emersion merged commit 6cc3a01 into emersion:master Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve pipewire buffertype handling #61

Improve pipewire buffertype handling #61

columbarius commented Oct 23, 2020 •

edited

Loading

danshick commented Oct 23, 2020 •

edited

Loading

columbarius commented Oct 23, 2020

danshick commented Oct 23, 2020

emersion Oct 23, 2020

emersion Oct 23, 2020

emersion Oct 23, 2020

emersion Oct 23, 2020

emersion commented Oct 23, 2020 •

edited

Loading

columbarius commented Oct 24, 2020

emersion commented Oct 24, 2020

columbarius commented Oct 29, 2020

emersion Jul 2, 2021

columbarius Jul 2, 2021

emersion Jul 2, 2021

emersion commented Jul 2, 2021

emersion left a comment

columbarius commented Jul 2, 2021

		}

		static void pwr_handle_stream_remove_buffer(void data, struct pw_buffer buffer) {

Improve pipewire buffertype handling #61

Improve pipewire buffertype handling #61

Conversation

columbarius commented Oct 23, 2020 • edited Loading

danshick commented Oct 23, 2020 • edited Loading

columbarius commented Oct 23, 2020

danshick commented Oct 23, 2020

emersion Oct 23, 2020

Choose a reason for hiding this comment

emersion Oct 23, 2020

Choose a reason for hiding this comment

emersion Oct 23, 2020

Choose a reason for hiding this comment

emersion Oct 23, 2020

Choose a reason for hiding this comment

emersion commented Oct 23, 2020 • edited Loading

columbarius commented Oct 24, 2020

emersion commented Oct 24, 2020

columbarius commented Oct 29, 2020

emersion Jul 2, 2021

Choose a reason for hiding this comment

columbarius Jul 2, 2021

Choose a reason for hiding this comment

emersion Jul 2, 2021

Choose a reason for hiding this comment

emersion commented Jul 2, 2021

emersion left a comment

Choose a reason for hiding this comment

columbarius commented Jul 2, 2021

columbarius commented Oct 23, 2020 •

edited

Loading

danshick commented Oct 23, 2020 •

edited

Loading

emersion commented Oct 23, 2020 •

edited

Loading