Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871173008:
Fix packetization logic to leave space for extensions in the last packet (Closed)
DescriptionFix packetization logic to leave space for extensions in the last packet
Change packetizer interface to explicitly return number of packets
instead of a last flag. Account for extra space needed in the last
packet.
BUG=webrtc:7588, webrtc:7594
Review-Url: https://codereview.webrtc.org/2871173008
Cr-Commit-Position: refs/heads/master@{#18244}
Committed: https://chromium.googlesource.com/external/webrtc/+/7a3006bae7f1c61371581ddfa320524041119f20
Patch Set 1 #Patch Set 2 : Fix packet buffer allocations bugs and old tests with incorrect assumptions about extensions locati… #
Total comments: 20
Patch Set 3 : Implemented Danilchap@ comments #
Total comments: 42
Patch Set 4 : Fix debug check in rtp_video_sender #Patch Set 5 : Implemented Danilchap@ comments #Patch Set 6 : Implement generic video packetizer unittests #
Total comments: 22
Patch Set 7 : Impelemnted Danilchap@ comments #
Total comments: 14
Patch Set 8 : Impelemented Danilchap@ comments #
Total comments: 26
Patch Set 9 : Implemented danilchap@ comments #
Total comments: 64
Patch Set 10 : Implemented Danilchap@ comments #
Total comments: 8
Patch Set 11 : Implemented Sprang@ and Danilchap@ comments #
Total comments: 30
Patch Set 12 : Implement Sprang@ and Danilchap@ comments #
Total comments: 18
Patch Set 13 : Implemented Danilchap@ comments #
Total comments: 25
Patch Set 14 : Implement Danilchap@ comments #
Total comments: 12
Patch Set 15 : Implement Danilchap@ comments #
Total comments: 22
Patch Set 16 : Impelement Danilchap@ comments #Messages
Total messages: 84 (46 generated)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/24552)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ilnik@webrtc.org changed reviewers: + asapersson@webrtc.org, danilchap@webrtc.org, henrik.lundin@webrtc.org, stefan@webrtc.org
Can you split this CL into serie of smaller CL? (Keeping this CL as reference for the end goal) e.g. 1st CL adjust packetizer interface, but RtpSenderVideo always tell that last packet should be same as previous packets and do not use TotalPackets function. When that reviewed and landed, implement new interface for each packetizer in its own CL. Then start feeding different header pattern for the normal packets and the last packet, stopping using last_packet flag. And Finally update interface again by removing the 'last_packet' flag from the NextPacket function.
On 2017/05/11 17:15:05, danilchap wrote: > Can you split this CL into serie of smaller CL? > (Keeping this CL as reference for the end goal) > e.g. 1st CL adjust packetizer interface, but RtpSenderVideo always tell that > last packet should be same as previous packets and do not use TotalPackets > function. > When that reviewed and landed, implement new interface for each packetizer in > its own CL. > Then start feeding different header pattern for the normal packets and the last > packet, stopping using last_packet flag. > And Finally update interface again by removing the 'last_packet' flag from the > NextPacket function. Wouldn't it be even harder to review? There will be several intermediate states, and everyone will have to check correctness of each of them. There also will be a lot of glue code, added and then removed later. Finally, in this CL you can already review each packetizer independently. Plus there's a trivial change to some tests and little change to common logic (rtp_format interface and rtp_video_receiver.cc) I really don't see merit in that. But, I will do it, if someone else agrees with you. Asa, Stefan, Henrik: would it be easier for you to review multiple CLs like Danil suggested?
Description was changed from ========== Fix packetization logic to leave space for extensions in the last packet Change packetizer interface to explicitly return number of packets instead of a last flag. Account for extra space needed in the last packet. BUG=webrtc:7594 ========== to ========== Fix packetization logic to leave space for extensions in the last packet Change packetizer interface to explicitly return number of packets instead of a last flag. Account for extra space needed in the last packet. BUG=webrtc:7588,webrtc:7594 ==========
https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format.h (right): https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format.h:27: size_t last_packet_extensions_len, Can it be simpler to use last_packet_payload_len instead? https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format.h:43: // Valid only before the first NextPacket() call. may be it will be logical for SetPayloadData to return this value instead? https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:303: std::unique_ptr<RtpPacketToSend> rtp_header_with_extensions( this name makes false impression rtp_header doesn't contain any extensions may be call it last_packet? https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:304: new RtpPacketToSend(*rtp_header)); in base/ptr_util.h there is helper for this scenario of using new: auto last_packet = rtc::MakeUnique<RtpPacketToSend>(*rtp_header); https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:375: bool last = false; is it used outside for loop? https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:376: size_t total_packets = packetizer->TotalPackets(); may be num_packets https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:377: for (size_t current_packet = 0; current_packet < total_packets; may be name iterator to show it is not a packet (specially that there lot's of variables nearby that are), but an index. Simple 'i' might work. https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:387: if (last) { may be instead of branch RTC_DCHECK_LE(packet->size(), packet_capacity);
https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format.h (right): https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format.h:27: size_t last_packet_extensions_len, On 2017/05/12 08:34:40, danilchap wrote: > Can it be simpler to use last_packet_payload_len instead? No, it considerately simplifies logic to know by what size last packet payload is smaller than the rest. https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format.h:43: // Valid only before the first NextPacket() call. On 2017/05/12 08:34:40, danilchap wrote: > may be it will be logical for SetPayloadData to return this value instead? Yes, you are right. I will change the interface to that. https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:303: std::unique_ptr<RtpPacketToSend> rtp_header_with_extensions( On 2017/05/12 08:34:40, danilchap wrote: > this name makes false impression rtp_header doesn't contain any extensions > may be call it last_packet? Done. https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:304: new RtpPacketToSend(*rtp_header)); On 2017/05/12 08:34:40, danilchap wrote: > in base/ptr_util.h there is helper for this scenario of using new: > auto last_packet = rtc::MakeUnique<RtpPacketToSend>(*rtp_header); Done. https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:375: bool last = false; On 2017/05/12 08:34:40, danilchap wrote: > is it used outside for loop? No. Moved inside. https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:376: size_t total_packets = packetizer->TotalPackets(); On 2017/05/12 08:34:40, danilchap wrote: > may be num_packets Done. https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:377: for (size_t current_packet = 0; current_packet < total_packets; On 2017/05/12 08:34:40, danilchap wrote: > may be name iterator to show it is not a packet (specially that there lot's of > variables nearby that are), but an index. > Simple 'i' might work. Done. https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:387: if (last) { On 2017/05/12 08:34:40, danilchap wrote: > may be instead of branch > RTC_DCHECK_LE(packet->size(), packet_capacity); Great idea. Done.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ilnik@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format.h (right): https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format.h:27: size_t last_packet_extensions_len, On 2017/05/12 09:17:57, ilnik wrote: > On 2017/05/12 08:34:40, danilchap wrote: > > Can it be simpler to use last_packet_payload_len instead? > > No, it considerately simplifies logic to know by what size last packet payload > is smaller than the rest. I meant the interface, not the implementation (each implementation can easily convert to format it finds more comfortable) E.g. why packetizer need to know anything about extensions? https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:42: size_t total_data = payload_size_ + last_packet_extension_len_; what this value is? doesn't look like total size of the bytes Packetizer will write to the payload fields. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:47: smaller_packets_ = num_packets_ - total_data % num_packets_; Can you explain why all these arithmetic steps needed? https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:50: assert(payload_per_packet_ <= max_payload_len_); since you are touching this line, change assert into RTC_DCHECK_LE https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:61: // last smaller_packets_ packets are 1 byte smaller than previous packets. Start comment Uppercase: Last smaller... https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:64: // whole payload fit into this packet which line this comment explain? https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:65: size_t current_payload = payload_per_packet_; doesn't look like variable contain payload may be next_packet_payload_size https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:68: // But this is the pre-last packet. Leave at least 1 payload byte for the probably better to remove 'But' and put this comment inside if (num_packets_ == 2) clause https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:72: RTC_DCHECK(last_packet_extension_len_ >= max_payload_len_ / 2); why do you check it? Do you rely on this assumption? RTC_DCHECK_GE if you do. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:73: current_payload -= 1; --maybe https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:80: // Put generic header in packet end comments with a . https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:90: may be RTC_DCHECK_EQ(num_packets_ == 0, payload_size_ == 0); https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:26: class RtpPacketizerGeneric : public RtpPacketizer { It is likely good idea to add some tests, specially since you change logic. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:58: size_t payload_per_packet_; payload_length_per_packet_? https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:60: size_t num_packets_; Likely this member deserve a comment: it is not obvious which direction this variable is changed during calls to NextPacket https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:61: size_t smaller_packets_; can you add comment what this member is for? https://google.github.io/styleguide/cppguide.html#Variable_Comments https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:358: last_packet->headers_size() - rtp_header->headers_size(); be explicit about your assumption last_packet->headers_size() >= rtp_header->headers_size() since you rely on it. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:380: std::unique_ptr<RtpPacketToSend> packet( may be auto packet = last ? std::move(last_packet) : rtc::MakeUnique<RtpPacketToSend>(*rtp_header); (since last_packet will not be used for anything else, no need to create a copy of it) https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:384: RTC_DCHECK_LE(packet->payload_size(), packet->capacity()); sorry for confusion, but packet->capacity() != packet_capacity Invariants like packet->payload_size() <= packet->capacity() and stronger one packet->size() <= packet->capacity() are guaranteed by RtpPacket itself. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:266: // Wait for last packet of the frame. May be better to explain why wait for the last packet. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:267: if (!header.markerBit) while normally early return is better, may be here it will more readable without it, to stress VideoRotation should be checked only on packets with marker bit. May be not, may be comment will be good enough.
https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format.h (right): https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format.h:27: size_t last_packet_extensions_len, On 2017/05/12 13:56:10, danilchap wrote: > On 2017/05/12 09:17:57, ilnik wrote: > > On 2017/05/12 08:34:40, danilchap wrote: > > > Can it be simpler to use last_packet_payload_len instead? > > > > No, it considerately simplifies logic to know by what size last packet payload > > is smaller than the rest. > > I meant the interface, not the implementation (each implementation can easily > convert to format it finds more comfortable) > > E.g. why packetizer need to know anything about extensions? It actually doesn't need to know about extensions. Just the size. I can rename the parameter somehow. Is the last_packet_reduction_len fine?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:42: size_t total_data = payload_size_ + last_packet_extension_len_; On 2017/05/12 13:56:10, danilchap wrote: > what this value is? doesn't look like total size of the bytes Packetizer will > write to the payload fields. It's payload + extra bytes in the last packet. We count them together, so the logic below will make all the packets of almost the same size. If we have 5 payload bytes and 1 byte of extensions in the last packet, and max_payload_len is 5, we can fit everything in 2 packets, and we will get (5+1)/2=3 bytes of payload or extensions in each packet. Thus first packet will have 3 bytes of payload, and second packet only 2. If we make calculations using only payload, we could fit that into 1 packet and then packets would have different sizes. Added comments about this. I though it would be obvious, but it's not. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:47: smaller_packets_ = num_packets_ - total_data % num_packets_; On 2017/05/12 13:56:10, danilchap wrote: > Can you explain why all these arithmetic steps needed? Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:50: assert(payload_per_packet_ <= max_payload_len_); On 2017/05/12 13:56:10, danilchap wrote: > since you are touching this line, change assert into RTC_DCHECK_LE Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:61: // last smaller_packets_ packets are 1 byte smaller than previous packets. On 2017/05/12 13:56:10, danilchap wrote: > Start comment Uppercase: > Last smaller... Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:64: // whole payload fit into this packet On 2017/05/12 13:56:10, danilchap wrote: > which line this comment explain? if() line below. Moved the comment. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:65: size_t current_payload = payload_per_packet_; On 2017/05/12 13:56:10, danilchap wrote: > doesn't look like variable contain payload > may be > next_packet_payload_size Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:68: // But this is the pre-last packet. Leave at least 1 payload byte for the On 2017/05/12 13:56:10, danilchap wrote: > probably better to remove 'But' and put this comment inside if (num_packets_ == > 2) clause Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:72: RTC_DCHECK(last_packet_extension_len_ >= max_payload_len_ / 2); On 2017/05/12 13:56:10, danilchap wrote: > why do you check it? Do you rely on this assumption? > RTC_DCHECK_GE if you do. This is sanity checking what it works as expected. This is really not best situation then we have to move 1 byte to the next packet - it messes size of two last packets. From my calculations it happens only in extreme cases where extensions take half of all capacity. It's not needed now, removed it. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:73: current_payload -= 1; On 2017/05/12 13:56:10, danilchap wrote: > --maybe Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:80: // Put generic header in packet On 2017/05/12 13:56:10, danilchap wrote: > end comments with a . Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:90: On 2017/05/12 13:56:10, danilchap wrote: > may be > RTC_DCHECK_EQ(num_packets_ == 0, payload_size_ == 0); Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:26: class RtpPacketizerGeneric : public RtpPacketizer { On 2017/05/12 13:56:10, danilchap wrote: > It is likely good idea to add some tests, > specially since you change logic. Yes, will add them later. However, this generic packetizer is used in a lot of video engine tests, so it's already covered quite well. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:58: size_t payload_per_packet_; On 2017/05/12 13:56:10, danilchap wrote: > payload_length_per_packet_? Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:60: size_t num_packets_; On 2017/05/12 13:56:10, danilchap wrote: > Likely this member deserve a comment: > it is not obvious which direction this variable is changed during calls to > NextPacket Changed to num_packets_left_. Now it's clear that it may be decreased. Still, added a comment. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:61: size_t smaller_packets_; On 2017/05/12 13:56:10, danilchap wrote: > can you add comment what this member is for? > https://google.github.io/styleguide/cppguide.html#Variable_Comments Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:358: last_packet->headers_size() - rtp_header->headers_size(); On 2017/05/12 13:56:10, danilchap wrote: > be explicit about your assumption > last_packet->headers_size() >= rtp_header->headers_size() > since you rely on it. Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:380: std::unique_ptr<RtpPacketToSend> packet( On 2017/05/12 13:56:10, danilchap wrote: > may be > auto packet = last ? std::move(last_packet) : > rtc::MakeUnique<RtpPacketToSend>(*rtp_header); > (since last_packet will not be used for anything else, no need to create a copy > of it) Done. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:384: RTC_DCHECK_LE(packet->payload_size(), packet->capacity()); On 2017/05/12 13:56:10, danilchap wrote: > sorry for confusion, but > packet->capacity() != packet_capacity > > Invariants like > packet->payload_size() <= packet->capacity() > and stronger one > packet->size() <= packet->capacity() > are guaranteed by RtpPacket itself. Done here something better now. It may be redundant, but it will help to show our assumptions. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:266: // Wait for last packet of the frame. On 2017/05/12 13:56:11, danilchap wrote: > May be better to explain why wait for the last packet. Fixed. Now comment is more informative. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:267: if (!header.markerBit) On 2017/05/12 13:56:11, danilchap wrote: > while normally early return is better, may be here it will more readable without > it, to stress VideoRotation should be checked only on packets with marker bit. > May be not, may be comment will be good enough. It can be on the packet with not marker bit (if it's the last packet of lower spatial layer).
https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format.h (right): https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format.h:27: size_t last_packet_extensions_len, On 2017/05/12 13:59:20, ilnik wrote: > On 2017/05/12 13:56:10, danilchap wrote: > > On 2017/05/12 09:17:57, ilnik wrote: > > > On 2017/05/12 08:34:40, danilchap wrote: > > > > Can it be simpler to use last_packet_payload_len instead? > > > > > > No, it considerately simplifies logic to know by what size last packet > payload > > > is smaller than the rest. > > > > I meant the interface, not the implementation (each implementation can easily > > convert to format it finds more comfortable) > > > > E.g. why packetizer need to know anything about extensions? > > It actually doesn't need to know about extensions. Just the size. I can rename > the parameter somehow. Is the last_packet_reduction_len fine? Found reason why you want this variable in the interface: when Packetizer make packets similar size, it should aim at semi-equal total packet size, not payload size. On the other side one may argue that it should try to make 'free space' semi-equal across packets. From that point of view last_packet_payload_len and last_packet_reduction_len are equally ok values to provide. But name a bit misleading because value counts only additional extensions. last_packet_reduction_len is better. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:267: if (!header.markerBit) On 2017/05/12 14:46:08, ilnik wrote: > On 2017/05/12 13:56:11, danilchap wrote: > > while normally early return is better, may be here it will more readable > without > > it, to stress VideoRotation should be checked only on packets with marker bit. > > May be not, may be comment will be good enough. > > It can be on the packet with not marker bit (if it's the last packet of lower > spatial layer). It might be in any packet (like before your CL), but must be only in packets with marker bit. Comment looks good (may be just add 'the' before extension) https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:31: initialize num_smaller_packets_ member too https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:49: size_t total_data = payload_size_ + last_packet_extension_len_; may be do not name variable 'data' when it represent size/length. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:57: // first packets: 14 bytes in 4 packets would be splitted as 4+4+3+3. did you consider 'small packets first, then larger packets'? https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:24: namespace RtpFormatVideoGeneric { do not use this namespace https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:37: std::vector<size_t> expected_sizes) { Likely it is more readable when helpers do not contain test logic. E.g. std::vector<size_t> NextPacketFillPayloadSizes(RtpPacketizerGeneric* packetizer) { RtpPacketToSend packet(nullptr); std::vector<size_t> result; while (packetizer->NextPacket(&packet)) { result.push_back(packet.payload_size()); } return result; } TEST(SuiteName, PacketizerRespectMaxPayloadSize) { const size_t max_payload_length = 5; const size_t last_packet_reduction_len = 2; RtpPacketizerGeneric packetizer(kVideoFrameKey, max_payload_length, last_packet_reduction_len); size_t num_packets = packetizer.SetPayloadData(kTestPayload, /*payload_size=*/12, nullptr); auto payload_sizes = NextPacketFillPayloadSizes(&packetizer); EXPECT_THAT(payload_sizes, SizeIs(num_packets)); EXPECT_THAT(payload_sizes, Each(Le(max_payload_lenght))); EXPECT_LE(payload_sizes.back(), max_payload_length - last_packet_reduction_len); // Do not test exact sizes, it would test implementation instead of guarantees Packetizer should provide. // Both those outputs should be accepted by the tests: // EXPECT_THAT(payload_sizes, ElementsAre(5, 5, 4, 2)); // EXPECT_THAT(payload_sizes, ElementsAre(4, 4, 5, 3)); } https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:50: RtpPacketToSend packet(kNoExtensions); alternative ways are ... packet(nullptr /* extensions */); ... packet(/*extensions=*/nullptr); Choose any of those 3 ways to your taste. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:358: size_t last_packet_extensions_len = this variable name make false impression there are no extensions in the rtp_header. wouldn't mind your suggestion from the interface: last_packet_reduction_len https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:374: what happen if num_packets is zero? https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:383: packet->capacity() - packet->headers_size()); do not use packet->capacity() here, it has different meaning (it is buffer capacity, while packet_capacity is mtu - transport headers - rtx/fec overhead) feel free to rename variable packet_capacity to max_rtp_packet_size or similar to avoid confusion. But you may use fact packet->header_size() + packet->payload_size() == packet->size(). (technically header_size + payload_size + padding_size == size, but padding_size always zero here and can be ignored)
https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_format.h (right): https://codereview.webrtc.org/2871173008/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_format.h:27: size_t last_packet_extensions_len, On 2017/05/12 18:56:15, danilchap wrote: > On 2017/05/12 13:59:20, ilnik wrote: > > On 2017/05/12 13:56:10, danilchap wrote: > > > On 2017/05/12 09:17:57, ilnik wrote: > > > > On 2017/05/12 08:34:40, danilchap wrote: > > > > > Can it be simpler to use last_packet_payload_len instead? > > > > > > > > No, it considerately simplifies logic to know by what size last packet > > payload > > > > is smaller than the rest. > > > > > > I meant the interface, not the implementation (each implementation can > easily > > > convert to format it finds more comfortable) > > > > > > E.g. why packetizer need to know anything about extensions? > > > > It actually doesn't need to know about extensions. Just the size. I can rename > > the parameter somehow. Is the last_packet_reduction_len fine? > > Found reason why you want this variable in the interface: when Packetizer make > packets similar size, it should aim at semi-equal total packet size, not payload > size. > On the other side one may argue that it should try to make 'free space' > semi-equal across packets. From that point of view last_packet_payload_len and > last_packet_reduction_len are equally ok values to provide. > > But name a bit misleading because value counts only additional extensions. > last_packet_reduction_len is better. Yes, I agree. https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2871173008/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:267: if (!header.markerBit) On 2017/05/12 18:56:15, danilchap wrote: > On 2017/05/12 14:46:08, ilnik wrote: > > On 2017/05/12 13:56:11, danilchap wrote: > > > while normally early return is better, may be here it will more readable > > without > > > it, to stress VideoRotation should be checked only on packets with marker > bit. > > > May be not, may be comment will be good enough. > > > > It can be on the packet with not marker bit (if it's the last packet of lower > > spatial layer). > > It might be in any packet (like before your CL), but must be only in packets > with marker bit. > Comment looks good (may be just add 'the' before extension) Done. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:31: On 2017/05/12 18:56:15, danilchap wrote: > initialize num_smaller_packets_ member too Done. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:49: size_t total_data = payload_size_ + last_packet_extension_len_; On 2017/05/12 18:56:15, danilchap wrote: > may be do not name variable 'data' when it represent size/length. I can't think of any better name. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:57: // first packets: 14 bytes in 4 packets would be splitted as 4+4+3+3. On 2017/05/12 18:56:15, danilchap wrote: > did you consider 'small packets first, then larger packets'? It's just one byte of difference. Doesn't make much impact either way. But using larger packets first is more clear, since we already have correct length computed. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:24: namespace RtpFormatVideoGeneric { On 2017/05/12 18:56:15, danilchap wrote: > do not use this namespace Done. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:37: std::vector<size_t> expected_sizes) { On 2017/05/12 18:56:15, danilchap wrote: > Likely it is more readable when helpers do not contain test logic. > E.g. > std::vector<size_t> NextPacketFillPayloadSizes(RtpPacketizerGeneric* packetizer) > { > RtpPacketToSend packet(nullptr); > std::vector<size_t> result; > while (packetizer->NextPacket(&packet)) { > result.push_back(packet.payload_size()); > } > return result; > } > > > TEST(SuiteName, PacketizerRespectMaxPayloadSize) { > const size_t max_payload_length = 5; > const size_t last_packet_reduction_len = 2; > RtpPacketizerGeneric packetizer(kVideoFrameKey, max_payload_length, > last_packet_reduction_len); > size_t num_packets = packetizer.SetPayloadData(kTestPayload, > /*payload_size=*/12, nullptr); > > auto payload_sizes = NextPacketFillPayloadSizes(&packetizer); > > EXPECT_THAT(payload_sizes, SizeIs(num_packets)); > EXPECT_THAT(payload_sizes, Each(Le(max_payload_lenght))); > EXPECT_LE(payload_sizes.back(), max_payload_length - > last_packet_reduction_len); > // Do not test exact sizes, it would test implementation instead of guarantees > Packetizer should provide. > // Both those outputs should be accepted by the tests: > // EXPECT_THAT(payload_sizes, ElementsAre(5, 5, 4, 2)); > // EXPECT_THAT(payload_sizes, ElementsAre(4, 4, 5, 3)); > } Done. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:50: RtpPacketToSend packet(kNoExtensions); On 2017/05/12 18:56:15, danilchap wrote: > alternative ways are > ... packet(nullptr /* extensions */); > ... packet(/*extensions=*/nullptr); > Choose any of those 3 ways to your taste. Done. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:358: size_t last_packet_extensions_len = On 2017/05/12 18:56:15, danilchap wrote: > this variable name make false impression there are no extensions in the > rtp_header. > wouldn't mind your suggestion from the interface: > last_packet_reduction_len Done. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:374: On 2017/05/12 18:56:15, danilchap wrote: > what happen if num_packets is zero? Like a previous implementation, we should return false. Done now. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:383: packet->capacity() - packet->headers_size()); On 2017/05/12 18:56:15, danilchap wrote: > do not use packet->capacity() here, it has different meaning (it is buffer > capacity, while packet_capacity is mtu - transport headers - rtx/fec overhead) > feel free to rename variable packet_capacity to max_rtp_packet_size or similar > to avoid confusion. > > But you may use fact > packet->header_size() + packet->payload_size() == packet->size(). > (technically header_size + payload_size + padding_size == size, but padding_size > always zero here and can be ignored) Redone this check now.
Looking better every round! https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:49: size_t total_data = payload_size_ + last_packet_extension_len_; On 2017/05/15 09:38:33, ilnik wrote: > On 2017/05/12 18:56:15, danilchap wrote: > > may be do not name variable 'data' when it represent size/length. > > I can't think of any better name. I think total_size/total_length/total_len/total_bytes are better: variable with '_data' sufix (e.g. payload_data) point to actual data. Comment above suggest you may consider virtual_payload_size/virtual_payload_len name https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:57: // first packets: 14 bytes in 4 packets would be splitted as 4+4+3+3. On 2017/05/15 09:38:33, ilnik wrote: > On 2017/05/12 18:56:15, danilchap wrote: > > did you consider 'small packets first, then larger packets'? > > It's just one byte of difference. Doesn't make much impact either way. But using > larger packets first is more clear, since we already have correct length > computed. function-wise it is sure the same. It just looked to me that some arithmetic will be noticeably simpler with smaller packets first. e.g.: payload_len_per_packet_ = total_size / num_packets_left_; // smaller size. num_larger_packets_ = total_size % num_packets_left_; // no extra if. But if that increase complexity in other parts making total complexity about the same, then no point to switch it. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:43: /* Fragment packets such that they are almost the same size, even accounting looking through few neighbor files, it seems multi-line comments are written using several single-line comments. So for local consistency might be good to write new comments that style too. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:47: size_t GetEffectivePacketsSizeDifference(std::vector<size_t> payload_sizes, may be const std::vector<size_t>& or rtc::ArrayView<const size_t> (i.e. do not pass std::vector<size_t> by value unless you save it. rtc::ArrayView though should be pass by value and can often be used instead of const vector& parameter. Though in this case, passing const vector& is perfectly fine too - it is an internal function with very limited use) https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:52: for (size_t i = 0; i + 1 < payload_sizes.size(); ++i) { may be use c++ for loop: for (size_t payload_size : payload_sizes) or std::minmax_element: auto minmax = std::minmax_element(payload_sizes.begin(), payload_sized.end()); return *minmax.second - *minmax.first; [though these approaches would require incrementing last packet payload size before passing to this function] https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:72: EXPECT_THAT(payload_sizes, SizeIs(num_packets)); this check probably do not belong to this test, but should be in a 'GenerateMinimumNumberOfPackets' tests https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:79: EXPECT_LE(sizes_difference, 1u); this check probably better to move to 'GeneratePacketsOfAlmostEqualSize' https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:82: TEST(RtpPacketizerVideoGeneric, RespectsMaxPayloadSize2) { instead of 1/2, can you name tests describing what scenario is tested? This is 'AllPacketsCanBeEqualSize' scenario? https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:103: TEST(RtpPacketizerVideoGeneric, RespectsLastPacketReductionLength1) { It is good that you check it in a separate test, but then remove explicit check for last packet in previous tests, and remove others checks from this test: Make it only test what it advertise it checks: that is easier to read.
https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:49: size_t total_data = payload_size_ + last_packet_extension_len_; On 2017/05/15 11:03:17, danilchap wrote: > On 2017/05/15 09:38:33, ilnik wrote: > > On 2017/05/12 18:56:15, danilchap wrote: > > > may be do not name variable 'data' when it represent size/length. > > > > I can't think of any better name. > > I think total_size/total_length/total_len/total_bytes are better: > variable with '_data' sufix (e.g. payload_data) point to actual data. > > Comment above suggest you may consider > virtual_payload_size/virtual_payload_len name Done. https://codereview.webrtc.org/2871173008/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:57: // first packets: 14 bytes in 4 packets would be splitted as 4+4+3+3. On 2017/05/15 11:03:17, danilchap wrote: > On 2017/05/15 09:38:33, ilnik wrote: > > On 2017/05/12 18:56:15, danilchap wrote: > > > did you consider 'small packets first, then larger packets'? > > > > It's just one byte of difference. Doesn't make much impact either way. But > using > > larger packets first is more clear, since we already have correct length > > computed. > > function-wise it is sure the same. > It just looked to me that some arithmetic will be noticeably simpler with > smaller packets first. > e.g.: > payload_len_per_packet_ = total_size / num_packets_left_; // smaller size. > num_larger_packets_ = total_size % num_packets_left_; // no extra if. > > But if that increase complexity in other parts making total complexity about the > same, then no point to switch it. Acknowledged. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:43: /* Fragment packets such that they are almost the same size, even accounting On 2017/05/15 11:03:17, danilchap wrote: > looking through few neighbor files, it seems multi-line comments are written > using several single-line comments. > So for local consistency might be good to write new comments that style too. Done. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:47: size_t GetEffectivePacketsSizeDifference(std::vector<size_t> payload_sizes, On 2017/05/15 11:03:18, danilchap wrote: > may be > const std::vector<size_t>& or > rtc::ArrayView<const size_t> > (i.e. do not pass std::vector<size_t> by value unless you save it. > rtc::ArrayView though should be pass by value and can often be used instead of > const vector& parameter. > Though in this case, passing const vector& is perfectly fine too - it is an > internal function with very limited use) It should have been const reference. Totally a typo. Thanks for catching that. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:52: for (size_t i = 0; i + 1 < payload_sizes.size(); ++i) { On 2017/05/15 11:03:18, danilchap wrote: > may be use c++ for loop: > for (size_t payload_size : payload_sizes) > or std::minmax_element: > auto minmax = std::minmax_element(payload_sizes.begin(), payload_sized.end()); > return *minmax.second - *minmax.first; > [though these approaches would require incrementing last packet payload size > before passing to this function] Done. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:72: EXPECT_THAT(payload_sizes, SizeIs(num_packets)); On 2017/05/15 11:03:17, danilchap wrote: > this check probably do not belong to this test, but should be in a > 'GenerateMinimumNumberOfPackets' tests Done. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:79: EXPECT_LE(sizes_difference, 1u); On 2017/05/15 11:03:17, danilchap wrote: > this check probably better to move to 'GeneratePacketsOfAlmostEqualSize' Done. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:82: TEST(RtpPacketizerVideoGeneric, RespectsMaxPayloadSize2) { On 2017/05/15 11:03:17, danilchap wrote: > instead of 1/2, can you name tests describing what scenario is tested? > This is 'AllPacketsCanBeEqualSize' scenario? Done. https://codereview.webrtc.org/2871173008/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:103: TEST(RtpPacketizerVideoGeneric, RespectsLastPacketReductionLength1) { On 2017/05/15 11:03:18, danilchap wrote: > It is good that you check it in a separate test, > but then remove explicit check for last packet in previous tests, and remove > others checks from this test: Make it only test what it advertise it checks: > that is easier to read. Done.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:54: auto difference = *minmax.second - *minmax.first; using size_t here instead of auto likely better. https://google.github.io/styleguide/cppguide.html#auto https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:114: const size_t min_num_packets = 4; // Computed by hand. may be extend the comment how it was computed: min_num_packets = roundup((payload_size + reduction_len) / (payload_per_packet - generic_header_size = 1)) https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:510: LOG(LS_ERROR) << "Payload header and one payload byte won't fit in packet."; may be 'in the first packet' (since only 1st packet includes SS data) https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:520: // Instead of making last packet smaller, we pretend that we must write may be also mention 1st packet that has extra SS header since you add both last_packet_extra_len and first_packet_extra_len to the total_bytes https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:521: // additional data into it. We account for this virtual payload while single space between 'this' and 'virtual' https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:526: payload_size_ + last_packet_reduction_len_ + SsDataLength(hdr_); nit: may be reorder terms in the order closer to how they appear on the wire: SSdataLength() + payload_size_ + last_packet_reduction_len https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:547: packet_bytes -= SsDataLength(hdr_); can packet_bytes become negative? https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:555: packet_bytes--; can packet_bytes be zero before decrement? https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h:48: // The payload data must be one encoded VP9 frame. may be extend comment 'VP9 layer frame' to avoid confusion with super frame https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:450: RtpPacketizerVp9 packetizer0(vp9_header, kPacketSize, 0); what is zero? can you name the constant or you consider it irrelevant for this test? https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:452: packetizer0.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation), 2u); can you name the constant 2, preferably with comment why it should be 2 since it is not obvious looking at the neighbor lines. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:728: Add test(s) where last_packet_reduction_len > 0
https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:54: auto difference = *minmax.second - *minmax.first; On 2017/05/16 10:25:11, danilchap wrote: > using size_t here instead of auto likely better. > https://google.github.io/styleguide/cppguide.html#auto Done. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:114: const size_t min_num_packets = 4; // Computed by hand. On 2017/05/16 10:25:11, danilchap wrote: > may be extend the comment how it was computed: > min_num_packets = roundup((payload_size + reduction_len) / (payload_per_packet - > generic_header_size = 1)) Now there's a comment about how exactly it was calculated. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:510: LOG(LS_ERROR) << "Payload header and one payload byte won't fit in packet."; On 2017/05/16 10:25:11, danilchap wrote: > may be 'in the first packet' > (since only 1st packet includes SS data) Done. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:520: // Instead of making last packet smaller, we pretend that we must write On 2017/05/16 10:25:11, danilchap wrote: > may be also mention 1st packet that has extra SS header since you add both > last_packet_extra_len and first_packet_extra_len to the total_bytes Done. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:521: // additional data into it. We account for this virtual payload while On 2017/05/16 10:25:11, danilchap wrote: > single space between 'this' and 'virtual' Done. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:526: payload_size_ + last_packet_reduction_len_ + SsDataLength(hdr_); On 2017/05/16 10:25:11, danilchap wrote: > nit: may be reorder terms in the order closer to how they appear on the wire: > SSdataLength() + payload_size_ + last_packet_reduction_len Done. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:547: packet_bytes -= SsDataLength(hdr_); On 2017/05/16 10:25:11, danilchap wrote: > can packet_bytes become negative? Actually, in some extreme cases, it can. The fix for that is done now. It could happen only if SS data is at least half of the packet, i.e. ~700 bytes. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:555: packet_bytes--; On 2017/05/16 10:25:11, danilchap wrote: > can packet_bytes be zero before decrement? Now it can't due to the fix above. There's still a special case then the whole frame is just 1 byte and it doesn't fit in one packet due to extensions length. Here we will make the first packet with 0 length payload and second packet with 1 byte of payload. But we can't do anything here. Note, in that case previous implementation would always fail to generate packets. I've added that assertion at the beginning now. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h:48: // The payload data must be one encoded VP9 frame. On 2017/05/16 10:25:11, danilchap wrote: > may be extend comment 'VP9 layer frame' > to avoid confusion with super frame Done. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:450: RtpPacketizerVp9 packetizer0(vp9_header, kPacketSize, 0); On 2017/05/16 10:25:12, danilchap wrote: > what is zero? can you name the constant or you consider it irrelevant for this > test? Done. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:452: packetizer0.SetPayloadData(kFrame, sizeof(kFrame), kNoFragmentation), 2u); On 2017/05/16 10:25:12, danilchap wrote: > can you name the constant 2, preferably with comment why it should be 2 since it > is not obvious looking at the neighbor lines. Done. https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:728: On 2017/05/16 10:25:12, danilchap wrote: > Add test(s) where last_packet_reduction_len > 0 Done.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:547: packet_bytes -= SsDataLength(hdr_); On 2017/05/16 12:24:08, ilnik wrote: > On 2017/05/16 10:25:11, danilchap wrote: > > can packet_bytes become negative? > > Actually, in some extreme cases, it can. The fix for that is done now. > It could happen only if SS data is at least half of the packet, i.e. ~700 bytes. Can you also add a test for this scenario. (btw, packet_bytes is unsigned, so 3 - 5 = 4294967294 on 32bit system) https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:539: size_t capacity = may be per_packet_capacity https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:543: size_t per_packet_data = (total_bytes + num_packets - 1) / num_packets; per_packet_bytes https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:546: size_t smaller_packets = num_packets - total_bytes % num_packets; may be num_smaller_packets https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:551: size_t packets_left = num_packets; num_packets_left https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:151: packetizer_->SetPayloadData(payload_.get(), payload_size_, NULL); since you are touching this line anyway, replace NULL with nullptr https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:440: const size_t kExtensionsLen = 0; prefer kLastPacketReudctionLen name to avoid false impression it is size of all header extensions of the last packet. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:441: // Calculated by hand. Headers are 2 bytes. One packet would be 2+10 bytes may be mention which headers (since there are so many different headers when composing an rtp packet): Without optional fields VP9 payload descriptor is 2 bytes. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:443: // capacity of 2*(8-2)=12 bytes. May be: One packet can contain |kPacketSize| - |kVp9MinDiscriptorSize| = 6 bytes of the frame payload, thus to fit 10 bytes two packets are required. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:459: kMinNumberOfPackets); may be rather than hijacking test designed to check something else, add a new one, with name matching expectations. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:501: vp9_header.num_spatial_layers = 3; is number of spatial layers (or presence of this field) relevant to this test?
https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:547: packet_bytes -= SsDataLength(hdr_); On 2017/05/17 10:31:19, danilchap wrote: > On 2017/05/16 12:24:08, ilnik wrote: > > On 2017/05/16 10:25:11, danilchap wrote: > > > can packet_bytes become negative? > > > > Actually, in some extreme cases, it can. The fix for that is done now. > > It could happen only if SS data is at least half of the packet, i.e. ~700 > bytes. > > Can you also add a test for this scenario. > (btw, packet_bytes is unsigned, so 3 - 5 = 4294967294 on 32bit system) Yep, my mistake about unsignedness. Thanks for catching that. Also added a test for that. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:539: size_t capacity = On 2017/05/17 10:31:19, danilchap wrote: > may be per_packet_capacity Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:543: size_t per_packet_data = (total_bytes + num_packets - 1) / num_packets; On 2017/05/17 10:31:19, danilchap wrote: > per_packet_bytes Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:546: size_t smaller_packets = num_packets - total_bytes % num_packets; On 2017/05/17 10:31:19, danilchap wrote: > may be num_smaller_packets Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:551: size_t packets_left = num_packets; On 2017/05/17 10:31:19, danilchap wrote: > num_packets_left Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:151: packetizer_->SetPayloadData(payload_.get(), payload_size_, NULL); On 2017/05/17 10:31:20, danilchap wrote: > since you are touching this line anyway, replace NULL with nullptr Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:440: const size_t kExtensionsLen = 0; On 2017/05/17 10:31:20, danilchap wrote: > prefer kLastPacketReudctionLen name > to avoid false impression it is size of all header extensions of the last > packet. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:441: // Calculated by hand. Headers are 2 bytes. One packet would be 2+10 bytes On 2017/05/17 10:31:20, danilchap wrote: > may be mention which headers (since there are so many different headers when > composing an rtp packet): > Without optional fields VP9 payload descriptor is 2 bytes. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:443: // capacity of 2*(8-2)=12 bytes. On 2017/05/17 10:31:20, danilchap wrote: > May be: > One packet can contain |kPacketSize| - |kVp9MinDiscriptorSize| = 6 bytes of the > frame payload, thus to fit 10 bytes two packets are required. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:459: kMinNumberOfPackets); On 2017/05/17 10:31:20, danilchap wrote: > may be rather than hijacking test designed to check something else, add a new > one, with name matching expectations. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:501: vp9_header.num_spatial_layers = 3; On 2017/05/17 10:31:20, danilchap wrote: > is number of spatial layers (or presence of this field) relevant to this test? No, it's from copy-paste. Removed.
https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:547: packet_bytes -= SsDataLength(hdr_); On 2017/05/17 10:31:19, danilchap wrote: > On 2017/05/16 12:24:08, ilnik wrote: > > On 2017/05/16 10:25:11, danilchap wrote: > > > can packet_bytes become negative? > > > > Actually, in some extreme cases, it can. The fix for that is done now. > > It could happen only if SS data is at least half of the packet, i.e. ~700 > bytes. > > Can you also add a test for this scenario. > (btw, packet_bytes is unsigned, so 3 - 5 = 4294967294 on 32bit system) Yep, my mistake about unsignedness. Thanks for catching that. Also added a test for that. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:539: size_t capacity = On 2017/05/17 10:31:19, danilchap wrote: > may be per_packet_capacity Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:543: size_t per_packet_data = (total_bytes + num_packets - 1) / num_packets; On 2017/05/17 10:31:19, danilchap wrote: > per_packet_bytes Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:546: size_t smaller_packets = num_packets - total_bytes % num_packets; On 2017/05/17 10:31:19, danilchap wrote: > may be num_smaller_packets Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:551: size_t packets_left = num_packets; On 2017/05/17 10:31:19, danilchap wrote: > num_packets_left Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:151: packetizer_->SetPayloadData(payload_.get(), payload_size_, NULL); On 2017/05/17 10:31:20, danilchap wrote: > since you are touching this line anyway, replace NULL with nullptr Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:440: const size_t kExtensionsLen = 0; On 2017/05/17 10:31:20, danilchap wrote: > prefer kLastPacketReudctionLen name > to avoid false impression it is size of all header extensions of the last > packet. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:441: // Calculated by hand. Headers are 2 bytes. One packet would be 2+10 bytes On 2017/05/17 10:31:20, danilchap wrote: > may be mention which headers (since there are so many different headers when > composing an rtp packet): > Without optional fields VP9 payload descriptor is 2 bytes. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:443: // capacity of 2*(8-2)=12 bytes. On 2017/05/17 10:31:20, danilchap wrote: > May be: > One packet can contain |kPacketSize| - |kVp9MinDiscriptorSize| = 6 bytes of the > frame payload, thus to fit 10 bytes two packets are required. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:459: kMinNumberOfPackets); On 2017/05/17 10:31:20, danilchap wrote: > may be rather than hijacking test designed to check something else, add a new > one, with name matching expectations. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:501: vp9_header.num_spatial_layers = 3; On 2017/05/17 10:31:20, danilchap wrote: > is number of spatial layers (or presence of this field) relevant to this test? No, it's from copy-paste. Removed.
sprang@webrtc.org changed reviewers: + sprang@webrtc.org
https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format.cc:45: return NULL; Would be great if you cleaned this file up with s/NULL/nullptr while you're here :) https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:90: packetization_mode == H264PacketizationMode::SingleNalUnit); quite an edge case, but maybe assert that max_payload_len > last_packet_reduction_len https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:184: max_payload_len_))) { nit: Maybe you can clarify this a bit by having a temporary length variable, I'm thinking something like size_t fragment_length = input_fragments_[i].length; if (i >= input_fragments_.size() - 1) { // Optionally comment this. fragment_length += last_packet_reduction_len_; } if (fragment_length > max_payload_len_) { fua } else { stap } wdyt? https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:215: (payload_left + extra_len + num_packets - 1) / num_packets; nit: maybe (num_packets - 1) for consistency with bytes_available above https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:219: num_packets - (payload_left + extra_len) % num_packets; If you use num_larger_packets instead you can get rid of the if statement below, and if you decrement num_larger_packets in the loop you can use that to determine if packet_length should be payload_per_packet + 1. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:247: total_packets_++; nit: just do total_packet_ += num_packets before the loop https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:248: num_packets--; nit: prefer pre-increments (here and otherwise) https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:115: expected_sizes.size()); nit: For EXPECT_EQ and ASSERT_EQ below, prefer having the expected value as the first argument, as that's what the error message is gonna say if the test fails. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:65: num_smaller_packets_ = 0; Same as commented for h264, num_larger_packets_left_ would probably be cleaner? https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:85: // Whole payload fits into this packet nit: end comment with period. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:88: // This is the pre-last packet. Leave at least 1 payload byte for the nit: s/pre-last/penultimate https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:65: const size_t payload_size = 13; nit: for constants use kCamelCase style https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:70: auto payload_sizes = NextPacketFillPayloadSizes(&packetizer); nit: for simpler types, prefer explicit type rather than auto auto is better for iterator types and lambdas etc that are very verbose or difficult type explicitly https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:319: last_packet_reduction_len_; Please comment that this is only used during calculations here, and is undone before returning. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:321: RTC_CHECK(part_info_.fragmentationLength[part_ix] <= capacity); nit: RTC_CHECK_LE https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:327: size_t total_partitions = end_part - part_ix; This confused me at first. Could you please comment that this is a precondition check necessary for the aggregation below to work, not a check if the next |total_partitions| fragments will fit in |capacity| bytes... https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:336: std::vector<std::pair<size_t, size_t> > score( nit: it's allowed to write >> in the type nowadays https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:121: size_t AgregatePartitions(size_t part_ix, size_t capacity); nit: AggregatePartitions https://codereview.webrtc.org/2871173008/diff/160001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:826: bool with_fec) { Maybe would be a good idea to add optional last-packet extension header to this test?
https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:115: expected_sizes.size()); On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: For EXPECT_EQ and ASSERT_EQ below, prefer having the expected value as the > first argument, as that's what the error message is gonna say if the test fails. Actually error message treat both parameters equally these days. So recommendation shifting other way around for consistency with regular == https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:559: // Must write at least one byte of the real payload to the packet. btw, why? or rather 'which payload is real'? (same as headers - there are lot of different headers in an rtp packet and thus different parts can be called payload). Here it might mean rtp_payload [including descriptor header] or codec payload - the one provided in payload_ variable [excluding rtp descriptor] https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:560: if (packet_bytes > SsDataLength(hdr_)) { is SSDataLength cheap function? may be cache it's result. https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:480: const size_t kExpectedNum = GTEST_ARRAY_SIZE_(kExpectedSizes); is it different from arraysize macro from base/arraysize.h ? https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:546: EXPECT_FALSE(packet.Marker()); why testing Marker bit?
https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format.cc:45: return NULL; On 2017/05/17 13:10:06, sprang_webrtc wrote: > Would be great if you cleaned this file up with s/NULL/nullptr while you're here > :) Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:90: packetization_mode == H264PacketizationMode::SingleNalUnit); On 2017/05/17 13:10:07, sprang_webrtc wrote: > quite an edge case, but maybe assert that max_payload_len > > last_packet_reduction_len Sure. However, in that case the old implementation would also fail. Added just in case. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:184: max_payload_len_))) { On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: Maybe you can clarify this a bit by having a temporary length variable, I'm > thinking something like > > size_t fragment_length = input_fragments_[i].length; > if (i >= input_fragments_.size() - 1) { > // Optionally comment this. > fragment_length += last_packet_reduction_len_; > } > if (fragment_length > max_payload_len_) { > fua > } else { > stap > } > > wdyt? Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:215: (payload_left + extra_len + num_packets - 1) / num_packets; On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: maybe (num_packets - 1) for consistency with bytes_available above Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:219: num_packets - (payload_left + extra_len) % num_packets; On 2017/05/17 13:10:06, sprang_webrtc wrote: > If you use num_larger_packets instead you can get rid of the if statement below, > and if you decrement num_larger_packets in the loop you can use that to > determine if packet_length should be payload_per_packet + 1. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:247: total_packets_++; On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: just do total_packet_ += num_packets before the loop Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:248: num_packets--; On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: prefer pre-increments (here and otherwise) Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:115: expected_sizes.size()); On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: For EXPECT_EQ and ASSERT_EQ below, prefer having the expected value as the > first argument, as that's what the error message is gonna say if the test fails. Actually, it's true that expected value is the first in the error message. At least I've seen it. And, once I paid attention to that detail, in all places around, expected value is the first. Done it in the same way here for consistency. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:65: num_smaller_packets_ = 0; On 2017/05/17 13:10:07, sprang_webrtc wrote: > Same as commented for h264, num_larger_packets_left_ would probably be cleaner? Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:85: // Whole payload fits into this packet On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: end comment with period. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:88: // This is the pre-last packet. Leave at least 1 payload byte for the On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: s/pre-last/penultimate Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:65: const size_t payload_size = 13; On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: for constants use kCamelCase style Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:70: auto payload_sizes = NextPacketFillPayloadSizes(&packetizer); On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: for simpler types, prefer explicit type rather than auto > auto is better for iterator types and lambdas etc that are very verbose or > difficult type explicitly Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:319: last_packet_reduction_len_; On 2017/05/17 13:10:07, sprang_webrtc wrote: > Please comment that this is only used during calculations here, and is undone > before returning. Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:321: RTC_CHECK(part_info_.fragmentationLength[part_ix] <= capacity); On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: RTC_CHECK_LE Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:327: size_t total_partitions = end_part - part_ix; On 2017/05/17 13:10:07, sprang_webrtc wrote: > This confused me at first. Could you please comment that this is a precondition > check necessary for the aggregation below to work, not a check if the next > |total_partitions| fragments will fit in |capacity| bytes... Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:336: std::vector<std::pair<size_t, size_t> > score( On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: it's allowed to write >> in the type nowadays Done. Old habits die hard =) https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:121: size_t AgregatePartitions(size_t part_ix, size_t capacity); On 2017/05/17 13:10:07, sprang_webrtc wrote: > nit: AggregatePartitions Done. https://codereview.webrtc.org/2871173008/diff/160001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:826: bool with_fec) { On 2017/05/17 13:10:07, sprang_webrtc wrote: > Maybe would be a good idea to add optional last-packet extension header to this > test? No. It will complicate that test greatly, however, it's already tested in module_unittests and video_engine_tests. https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:559: // Must write at least one byte of the real payload to the packet. On 2017/05/17 13:21:54, danilchap wrote: > btw, why? > or rather 'which payload is real'? (same as headers - there are lot of different > headers in an rtp packet and thus different parts can be called payload). > Here it might mean rtp_payload [including descriptor header] or codec payload - > the one provided in payload_ variable [excluding rtp descriptor] It's a codec payload. I thought the packets without any actual codec payload may be dropped. Now, it actually makes sense that no one will parse codec descriptors to check for that. Still, it makes very little sense to send packets with 0 actual codec payload. https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:560: if (packet_bytes > SsDataLength(hdr_)) { On 2017/05/17 13:21:54, danilchap wrote: > is SSDataLength cheap function? may be cache it's result. Done. https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:480: const size_t kExpectedNum = GTEST_ARRAY_SIZE_(kExpectedSizes); On 2017/05/17 13:21:54, danilchap wrote: > is it different from arraysize macro from base/arraysize.h ? I have no idea. It's used everythere in that file, so I just went with it. https://codereview.webrtc.org/2871173008/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9_unittest.cc:546: EXPECT_FALSE(packet.Marker()); On 2017/05/17 13:21:54, danilchap wrote: > why testing Marker bit? It wouldn't hurt. Additional check that everything is correct with the number of returned packets.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:115: expected_sizes.size()); On 2017/05/17 15:06:33, ilnik wrote: > On 2017/05/17 13:10:07, sprang_webrtc wrote: > > nit: For EXPECT_EQ and ASSERT_EQ below, prefer having the expected value as > the > > first argument, as that's what the error message is gonna say if the test > fails. > > Actually, it's true that expected value is the first in the error message. At > least I've seen it. And, once I paid attention to that detail, in all places > around, expected value is the first. Done it in the same way here for > consistency. Let's make some test fail with EXPECT_EQ failure... EXPECT_EQ(1, 2); Result messages: ../../webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:95: Failure Expected: 1 To be equal to: 2 I would say this message works equally fine both when value under test 1st parameter and 2nd parameter. https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi... "Historical note: Before February 2016 *_EQ had a convention of calling it as ASSERT_EQ(expected, actual), so lots of existing code uses this order. Now *_EQ treats both parameters in the same way."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:115: expected_sizes.size()); On 2017/05/17 15:28:39, danilchap wrote: > On 2017/05/17 15:06:33, ilnik wrote: > > On 2017/05/17 13:10:07, sprang_webrtc wrote: > > > nit: For EXPECT_EQ and ASSERT_EQ below, prefer having the expected value as > > the > > > first argument, as that's what the error message is gonna say if the test > > fails. > > > > Actually, it's true that expected value is the first in the error message. At > > least I've seen it. And, once I paid attention to that detail, in all places > > around, expected value is the first. Done it in the same way here for > > consistency. > > Let's make some test fail with EXPECT_EQ failure... > EXPECT_EQ(1, 2); Result messages: > ../../webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:95: Failure > Expected: 1 > To be equal to: 2 > > I would say this message works equally fine both when value under test 1st > parameter and 2nd parameter. > > https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi... > "Historical note: Before February 2016 *_EQ had a convention of calling it as > ASSERT_EQ(expected, actual), so lots of existing code uses this order. Now *_EQ > treats both parameters in the same way." Yes, you are right. It just that I saw that the first number is labeled "Expected:". If one carefully reads the second line too, and thinks about it a bit, two numbers are treated equally. Still, I'll leave this part of code in the same style as everything around it: expected value first. I'll remember this for the future.
lgtm with nits https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:115: expected_sizes.size()); On 2017/05/17 13:21:54, danilchap wrote: > On 2017/05/17 13:10:07, sprang_webrtc wrote: > > nit: For EXPECT_EQ and ASSERT_EQ below, prefer having the expected value as > the > > first argument, as that's what the error message is gonna say if the test > fails. > > Actually error message treat both parameters equally these days. > So recommendation shifting other way around for consistency with regular == Right, thanks. Old habits... https://codereview.webrtc.org/2871173008/diff/160001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:826: bool with_fec) { On 2017/05/17 15:06:34, ilnik wrote: > On 2017/05/17 13:10:07, sprang_webrtc wrote: > > Maybe would be a good idea to add optional last-packet extension header to > this > > test? > > No. It will complicate that test greatly, however, it's already tested in > module_unittests and video_engine_tests. Acknowledged. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:547: // i.e. if 14 bytes were splitted between 4 packets, it would be 3+3+4+4. nit: s/splitted/split https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:582: assert(bytes_processed == payload_size_); RTC_CHECK_EQ
https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:547: // i.e. if 14 bytes were splitted between 4 packets, it would be 3+3+4+4. On 2017/05/18 09:48:20, sprang_webrtc wrote: > nit: s/splitted/split Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:582: assert(bytes_processed == payload_size_); On 2017/05/18 09:48:20, sprang_webrtc wrote: > RTC_CHECK_EQ Done.
https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:65: const size_t payload_size = 13; On 2017/05/17 15:06:33, ilnik wrote: > On 2017/05/17 13:10:07, sprang_webrtc wrote: > > nit: for constants use kCamelCase style > > Done. fyi: "This convention is optional for variables of other storage classes, e.g. automatic variables" https://google.github.io/styleguide/cppguide.html#Constant_Names https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:91: RTC_CHECK(max_payload_len > last_packet_reduction_len); RTC_CHECK_GT https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:891: add tests for non-zero last_packet_reduction_len https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:74: // Last smaller_packets_ packets are 1 byte smaller than previous packets. update the comment https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:28: using ::testing::ElementsAreArray; may be sort usings alphabetically https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:47: size_t GetEffectivePacketsSizeDifference(std::vector<size_t>* kPayloadSizes, "The names of variables (including function parameters) are all lowercase, with underscores between words." https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:70: std::vector<size_t> kPayloadSizes = NextPacketFillPayloadSizes(&packetizer); do not name variable payload_sizes as a constant. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:286: size_t bytes_per_packet = (total_bytes) / num_packets_left; remove () round total_bytes https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:290: // Only first num_larger_packets are 1 byte wider than the rest. Aren't |num_larger_packets| last? https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:187: TEST_F(RtpPacketizerVp8Test, TestAggregateModeExtentionNotFits) { may be do not mention an abstract extension in the test name. e.g. TestAggregateModeLastPacketReductionCauseExtraPacket https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:194: RtpPacketizerVp8 packetizer(hdr_info_, kMaxPayloadSize, 5, kAggregate); since this '5' is important for the test, name it: e.g. /*last_packet_reduction_len=*/5 or const size_t kLastPacketReductionLen = 5 for local consistency https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:199: // The expected sizes are obtained by running a verified good implementation. can you mention which implementation that is? Can I check it too? https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:356: helper_->payload_size(), NULL); nullptr instead of NULL here and below https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:376: TEST_F(RtpPacketizerVp8Test, TestEqualSizeWithExtention) { may be LastPacketReduction instead of Extension
https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:91: RTC_CHECK(max_payload_len > last_packet_reduction_len); On 2017/05/18 11:14:24, danilchap wrote: > RTC_CHECK_GT Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:891: On 2017/05/18 11:14:24, danilchap wrote: > add tests for non-zero last_packet_reduction_len Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:74: // Last smaller_packets_ packets are 1 byte smaller than previous packets. On 2017/05/18 11:14:24, danilchap wrote: > update the comment Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:28: using ::testing::ElementsAreArray; On 2017/05/18 11:14:24, danilchap wrote: > may be sort usings alphabetically Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:70: std::vector<size_t> kPayloadSizes = NextPacketFillPayloadSizes(&packetizer); On 2017/05/18 11:14:24, danilchap wrote: > do not name variable payload_sizes as a constant. Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:286: size_t bytes_per_packet = (total_bytes) / num_packets_left; On 2017/05/18 11:14:24, danilchap wrote: > remove () round total_bytes Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:290: // Only first num_larger_packets are 1 byte wider than the rest. On 2017/05/18 11:14:24, danilchap wrote: > Aren't |num_larger_packets| last? Forgot to change the comment according to changed logic. Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:187: TEST_F(RtpPacketizerVp8Test, TestAggregateModeExtentionNotFits) { On 2017/05/18 11:14:25, danilchap wrote: > may be do not mention an abstract extension in the test name. > e.g. > TestAggregateModeLastPacketReductionCauseExtraPacket Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:194: RtpPacketizerVp8 packetizer(hdr_info_, kMaxPayloadSize, 5, kAggregate); On 2017/05/18 11:14:24, danilchap wrote: > since this '5' is important for the test, name it: > e.g. /*last_packet_reduction_len=*/5 > or const size_t kLastPacketReductionLen = 5 > for local consistency Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:199: // The expected sizes are obtained by running a verified good implementation. On 2017/05/18 11:14:24, danilchap wrote: > can you mention which implementation that is? Can I check it too? The tested implementation, verified by hand. Better to change that to "calculated by hand". https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:356: helper_->payload_size(), NULL); On 2017/05/18 11:14:24, danilchap wrote: > nullptr instead of NULL here and below Done. https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:376: TEST_F(RtpPacketizerVp8Test, TestEqualSizeWithExtention) { On 2017/05/18 11:14:24, danilchap wrote: > may be LastPacketReduction instead of Extension Done.
https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc:47: size_t GetEffectivePacketsSizeDifference(std::vector<size_t>* kPayloadSizes, On 2017/05/18 11:14:24, danilchap wrote: > "The names of variables (including function parameters) are all lowercase, with > underscores between words." > https://google.github.io/styleguide/cppguide.html#Variable_Names Done.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:334: std::vector<std::pair<size_t, size_t>> score( consider using helper struct instead of pair. with pair one need to remember meaning of first and second to follow the code. And this block is far from small and trivial to ask reader to. struct PartititionScore { size_t num_packets = std::numeric_limits<size_t>::max(); size_t largest_packet_len = std::numeric_limits<size_t>::max() }; std::vector<PartititionScore> scores(total_partitions + 1); scores[0] = {0, 0}; https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:343: for (size_t left = 0; left < total_partitions; ++left) { what is left? https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (right): https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:111: void SplitPayloadBalanced(size_t payload_offset, may be name it with GeneratePackets prefix: function with only input parameter and no return value made me wonder how it output it's result. GeneratePackets would imply it fills packets_ member [using QueuePacket]. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:114: bool last, may be last_partition? https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:115: size_t part_ix); do you mind renaming parameter to part_idx? (or partition_index) to make it clearer. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:117: // Helper function. Aggregates partitions starting at |part_ix| to packets of nit: not sure if mentioning that this is Helper function is beneficial: It is private function which imply it is a helper. Probably better to start comment directly with description what this function do: save a bit of time for the reader. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:133: // Test is disabled: https://code.google.com/p/webrtc/issues/detail?id=4019. with you changes, can this test be reenabled? https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:242: const size_t kSizeVector[] = {3, 4, 2, 5, 2, 4}; what happen with same kSizeVector but with kLastPacketReductionLen = 2? https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:393: const size_t kExpectedSizes[] = {13, 13, 14, 14, 9}; Can you add comment explaining expected sizes because I do not understand why they are correct: Total payload size is 30+10+3 = 43 bytes. descriptor (with 2-byte picture id) is 3 bytes, so expected size state that total payload put into packets will be 10 + 10 + 11 + 11 + 6 = 48 bytes.
https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:334: std::vector<std::pair<size_t, size_t>> score( On 2017/05/19 12:03:46, danilchap wrote: > consider using helper struct instead of pair. > with pair one need to remember meaning of first and second to follow the code. > And this block is far from small and trivial to ask reader to. > > struct PartititionScore { > size_t num_packets = std::numeric_limits<size_t>::max(); > size_t largest_packet_len = std::numeric_limits<size_t>::max() > }; > > std::vector<PartititionScore> scores(total_partitions + 1); > scores[0] = {0, 0}; Done, however, I couldn't get ={0,0} to work and also had to expand comparison below to explicitly compare num_packets and then largest_packet_len. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:343: for (size_t left = 0; left < total_partitions; ++left) { On 2017/05/19 12:03:46, danilchap wrote: > what is left? Remaining number of partitions to aggregate. I suppose paritions_left will be clear enough. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (right): https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:111: void SplitPayloadBalanced(size_t payload_offset, On 2017/05/19 12:03:46, danilchap wrote: > may be name it with GeneratePackets prefix: > function with only input parameter and no return value made me wonder how it > output it's result. > GeneratePackets would imply it fills packets_ member [using QueuePacket]. Done. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:114: bool last, On 2017/05/19 12:03:46, danilchap wrote: > may be last_partition? Done. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:115: size_t part_ix); On 2017/05/19 12:03:46, danilchap wrote: > do you mind renaming parameter to part_idx? (or partition_index) to make it > clearer. Done. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:117: // Helper function. Aggregates partitions starting at |part_ix| to packets of On 2017/05/19 12:03:46, danilchap wrote: > nit: not sure if mentioning that this is Helper function is beneficial: It is > private function which imply it is a helper. > Probably better to start comment directly with description what this function > do: save a bit of time for the reader. Done. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:133: // Test is disabled: https://code.google.com/p/webrtc/issues/detail?id=4019. On 2017/05/19 12:03:46, danilchap wrote: > with you changes, can this test be reenabled? Yes, I've reenabled it. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:242: const size_t kSizeVector[] = {3, 4, 2, 5, 2, 4}; On 2017/05/19 12:03:46, danilchap wrote: > what happen with same kSizeVector but with kLastPacketReductionLen = 2? Nothing really changes. last packet still has 4 free bytes (13-9). This solution will also generate smallest largest packet. If, however kLastPacketReductionLen were be 5 or 6, there would still be a way to fit everything in 3 packets (all vp8 payload descriptors are 3 bytes, so we have 10 free bytes in each packet). aggregation would be 3+4+2 in the first packet, 5+2 in the second and 4 in the last packet. Alternatively, it may be 3+4/2+5+2/4 which has the same score. I fill there's no need to add a new test as there are already tests for that below. https://codereview.webrtc.org/2871173008/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:393: const size_t kExpectedSizes[] = {13, 13, 14, 14, 9}; On 2017/05/19 12:03:46, danilchap wrote: > Can you add comment explaining expected sizes because I do not understand why > they are correct: > Total payload size is 30+10+3 = 43 bytes. > descriptor (with 2-byte picture id) is 3 bytes, > so expected size state that total payload put into packets will be > 10 + 10 + 11 + 11 + 6 = 48 bytes. Descriptor is 4 bytes in that case according to rfc: https://tools.ietf.org/html/rfc7741#page-7 (one required byte, extended control bits byte, and 2-byte pictureID). Thus, we get 9+9+10+10+5=43=30+10+3. Added a comment.
https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:259: bool last = (part_ix + 1) == num_partitions_; last_partition? https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:261: current_capacity -= last_packet_reduction_len_; current_packet_capacity? https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:265: GeneratePacketsAggregatePartitions(part_ix, per_packet_capacity); with current_[packet_]capacity, and per_packet_capacity that differ on last[_partition] which doesn't mean last_packet (both because last packet may contain several partition and last partition can be split across several packets) Can you add a bit of explanation why this logic is correct. Tests convince me it is, but code doesn't explain how. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:271: part_ix++; nit: though for scalar types style equally allow both pre-increment and post-increment, in webrtc pre-increment is way more common. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:331: // Aggregate partitions start..finish-1 to blocks of size at most |capacity| Do you know if notation [start, finish) is common enough to use it? https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:348: // Best number of partitions in the first packet in block realizing optimal sorry, I completely failed to understand the comment. Can you rephrase it? https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:350: std::vector<size_t> best_block_size(total_partitions + 1, 0); can it make sense to add an extra field to PartitionScore instead? https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:359: // One more packet. avoid comments that just repeat what the code does. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:374: if (current_score.num_packets < scores[new_partitions_left].num_packets || sorry for forcing you to write more code. to write a bit less, you might try out std::tie: if (std::tie(current_score.num_packets, current_score.largest_packet_len) < std::tie(scores[new_partitions_left].num_packets, scores[new_partitions_left].largest_packet_len) { ... Or... add a small helper method to the ParticipantScore struct: bool BetterThan(const PartititionScore& other) const { if (num_packet < other.num_packets) return true; if (num_packet > other.num_packets) return false; return largest_packet_len < other.largers_packet_len; } this code here will look like if (current_score.BetterThan(scores[new_partitions_left])) { scores[new_partitions_left] = current_score; best_block_size[new_partitions_left] = ...; } https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (right): https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:88: enum AggregationMode { used? https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:116: const size_t kExpectedSizes[] = {9, 9, 12, 13, 13, 13}; the result of this test changed not because you added support for reduced last packet, but because you removed balanced aggregate mode. Can you reflect that in the CL description. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:393: // 15-4=11 and leave 5 free bytes in the last packet. All packets are almost nit: add spaces around binary operators: '15 - 4 = 11'
https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:259: bool last = (part_ix + 1) == num_partitions_; On 2017/05/22 12:32:00, danilchap wrote: > last_partition? Done. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:261: current_capacity -= last_packet_reduction_len_; On 2017/05/22 12:32:00, danilchap wrote: > current_packet_capacity? Done. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:265: GeneratePacketsAggregatePartitions(part_ix, per_packet_capacity); On 2017/05/22 12:31:59, danilchap wrote: > with current_[packet_]capacity, and per_packet_capacity that differ on > last[_partition] which doesn't mean last_packet (both because last packet may > contain several partition and last partition can be split across several > packets) > Can you add a bit of explanation why this logic is correct. > Tests convince me it is, but code doesn't explain how. Here it's only checked if current partition fits into a single packet. For last partition it will be always the last packet. Commented on it. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:271: part_ix++; On 2017/05/22 12:32:00, danilchap wrote: > nit: though for scalar types style equally allow both pre-increment and > post-increment, in webrtc pre-increment is way more common. Done. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:331: // Aggregate partitions start..finish-1 to blocks of size at most |capacity| On 2017/05/22 12:32:00, danilchap wrote: > Do you know if notation [start, finish) is common enough to use it? I don't know if it's common, I think current notation is clear enough. Also, changed names of variables in the comment to correspond with what actually is used in the code. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:348: // Best number of partitions in the first packet in block realizing optimal On 2017/05/22 12:31:59, danilchap wrote: > sorry, I completely failed to understand the comment. Can you rephrase it? Done. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:350: std::vector<size_t> best_block_size(total_partitions + 1, 0); On 2017/05/22 12:32:00, danilchap wrote: > can it make sense to add an extra field to PartitionScore instead? Not really, IMHO, because it's not a score. It's a separate data structure used to restore the optimal path in the state space. In that sense, |score| is the minimal distance in the state space. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:359: // One more packet. On 2017/05/22 12:32:00, danilchap wrote: > avoid comments that just repeat what the code does. Done. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:374: if (current_score.num_packets < scores[new_partitions_left].num_packets || On 2017/05/22 12:32:00, danilchap wrote: > sorry for forcing you to write more code. to write a bit less, you might try out > std::tie: > if (std::tie(current_score.num_packets, current_score.largest_packet_len) < > std::tie(scores[new_partitions_left].num_packets, > scores[new_partitions_left].largest_packet_len) { ... > > Or... add a small helper method to the ParticipantScore struct: > bool BetterThan(const PartititionScore& other) const { > if (num_packet < other.num_packets) return true; > if (num_packet > other.num_packets) return false; > return largest_packet_len < other.largers_packet_len; > } > > this code here will look like > if (current_score.BetterThan(scores[new_partitions_left])) { > scores[new_partitions_left] = current_score; > best_block_size[new_partitions_left] = ...; > } I have added operator< to the struct. BetterThan() method is more suitable for Java than C++ IMHO. Should I move the PartitionScore struct out of the function? https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (right): https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:88: enum AggregationMode { On 2017/05/22 12:32:00, danilchap wrote: > used? I removed all uses of it, but forgot to remove the struct itself. Thanks. https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:116: const size_t kExpectedSizes[] = {9, 9, 12, 13, 13, 13}; On 2017/05/22 12:32:00, danilchap wrote: > the result of this test changed not because you added support for reduced last > packet, but because you removed balanced aggregate mode. > Can you reflect that in the CL description. No, in that particular test, it is just a bug that last partition was split into 4 packets, instead of 3 now. It's still balanced. I've removed flags because in all packetizers they were always set to true. Besides, this is a strict mode test, not an aggregated mode test. Finally, I changed balanced aggregate mode a bit, but not removed it (now it always generates the smallest number of packets, but other than that it's always as balanced as possible). https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:393: // 15-4=11 and leave 5 free bytes in the last packet. All packets are almost On 2017/05/22 12:32:00, danilchap wrote: > nit: add spaces around binary operators: '15 - 4 = 11' Done.
https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:374: if (current_score.num_packets < scores[new_partitions_left].num_packets || On 2017/05/22 14:14:09, ilnik wrote: > On 2017/05/22 12:32:00, danilchap wrote: > > sorry for forcing you to write more code. to write a bit less, you might try > out > > std::tie: > > if (std::tie(current_score.num_packets, current_score.largest_packet_len) < > > std::tie(scores[new_partitions_left].num_packets, > > scores[new_partitions_left].largest_packet_len) { ... > > > > Or... add a small helper method to the ParticipantScore struct: > > bool BetterThan(const PartititionScore& other) const { > > if (num_packet < other.num_packets) return true; > > if (num_packet > other.num_packets) return false; > > return largest_packet_len < other.largers_packet_len; > > } > > > > this code here will look like > > if (current_score.BetterThan(scores[new_partitions_left])) { > > scores[new_partitions_left] = current_score; > > best_block_size[new_partitions_left] = ...; > > } > > I have added operator< to the struct. BetterThan() method is more suitable for > Java than C++ IMHO. Should I move the PartitionScore struct out of the function? Prefer it inside the function to stress it is not used anywhere else. https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:321: NextAggregatePacket(rtp_packet, total_packets_ == 1); can you name 2nd parameter. e.g. bool is_last_packet = total_packets_ == 1; NextAggregatePacket(rtp_packet, is_last_packet); https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right): https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:95: size_t total_packets_; may be num_packets_left_ or is it needed? (can it be different than packets_.size()?) https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:1: /* did you test single nalu with last packet reduction? https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:333: // Aggregate partitions |part_idx|..|end_part|-1 to blocks of size at most nit: put -1 inside ||: it is boundary of 'code' not 'variable name' https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:344: bool operator <(const PartitionScore &other) { ") const {" for symmetry with |other| put & to the type rather than parameter name. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:381: // size is better. I still think that using function name 'BetterThan' or just 'better' would allow to remove this comment and improve readabiliy (no need to do extra step to check if 'less is better' or 'bigger is better'). Feel free to disagree and ignore this comment.
https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right): https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:321: NextAggregatePacket(rtp_packet, total_packets_ == 1); On 2017/05/23 12:17:27, danilchap wrote: > can you name 2nd parameter. > e.g. > bool is_last_packet = total_packets_ == 1; > NextAggregatePacket(rtp_packet, is_last_packet); Done. https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right): https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:95: size_t total_packets_; On 2017/05/23 12:17:27, danilchap wrote: > may be num_packets_left_ > or is it needed? (can it be different than packets_.size()?) Yes, num_packets_left_ is better. Still, It's different from packets_.size(), because in this implementation some items in packets_ are parts of an 'aggregate' packet. https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right): https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:1: /* On 2017/05/23 12:17:27, danilchap wrote: > did you test single nalu with last packet reduction? Not needed, because it's just a single packet always. It will either fit or not. If not, all kind of rtc_checks are in place, and nothing can be done here. https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:333: // Aggregate partitions |part_idx|..|end_part|-1 to blocks of size at most On 2017/05/23 12:17:27, danilchap wrote: > nit: put -1 inside ||: it is boundary of 'code' not 'variable name' Done. https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:344: bool operator <(const PartitionScore &other) { On 2017/05/23 12:17:27, danilchap wrote: > ") const {" for symmetry with |other| > put & to the type rather than parameter name. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.webrtc.org/2871173008/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:381: // size is better. On 2017/05/23 12:17:27, danilchap wrote: > I still think that using function name 'BetterThan' or just 'better' would allow > to remove this comment and improve readabiliy (no need to do extra step to check > if 'less is better' or 'bigger is better'). > Feel free to disagree and ignore this comment. Acknowledged. This comment is a bit redundant here, and could be removed, but I will leave it for additional clarity.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:58: // first packets: 14 bytes in 4 packets would be splitted as 3+3+4+4. in some last packets https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:59: // There are exactly total_data % num_packets larger packets. this line feel redundant (duplicates code in next line) https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:77: payload_len_per_packet_++; ++payload_... https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:103: num_packets_left_--; --num_packets_left_; https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:62: // Number of packets, which will be 1 byte less than the rest. 1 byte more https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:329: end_part++; ++end_part https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (left): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:71: // Returns true on success, false otherwise. may be do not delete comment about return value. (comment itself might be shorter though: either "Returns true on success" or "Returns false on failure" is enough.) https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:105: // packet should be reduced by last_packet_reduction_len_. then the \last/ packet https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:113: // given capacity. Last packet, if containing last partition of the frame may be |capacity| https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:546: // Several last packets are 1 byte smaller than the rest. 1 byte larger https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h:72: // |last| indicates if the packet is last. probably drop the: 'if packet is last' may be add '...is last in the frame.' or 'is last to generate.'
Thanks for finding time to review this. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:58: // first packets: 14 bytes in 4 packets would be splitted as 3+3+4+4. On 2017/05/23 15:55:03, danilchap wrote: > in some last packets Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:59: // There are exactly total_data % num_packets larger packets. On 2017/05/23 15:55:03, danilchap wrote: > this line feel redundant (duplicates code in next line) Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:77: payload_len_per_packet_++; On 2017/05/23 15:55:03, danilchap wrote: > ++payload_... Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.cc:103: num_packets_left_--; On 2017/05/23 15:55:03, danilchap wrote: > --num_packets_left_; Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h:62: // Number of packets, which will be 1 byte less than the rest. On 2017/05/23 15:55:03, danilchap wrote: > 1 byte more Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:329: end_part++; On 2017/05/23 15:55:03, danilchap wrote: > ++end_part Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (left): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:71: // Returns true on success, false otherwise. On 2017/05/23 15:55:03, danilchap wrote: > may be do not delete comment about return value. > (comment itself might be shorter though: either "Returns true on success" or > "Returns false on failure" is enough.) Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:105: // packet should be reduced by last_packet_reduction_len_. On 2017/05/23 15:55:03, danilchap wrote: > then the \last/ packet Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h:113: // given capacity. Last packet, if containing last partition of the frame On 2017/05/23 15:55:03, danilchap wrote: > may be |capacity| Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:546: // Several last packets are 1 byte smaller than the rest. On 2017/05/23 15:55:03, danilchap wrote: > 1 byte larger Done. https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h (right): https://codereview.webrtc.org/2871173008/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h:72: // |last| indicates if the packet is last. On 2017/05/23 15:55:03, danilchap wrote: > probably drop the: 'if packet is last' > may be add '...is last in the frame.' > or 'is last to generate.' Done.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2871173008/#ps300001 (title: "Impelement Danilchap@ comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by ilnik@webrtc.org
The CQ bit was checked by ilnik@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1495556408863580,
"parent_rev": "d019667c001679b9c73b63735f2c2fca92c530d6", "commit_rev":
"7a3006bae7f1c61371581ddfa320524041119f20"}
Message was sent while issue was closed.
Description was changed from ========== Fix packetization logic to leave space for extensions in the last packet Change packetizer interface to explicitly return number of packets instead of a last flag. Account for extra space needed in the last packet. BUG=webrtc:7588,webrtc:7594 ========== to ========== Fix packetization logic to leave space for extensions in the last packet Change packetizer interface to explicitly return number of packets instead of a last flag. Account for extra space needed in the last packet. BUG=webrtc:7588,webrtc:7594 Review-Url: https://codereview.webrtc.org/2871173008 Cr-Commit-Position: refs/heads/master@{#18244} Committed: https://chromium.googlesource.com/external/webrtc/+/7a3006bae7f1c61371581ddfa... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/external/webrtc/+/7a3006bae7f1c61371581ddfa... |
