Repository navigation
feat(libwebrtc): implement AudioCodecInfo constructor - #14381
oneandonlydean merged 2 commits into
Conversation
|
Thanks for this; the layout and the kept BSD notice look right at The constructor throws Merge order against #14246 is free: only the count line in |
|
Done in 821b209: the constructor now throws only for a null object and stores all five arguments as given, the six bound assertions are gone, and new cases such as min > default and all-negative values check they are stored unchanged. Swapping the min and max stores, or putting back a min > default throw, makes the test fail. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
Progress report (ea3ae18 → 4569246)📈 System libraries: 27.88% (+0.01%, +1 implemented functions) ✅ 1 implemented
Changed libraries (implemented exports)
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — no blocking findings
The AudioCodecInfo five-argument constructor stores its arguments into a 32-byte struct whose LP64 offsets I checked, its NID matches the mangled name under the repository's own NID function, and the constructor sets the two defaults the PR states; the layout and the WebRTC source it follows are cited, not checked against WebRTC here.
What I ran
- Exact head
821b209e953ef669e48e3fef3a5a5852704e1dcd, basee67cc81869. - Environment: Linux,
/usr/bin/python33.11.2, no build. - Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14381 --output-dir /dev/shm/reviewer-run/precheck/14381 - ABI audit by hand (helper absent from the head tree):
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base e67cc81869 --head 821b209e953ef669e48e3fef3a5a5852704e1dcd --json(exit 0;passed_with_warnings; 60 findings of kindnid-alias-differs-from-symbol-hash, none for the new constructor; 412 symbols). The new constructor's computed NID isuPZSgYg7CZI, the value the export carries. - NID recomputed with
tools/nid_names.py(compute_nid) for_ZN6webrtc14AudioCodecInfoC1Eimiii:uPZSgYg7CZI. - Test-quality helper on
core/libs/prx/libwebrtc/tests/AudioCodecInfo.cppfrom a head extract: one literal-path registration heuristic. - Struct layout checked against the
static_asserts ininclude/AudioCodecInfo.hpp(size 32; offsets 0, 8, 16, 20, 24, 28, 29), which I recomputed by hand for LP64 (int,std::uint64_t, threeint, twobool).
Results
Precheck report, checks table, unedited (/dev/shm/reviewer-run/precheck/14381/report.md):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check e67cc818696fd6e6c58ac80adc6550c54aed68e8...821b209e953ef669e48e3fef3a5a5852704e1dcd |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base e67cc818696fd6e6c58ac80adc6550c54aed68e8 --head 821b209e953ef669e48e3fef3a5a5852704e1dcd --title feat(libwebrtc): implement AudioCodecInfo constructor --body /dev/shm/reviewer-run/precheck/14381/pr-body.md |
| changed_export_abi_audit | failed | /usr/bin/python3 tools/pr_abi_audit.py --base e67cc818696fd6e6c58ac80adc6550c54aed68e8 --head 821b209e953ef669e48e3fef3a5a5852704e1dcd --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14381 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14381/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /usr/bin/python3 /dev/shm/reviewer-run/precheck/14381/head-821b209e953e-c9fb54fd/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14381/head-821b209e953e-c9fb54fd --json core/libs/prx/libwebrtc/CMakeLists.txt core/libs/prx/libwebrtc/Export.cpp core/libs/prx/libwebrtc/include/Api.hpp core/libs/prx/libwebrtc/include/AudioCodecInfo.hpp core/libs/prx/libwebrtc/src/AudioCodecInfo.cpp core/libs/prx/libwebrtc/tests/AudioCodecInfo.cpp docs/dev/debt/libwebrtc.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Negative control
Not run here. The C++ test needs a build.
Findings
Blocking: none.
Non-blocking:
- The new source file carries WebRTC's BSD licence header (copyright 2016 and 2011 notices). The same library already has WebRTC-licensed sources (
src/RtcpCommonHeader.cppcarries the same 2016 header), so this follows precedent; whether copied licence text belongs in these files is the same maintainer question as in #14431. - The WebRTC revisions cited (M127 and M134) and the field order were not fetched here; the struct matches my recollection of
audio_format.h, but that is not a check.
Sources
- Constructor at head: https://github.com/boykopovar/AnyPS5/blob/821b209e953ef669e48e3fef3a5a5852704e1dcd/core/libs/prx/libwebrtc/src/AudioCodecInfo.cpp
- Layout assertions at head: https://github.com/boykopovar/AnyPS5/blob/821b209e953ef669e48e3fef3a5a5852704e1dcd/core/libs/prx/libwebrtc/include/AudioCodecInfo.hpp
Not verified
- The WebRTC source the PR adapts (M127 and M134) was not fetched; the field order and defaults are the author's citation.
- The C++ test was not built or run here.
- Hosted: at this head
conventions,eligibility,labels,library-portabilityandreportsucceeded;linux,register-context,relinker,relinker-asanandwrite-watchwere skipped.
|
Thanks. On the unverified points: |
ec54a59 to
aa87ad4
Compare
aa87ad4 to
3ea8bc4
Compare
…pstream does Upstream checks the sample rate, channel count and bitrate bounds only with RTC_DCHECK, which is compiled out of the release builds titles ship. Keep the null-object throw and store every argument as given. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3ea8bc4 to
4569246
Compare
What
Implement
_ZN6webrtc14AudioCodecInfoC1Eimiii(NIDuPZSgYg7CZI) as a typed C export with an explicit object pointer. It stores all five arguments and initializes comfort noise to true and network adaption to false.The constructor and struct are identical at WebRTC M127
e0b28a6a81a989c1f5c89e30fcd247870047390dand M1348d78f5de6c27b2c793039989ea381f1428fb0100, inapi/audio_codecs/audio_format.ccand.h. Sony's exact revision and console parity remain unverified. The implementation retains the upstream copyright notice and BSD licence.Assert the 32-byte guest layout and every field offset (0, 8, 16, 20, 24, 28, 29). LP64 gives 4-byte ints, an 8-byte channel count and two 1-byte flags; alignment adds four internal padding bytes and two tail bytes. A fixed-width channel count also preserves the guest layout on Windows.
Null objects and violations of the five upstream DCHECK preconditions throw
std::invalid_argumentlocally before writing. These exceptions are defensive local behavior; release upstream does not enforce those DCHECKs or provide this exception contract.Update the throwing-export inventory from 385 to 384. The previous 325 figure counts only named
__func__stubs and omits 60 literal-NID stubs; the debt total includes both. The 31 unresolved data exports are unchanged.Tested
Linux x86-64, GCC 15, Release, configured full build:
cmake --build build-full --target libs guest_webrtc_audio_codec_info_tests --parallel 6: passed.ctest --test-dir build-full -R 'guest_webrtc|public_headers' --output-on-failure: 72/72 passed in 146.83 seconds. Constructor test: 0.01 seconds.origin/mainlibrary with its original declaration and throwing definition, then ran the final test executable against it. It linked and exited 1 with_ZN6webrtc14AudioCodecInfoC1Eimiii_nid_postfix not implemented.python3 tools/check_conventions.py --base origin/mainandgit diff --check: passed.python3 tools/pr_overlap.py --branch feat/libwebrtc-audio-codec-infoafter committing. Shared inventory/header/CMake edits may need reconciliation as other PRs merge; no semantic dependency. The tool returned exit 2 because documentation cleanup docs(debt): separate entries with blank lines and drop derived export totals #14212 conflicts with main; manually read its libwebrtc debt diff, which removes inventory numbers and changes spacing, with no constructor implementation.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)