Closed Bug 1716747 Opened 4 months ago Closed 3 months ago

Webrtc: pipewire use MANDATORY FLAG for dmabuf modifier

Categories

(Core :: WebRTC: Audio/Video, enhancement)

Firefox 89
enhancement

Tracking

()

RESOLVED FIXED
92 Branch
Tracking Status
firefox92 --- fixed

People

(Reporter: co1umbarius, Assigned: aryx)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

  • Start a webrtc screencast via firefox.
  • Open a terminal and run pw-cli
  • Enter enum-params <firefox pipewire port number> 3

Note: you can get the <firefox pipewire port number> with pw-dot --detail && xdot pw.dot

Actual results:

You see one enum_format with all supported formats and a modifier

Expected results:

You should see two enum_formats: one with a modifier and the flag SPA_POD_PROP_FLAG_MANDATORY and one enum_formats with no modifier.

This is a follow up to https://bugzilla.mozilla.org/show_bug.cgi?id=1676837.
The SPA_POD_PROP_FLAG_MANDATORY was introduced in pipewire 0.3.19.

Required changes: This would require to change https://hg.mozilla.org/mozilla-central/file/tip/third_party/libwebrtc/webrtc/modules/desktop_capture/linux/base_capturer_pipewire.cc#l303 to a manually build param in the style of https://github.com/columbarius/obs-studio/blob/egl-modifiers/plugins/linux-capture/pipewire.c#L345

Note: (1<<3) has the value of SPA_POD_PROP_FLAG_MANDATORY. The example is my testing branch of obs-studio to improve dmabuf screencast.

I would like to provide a patch for this with a similar static inline struct spa_pod *build_format function.

The Bugbug bot thinks this bug should belong to the 'Core::WebRTC' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Component: Untriaged → WebRTC
Product: Firefox → Core

The SPA_POD_PROP_FLAG_MANDATORY was introduced in pipewire 0.3.19.

It was 0.3.29.

Attached patch webrtc_pipewire_modifier.patch (obsolete) — Splinter Review
Attached patch webrtc_pipewire_modifier.patch (obsolete) — Splinter Review

Fixed a variable name

Wasn't able to get the buildsetup working, so this patch is untested/unbuild.

So, this patch does not seem to apply at all to current libwebrtc, although it does look like it might apply to the version of libwebrtc we're working on updating to over in bug 1654112.

Unless this is really critical, adding yet another divergence to our import of the libwebrtc code is not worth it.

Status: UNCONFIRMED → RESOLVED
Closed: 4 months ago
Component: WebRTC → WebRTC: Audio/Video
Resolution: --- → WONTFIX

Sry, I'm not that familiar with mercurial and the firefox repository, but if that (https://hg.mozilla.org/users/na-g_nostrum.com/libwebrtc-merge-2H2020/file/tip/third_party/libwebrtc/modules/desktop_capture/linux/base_capturer_pipewire.cc) is the updated state after bug 1654112 this patch is still needed.

If you want less divergence as possible, please remove the lines
https://hg.mozilla.org/mozilla-central/file/tip/third_party/libwebrtc/webrtc/modules/desktop_capture/linux/base_capturer_pipewire.cc#l129
and
https://hg.mozilla.org/mozilla-central/file/tip/third_party/libwebrtc/webrtc/modules/desktop_capture/linux/base_capturer_pipewire.cc#l312

Post pipewire 0.3.29 the portal implementations will use the MANDATORY flag to announce pipewire enum_params with modifiers meant for dmabuf buffers and ones without for shm buffers. With the current announcement of enum_params by firefox it will either look on the portal side as only dmabufs with linear modifiers are supported (and no fallback to shm buffers), or the enum_param will be matched with a enum_param from the portal side meant to hold shm formats. Then the 0 modifier will be added, which can result in an invalid combination.

So imho it would be best to not announce dmabuf capabilities in this branch and try to fix it in the updated one.

(In reply to Byron Campen [:bwc] from comment #7)

So, this patch does not seem to apply at all to current libwebrtc, although it does look like it might apply to the version of libwebrtc we're working on updating to over in bug 1654112.

Unless this is really critical, adding yet another divergence to our import of the libwebrtc code is not worth it.

Sry, there is no edit function after submitting a comment.

I just cloned the hg repository and added my changes there, so i'm probably working on the updated version.
Would you recommend suggesting my patch there?

I think the right first step is to first see if the bug exists in Chrome, and if so, get it fixed in libwebrtc. Then we could cherry-pick the fix, possibly.

(In reply to Byron Campen [:bwc] from comment #10)

I think the right first step is to first see if the bug exists in Chrome, and if so, get it fixed in libwebrtc. Then we could cherry-pick the fix, possibly.

This also exists in chromium and libwebrtc, so it's probably best to fix it there.

Could you please link me the branches which are currently in use for webrtc, so i can revert/modify the changes done in bug 1676837.
They looked promising at the time, but it turned out, that there is a better solution an the changes will cause problems.

^

Flags: needinfo?(dminor)

As far as I can tell, reverting https://bugzilla.mozilla.org/show_bug.cgi?id=1676837 will apply cleanly to what is currently in mozilla-central, so please go ahead and prepare a patch.

We'll want to pick this up for the update as well, but I don't think that is something you need to worry about, it should happen automatically when the update is rebased prior to landing.

Flags: needinfo?(dminor)

This should give portal implementations like gnome the chance to handout linear dmabufs (because the dmabuf flag is set in SPA_PARAM_BUFFERS_dataType) and not only announcing an enum_format with an modifier will let portal implementations move forward improving capability detection.

Attachment #9228001 - Attachment is obsolete: true
Attachment #9228002 - Attachment is obsolete: true

gentle ping

Dan, can you field the review for this?

Flags: needinfo?(dminor)

Comment on attachment 9228869 [details] [diff] [review]
webrtc_pipewire_modifier-v2.patch

I think it would be better if Nico had a look, since he's been more recently involved with this. If Nico's not available, I'll do my best.

Flags: needinfo?(dminor)
Attachment #9228869 - Flags: review?(na-g)

I am reopening this as it seems like we want to land this. Co1umbarius I have a few questions if you have a moment, have you filed a bug with libwebrtc? If so, could you link it here? From reading the bug comments, I am not able to infer the exact impact to users this patch will have. Would you mind commenting? Thanks for your work on this!

Flags: needinfo?(co1umbarius)
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---

I didn't filed a but with libwebrtc, since the particular changes in my patch only effect the implementation in firefox.

The patch only reverses changes which looked at the time the pipewire patches from fedora landed usefull regarding dmabuf modifier negotiation.

The problem was that if a client didn't announce a modifier and another one did, pipewire would treat the client without modifier as if it could support all. This results in in wrong negotiating via pipewire, since firefox, or in general libwebrtc only supports linear buffers (modifier 0). Thats why i opened my first MR.

Since pipewire 0.3.29 the situation has changed. There is now a flag to indicate, that an announced format option (enumformat) should only be matched to another one, if a property is set (in short: only match enumformats containing modifiers with others with modifiers and analog for those without.). This would require firefox or libwebrtc to announce two enumformats, one for shm buffers and one for dmabufs.
This "new" approach will be discussed next week at guadec to make sure all portals will handle negotiation wrt. dmabufs the same (disclamer: I'm the one adding the pipewire flag and implementing it to the wlroots portal).

This has the problem, that with the new propasal current firefox will only announce it's capability for dmabufs and not for shm buffers. The problem is here, while dmabufs can be more performant, this isn't really the case when the buffer gets mmaped. Additionally shm buffers will work in all cases and in most cases are more stable and more performant in reality (mmapping dmabufs on an external amd gpu is really slow and dmabufs are not supported on nvidia). This problem didn't occure, since all portal implementations ignored the negotiated modifier information for now.

My first patch was to implement the correct behaviour for the new proposal in firefox, which shouldn't interfere with the old approach, but that's probably better done in libwebrtc as soon as all portals give their ok.
The second patch only removes the problematic part of my first patch (removing the modifier and using CHOICE_FLAGS_Int instead of Int, which is internaly treated the same, but the choice_flags is the correct one).

I hope this answers most of your questions. Feel free to ask again, if i left something unclear.

Flags: needinfo?(co1umbarius)
Attachment #9228869 - Flags: review?(na-g) → review+

(In reply to Nico Grunbaum [:ng, @chew:mozilla.org] from comment #21)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94f12b6a624c7e589a09ae503424596a4465339f

Does this look related to the Yellow Screen of Death failure you are hunting down? I am retriggering the test a few dozen times. That one, possibly unrelated screen sharing test failure aside, it looks good.

Flags: needinfo?(docfaraday)

(In reply to Nico Grunbaum [:ng, @chew:mozilla.org] from comment #22)

(In reply to Nico Grunbaum [:ng, @chew:mozilla.org] from comment #21)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94f12b6a624c7e589a09ae503424596a4465339f

Does this look related to the Yellow Screen of Death failure you are hunting down? I am retriggering the test a few dozen times. That one, possibly unrelated screen sharing test failure aside, it looks good.

I see a couple of cases of the ysod bug, and some more of bug 1714410. I've starred them, and am not seeing any other mda failures.

Flags: needinfo?(docfaraday)

Thanks Byron! Check in requested.

Attachment #9228869 - Flags: checkin?(na-g) → checkin?
Assignee: nobody → aryx.bugmail

Could you please ping me, when this patch reaches nightly, or any other state, where i can test a build?

Here is a try build with that patch applied to latest tip: https://treeherder.mozilla.org/jobs?repo=try&revision=f31a6d46dc630d87a04d15acc2e0cfc07288fb3e

Once it finished you can download it there from "Linux x64 opt" -> "B" -> "Artifacts" -> "target.tar.bz2" or via mozregression --repo try --launch f31a6d46dc630d87a04d15acc2e0cfc07288fb3e

Thanks! Downloaded it and it works nicely (on the wlroots portal).

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/4434fca827f8
Stop announcing modifier 0 and fix podtype of SPA_PARAM_BUFFERS_dataType.
Status: REOPENED → RESOLVED
Closed: 4 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch

Got FF 92 from my distro, checked it and it works fine. For the improved DMA-BUF support, there is a patch developed for libwebrtc.

See:

Anyway, thanks for merging!

You need to log in before you can comment on or make changes to this bug.