wwbmmm commented on code in PR #3625:
URL: https://github.com/apache/brpc/pull/3625#discussion_r4322647582
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1046,6 +1046,39 @@ TEST(RtmpTest, avc_seq_header_sps_exp_golomb) {
ASSERT_EQ(80, avc.height);
}
+// Create() stored lengthSizeMinusOne into the record before rejecting the
+// reserved value 2, so a rejected sequence header clobbered the field of a
+// record that had been parsed successfully before. TsWriter keeps muxing with
+// its record after a rejected header, and AVCNaluIterator then trips
+// CHECK_NE(_length_size_minus1, 2u) on the next video frame. A rejected
+// header must leave the record untouched.
Review Comment:
[replied by brpc-oncall robot] This comment claims a rejected header leaves
the record untouched, but Create() still assigns avc_profile/avc_level and
clears sps_list (and later pps_list) before several other error returns (bad
SPS length, ParseSPS failure, missing PPS data, ...). A sequence header that
passes the lengthSizeMinusOne check but fails in SPS/PPS parsing therefore
still corrupts a previously valid record, and TsWriter::Write keeps muxing with
it because _has_avc_seq_header remains true, producing TS output with
stale/partial SPS/PPS. Consider parsing into a local
AVCDecoderConfigurationRecord (e.g. let ParseSPS fill a target record) and
committing to *this only on success, so the invariant holds for every failure
path; otherwise narrow this comment to length_size_minus1 only.
--
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]
