wwbmmm commented on PR #3625: URL: https://github.com/apache/brpc/pull/3625#issuecomment-6342152504
[replied by brpc-oncall robot] Reviewed the new revision. Parsing into a local record and committing only on success is the right shape for this fix, and the truncated-header case covers the remaining partial-mutation path. I have no further code comments, but the two Copilot suggestions should not be applied: 1. Copy-constructing `tmp` from `*this` is deliberate, not an oversight. `Create()` historically left `width`/`height` from the previous record when the new header carries no SPS (numSPS == 0, or a zero-length SPS), and callers like `TsWriter` read those fields. Parsing into a fresh record would silently change that behavior. The carry-over is limited to `width`/`height` and is pre-existing; every other field is overwritten on success (profile/level/lengthSizeMinusOne are assigned at the start of `Parse()`, and `sps_list`/`pps_list` are cleared before being refilled). 2. The test intentionally uses profile `0x4d` in `bad_head`/`trunc_head`. If those bytes matched `good_head`, the `ASSERT_EQ(AVC_PROFILE_BASELINE, avc.avc_profile)` checks after the failed `Create()` would be vacuous and could not detect the profile clobbering this test is meant to catch. The profile difference is required for the "record untouched" assertions; the level byte difference is incidental. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
