Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495553002:
Add overhead per packet observer to the rtp_sender. (Closed)
|
Created:
4 years, 1 month ago by michaelt Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, danilchap, yujie_mao (webrtc), zhuangzesen_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd overhead per packet observer to the rtp_sender.
BUG=webrtc:6638
Committed: https://crrev.com/4da304407c993ccfc829bfe3f338b66a1f6c1a31
Cr-Commit-Position: refs/heads/master@{#15124}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix TransportSequenceNumbers bug. #
Total comments: 6
Patch Set 3 : Respond to comments. #Patch Set 4 : Respond to comments. #
Total comments: 8
Patch Set 5 : Respond to comments. #
Total comments: 7
Patch Set 6 : Move overhead_size_observer_ check out of send_critsect_. #Patch Set 7 : Rename RtpOverheadSizeChanged to UpdateRtpOverheadSize. #
Total comments: 16
Patch Set 8 : Respond to comments. #Patch Set 9 : Renamed OverheadSizeObserver to OverheadObserver. #Patch Set 10 : Rebased. #
Messages
Total messages: 56 (32 generated)
The CQ bit was checked by michaelt@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/...
Description was changed from ========== Add overhead per packet observer to the rtp_sender. BUG=webrtc:6638 ========== to ========== Add overhead per packet observer to the rtp_sender. BUG=webrtc:6638 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
Hi, Here the first CL for adding overhead to BWE.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/18293)
The CQ bit was checked by michaelt@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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/12975)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by michaelt@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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/13039)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by michaelt@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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/19565)
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by michaelt@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.
Good work! I have some naming suggestions. https://codereview.webrtc.org/2495553002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2495553002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1482: TEST_F(RtpSenderTest, OnOverheadPerPacketChanged) { OnOverheadSizeChanged https://codereview.webrtc.org/2495553002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1483: class MockOverheadPerPacketObserver : public OverheadPerPacketObserver { I prefer to place it in an unnamed namespace. You can put it right before the this TEST_F https://codereview.webrtc.org/2495553002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1504: // RTP Overhead is with sequence number extension is 20B. remove "is"
https://codereview.webrtc.org/2495553002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2495553002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1482: TEST_F(RtpSenderTest, OnOverheadPerPacketChanged) { On 2016/11/14 10:35:41, minyue-webrtc wrote: > OnOverheadSizeChanged Done. https://codereview.webrtc.org/2495553002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1483: class MockOverheadPerPacketObserver : public OverheadPerPacketObserver { On 2016/11/14 10:35:41, minyue-webrtc wrote: > I prefer to place it in an unnamed namespace. You can put it right before the > this TEST_F Acknowledged. https://codereview.webrtc.org/2495553002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1504: // RTP Overhead is with sequence number extension is 20B. On 2016/11/14 10:35:41, minyue-webrtc wrote: > remove "is" Done.
It seems that some my earlier comments were missing, interesting, I add them back. PTAL. https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right): https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:87: OverheadPerPacketObserver* overhead_per_packet_observer = nullptr; I hope it is renamed to OverheadSizeOberver https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.h:62: OverheadPerPacketObserver* const overhead_rate_observer); no need "const" https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.h:331: int transport_overhead_per_packet_ GUARDED_BY(send_critsect_); size_t transport_overhead_bytes_per_packet_ https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.h:332: size_t rtp_overhead_per_packet_ GUARDED_BY(send_critsect_); rtp_overhead_bytes_per_packet_ https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.h:335: OverheadPerPacketObserver* const overhead_per_packet_observer_; OverheadSizeObserver* const overhead_size_observer_;
https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right): https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:87: OverheadPerPacketObserver* overhead_per_packet_observer = nullptr; On 2016/11/14 11:18:06, minyue-webrtc wrote: > I hope it is renamed to OverheadSizeOberver Done. https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.h:62: OverheadPerPacketObserver* const overhead_rate_observer); On 2016/11/14 11:18:06, minyue-webrtc wrote: > no need "const" Done. https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.h:331: int transport_overhead_per_packet_ GUARDED_BY(send_critsect_); On 2016/11/14 11:18:06, minyue-webrtc wrote: > size_t transport_overhead_bytes_per_packet_ Done. https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.h:332: size_t rtp_overhead_per_packet_ GUARDED_BY(send_critsect_); On 2016/11/14 11:18:06, minyue-webrtc wrote: > rtp_overhead_bytes_per_packet_ Done. https://codereview.webrtc.org/2495553002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.h:335: OverheadPerPacketObserver* const overhead_per_packet_observer_; On 2016/11/14 11:18:06, minyue-webrtc wrote: > OverheadSizeObserver* const overhead_size_observer_; Done.
https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:573: if (has_transport_seq_no) { Could you please change seq_no to seq_num? Thanks https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1255: RtpPacketToSend* packet, Should be possible to pass in a const RtpPacketToSend&, right? https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1264: rtp_overhead_bytes_per_packet_ != packet->headers_size()) { Why do we report this only if we have transport sequence numbers? Seems like it's independent. https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1265: overhead_size_observer_->OnOverheadSizeChanged( We shouldn't hold the send_critsect_ when calling this. Btw, I'm curious what you think of using a getter for this instead of an observer interface? Seems like it would potentially reduce the code needed. Or do we need to know the overhead for every packet? That is a bit hard to judge without seeing how you're going to use this.
https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:573: if (has_transport_seq_no) { On 2016/11/15 09:59:47, stefan-webrtc (holmer) wrote: > Could you please change seq_no to seq_num? Thanks Done. https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1255: RtpPacketToSend* packet, Right. https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1264: rtp_overhead_bytes_per_packet_ != packet->headers_size()) { I general you are right. Will decouple overhead_size from the transport sequence numbers at this place. We just have to keep in mind that the overhead reduction just have to be done in the case we use TWCC. https://codereview.webrtc.org/2495553002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1265: overhead_size_observer_->OnOverheadSizeChanged( Changed the impl. so that OnOverheadSizeChanged is not called when we hold send_critsect_. We don't need to know the overhead for each packet. Since the ANA will not know when the Overhead will change it has to ask for every calculation of the payload bit rate. Which will be quite a lot during the call. While o the other-hand the overhead should be stable during a call and OnOverheadSizeChanged should just be called on the call start and on route changes. So i think the observer is the better solution for the problem.
https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1244: if (!overhead_size_observer_ || There's no need to check the overhead_size_observer_ within the critical section. Move that check to the top and return early without taking the crit sect. https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1270: if (!overhead_size_observer_ || Same here. https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:266: void RtpOverheadSizeChanged(const RtpPacketToSend& packet); OnRtpOverheadSizeChanged, or perhaps UpdateRtpOverheadSize()?
https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1244: if (!overhead_size_observer_ || On 2016/11/15 11:50:14, stefan-webrtc (holmer) wrote: > There's no need to check the overhead_size_observer_ within the critical > section. Move that check to the top and return early without taking the crit > sect. Done. https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1270: if (!overhead_size_observer_ || On 2016/11/15 11:50:14, stefan-webrtc (holmer) wrote: > Same here. Done.
lgtm, but please find a different name for RtpOverheadSizeChanged. See my suggestions. https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:266: void RtpOverheadSizeChanged(const RtpPacketToSend& packet); On 2016/11/15 11:50:14, stefan-webrtc (holmer) wrote: > OnRtpOverheadSizeChanged, or perhaps UpdateRtpOverheadSize()? I'd prefer if we rename this.
https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2495553002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:266: void RtpOverheadSizeChanged(const RtpPacketToSend& packet); On 2016/11/15 16:05:49, stefan-webrtc (holmer) wrote: > On 2016/11/15 11:50:14, stefan-webrtc (holmer) wrote: > > OnRtpOverheadSizeChanged, or perhaps UpdateRtpOverheadSize()? > > I'd prefer if we rename this. Done.
https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1240: void RTPSender::SetTransportOverhead(int transport_overhead) { how about void RTPSender::SetTransportOverheadSize(size_t transport_overhead_bytes_per_packet) https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1246: if (rtp_overhead_bytes_per_packet_ == transport_overhead_bytes_per_packet_? https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1257: void RTPSender::AddPacketToTransportFeedback(uint16_t packet_id, To me, this help function does not bring much more value, do we really need it? How about the way before the change?
https://codereview.webrtc.org/2495553002/diff/180001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2495553002/diff/180001/webrtc/common_types.h#ne... webrtc/common_types.h:306: virtual void OnOverheadSizeChanged(int overhead_bytes_per_packet) = 0; size_t look like preferred type for a variable representing size: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
https://codereview.webrtc.org/2495553002/diff/180001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2495553002/diff/180001/webrtc/common_types.h#ne... webrtc/common_types.h:306: virtual void OnOverheadSizeChanged(int overhead_bytes_per_packet) = 0; On 2016/11/16 08:55:05, danilchap wrote: > size_t look like preferred type for a variable representing size: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Acknowledged. https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1240: void RTPSender::SetTransportOverhead(int transport_overhead) { SetTransportOverheadSize is used in all the rest of the functions. Since the value is passed down from audio/video stream. We can change it but, would prefer to have the same name for all the functions. So maybe change it in a different CL ? https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1246: if (rtp_overhead_bytes_per_packet_ == Oops. I may should write a unit test for this :) https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1257: void RTPSender::AddPacketToTransportFeedback(uint16_t packet_id, To me the helper function brings a bit more value and i would like to keep it. For example. We like to add the Overhead for the packets in future. Without this function we have to change the code in three place, While with this function we have to do it just at one place.
https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1240: void RTPSender::SetTransportOverhead(int transport_overhead) { On 2016/11/16 09:27:09, michaelt wrote: > SetTransportOverheadSize is used in all the rest of the functions. > Since the value is passed down from audio/video stream. We can change it but, > would prefer to have the same name for all the functions. > So maybe change it in a different CL ? > I see. After rethinking, I think overhead has a sense of "size" in it. So no need to add "size" after overhead. I'd hold back my suggestion about OverheadSizeObserver. my apologies. OverheadObserver should be good. BTW, how about my other part of the previous comment: size_t on transport_overhead and adding _byte_per_packet? https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1246: if (rtp_overhead_bytes_per_packet_ == On 2016/11/16 09:27:09, michaelt wrote: > Oops. I may should write a unit test for this :) > no need for unittest (too many of these may possible), but need careful writing and reviewing. https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1257: void RTPSender::AddPacketToTransportFeedback(uint16_t packet_id, On 2016/11/16 09:27:09, michaelt wrote: > To me the helper function brings a bit more value and i would like to keep it. > For example. We like to add the Overhead for the packets in future. Without this > function we have to change the code in three place, While with this function we > have to do it just at one place. > Acknowledged.
https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1240: void RTPSender::SetTransportOverhead(int transport_overhead) { Done https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1246: if (rtp_overhead_bytes_per_packet_ == I already added a test for this. Should I remove it ?
https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1240: void RTPSender::SetTransportOverhead(int transport_overhead) { On 2016/11/16 13:29:37, michaelt wrote: > Done > Have you uploaded new patch set? https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1246: if (rtp_overhead_bytes_per_packet_ == On 2016/11/16 13:29:37, michaelt wrote: > I already added a test for this. Should I remove it ? > Ok. I missed it, can take a look now. I think the important failure we want to catch in the test is a case that we should update but we do not update. We do not care much if OnOverheadSizeChanged is called twice. So I would remove that test.
https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2495553002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1240: void RTPSender::SetTransportOverhead(int transport_overhead) { On 2016/11/16 15:13:38, minyue-webrtc wrote: > On 2016/11/16 13:29:37, michaelt wrote: > > Done > > > > Have you uploaded new patch set? Now yes :)
lgtm thanks!
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2495553002/#ps220001 (title: "Renamed OverheadSizeObserver to OverheadObserver.")
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
Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12583) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14944) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18868)
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2495553002/#ps240001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add overhead per packet observer to the rtp_sender. BUG=webrtc:6638 ========== to ========== Add overhead per packet observer to the rtp_sender. BUG=webrtc:6638 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add overhead per packet observer to the rtp_sender. BUG=webrtc:6638 ========== to ========== Add overhead per packet observer to the rtp_sender. BUG=webrtc:6638 Committed: https://crrev.com/4da304407c993ccfc829bfe3f338b66a1f6c1a31 Cr-Commit-Position: refs/heads/master@{#15124} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/4da304407c993ccfc829bfe3f338b66a1f6c1a31 Cr-Commit-Position: refs/heads/master@{#15124} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
