Repository navigation
feat(libwebrtc): implement the webrtc::AudioEncoderRuntimeConfig destructor - #15269
alvaroelpob wants to merge 4 commits into
Conversation
Progress report (0fbe14c → 77de878)📈 System libraries: 29.1% (+0.0026%, +1 implemented functions) ✅ 1 implemented
⚪ 1 behaviourally empty (constant return or throw)
Changed libraries (implemented exports)
|
0146af9 to
f080dab
Compare
|
Rebased onto main 4f41f4c and recomputed the debt counter. Build, |
BunnyFl
left a comment
There was a problem hiding this comment.
@alvaroelpob approved at f080dabe68.
What I ran: this head merged onto main (merge 05749b4370), Windows 11, WinLibs GCC, Release.
guest_webrtc_audio_encoder_runtime_configandpublic_headerspass (2 of 2).tools/check_conventions.py --base upstream/main --headon this head: all checks pass.- Mutation: with a
throwat the top of the new body incore/libs/prx/libwebrtc/src/AudioEncoderRuntimeConfig.cpp,guest_webrtc_audio_encoder_runtime_configfails (1 of 1). So the test checks the change.
Firmware: this also settles what the debt entry leaves open. In 13.42 system/common/lib/libwebrtc.sprx (sha256 d59d0caf6967329b39201a2a0bd6df7b994516f21701b9801908db6735ae5314), _ZN6webrtc25AudioEncoderRuntimeConfigD1Ev (Qj85effg-HQ) is a lone ret at 0x6176c0. That's read from disassembly, not run. So the empty body matches the console module, not only upstream. If you like, the debt entry can say so instead of "nothing is checked on the console".
Merge order: this PR and its siblings (#15266–#15269 and #15272) each change the same 351 exports line in docs/dev/debt/libwebrtc.md, and so does my #15609. Whichever lands later only needs that count rebased. I haven't tried merging the siblings into each other.
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
The empty webrtc::AudioEncoderRuntimeConfig destructor matches upstream's defaulted destructor. It merges, builds and passes its test and public_headers, and the test fails once the throwing stub is put back.
What I ran
Head f080dabe68e245ff0023206b18de37fcf68a79b8, main 1c8c0efc8c41314402e6966da5bef6ea3d639d10, merged with --no-ff into tree 973c6dd3fdcb38dcf03e48ba331d510781204e69. Machine: Debian 13 VM, 32 vCPU (Xeon Gold 6136), GCC 14.2.0, Release, Ninja.
git diff --check 1c8c0efc8c41314402e6966da5bef6ea3d639d10...f080dabe68e245ff0023206b18de37fcf68a79b8
python3 tools/check_conventions.py --base 1c8c0efc8c41314402e6966da5bef6ea3d639d10
ctest --test-dir build --output-on-failure -R "guest_webrtc_audio_encoder_runtime_config|public_headers"
git show <main|merged>:core/libs/prx/libwebrtc/Export.cpp | grep -c "NotImplemented_nid_no_patch(__func__)"
# negative control (see below)
ninja -C build -j32 -l32 guest_webrtc_audio_encoder_runtime_config_tests libwebrtc
ctest --test-dir build --output-on-failure -R "^guest_webrtc_audio_encoder_runtime_config$"
git status --porcelain
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
Configure / build / libs (script) |
OK |
| Selected tests (script, 833 run) | 829 passed, 3 expected skips, no new failures or timeouts; 1 known failure that also fails on main (unwrap_self_UnwrapTest_test_new_output_inherits_the_destination_directory_group) |
guest_webrtc_audio_encoder_runtime_config |
passed (0.01 s) |
public_headers + 64 public_headers_compile_* |
65/65 passed |
| Debt counter | Export.cpp stubs 291 → 290 and the debt line 351 → 350: both drop by one |
| Vulkan validation | not applicable (no AGC, shader or VideoOut files changed) |
Negative control
I replaced the new body in core/libs/prx/libwebrtc/src/AudioEncoderRuntimeConfig.cpp with the old behaviour, NotImplemented_nid_no_patch(__func__) (and added the General.hpp include it needs), then rebuilt. guest_webrtc_audio_encoder_runtime_config failed:
output
1/1 Test #3602: guest_webrtc_audio_encoder_runtime_config ...Subprocess aborted***Exception: 0.01 sec
terminate called after throwing an instance of 'std::runtime_error'
what(): _ZN6webrtc25AudioEncoderRuntimeConfigD1Ev_nid_postfix not implemented
I then restored the file from the merged tree, rebuilt and re-ran the test, which passed. git status --porcelain is empty.
Findings
Blocking
None.
Non-blocking
- The description's count is out of date after the rebase. The "What" section says the inventory drops "from 352 to 351". On the merged tree,
docs/dev/debt/libwebrtc.md:5goes from 351 to 350, which matches the 291 → 290 drop inNotImplemented_nid_no_patchstubs inExport.cpp. The file is correct; only the PR text is stale. As BunnyFl noted, sibling PRs that change the same line will need the count rebased whichever lands first. - Optional wording change in the debt entry. BunnyFl reports from firmware 13.42 disassembly that the console's
D1Evis a loneret. I did not check that. If the author accepts it, the entry atdocs/dev/debt/libwebrtc.md:24could say so instead of "nothing is checked on the console".
Other points I checked, with no change needed:
Api.hpp:398is still the only declaration, nowvoid(void*), which matches a D1 destructor takingthis.- The licence header follows the pattern of the sibling
src/*.cppfiles. - The test registration matches the existing
guest_webrtc_*entries: 20 s timeout and the WindowsPATHmodification.
Not verified
- Windows/MinGW build and test. BunnyFl reports they pass.
- Behaviour on the console: I had no firmware and did not run a title.
- The upstream M127/M134
audio_network_adaptor_config.cccontents. I had no network access, so I relied on the description's citation that the destructor is= defaultthere.
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (1, not counted against this PR)Validated but not compared (53, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 417 tests)Skipped (4)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
0ea9e97 to
4ac7620
Compare
…or leaves the object
…udioEncoderRuntimeConfig destructor entry
4ac7620 to
77de878
Compare
What
Implements the
webrtc::AudioEncoderRuntimeConfigdestructor in libwebrtc, which threw and was declared without parameters.The module's WebRTC revision lies between M127 and M134, as recorded in the debt file.
audio_network_adaptor_config.cc(M127, M134) defines it= defaultat both. Its six optionalint,float,boolandsize_tsettings are trivially destructible. So it leaves the object as it is.src/AudioEncoderRuntimeConfig.cppstarts with that file's WebRTC notice and the BSD licence. It removes 1 export from the inventory.Tested
Ubuntu 26.04 (WSL2, kernel 6.18), GCC 15.2.0, Release, full build with
BUILD_TESTING=ON.guest_webrtc_audio_encoder_runtime_configtest: destroying 48 bytes filled with 0xA5 leaves every byte as it was.ctest -j16on head 0146af9: 3596/3596 pass.check_conventions.py --base upstream/mainpasses.pr_overlap.py: no open pull request implements these functions. The shared lines are the pending-export count indocs/dev/debt/libwebrtc.mdand the source, test and declaration lists inCMakeLists.txtandApi.hpp.main4f41f4c, head f080dab, to resolve the conflicts on the sharedCMakeLists.txt,Export.cppand debt-file lines; both sides are kept and the debt count is recomputed fromExport.cpp. Full build andctest -j16: 3796/3800 pass;guest_sce_net,guest_poll,guest_select_waitandguest_memoryfailed under that load, pass when run alone, andmain4f41f4c passes all 3799 with-j16on the same machine.check_conventions.py --base upstream/mainpasses.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)