Closed Bug 1819044 Opened 2 years ago Closed 2 years ago

bug 1817263 broke non-pipewire builds

Categories

(Core :: WebRTC, defect)

Product:
Core
Component:
WebRTC
Platform:
Unspecified
OpenBSD
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
112 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- unaffected
firefox110 --- unaffected
firefox111 + fixed
firefox112 + fixed

People

(Reporter: gaston, Assigned: jgrulich)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Bug 1819044 - fix build non-pipewire builds (attempt #2)
48 bytes, text/x-phabricator-request
dmeehan
: approval-mozilla-beta+
Details | Review

https://hg.mozilla.org/releases/mozilla-beta/rev/58403f4370c3#l5.30 broke beta builds on OpenBSD which doesn't have pipewire.

/usr/obj/ports/firefox-111.0beta6/firefox-111.0/third_party/libwebrtc/modules/desktop_capture/desktop_capturer.cc:102:15: error: no member named 'allow_pipewire' in 'webrtc::DesktopCaptureOptions'
  if (options.allow_pipewire() && DesktopCapturer::IsRunningUnderWayland()) {
      ~~~~~~~ ^
1 error generated.
Attached file Bug 1819044 - fix build non-pipewire builds (obsolete) —

We should check only for PipeWire presence when building code specific
to PipeWire.

Assignee: nobody → grulja
Status: NEW → ASSIGNED

(In reply to grulja from comment #1)

Created attachment 9319986 [details]
Bug 1819044 - fix build non-pipewire builds

We should check only for PipeWire presence when building code specific
to PipeWire.

sadly, with that patch, beta still fails for me with a different error:

/usr/obj/ports/firefox-111.0beta6/firefox-111.0/third_party/libwebrtc/modules/desktop_capture/linux/wayland/base_capturer_pipewire.h:56:58: error: only virtual member functions can be marked 'override'
  void UpdateResolution(uint32_t width, uint32_t height) override;

my bet is on https://hg.mozilla.org/mozilla-central/rev/acd6266642951aacf8915a56777c780cae9e9af3#l5.12 or i failed at merging the patch locally. rechecking.

of course this also broke mozilla-central builds on OpenBSD, so maybe all platforms which dont have pipewire ?

(In reply to Landry Breuil (:gaston) from comment #2)

(In reply to grulja from comment #1)

Created attachment 9319986 [details]
Bug 1819044 - fix build non-pipewire builds

We should check only for PipeWire presence when building code specific
to PipeWire.

sadly, with that patch, beta still fails for me with a different error:

/usr/obj/ports/firefox-111.0beta6/firefox-111.0/third_party/libwebrtc/modules/desktop_capture/linux/wayland/base_capturer_pipewire.h:56:58: error: only virtual member functions can be marked 'override'
  void UpdateResolution(uint32_t width, uint32_t height) override;

my bet is on https://hg.mozilla.org/mozilla-central/rev/acd6266642951aacf8915a56777c780cae9e9af3#l5.12 or i failed at merging the patch locally. rechecking.

of course this also broke mozilla-central builds on OpenBSD, so maybe all platforms which dont have pipewire ?

You are right, I missed that one. Can you try now?

(In reply to grulja from comment #3)

(In reply to Landry Breuil (:gaston) from comment #2)

(In reply to grulja from comment #1)

Created attachment 9319986 [details]
Bug 1819044 - fix build non-pipewire builds

We should check only for PipeWire presence when building code specific
to PipeWire.

sadly, with that patch, beta still fails for me with a different error:

/usr/obj/ports/firefox-111.0beta6/firefox-111.0/third_party/libwebrtc/modules/desktop_capture/linux/wayland/base_capturer_pipewire.h:56:58: error: only virtual member functions can be marked 'override'
  void UpdateResolution(uint32_t width, uint32_t height) override;

my bet is on https://hg.mozilla.org/mozilla-central/rev/acd6266642951aacf8915a56777c780cae9e9af3#l5.12 or i failed at merging the patch locally. rechecking.

of course this also broke mozilla-central builds on OpenBSD, so maybe all platforms which dont have pipewire ?

You are right, I missed that one. Can you try now?

Build not finished yet but it seems the latest revision of this goes past webrtc, so a good sign of progress.. will confirm once linking libxul succeeds in a while.

Set release status flags based on info from the regressing bug 1817263

status-firefox110: --- → unaffected
status-firefox111: --- → affected
status-firefox112: --- → affected
status-firefox-esr102: --- → unaffected
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7a879ad084a6 fix build non-pipewire builds r=webrtc-reviewers,pehrsons

libxul failed to link. retrying a clean build...

ld: error: undefined hidden symbol: webrtc::DesktopCapturer::IsRunningUnderWayland()
>>> referenced by desktop_capture_impl.cc:393 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_capture_impl.cc:393)
>>>               lto.tmp:(webrtc::DesktopCaptureImpl::LazyInitDesktopCapturer())
>>> referenced by desktop_device_info.cc:418 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_device_info.cc:418)
>>>               lto.tmp:(webrtc::DesktopDeviceInfoImpl::InitializeScreenList())
>>> referenced by desktop_device_info.cc:280 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_device_info.cc:280)
>>>               lto.tmp:(webrtc::DesktopDeviceInfoImpl::InitializeWindowList())
>>> referenced 1 more times
ld: error: undefined hidden symbol: webrtc::DesktopCapturer::CreateGenericCapturer(webrtc::DesktopCaptureOptions const&)
>>> referenced by desktop_capture_impl.cc:395 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_capture_impl.cc:395)
>>>               lto.tmp:(webrtc::DesktopCaptureImpl::LazyInitDesktopCapturer())
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)

<sadface.jpg>

(In reply to Landry Breuil (:gaston) from comment #7)

libxul failed to link. retrying a clean build...

ld: error: undefined hidden symbol: webrtc::DesktopCapturer::IsRunningUnderWayland()
>>> referenced by desktop_capture_impl.cc:393 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_capture_impl.cc:393)
>>>               lto.tmp:(webrtc::DesktopCaptureImpl::LazyInitDesktopCapturer())
>>> referenced by desktop_device_info.cc:418 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_device_info.cc:418)
>>>               lto.tmp:(webrtc::DesktopDeviceInfoImpl::InitializeScreenList())
>>> referenced by desktop_device_info.cc:280 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_device_info.cc:280)
>>>               lto.tmp:(webrtc::DesktopDeviceInfoImpl::InitializeWindowList())
>>> referenced 1 more times
ld: error: undefined hidden symbol: webrtc::DesktopCapturer::CreateGenericCapturer(webrtc::DesktopCaptureOptions const&)
>>> referenced by desktop_capture_impl.cc:395 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_capture_impl.cc:395)
>>>               lto.tmp:(webrtc::DesktopCaptureImpl::LazyInitDesktopCapturer())
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)

<sadface.jpg>

That's weird, because it's already guarded by #if defined(WEBRTC_USE_PIPEWIRE)

(In reply to grulja from comment #8)

(In reply to Landry Breuil (:gaston) from comment #7)

libxul failed to link. retrying a clean build...

ld: error: undefined hidden symbol: webrtc::DesktopCapturer::IsRunningUnderWayland()
>>> referenced by desktop_capture_impl.cc:393 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_capture_impl.cc:393)
>>>               lto.tmp:(webrtc::DesktopCaptureImpl::LazyInitDesktopCapturer())
>>> referenced by desktop_device_info.cc:418 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_device_info.cc:418)
>>>               lto.tmp:(webrtc::DesktopDeviceInfoImpl::InitializeScreenList())
>>> referenced by desktop_device_info.cc:280 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_device_info.cc:280)
>>>               lto.tmp:(webrtc::DesktopDeviceInfoImpl::InitializeWindowList())
>>> referenced 1 more times
ld: error: undefined hidden symbol: webrtc::DesktopCapturer::CreateGenericCapturer(webrtc::DesktopCaptureOptions const&)
>>> referenced by desktop_capture_impl.cc:395 (/usr/obj/ports/firefox-111.0beta6/firefox-111.0/dom/media/systemservices/video_engine/desktop_capture_impl.cc:395)
>>>               lto.tmp:(webrtc::DesktopCaptureImpl::LazyInitDesktopCapturer())
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)

<sadface.jpg>

That's weird, because it's already guarded by #if defined(WEBRTC_USE_PIPEWIRE)

same thing with a clean rebuild. Will have to investigate more when time permits... but if you have the right/better fix i'll be able to test.

for some unknown reason WEBRTC_USE_PIPEWIRE is defined for us while it's sure we dont have pipewire on openbsd.

../build-amd64/dom/media/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE -DMOZILLA_INTERNAL_API -DOS_POSIX=1 -DOS_OPENBSD=1 -DOS_BSD=1

something might be wrong somewhere in the defines...

grrr that makes no sense. WEBRTC_USE_PIPEWIRE is hardcoded to true for everyone using GTK in https://searchfox.org/mozilla-central/source/dom/media/webrtc/third_party_build/webrtc.mozbuild#39 .. so i don't understand the initial failure. And in all cases, at least on OpenBSD pipewire isnt available, dunno for the other BSDs.

(In reply to Landry Breuil (:gaston) from comment #11)

grrr that makes no sense. WEBRTC_USE_PIPEWIRE is hardcoded to true for everyone using GTK in https://searchfox.org/mozilla-central/source/dom/media/webrtc/third_party_build/webrtc.mozbuild#39 .. so i don't understand the initial failure. And in all cases, at least on OpenBSD pipewire isnt available, dunno for the other BSDs.

That's a wrong assumption as it's not related to GTK, but rather to Wayland, because you cannot share anything on Wayland without using xdg-desktop-portal and PipeWire. I think this should be a build option or there should be some detection of PipeWire, thought that might not work that well as I think PipeWire is or might be dlopened? I have just a very basic knowledge of FF build system, perhaps someone else can look into this, but I can also give it a try.

i dont understand how WEBRTC_USE_PIPEWIRE is set or not. For me it should only be set on linux, at least that's what i understand from https://searchfox.org/mozilla-central/source/third_party/libwebrtc/modules/desktop_capture/desktop_capture_gn/moz.build but there's also occurences in https://searchfox.org/mozilla-central/source/third_party/libwebrtc/modules/desktop_capture/BUILD.gn though i dunno if this gn goo is used.

it seems defined, but not eveywhere..

moz:/usr/obj/ports/firefox-111.0beta6/build-amd64/ $grep -r RTC_USE_PIPE .|grep DEFIN
./ipc/glue/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE -DOS_POSIX=1 -DOS_OPENBSD=1 -DOS_BSD=1 '-DMOZ_CHILD_PROCESS_NAME="plugin-container"' '-DMOZ_CHILD_PROCESS_BUNDLE="plugin-container.app/Contents/MacOS/"'
./dom/media/webrtc/common/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE
./dom/media/webrtc/jsapi/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE
./dom/media/webrtc/jsep/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE
./dom/media/webrtc/libwebrtcglue/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE
./dom/media/webrtc/transportbridge/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE
./dom/media/webrtc/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE -DOS_POSIX=1 -DOS_OPENBSD=1 -DOS_BSD=1
./dom/media/systemservices/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE -DOS_POSIX=1 -DOS_OPENBSD=1 -DOS_BSD=1
./dom/media/backend.mk:DEFINES += -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_MOZILLA_BUILD -DRTC_ENABLE_VP9 -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_BSD -DWEBRTC_USE_X11 -DWEBRTC_USE_PIPEWIRE -DMOZILLA_INTERNAL_API -DOS_POSIX=1 -DOS_OPENBSD=1 -DOS_BSD=1

help from developers knowledgeable in this area welcome :)

Flags: needinfo?(apehrson)

Can you check with https://phabricator.services.mozilla.com/D171071?

For gecko WEBRTC_USE_PIPEWIRE is defined here.

For libwebrtc WEBRTC_USE_PIPEWIRE is defined in 4 different configurations: x64 linux, x86 linux, arm64 linux, and arm.

These 4 libwebrtc sites are generated from libwebrtc's BUILD.gn files, where (for a mozilla build) it gets defined when is_linux and rtc_use_pipewire

And rtc_use_pipewire = is_linux && use_sysroot.

Short term we should fix the gecko define. Would moving it up to be under the if CONFIG['OS_TARGET'] == 'Linux': work?

Long term we should figure out how to get these defines directly from libwebrtc.

Flags: needinfo?(apehrson)

(In reply to Andreas Pehrson [:pehrsons] from comment #15)

Short term we should fix the gecko define. Would moving it up to be under the if CONFIG['OS_TARGET'] == 'Linux': work?

This might not work on 100% as there might be still Linux distributions that don't ship PipeWire. I don't know how old distributions do you support, but it's the reason why WebRTC implemented dlopening of PipeWire so it's not a build dependency.

Long term we should figure out how to get these defines directly from libwebrtc.

That would be ideal.

Anyway, this failure was caused by my change as I didn't assume PipeWire is enabled in gecko most of the time. I submitted updated fix under the same review request that should make it work for everyone, without needing to fix anything on FF side.

https://hg.mozilla.org/mozilla-central/rev/7a879ad084a6

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox112: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 112 Branch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

(In reply to grulja from comment #16)

(In reply to Andreas Pehrson [:pehrsons] from comment #15)

Short term we should fix the gecko define. Would moving it up to be under the if CONFIG['OS_TARGET'] == 'Linux': work?

This might not work on 100% as there might be still Linux distributions that don't ship PipeWire. I don't know how old distributions do you support, but it's the reason why WebRTC implemented dlopening of PipeWire so it's not a build dependency.

This is a bigger issue, see bug 1799596.

Long term we should figure out how to get these defines directly from libwebrtc.

That would be ideal.

Anyway, this failure was caused by my change as I didn't assume PipeWire is enabled in gecko most of the time. I submitted updated fix under the same review request that should make it work for everyone, without needing to fix anything on FF side.

Your new patch looks like it would work, yes. Since your previous rev already landed could you rebase and put it up as a new patch?

(In reply to Andreas Pehrson [:pehrsons] from comment #18)

(In reply to grulja from comment #16)

(In reply to Andreas Pehrson [:pehrsons] from comment #15)

Short term we should fix the gecko define. Would moving it up to be under the if CONFIG['OS_TARGET'] == 'Linux': work?

This might not work on 100% as there might be still Linux distributions that don't ship PipeWire. I don't know how old distributions do you support, but it's the reason why WebRTC implemented dlopening of PipeWire so it's not a build dependency.

This is a bigger issue, see bug 1799596.

FF already uses all the new screen sharing code from WebRTC.

Long term we should figure out how to get these defines directly from libwebrtc.

That would be ideal.

Anyway, this failure was caused by my change as I didn't assume PipeWire is enabled in gecko most of the time. I submitted updated fix under the same review request that should make it work for everyone, without needing to fix anything on FF side.

Your new patch looks like it would work, yes. Since your previous rev already landed could you rebase and put it up as a new patch?

Already did: https://phabricator.services.mozilla.com/D171071. Or does it need to be a new request?

(In reply to grulja from comment #19)

Already did: https://phabricator.services.mozilla.com/D171071. Or does it need to be a new request?

Yes please. Less confusion if the phabricator revs match what has landed.

Attachment #9319986 - Attachment is obsolete: true

Make the new API available to everyone and just return an empty capturer
in case when building without PipeWire. It will not make any difference
because using X11 based capturers on Wayland is useless anyway so if we
fail for missing PipeWire on Wayland, it will have the same outcome.

(In reply to grulja from comment #21)

Created attachment 9320216 [details]
Bug 1819044 - fix build non-pipewire builds (attempt #2)

Make the new API available to everyone and just return an empty capturer
in case when building without PipeWire. It will not make any difference
because using X11 based capturers on Wayland is useless anyway so if we
fail for missing PipeWire on Wayland, it will have the same outcome.

Would it sensible to present this information to the user? It seems like this state of affairs regarding Wayland captures on non-pipewire shipping platforms may not change any time soon.

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

(In reply to grulja from comment #21)

Created attachment 9320216 [details]
Bug 1819044 - fix build non-pipewire builds (attempt #2)

Make the new API available to everyone and just return an empty capturer
in case when building without PipeWire. It will not make any difference
because using X11 based capturers on Wayland is useless anyway so if we
fail for missing PipeWire on Wayland, it will have the same outcome.

Would it sensible to present this information to the user? It seems like this state of affairs regarding Wayland captures on non-pipewire shipping platforms may not change any time soon.

I think it might be useful to advertise why things are not working, thought I don't expect many platforms that don't ship PipeWire to also use Wayland, but I might be wrong.

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ad247b0aac89 fix build non-pipewire builds (attempt #2) r=webrtc-reviewers,pehrsons

The patch landed in nightly and beta is affected.
:grulja, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(grulja)

Beta/Release Uplift Approval Request

  • User impact if declined: Broken build on systems where PipeWire is not available.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: Build FF on a system without PipeWire.
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It doesn't alter any behavior, just makes some of the code hidden to avoid build failure for missing PipeWire.
  • String changes made/needed: No
Flags: needinfo?(grulja)

still slowly building what's been commited... will confirm in a while, but i had a successful build of 111b6 by simply replacing options.allow_pipewire() by false - which was the original build failure. Not 100% certain to understand what the code is supposed to be now with all those ifdefs moved around :)

:grulja Uplifts are requested in the Details page of the attachment
https://wiki.mozilla.org/Release_Management/Uplift_rules

(In reply to Landry Breuil (:gaston) from comment #27)

still slowly building what's been commited... will confirm in a while, but i had a successful build of 111b6 by simply replacing options.allow_pipewire() by false - which was the original build failure. Not 100% certain to understand what the code is supposed to be now with all those ifdefs moved around :)

Hmm, I actually think this will also fail even with the patch I submitted, but this was already present in FF before and it was not introduced by my change.

This is because DesktopCaptureOptions::set_allow_pipewire(bool allow) is guarded in WebRTC by WEBRTC_USE_PIPEWIRE which will be set to FALSE during the build, but in gecko this define will be TRUE so there will be a mismatch and it will attempt to use API that will not be available.

I will look into it.

(In reply to Donal Meehan [:dmeehan] from comment #28)

:grulja Uplifts are requested in the Details page of the attachment
https://wiki.mozilla.org/Release_Management/Uplift_rules

Sorry, I didn't know that, still learning. I just saw somewhere in the documentation this comment and I filled it with the information specific to this change.

(In reply to grulja from comment #30)

(In reply to Donal Meehan [:dmeehan] from comment #28)

:grulja Uplifts are requested in the Details page of the attachment
https://wiki.mozilla.org/Release_Management/Uplift_rules

Sorry, I didn't know that, still learning. I just saw somewhere in the documentation this comment and I filled it with the information specific to this change.

Thanks! No worries, do you want to try it? You can reuse the same text in Comment 26

Flags: needinfo?(grulja)

(In reply to Donal Meehan [:dmeehan] from comment #31)

(In reply to grulja from comment #30)

(In reply to Donal Meehan [:dmeehan] from comment #28)

:grulja Uplifts are requested in the Details page of the attachment
https://wiki.mozilla.org/Release_Management/Uplift_rules

Sorry, I didn't know that, still learning. I just saw somewhere in the documentation this comment and I filled it with the information specific to this change.

Thanks! No worries, do you want to try it? You can reuse the same text in Comment 26

We actually need both changes that were submitted under this bug. There is only the latest change in the attachement so how do I request it for both?

Flags: needinfo?(grulja)

(In reply to grulja from comment #32)

(In reply to Donal Meehan [:dmeehan] from comment #31)

(In reply to grulja from comment #30)

(In reply to Donal Meehan [:dmeehan] from comment #28)

:grulja Uplifts are requested in the Details page of the attachment
https://wiki.mozilla.org/Release_Management/Uplift_rules

Sorry, I didn't know that, still learning. I just saw somewhere in the documentation this comment and I filled it with the information specific to this change.

Thanks! No worries, do you want to try it? You can reuse the same text in Comment 26

We actually need both changes that were submitted under this bug. There is only the latest change in the attachement so how do I request it for both?

It's confusing for uplifts when changes are made in a phabricatior revision that already landed.
Looking I see:

  1. https://hg.mozilla.org/mozilla-central/rev/7a879ad084a6 merged to central under this bug.
  2. https://hg.mozilla.org/integration/autoland/rev/ad247b0aac89 is on autoland and will merge to central in the next push.
    Which of these is fix that is needed to address the problem?

(In reply to grulja from comment #29)

(In reply to Landry Breuil (:gaston) from comment #27)

still slowly building what's been commited... will confirm in a while, but i had a successful build of 111b6 by simply replacing options.allow_pipewire() by false - which was the original build failure. Not 100% certain to understand what the code is supposed to be now with all those ifdefs moved around :)

Hmm, I actually think this will also fail even with the patch I submitted, but this was already present in FF before and it was not introduced by my change.

This is because DesktopCaptureOptions::set_allow_pipewire(bool allow) is guarded in WebRTC by WEBRTC_USE_PIPEWIRE which will be set to FALSE during the build, but in gecko this define will be TRUE so there will be a mismatch and it will attempt to use API that will not be available.

I will look into it.

Right, that explains the original failure. But.. i'll have to triple check then, because i think i've applied both of the patches that were commited to 111.0b6, it built and libxul linked without missing symbols.

Index: third_party/libwebrtc/modules/desktop_capture/desktop_capturer.cc
--- third_party/libwebrtc/modules/desktop_capture/desktop_capturer.cc.orig
+++ third_party/libwebrtc/modules/desktop_capture/desktop_capturer.cc
@@ -25,7 +25,7 @@
 #include "rtc_base/win/windows_version.h"
 #endif  // defined(RTC_ENABLE_WIN_WGC)
 
-#if defined(WEBRTC_USE_PIPEWIRE) || defined(WEBRTC_USE_X11)
+#if defined(WEBRTC_USE_PIPEWIRE)
 #include <gtk/gtk.h>
 #include <gtk/gtkx.h>
 #include "modules/desktop_capture/linux/wayland/base_capturer_pipewire.h"
@@ -84,7 +84,6 @@ std::unique_ptr<DesktopCapturer> DesktopCapturer::Crea
   return capturer;
 }
 
-#if defined(WEBRTC_USE_PIPEWIRE) || defined(WEBRTC_USE_X11)
 // static
 std::unique_ptr<DesktopCapturer> DesktopCapturer::CreateGenericCapturer(
     const DesktopCaptureOptions& options) {
@@ -99,14 +98,14 @@ std::unique_ptr<DesktopCapturer> DesktopCapturer::Crea
 // static
 std::unique_ptr<DesktopCapturer> DesktopCapturer::CreateRawGenericCapturer(
     const DesktopCaptureOptions& options) {
+#if defined(WEBRTC_USE_PIPEWIRE)
   if (options.allow_pipewire() && DesktopCapturer::IsRunningUnderWayland()) {
     return std::make_unique<BaseCapturerPipeWire>(options,
                                                   CaptureType::kAnyScreenContent);
   }
-
+#endif  // defined(WEBRTC_USE_PIPEWIRE)
   return nullptr;
 }
-#endif  // defined(WEBRTC_USE_PIPEWIRE) || defined(WEBRTC_USE_X11)
 
 // static
 std::unique_ptr<DesktopCapturer> DesktopCapturer::CreateScreenCapturer(
Index: third_party/libwebrtc/modules/desktop_capture/desktop_capturer.h
--- third_party/libwebrtc/modules/desktop_capture/desktop_capturer.h.orig
+++ third_party/libwebrtc/modules/desktop_capture/desktop_capturer.h
@@ -159,11 +159,9 @@ class RTC_EXPORT DesktopCapturer {
   // The return value if `pos` is out of the scope of the source is undefined.
   virtual bool IsOccluded(const DesktopVector& pos);
 
-#if defined(WEBRTC_USE_PIPEWIRE) || defined(WEBRTC_USE_X11)
   // Creates a DesktopCapturer instance which targets to capture windows and screens.
   static std::unique_ptr<DesktopCapturer> CreateGenericCapturer(
       const DesktopCaptureOptions& options);
-#endif
 
   // Creates a DesktopCapturer instance which targets to capture windows.
   static std::unique_ptr<DesktopCapturer> CreateWindowCapturer(
@@ -193,12 +191,10 @@ class RTC_EXPORT DesktopCapturer {
   // CroppingWindowCapturer needs to create raw capturers without wrappers, so
   // the following two functions are protected.
 
-#if defined(WEBRTC_USE_PIPEWIRE) || defined(WEBRTC_USE_X11)
   // Creates a platform specific DesktopCapturer instance which targets to
   // capture windows and screens.
   static std::unique_ptr<DesktopCapturer> CreateRawGenericCapturer(
       const DesktopCaptureOptions& options);
-#endif
 
   // Creates a platform specific DesktopCapturer instance which targets to
   // capture windows.

(In reply to Landry Breuil (:gaston) from comment #34)

Right, that explains the original failure. But.. i'll have to triple check then, because i think i've applied both of the patches that were commited to 111.0b6, it built and libxul linked without missing symbols.

Did you remove your change that you did manually before in order to compile things?

(In reply to Donal Meehan [:dmeehan] from comment #33)

It's confusing for uplifts when changes are made in a phabricatior revision that already landed.
Looking I see:

  1. https://hg.mozilla.org/mozilla-central/rev/7a879ad084a6 merged to central under this bug.
  2. https://hg.mozilla.org/integration/autoland/rev/ad247b0aac89 is on autoland and will merge to central in the next push.
    Which of these is fix that is needed to address the problem?

We need both.

(In reply to grulja from comment #35)

(In reply to Landry Breuil (:gaston) from comment #34)

Right, that explains the original failure. But.. i'll have to triple check then, because i think i've applied both of the patches that were commited to 111.0b6, it built and libxul linked without missing symbols.

Did you remove your change that you did manually before in order to compile things?

Yes, see "final" patches in my above comment, also in https://cgit.rhaalovely.net/mozilla-firefox/commit/?h=beta&id=2d1b46fb8419f4849f3cbb2fac7a6379fbaee595

:mjf Could you set a severity for this issue, so we can determine whether we might want to uplift this fix to 111?

Flags: needinfo?(mfroman)

I'm assuming it needs to be uplifted based on the comments above, but I don't know what severity level will allow that to happen. Setting S2 for now, but feel free to adjust to whatever it needs to be for the uplift to happen.

Severity: -- → S2
Flags: needinfo?(mfroman) → needinfo?(jhirsch)

(In reply to Pulsebot from comment #24)

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ad247b0aac89
fix build non-pipewire builds (attempt #2) r=webrtc-reviewers,persons

This landed in central
https://hg.mozilla.org/mozilla-central/rev/7a879ad084a6e9768479c73cc5c3f4e9d95a2ab9

https://hg.mozilla.org/mozilla-central/rev/ad247b0aac89

Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
Resolution: --- → FIXED

fwiw, my last build of unpatched m-c (so includes both commits, no other local modifications) is green on my OpenBSD buildbot : http://buildbot.rhaalovely.net/nine/#/builders/3/builds/1610 -> both commits should be backported to beta.

(In reply to Jared Hirsch [:jhirsch] (he/him) (Needinfo please) from comment #37)

:mjf Could you set a severity for this issue, so we can determine whether we might want to uplift this fix to 111?

if you dont uplift it to beta, tier3 will fail to build and have to carry patches, that's all.

Status: RESOLVED → VERIFIED

Hi :grulja could you please add a beta uplift request on the attachment?
Since there were more patches that landed, could you explicitly list them in the section "List of other uplifts needed"
It looks like it could be the following, but will need you confirmation:
https://hg.mozilla.org/mozilla-central/rev/7a879ad084a6
https://hg.mozilla.org/mozilla-central/rev/7a879ad084a6e9768479c73cc5c3f4e9d95a2ab9
https://hg.mozilla.org/mozilla-central/rev/ad247b0aac89

Let me know if you have any questions. In the beta uplift process, release management graft patches to the beta repo. Typically one patch would match one phabricator revision. If there were multiple patches then each patch would correspond in a phabricator attachment.

Flags: needinfo?(grulja)

Comment on attachment 9320216 [details]
Bug 1819044 - fix build non-pipewire builds (attempt #2)

Beta/Release Uplift Approval Request

  • User impact if declined: Broken build on systems where PipeWire is not available.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: Build FF on a system without PipeWire.
  • List of other uplifts needed: https://hg.mozilla.org/mozilla-central/rev/7a879ad084a6e9768479c73cc5c3f4e9d95a2ab9
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String changes made/needed:
  • Is Android affected?: No
Flags: needinfo?(grulja)
Attachment #9320216 - Flags: approval-mozilla-beta?

Comment on attachment 9320216 [details]
Bug 1819044 - fix build non-pipewire builds (attempt #2)

Approved for 111.0b8

Attachment #9320216 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(jhirsch)

http://buildbot.rhaalovely.net/nine/#/builders/2/builds/1573 build from m-b with the above commits were green, will confirm again that b8 is fine.

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

Attachment

General

Creator:
Created:
Updated:
Size: