Repository navigation
feat(libwebrtc): implement FileWrapper output and ownership - #14246
mkrieger95 wants to merge 1 commit into
Conversation
Progress report (ba62780 → ed8d036)📈 System libraries: 26.25% (+0.01%, +4 implemented functions) ✅ 4 implemented
Changed libraries (implemented exports)
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The implementation and focused tests look coherent on source inspection, but the PR's own overlap report leaves required landing-order coordination unresolved.
What I ran
Reviewed head df83dac6c54f75d1a55ad77bdd31a65b9302576c against PR base 5d3ad2112f60f35d463dc2c9d96d1c9d35871015; the merge preview against current main 8c91f25b3924a8419096aa368ce44043a5894558 was clean. Environment: Linux x86_64, Python 3.11.2, GCC 12.2.0, Git 2.39.5. I did not build or run CTest.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14246
python3 tools/pr_abi_audit.py --base 5d3ad2112f60f35d463dc2c9d96d1c9d35871015 --head df83dac6c54f75d1a55ad77bdd31a65b9302576c
python3 tools/pr_test_quality.py --root /marc/AnyPS5 core/libs/CMakeLists.txt core/libs/prx/libwebrtc/Export.cpp core/libs/prx/libwebrtc/include/Api.hpp core/libs/prx/libwebrtc/include/FileWrapper.hpp core/libs/prx/libwebrtc/libwebrtc.hpp core/libs/prx/libwebrtc/src/FileWrapper.cpp core/libs/tests/GuestWebrtcFileWrapper.cpp docs/dev/debt/libwebrtc.md
python3 tools/pr_stack_check.py 14246 --repo boykopovar/AnyPS5
python3 tools/pr_overlap.py --branch refs/review/pr-14246 --repo boykopovar/AnyPS5 --jobs 8
gh pr view 14246 --repo boykopovar/AnyPS5 --json state,headRefOid,updatedAt,statusCheckRollupResults
| Check | Result |
|---|---|
| Diff check, conventions and PR template | Passed in precheck. |
| ABI audit | Passed with warnings: the four new FileWrapper symbols each have a declaration and definition; the 60 warnings concern the existing NID-bound libwebrtcUnknown00–59 exports. This lexical check does not certify binary ABI. |
| Test-quality heuristic | One possible-unregistered-test warning; manually confirmed GuestWebrtcFileWrapper.cpp is registered in core/libs/CMakeLists.txt. The heuristic is a false positive here. |
| Merge preview | Clean against current main 8c91f25b3924a8419096aa368ce44043a5894558. |
| Hosted CI | Eligibility, conventions, portability, intake, labels and report passed; Windows was in progress. Linux and specialized jobs were skipped. |
| Author-reported tests | The description reports a Linux Release build, seven focused CTests passing, and mutation checks from before the rebase. I did not independently run them; the mutation checks are explicitly from before the rebase. |
| Local build and CTest | Not run. |
| Open-PR overlap check | The local helper could not complete because it failed parsing the GitHub response. The PR description's own scan reports shared CMake/debt metadata edits with open #14087, #14095, #14116, #14120, #14129, #14148 and #14232, while stating that no order agreement is claimed. I verified those PRs remain open. |
Raw output excerpts
Precheck conventions output:
All conventions checks passed.
ABI audit summary:
ABI audit: passed_with_warnings
Test-quality heuristic output for the changed C++ test:
core/libs/tests/GuestWebrtcFileWrapper.cpp:1: possible-unregistered-cpp-test: Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage.
Heuristic findings require manual interpretation.
Overlap-helper error:
pr_overlap: Invalid \\uXXXX escape: line 1 column 1193051 (char 1193050)
Findings
Blocking
The PR's description says its overlap scan found shared CMake/debt metadata edits with open PRs #14087, #14095, #14116, #14120, #14129, #14148 and #14232, and explicitly says no landing-order agreement is claimed. CONTRIBUTING.md requires the authors of PRs that change the same lines to agree which lands first and declare that order with Depends on. The checklist currently says Depends on: none. Please coordinate the order with the affected authors and update the checklist (or revise the changes so the overlap no longer applies).
Non-blocking
No source-level defect was established in this pass. The test-registration heuristic warning is resolved by the registration visible in the changed CMake file.
Sources
- Overlap-order rule at the PR base
- PR description with the overlap scan and current
Depends onentry - CMake registration at the reviewed head
Not verified
I did not build or run the focused CTests or the negative controls. The reported focused tests are author-provided, and the mutation checks predate this rebase. Hosted Windows was still running; Linux and specialized jobs were skipped. No PS5 or title behavior was tested.
To get this to a good state
Agree and record the landing order for the reported open overlaps, then update Depends on and rerun the overlap check. I will recheck the exact new head and rerun the applicable no-build checks.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
df83dac to
ed8d036
Compare
What
Implement four previously throwing libwebrtc exports:
webrtc::FileWrapper::Close,Flush,Write(const void*, size_t)and the move constructor. Operations use the existing guest libcFileStreamAPIs, preserving guest stream tracking and platform handling.Supersedes boykopovar/AnyPS5#13882.
The eight-byte object owns one stream pointer. Write reports whether all requested bytes were accepted; Flush forwards stream errors; Close clears ownership even when the close reports a write failure and succeeds when already empty. Move construction transfers ownership and empties the source without reading uninitialized destination storage.
Source evidence: compared
file_wrapper.handfile_wrapper.ccat M127 with M134 header and implementation. These four methods, signatures and one-pointer layout are identical. Those versions bracket the module's inferred revision from its rtc namespace and InterleavedView exports, as investigated in boykopovar/AnyPS5#13687. This code is written against that contract; no new third-party dependency or copied source is added.PS5 ABI and behavior have not been measured. Invalid wrappers/move objects, Flush or Write without an open stream, and nonempty writes from null data throw for upstream contract violations. These limitations are recorded in
docs/dev/debt/libwebrtc.md. No title unblock is claimed.Tested
Linux x86-64, GCC 14.2, Release,
BUILD_TESTING=ON; based on upstreamba627802e2210187d417deb64c8f2fed8e33011fon 2026-11-01 UTC.guest_webrtc_file_wrapper,guest_webrtc_vad,guest_webrtc_fast_int_to_buffer,guest_webrtc_time,guest_webrtc_exp_filter,guest_webrtc_rtcp_common_header,guest_webrtc_opus.python3 core/libs/tests/test_public_headers.py --audit-only: 399 libraries, 26,227 exports pass.python3 tools/check_conventions.py --base origin/mainandgit diff --check: pass./dev/fullchecks failed flush, failed implicit flush during close, and pointer clearing even when close returns false.Not tested: Windows, a title or a PS5. No console-parity claim; no full-suite or whole-project-build claim.
Overlap check at
ed8d036367a423d4131eda0488751fc3c9d56d0b:python3 tools/pr_overlap.py --branch feat/webrtc-file-wrapper-v2 --repo boykopovar/AnyPS5scanned 1,662 open PRs againstba627802e2210187d417deb64c8f2fed8e33011f. The reported same exports/new files are this PR itself (#14246), whose remote head was still the previous revision during the scan. No other completed comparison reports conflicting lines, exports or new files. Full pairwise merge previews are clean with #14087, #14095, #14116, #14120, #14129, #14148, #14232 and #14381. This resolves the same-line debt overlaps raised in review by revising our edits, so no landing-order dependency is needed.The helper still exits 2 because existing conflicts with main prevent complete comparisons for #5021, #7275, #8650, #9156, #9162, #9293, #9297, #9302, #9383, #12642, #13253, #13256 (shared CMake file) and #14212 (shared debt file). Each shared-file patch was inspected manually. Three-way merges of those files using this head have exactly the same conflict hunks as merges using unmodified main; this PR adds no conflict hunks. The CMake edits register unrelated targets or configure macOS linking, with no FileWrapper references; #14212 changes debt formatting/counts and its pre-existing conflict hunks do not contain our new entry.
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)