Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2579613003:
Add TransportFeedbackPacketLossTracker. (Closed)
|
Created:
4 years ago by minyue-webrtc Modified:
3 years, 11 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd TransportFeedbackPacketLossTracker.
This CL is to calculate packet loss metrics from TransportFeedback. The outcome of this will be passed down to audio encoder.
BUG=webrtc:6904
Review-Url: https://codereview.webrtc.org/2579613003
Cr-Commit-Position: refs/heads/master@{#16217}
Committed: https://chromium.googlesource.com/external/webrtc/+/435ddf978d83794c16f842ba779e22b5d96ff161
Patch Set 1 #
Total comments: 13
Patch Set 2 : adding two unittests #Patch Set 3 : on Erik's comments #Patch Set 4 : episode duration -> consecutive packet loss #
Total comments: 19
Patch Set 5 : on Stefan's comments #Patch Set 6 : moving to voice engine #
Total comments: 54
Patch Set 7 : on Karl's comments #Patch Set 8 : adding a missing file (continuation of PS7) #
Total comments: 21
Patch Set 9 : on comments #
Total comments: 2
Patch Set 10 : rebase #
Total comments: 1
Patch Set 11 : undo mocking #
Total comments: 14
Patch Set 12 : on comments #
Messages
Total messages: 63 (27 generated)
Description was changed from ========== Add TransportFeedbackPacketLossTracker. BUG= ========== to ========== Add TransportFeedbackPacketLossTracker. BUG= ==========
The CQ bit was checked by minyue@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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11486)
The CQ bit was checked by minyue@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/...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11488)
Description was changed from ========== Add TransportFeedbackPacketLossTracker. BUG= ========== to ========== Add TransportFeedbackPacketLossTracker. BUG=webrtc:6904 ==========
minyue@webrtc.org changed reviewers: + sprang@webrtc.org, stefan@webrtc.org
Hi Erik and Stefan, Would you help review this CL?
https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:41: default; maybe just define this in the header instead then, or even remove it and leave it as implicit https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:94: ForwardDiff(ReferenceSequenceNumber(), seq_num) >= kSeqNumHalf) { I know we have discussed this in the past, but it still feel having window size based on num packets and kSeqNumHalf as size is excessive. With a low rate this will turn into a gigantic window. For instance, 20 packets per second will yield up to a 23min window size, yielding poor ability to quickly adapt to changing network conditions and wasting memory. There are many ways we could mitigate this. For instance, save a map from sequence number to time, where time -1 means not received. Then you could easily cull the tail of the window based on either time or size. https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:137: // If repeated reports contradict each other, we let the later one rule. I think this is not in accordance with our draft spec: https://tools.ietf.org/html/draft-holmer-rmcat-transport-wide-cc-extensions-01 Note: In the case the base sequence number is decreased, creating a window overlapping the previous feedback messages, the status for any packets previously reported as received must be marked as "Packet not received" and thus no delta included for that symbol. ...and I think neither is out implementation, which actually sends the status/time twice in case of reordering :) I think you can return if (ret.first->second || ret.first->second == received) https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:205: // goes to the end. nit: s/goes/go https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.h:46: void Validate() const; // Only used for tests. nit: Join comments into one. https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.h:60: void UndoPacketStatus(std::map<uint16_t, bool>::const_iterator it); can we typedef std::map<uint16_t, bool>::const_iterator to something shorter?
Hi Erik, Thanks for reviewing! I have updated the CL except for the big change that you suggested, which I prefer taking as future work. But note that I found a problem myself: the so called episode duration can be misleading, because when we have unknown packet status, say L L ? ? R L L ? ? ? R (L: lost, R: receive, ?: unknown) we cannot estimate duration and we return infinity in current implementation. But infinity does not look a good value in this example. Instead, I would like to return probability of L->L. So I give you a head up on a coming change. https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:41: default; On 2016/12/16 10:16:06, språng wrote: > maybe just define this in the header instead then, or even remove it and leave > it as implicit Done. https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:94: ForwardDiff(ReferenceSequenceNumber(), seq_num) >= kSeqNumHalf) { On 2016/12/16 10:16:06, språng wrote: > I know we have discussed this in the past, but it still feel having window size > based on num packets and kSeqNumHalf as size is excessive. > With a low rate this will turn into a gigantic window. For instance, 20 packets > per second will yield up to a 23min window size, yielding poor ability to > quickly adapt to changing network conditions and wasting memory. > There are many ways we could mitigate this. For instance, save a map from > sequence number to time, where time -1 means not received. Then you could easily > cull the tail of the window based on either time or size. I agree with you. But this can be a big change for the moment. Can temporarily rely on the max_window_size to avoid this? https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:137: // If repeated reports contradict each other, we let the later one rule. On 2016/12/16 10:16:06, språng wrote: > I think this is not in accordance with our draft spec: > > https://tools.ietf.org/html/draft-holmer-rmcat-transport-wide-cc-extensions-01 > > Note: In the case the base sequence number is decreased, creating a > window overlapping the previous feedback messages, the status for any > packets previously reported as received must be marked as "Packet not > received" and thus no delta included for that symbol. > > ...and I think neither is out implementation, which actually sends the > status/time twice in case of reordering :) > > I think you can return if (ret.first->second || ret.first->second == received) Done. https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:205: // goes to the end. On 2016/12/16 10:16:06, språng wrote: > nit: s/goes/go Done. https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.h:46: void Validate() const; // Only used for tests. On 2016/12/16 10:16:06, språng wrote: > nit: Join comments into one. Done. https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.h:60: void UndoPacketStatus(std::map<uint16_t, bool>::const_iterator it); On 2016/12/16 10:16:06, språng wrote: > can we typedef std::map<uint16_t, bool>::const_iterator to something shorter? Done.
Hi, I have made a change on output metric according to my last comment. It is simple to make such a change. PTAL again.
https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:124: // if older status said that the packet was lost but newer one says it If https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:125: // it received, we take the newer one. is received https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:138: if (received) { I would have split this method in two so that I had a method "SetPacketStatus" or something like that, which was the exact opposite of UndoPacketStatus below. I think it would improve readability. https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:195: return packet_status_window_.end(); How does this work? Let's say we have this: 1 2 3 ... FFFF ^---ref ^---it moving to the previous gives us: 1 2 3 ... FFFF ^---ref ^---it does this mean we'll never take the first element into account? Or maybe this shouldn't ever happen for some reason? https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:196: if (it == packet_status_window_.begin()) { Can this happen? Isn't it always between end() and ref_packet_status_? https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:218: void TransportFeedbackPacketLossTracker::Validate() const { // Testing only! I don't think we should have test code here. This should be moved to the unittest file. Perhaps we can write the test code in a less specific way to avoid accessing class internals? https://codereview.webrtc.org/2579613003/diff/100001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/100001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:24: class FuzzTransportFeedback : public TransportFeedback { Shouldn't it be possible to write this without inheritance? I think that would be a lot cleaner as we then won't have to add virtual to the methods of TransportFeedback just for this test.
Hi Stefan, Thanks for the comments! see my feedback. New code not yet submitted. Please provide your thoughts on my questions inline first. https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:124: // if older status said that the packet was lost but newer one says it On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > If Done. https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:125: // it received, we take the newer one. On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > is received Done. https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:138: if (received) { On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > I would have split this method in two so that I had a method "SetPacketStatus" > or something like that, which was the exact opposite of UndoPacketStatus below. > I think it would improve readability. Done. https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:195: return packet_status_window_.end(); On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > How does this work? > > Let's say we have this: > 1 2 3 ... FFFF > ^---ref > ^---it > > moving to the previous gives us: > 1 2 3 ... FFFF > ^---ref > ^---it > > does this mean we'll never take the first element into account? Or maybe this > shouldn't ever happen for some reason? It is important to know that ::end refers to "past-the-end element is the theoretical element that would follow the last element in the vector. It does not point to any element." Here we only use "end" for its "no ref" property. It is like "queue.find(sth) == queue.end()" means sth does not exist. Another solution may be DCHECK(it != ref_packet_status_), then we have to treat the case that "it == ref_packet_status_" before calling the method. https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:196: if (it == packet_status_window_.begin()) { On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > Can this happen? Isn't it always between end() and ref_packet_status_? no, this can happen, using elements between end() and ref_packet_status_ is not enough. say if ref_packet_status_ = 0xfffe, end() can not be larger than 0xffff (due to the natural ordering of map.) https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:218: void TransportFeedbackPacketLossTracker::Validate() const { // Testing only! On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > I don't think we should have test code here. This should be moved to the > unittest file. Perhaps we can write the test code in a less specific way to > avoid accessing class internals? To be honest, I have been struggling a lot on whether to place this here. Finally, I decided to do this for the following reasons 1. This is used in both unittests and fuzzer tests, so we don't want to duplicate the function 2. The method is very tight to the implementation details (as opposed to check input-output relation). It might be easier to maintain if we place it here. (say, if we change something, we are somehow reminded to change this method, otherwise, we may need to wait for units/fuzzer tests to fail to see such a need.) https://codereview.webrtc.org/2579613003/diff/100001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/100001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:24: class FuzzTransportFeedback : public TransportFeedback { On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > Shouldn't it be possible to write this without inheritance? I think that would > be a lot cleaner as we then won't have to add virtual to the methods of > TransportFeedback just for this test. It will somewhat complicated. TransportFeedback has a Build method, but it has its limitations and can be hard to use in our unittest and our fuzzer test. (and we don't need to test the logic of Build().) I don't think we want add any other public methods. Another idea is to create an abstract class for TransportFeedback. Then TransportFeedback can be "final" and the new tests (old tests can also) can inherit from the abstract class. WDYT?
Looks ok to me. Since I'll be OOO for a while I'll remove myself as reviewer and rely on stefan instead. https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:94: ForwardDiff(ReferenceSequenceNumber(), seq_num) >= kSeqNumHalf) { On 2016/12/16 12:51:51, minyue-webrtc wrote: > On 2016/12/16 10:16:06, språng wrote: > > I know we have discussed this in the past, but it still feel having window > size > > based on num packets and kSeqNumHalf as size is excessive. > > With a low rate this will turn into a gigantic window. For instance, 20 > packets > > per second will yield up to a 23min window size, yielding poor ability to > > quickly adapt to changing network conditions and wasting memory. > > There are many ways we could mitigate this. For instance, save a map from > > sequence number to time, where time -1 means not received. Then you could > easily > > cull the tail of the window based on either time or size. > > I agree with you. But this can be a big change for the moment. Can temporarily > rely on the max_window_size to avoid this? Ok, fair enough.
sprang@webrtc.org changed reviewers: - sprang@webrtc.org
Hi Stefan, The new patch set include changes according to your earlier comments. Please take a look at it plus some discussions brought up in my earlier comments. https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:195: return packet_status_window_.end(); On 2016/12/19 10:21:55, minyue-webrtc wrote: > On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > > How does this work? > > > > Let's say we have this: > > 1 2 3 ... FFFF > > ^---ref > > ^---it > > > > moving to the previous gives us: > > 1 2 3 ... FFFF > > ^---ref > > ^---it > > > > does this mean we'll never take the first element into account? Or maybe this > > shouldn't ever happen for some reason? > > It is important to know that ::end refers to "past-the-end element is the > theoretical element that would follow the last element in the vector. It does > not point to any element." Here we only use "end" for its "no ref" property. It > is like "queue.find(sth) == queue.end()" means sth does not exist. > > Another solution may be DCHECK(it != ref_packet_status_), then we have to treat > the case that "it == ref_packet_status_" before calling the method. I agree with you on the weirdness of this. Since Next(Prev(it)) might not equal it in some cases. I modified it a bit so that 1. if it == ref_packet_status_, no Prev(it) can be allowed. 2. if it == packet_status_window_.end(), Prev(it) points to the last element of the map
https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:195: return packet_status_window_.end(); On 2016/12/20 11:40:34, minyue-webrtc wrote: > On 2016/12/19 10:21:55, minyue-webrtc wrote: > > On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > > > How does this work? > > > > > > Let's say we have this: > > > 1 2 3 ... FFFF > > > ^---ref > > > ^---it > > > > > > moving to the previous gives us: > > > 1 2 3 ... FFFF > > > ^---ref > > > ^---it > > > > > > does this mean we'll never take the first element into account? Or maybe > this > > > shouldn't ever happen for some reason? > > > > It is important to know that ::end refers to "past-the-end element is the > > theoretical element that would follow the last element in the vector. It does > > not point to any element." Here we only use "end" for its "no ref" property. > It > > is like "queue.find(sth) == queue.end()" means sth does not exist. > > > > Another solution may be DCHECK(it != ref_packet_status_), then we have to > treat > > the case that "it == ref_packet_status_" before calling the method. > > I agree with you on the weirdness of this. Since Next(Prev(it)) might not equal > it in some cases. > > I modified it a bit so that > 1. if it == ref_packet_status_, no Prev(it) can be allowed. > 2. if it == packet_status_window_.end(), Prev(it) points to the last element of > the map I think this is a bit hard to follow, but I'm not sure it's possible to make it much simpler... https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:196: if (it == packet_status_window_.begin()) { On 2016/12/19 10:21:55, minyue-webrtc wrote: > On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > > Can this happen? Isn't it always between end() and ref_packet_status_? > > no, this can happen, using elements between end() and ref_packet_status_ is not > enough. say if > > ref_packet_status_ = 0xfffe, end() can not be larger than 0xffff (due to the > natural ordering of map.) Agree https://codereview.webrtc.org/2579613003/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/transport_feedback_packet_loss_tracker.cc:218: void TransportFeedbackPacketLossTracker::Validate() const { // Testing only! On 2016/12/19 10:21:55, minyue-webrtc wrote: > On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > > I don't think we should have test code here. This should be moved to the > > unittest file. Perhaps we can write the test code in a less specific way to > > avoid accessing class internals? > > To be honest, I have been struggling a lot on whether to place this here. > Finally, I decided to do this for the following reasons > 1. This is used in both unittests and fuzzer tests, so we don't want to > duplicate the function I don't think this is a good reason to have the test code be part of production code. You can have a helper file for your unittests which is shared by the unittests and the fuzzer. > 2. The method is very tight to the implementation details (as opposed to check > input-output relation). It might be easier to maintain if we place it here. > (say, if we change something, we are somehow reminded to change this method, > otherwise, we may need to wait for units/fuzzer tests to fail to see such a > need.) That's a bad sign. We shouldn't be writing test code that relies on implementation details in my opinion. Isn't what you truly care about the loss rates that you get from GetPacketLossRates()? Then I'd imagine that it should be possible to pass in a feedback message and put expectations on the loss rates instead? Another bad thing with this is that you will crash the test executable if these tests start failing. https://codereview.webrtc.org/2579613003/diff/100001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/100001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:24: class FuzzTransportFeedback : public TransportFeedback { On 2016/12/19 10:21:55, minyue-webrtc wrote: > On 2016/12/19 09:17:00, stefan-webrtc (holmer) wrote: > > Shouldn't it be possible to write this without inheritance? I think that would > > be a lot cleaner as we then won't have to add virtual to the methods of > > TransportFeedback just for this test. > > It will somewhat complicated. TransportFeedback has a Build method, but it has > its limitations and can be hard to use in our unittest and our fuzzer test. > (and we don't need to test the logic of Build().) I don't think we want add any > other public methods. > > Another idea is to create an abstract class for TransportFeedback. Then > TransportFeedback can be "final" and the new tests (old tests can also) can > inherit from the abstract class. WDYT? > I see. So basically what you want is a mock/fake version of TransportFeedback? Note that you'll probably test your class with more erroneous cases than what it will see in practice since the real TransportFeedback already checks some of these problems. This means we'll have checks for bitstream errors in two places. That's a reason why it may be better to built real TransportFeedback objects. If I would write a fuzzer I would have done something like this: while (size > 0) { if (size < sizeof(uint16_t)) return; size_t packet_length = ByteReader<uint16_t>(data); size -= packet_length; packet_length = std::min(packet_length, size); std::unique_ptr<TransportFeedback> feedback = TransportFeedback::ParseFrom(data, packet_length); size -= packet_length; tracker.OnReceivedTransportFeedback(feedback); }
Ok to leave the tests as is for now, but add a TODO to remove the Validation() method. As agreed, move this code to somewhere closer to channel.cc where it's used. Also please give some more details in the description. Thanks!
Description was changed from ========== Add TransportFeedbackPacketLossTracker. BUG=webrtc:6904 ========== to ========== Add TransportFeedbackPacketLossTracker. This CL is to calculate packet loss metrics from TransportFeedback. The outcome of this will be passed down to audio encoder. BUG=webrtc:6904 ==========
On 2016/12/20 12:39:33, stefan-webrtc (holmer) wrote: > Ok to leave the tests as is for now, but add a TODO to remove the Validation() > method. > > As agreed, move this code to somewhere closer to channel.cc where it's used. > Also please give some more details in the description. > > Thanks! Hi Stefan, I have moved this to voice engine, and added comments arround the Validate(). The CL description is also updated.
minyue@webrtc.org changed reviewers: + henrikg@webrtc.org
+henrikg for owner's review. Henrik, This is a tool to estimate packet loss metrics from TransportFeedback. The calculation is closed related to the definition of TransportFeedback, and I have asked Stefan and Erik to review. We decide to place it in voice engine since it will be only used for audio encoder, and voice engine bridges TransportFeedbackObserver and audio encoder. Would you please help review this? Thanks.
lgtm from my point of view, but the voice engine changes should be reviewed by an owner too. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:48: num_statuses = (num_statuses & 0xFFFE) + 1; // To make num_statuses > 1. I'd write this like this instead: num_statuses = std::max(num_statuses, 1);
minyue@webrtc.org changed reviewers: + kwiberg@webrtc.org
As discussed offline, kwiberg@ or solenberg@ should look at the VoiceEngine changes. I can owner stamp after review by kwiberg@. kwiberg@ should probably be added as owner of voice_engine.
On 2016/12/21 10:27:14, Henrik Grunell (WebRTC) wrote: > As discussed offline, kwiberg@ or solenberg@ should look at the VoiceEngine > changes. > > I can owner stamp after review by kwiberg@. > > kwiberg@ should probably be added as owner of voice_engine. I added Karl.
https://codereview.webrtc.org/2579613003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:50: virtual std::vector<TransportFeedback::StatusSymbol> GetStatusVector() const; I realize this is the quickest way to let you customize these two methods while keeping the rest of this class's behavior, but I'm really not a fan of state in base classes or using virtual methods for anything but interfaces. I'm lazy, and dislike having to work hard to read code... If I were you, I'd consider something like defining an interface for the behavior of these two methods, and letting the TransportFeedback constructor take an argument of this type. (It would probably be even better to turn TransportFeedback and/or Rtpfb into interfaces and have normal and fuzzer implementations of them, but this may be a lot more work than you want.) But don't do this if you don't think it'd be an improvement. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:50: while (statuses_.size() < num_statuses) { For simplicity, make the outer loop infinite, and rely on the exit check on line 64. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:86: rtc::ArrayView<const uint8_t> data_; const? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:99: uint32_t dummy = ByteReader<uint32_t>::ReadBigEndian(data); You're using this value below, so is it really a dummy? Also, it (and some more variables below) could be const. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:106: std::min(kSeqNumHalf, min_window_size + ((dummy >> 16) & 0x7FFF)); OK, so min_window_size and max_window_size are two arbitrary numbers such that 1 <= min_window_size <= max_window_size <= 0x8000 It took me two minutes to decode this, so a comment would be nice. Or maybe a simpler implementation, like this one: const size_t min_window_size = 1 + (0x7fff & ByteReader<uint16_t>::ReadBigEndian(data)); data += sizeof(uint16_t); const size_t max_window_size = 1 + (0x7fff & ByteReader<uint16_t>::ReadBigEndian(data)); data += sizeof(uint16_t); size += 2 * sizeof(uint16_t); if (max_window_size < min_window_size) swap(min_window_size, max_window_size); https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:21: constexpr uint16_t kSeqNumHalf = 0x8000u; Do you need the "u" suffix? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:34: RTC_DCHECK_GT(min_window_size, 0u); You can write just "0" nowadays. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:59: uint16_t diff = ForwardDiff(ReferenceSequenceNumber(), seq_num); const? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:85: uint16_t seq_num = base_seq_num; Initialize in the for loop, together with the other loop variable? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:95: bool received = fb_vector[i] != const? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:100: // Make sure that the window holds up to |max_window_size_| items. "up to" -> "at most" https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:109: size_t total = num_lost_packets_ + num_received_packets_; const? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class TransportFeedbackPacketLossTracker final { Bonus points for using "final"! https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:41: float* consecutive_packet_loss_rate) const; Consider returning Optional<PacketLossRates>, where struct PacketLossRates { float single_rate; float two_in_a_row_rate; }; https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:44: void Validate() const; Make this private, and DCHECK it at the beginning and/or end of methods? See webrtc/base/buffer.h for an example of this. Or is it too expensive? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:49: typedef std::map<uint16_t, bool> PacketStatus; Is there a difference between false and a missing entry? If so, document. If not, use std::set instead? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:33: constexpr uint16_t kBases[4] = {0x0000, 0x3456, 0xc032, 0xfffe}; Remove the explicit "4". https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:45: std::vector<bool> reception_status_vec) { Pass the vector by const reference? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:66: // Sanity check on an empty window Here and in several other comments: period at the end. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:78: // Sanity on partially filled window Sanity check. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:307: // Entries in the second quadrant treated like those in the first. What's a quadrant, in this context? They're mentioned in numerous comments, but you never define the term. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:335: // Insertion into the third quardrant moves the base of the window. spelling
On 2016/12/21 10:27:14, Henrik G WebRTC OOO back Jan10 wrote: > I can owner stamp after review by kwiberg@. I'm a root OWNER, so you don't have to do that. > kwiberg@ should probably be added as owner of voice_engine. Yes, that might be a good idea.
Hi Karl, Thanks for the comments! See my new patch set. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:50: virtual std::vector<TransportFeedback::StatusSymbol> GetStatusVector() const; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > I realize this is the quickest way to let you customize these two methods while > keeping the rest of this class's behavior, but I'm really not a fan of state in > base classes or using virtual methods for anything but interfaces. I'm lazy, and > dislike having to work hard to read code... > > If I were you, I'd consider something like defining an interface for the > behavior of these two methods, and letting the TransportFeedback constructor > take an argument of this type. (It would probably be even better to turn > TransportFeedback and/or Rtpfb into interfaces and have normal and fuzzer > implementations of them, but this may be a lot more work than you want.) But > don't do this if you don't think it'd be an improvement. I would really like to have a TransportFeedback interface. I was lazy and did not want to make a big change. But I think it should be a good thing to do and so I tried. See the new patch set. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:48: num_statuses = (num_statuses & 0xFFFE) + 1; // To make num_statuses > 1. On 2016/12/20 17:03:19, stefan-webrtc_OOO_Jan9 wrote: > I'd write this like this instead: > num_statuses = std::max(num_statuses, 1); sure.better to read although requires some casting. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:50: while (statuses_.size() < num_statuses) { On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > For simplicity, make the outer loop infinite, and rely on the exit check on line > 64. yes, nice! https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:86: rtc::ArrayView<const uint8_t> data_; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > const? Done. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:99: uint32_t dummy = ByteReader<uint32_t>::ReadBigEndian(data); On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > You're using this value below, so is it really a dummy? Also, it (and some more > variables below) could be const. not needed any longer. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:106: std::min(kSeqNumHalf, min_window_size + ((dummy >> 16) & 0x7FFF)); On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > OK, so min_window_size and max_window_size are two arbitrary numbers such that > > 1 <= min_window_size <= max_window_size <= 0x8000 > > It took me two minutes to decode this, so a comment would be nice. Or maybe a > simpler implementation, like this one: > > const size_t min_window_size = > 1 + (0x7fff & ByteReader<uint16_t>::ReadBigEndian(data)); > data += sizeof(uint16_t); > const size_t max_window_size = > 1 + (0x7fff & ByteReader<uint16_t>::ReadBigEndian(data)); > data += sizeof(uint16_t); > size += 2 * sizeof(uint16_t); > if (max_window_size < min_window_size) > swap(min_window_size, max_window_size); Yes, I did your suggestion but with some changes. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:21: constexpr uint16_t kSeqNumHalf = 0x8000u; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > Do you need the "u" suffix? maybe not. But I always get windows bot errors on "missing 'f' in float a = 0.0". So I try to add suffix whenever applicable. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:34: RTC_DCHECK_GT(min_window_size, 0u); On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > You can write just "0" nowadays. I noticed that. but maybe good to keep the habit? https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:59: uint16_t diff = ForwardDiff(ReferenceSequenceNumber(), seq_num); On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > const? Done. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:85: uint16_t seq_num = base_seq_num; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > Initialize in the for loop, together with the other loop variable? that was my initial attempt, but the variables to be initialized has to be the same type. now size_t and uint16_t respectively. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:95: bool received = fb_vector[i] != On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > const? Done. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:100: // Make sure that the window holds up to |max_window_size_| items. On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > "up to" -> "at most" Done. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:109: size_t total = num_lost_packets_ + num_received_packets_; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > const? Done. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:25: class TransportFeedbackPacketLossTracker final { On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > Bonus points for using "final"! Acknowledged. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:41: float* consecutive_packet_loss_rate) const; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > Consider returning Optional<PacketLossRates>, where > > struct PacketLossRates { > float single_rate; > float two_in_a_row_rate; > }; Actually, the return API is subject to a change. We do not really need packet_loss_rate for the moment. So I would like to change it to Optional<float> GetConsecutivePacketLossRate(), or something like that. To make this CL slightly more focused, I would do it as a follow up, or a later patch set. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:44: void Validate() const; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > Make this private, and DCHECK it at the beginning and/or end of methods? See > webrtc/base/buffer.h for an example of this. Or is it too expensive? We think it is too expensive. We currently keep this here to allow fuzzer test. Once it is good enough, we'd remove it (move it to unittest) https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:49: typedef std::map<uint16_t, bool> PacketStatus; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > Is there a difference between false and a missing entry? If so, document. If > not, use std::set instead? yes, missing entries are not taken into account say 1???01??1 we count 4 statuses, out of which 1 is lost. This way, we try to minimize the effect of feedback-losses on the downlink. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:33: constexpr uint16_t kBases[4] = {0x0000, 0x3456, 0xc032, 0xfffe}; On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > Remove the explicit "4". Done. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:45: std::vector<bool> reception_status_vec) { On 2016/12/22 02:31:42, kwiberg-webrtc wrote: > Pass the vector by const reference? yes of course. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:66: // Sanity check on an empty window On 2016/12/22 02:31:42, kwiberg-webrtc wrote: > Here and in several other comments: period at the end. sure. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:78: // Sanity on partially filled window On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > Sanity check. Done. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:307: // Entries in the second quadrant treated like those in the first. On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > What's a quadrant, in this context? They're mentioned in numerous comments, but > you never define the term. Ok. will define at the first mentioning of it. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:335: // Insertion into the third quardrant moves the base of the window. On 2016/12/22 02:31:42, kwiberg-webrtc wrote: > spelling eagle eye :)
Much better! Most of the stuff I commented on are optional---I believe the "final" and the requests for more comments are the only exceptions. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:106: std::min(kSeqNumHalf, min_window_size + ((dummy >> 16) & 0x7FFF)); On 2016/12/28 14:48:32, minyue-webrtc wrote: > On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > > OK, so min_window_size and max_window_size are two arbitrary numbers such that > > > > 1 <= min_window_size <= max_window_size <= 0x8000 > > > > It took me two minutes to decode this, so a comment would be nice. Or maybe a > > simpler implementation, like this one: > > > > const size_t min_window_size = > > 1 + (0x7fff & ByteReader<uint16_t>::ReadBigEndian(data)); > > data += sizeof(uint16_t); > > const size_t max_window_size = > > 1 + (0x7fff & ByteReader<uint16_t>::ReadBigEndian(data)); > > data += sizeof(uint16_t); > > size += 2 * sizeof(uint16_t); > > if (max_window_size < min_window_size) > > swap(min_window_size, max_window_size); > > Yes, I did your suggestion but with some changes. Acknowledged. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:21: constexpr uint16_t kSeqNumHalf = 0x8000u; On 2016/12/28 14:48:32, minyue-webrtc wrote: > On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > > Do you need the "u" suffix? > > maybe not. But I always get windows bot errors on "missing 'f' in float a = > 0.0". So I try to add suffix whenever applicable. It's better to be in the habit of omitting extra cruft unless you're not absolutely sure it's necessary, and adding it when the bots tell you to. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:34: RTC_DCHECK_GT(min_window_size, 0u); On 2016/12/28 14:48:32, minyue-webrtc wrote: > On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > > You can write just "0" nowadays. > > I noticed that. but maybe good to keep the habit? Not really. It's better to start out writing code without cruft you don't *know* is necessary, and let the bots tell you when you were too optimistic. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:85: uint16_t seq_num = base_seq_num; On 2016/12/28 14:48:32, minyue-webrtc wrote: > On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > > Initialize in the for loop, together with the other loop variable? > > that was my initial attempt, but the variables to be initialized has to be the > same type. now size_t and uint16_t respectively. Oh, right. Then how about const uint16_t seq_num = i + base_seq_num; in the loop body? (With a cast. static_cast, since we don't have dchecked_cast...) That should be even easier to read, because of the const and the reduced scope. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:41: float* consecutive_packet_loss_rate) const; On 2016/12/28 14:48:33, minyue-webrtc wrote: > On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > > Consider returning Optional<PacketLossRates>, where > > > > struct PacketLossRates { > > float single_rate; > > float two_in_a_row_rate; > > }; > > Actually, the return API is subject to a change. We do not really need > packet_loss_rate for the moment. So I would like to change it to > > Optional<float> GetConsecutivePacketLossRate(), or something like that. > > To make this CL slightly more focused, I would do it as a follow up, or a later > patch set. Acknowledged. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:44: void Validate() const; On 2016/12/28 14:48:33, minyue-webrtc wrote: > On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > > Make this private, and DCHECK it at the beginning and/or end of methods? See > > webrtc/base/buffer.h for an example of this. Or is it too expensive? > > We think it is too expensive. We currently keep this here to allow fuzzer test. > Once it is good enough, we'd remove it (move it to unittest) Acknowledged. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:49: typedef std::map<uint16_t, bool> PacketStatus; On 2016/12/28 14:48:33, minyue-webrtc wrote: > On 2016/12/22 02:31:41, kwiberg-webrtc wrote: > > Is there a difference between false and a missing entry? If so, document. If > > not, use std::set instead? > > yes, missing entries are not taken into account > > say 1???01??1 > > we count 4 statuses, out of which 1 is lost. This way, we try to minimize the > effect of feedback-losses on the downlink. Please document this. https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2579613003/diff/140001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:335: // Insertion into the third quardrant moves the base of the window. On 2016/12/28 14:48:33, minyue-webrtc wrote: > On 2016/12/22 02:31:42, kwiberg-webrtc wrote: > > spelling > > eagle eye :) No, I was just searching for uses of "quadrant", and noticed a curious lack of a search hit here.... https://codereview.webrtc.org/2579613003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:46: class TransportFeedback : public Rtpfb, public TransportFeedbackInterface { Can this class be final now? https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:47: num_statuses = std::max(num_statuses, static_cast<uint16_t>(1)); I think you can skip the cast if you specify the template argument explicitly: num_statuses = std::max<uint16_t>(...); https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:52: break; Maybe say "return" here, for clarity? Your call. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:125: static_cast<size_t>(std::max(static_cast<uint16_t>(1), Specify template argument explicitly and avoid cast? https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:130: static_cast<size_t>(std::max(static_cast<uint16_t>(1), Here too? https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:303: // The sequence number is used in a looped manner. 0xFFFF is followed by 0x0000. I think the usual term is "wrapped", as in "after 0xffff, the sequence number wraps back to 0x0000". https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:305: // verify the behavior of TransportFeedbackPacketLossTracker over them. So each quadrant is 14 bits' worth of sequence numbers? With no fixed position (i.e. the first quadrant being 0x0000-0x3ffff, etc.) because the absolute offset doesn't matter? Probably worth spelling out (especially if I still got it wrong!). https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:401: TEST(TransportFeedbackPacketLossTrackerTest, ThirdQuadrantObservesMaxWindow) { Aha, who's the eagle eye, I wonder...
elad.alon@webrtc.org changed reviewers: + elad.alon@webrtc.org
https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:187: const auto& pre = PreviousPacketStatus(it); (See comment below first.) What if there is no previous element, due to [it] being the only element? Here we'll get correct behavior because the current implementation would just return [it] itself, and its sequence number won't be the expected sequence number. But I think that would be brittle. Better, IMHO, to fix PreviousPacketStatus to report no-previous-element, and check for this indication here, like we do for NextPacketStatus. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:206: if (it == packet_status_window_.begin()) { If the window only has one element, and you call PreviousPacketStatus on that specific element, the result would be the same element, right? I'm assuming this is not intentional?
https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: RTC_DCHECK(it != ref_packet_status_); Okay, I see I've missed that. If you think this is enough, please ignore my previous comments.
Hi reviewers, Comments are addressed and please TLA. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:46: class TransportFeedback : public Rtpfb, public TransportFeedbackInterface { On 2017/01/05 02:51:12, kwiberg-webrtc wrote: > Can this class be final now? sorry, I wanted to make it that way but PacketCounter (test code) makes it hard, see https://cs.chromium.org/chromium/src/third_party/webrtc/test/rtcp_packet_pars... Fixing PacketCounter may be a bit out-reached. but can be a follow-up. I add a TODO here. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:47: num_statuses = std::max(num_statuses, static_cast<uint16_t>(1)); On 2017/01/05 02:51:12, kwiberg-webrtc wrote: > I think you can skip the cast if you specify the template argument explicitly: > > num_statuses = std::max<uint16_t>(...); oh, sounds nice https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:52: break; On 2017/01/05 02:51:12, kwiberg-webrtc wrote: > Maybe say "return" here, for clarity? Your call. that is better. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:125: static_cast<size_t>(std::max(static_cast<uint16_t>(1), On 2017/01/05 02:51:12, kwiberg-webrtc wrote: > Specify template argument explicitly and avoid cast? Done. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:130: static_cast<size_t>(std::max(static_cast<uint16_t>(1), On 2017/01/05 02:51:12, kwiberg-webrtc wrote: > Here too? Done. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:187: const auto& pre = PreviousPacketStatus(it); On 2017/01/13 12:28:04, elad.alon wrote: > (See comment below first.) > What if there is no previous element, due to [it] being the only element? Here > we'll get correct behavior because the current implementation would just return > [it] itself, and its sequence number won't be the expected sequence number. But > I think that would be brittle. Better, IMHO, to fix PreviousPacketStatus to > report no-previous-element, and check for this indication here, like we do for > NextPacketStatus. See my answers below. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:199: RTC_DCHECK(it != ref_packet_status_); On 2017/01/13 12:34:44, elad.alon wrote: > Okay, I see I've missed that. If you think this is enough, please ignore my > previous comments. I actually wanted to mimic the behavior of std iterator. The element before "begin" is undefined behavior. See http://stackoverflow.com/questions/19417171/stl-iterator-before-stdmapbegin So the caller has to be careful, which is roughly what the DCHECK here does. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:206: if (it == packet_status_window_.begin()) { On 2017/01/13 12:28:04, elad.alon wrote: > If the window only has one element, and you call PreviousPacketStatus on that > specific element, the result would be the same element, right? I'm assuming this > is not intentional? no. If the window contains only one element, the DCHECK on line 199 will be already triggered. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:303: // The sequence number is used in a looped manner. 0xFFFF is followed by 0x0000. On 2017/01/05 02:51:12, kwiberg-webrtc wrote: > I think the usual term is "wrapped", as in "after 0xffff, the sequence number > wraps back to 0x0000". the word was chosen to explain why "quadrant", as a name, makes sense. I now change "looped" to "circular", and see if that reads easier. https://codereview.webrtc.org/2579613003/diff/180001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:305: // verify the behavior of TransportFeedbackPacketLossTracker over them. On 2017/01/05 02:51:12, kwiberg-webrtc wrote: > So each quadrant is 14 bits' worth of sequence numbers? With no fixed position > (i.e. the first quadrant being 0x0000-0x3ffff, etc.) because the absolute offset > doesn't matter? Probably worth spelling out (especially if I still got it > wrong!). I updated the wording here. See if that helps.
Rebased also. ~~~~~ Then I want Erik to take a look at the TransportFeedbackInterface https://codereview.webrtc.org/2579613003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2579613003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:23: class TransportFeedbackInterface { Hi Erik, I would like you take a look at my way of abstracting TransportFeedback. I may include more (or less) APIs than necessary.
danilchap@webrtc.org changed reviewers: + danilchap@webrtc.org
Recently I refactored TransportFeedback hoping it become usable without mocking. was it good enough for that? https://codereview.webrtc.org/2579613003/diff/200001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2579613003/diff/200001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:41: std::vector<TransportFeedback::StatusSymbol> statuses; e.g. here this implementation might be used instead of mock: const int64_t kBaseTimeUs = 42; TransportFeedback test_feedback; test_feedback.SetBase(base_sequence_number, kBaseTimeUs); uint16_t seq_no = base_sequence_number; for (bool status : reception_status_vec) { if (status) test_feedback.AddReceivedPacket(seq_no, kBaseTimeUs); ++seq_no; } tracker->OnReceivedTransportFeedback(test_feedback); tracker->Validate();
https://codereview.webrtc.org/2579613003/diff/200001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2579613003/diff/200001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:41: std::vector<TransportFeedback::StatusSymbol> statuses; On 2017/01/19 11:26:54, danilchap wrote: > e.g. here this implementation might be used instead of mock: > > const int64_t kBaseTimeUs = 42; > TransportFeedback test_feedback; > test_feedback.SetBase(base_sequence_number, kBaseTimeUs); > uint16_t seq_no = base_sequence_number; > for (bool status : reception_status_vec) { > if (status) > test_feedback.AddReceivedPacket(seq_no, kBaseTimeUs); > ++seq_no; > } > tracker->OnReceivedTransportFeedback(test_feedback); > tracker->Validate(); right. will do
Hi Danil, I undid my mocking and used the newly added method in TransportFeedback instead. The only limitation of the real TransportFeedback is that it needs at least a received packet to work. Therefore, I must remove the AllLost unittest. ~~~~~~ Hi Karl, You may still like the interface, but it becomes unnecessary after new APIs Danil introduced in TransportFeedback. So I reverted it. I hope you don't mind.
thank you, rtcp::TransportFeedback usage lgtm. Yes, TransportFeedback doesn't support all-lost feedbacks. That use case doesn't look real to me: transport sequence number considered missing when larger sequence number was received, i.e. there should be at least one reported received packet. I can add all-lost support if you plan to create this kind of feedbacks. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:96: fb_vector[i] != GetStatusVector is unused now, so I can adjusted it to your need : remove enum StatusSymbol and make it return std::vector<bool> simplifying both it's implementation and this line. Want me to?
if all-lost is not a valid case, I do not recommend you add it in TransportFeedback. We tested it because the tracker we introduce here can handle it. But it comes as a natural outcome. No specific need to test that. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:96: fb_vector[i] != On 2017/01/19 16:24:29, danilchap wrote: > GetStatusVector is unused now, so I can adjusted it to your need : remove enum > StatusSymbol and make it return std::vector<bool> simplifying both it's > implementation and this line. Want me to? I cannot judge the usefulness of StatusSymbol::ReceivedSmallDelta/kReceivedLargeDelta, so it is up to your team to decide. I can only claim that they are not needed and will unlikely be needed in this use case.
On 2017/01/19 17:10:15, minyue-webrtc wrote: > if all-lost is not a valid case, I do not recommend you add it in > TransportFeedback. We tested it because the tracker we introduce here can handle > it. But it comes as a natural outcome. No specific need to test that. > > https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... > File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): > > https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... > webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:96: fb_vector[i] > != > On 2017/01/19 16:24:29, danilchap wrote: > > GetStatusVector is unused now, so I can adjusted it to your need : remove enum > > StatusSymbol and make it return std::vector<bool> simplifying both it's > > implementation and this line. Want me to? > > I cannot judge the usefulness of > StatusSymbol::ReceivedSmallDelta/kReceivedLargeDelta, so it is up to your team > to decide. I can only claim that they are not needed and will unlikely be needed > in this use case. Hi reviewers, A kindly ping for final comments. ~~~~ Karl, Now the mocking of TransportFeedback is removed. The CL becomes limited to VoE, which makes it clearer. ~~~ Elad, Look at my replies to your comments and see if they make sense.
lgtm
https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker_unittest.cc:36: void AddTransportFeedbackAndValidate( > The only limitation of the real TransportFeedback is that it needs at least a received packet to work. With the mocking gone, we can no use a status vector which terminates in a lost packet. So not only {LOST} is impossible, but {RECEIVED, LOST} is also impossible. I'll add an RTC_CHECK for that in my CL; please don't delay over this.
lgtm + some minor comments https://codereview.webrtc.org/2579613003/diff/240001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:22: using TransportFeedback = rtcp::TransportFeedback; I'm counting just two uses of this alias. You can probably remove it. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:176: RTC_DCHECK_GT(num_received_packets_, 0u); I think I've already told you this, but the "u" suffix is no longer needed. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017! This probably applies to other files too. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:48: // true if it is received. What does false mean? What does not present mean?
https://codereview.webrtc.org/2579613003/diff/240001/webrtc/test/fuzzers/tran... File webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. 2017 https://codereview.webrtc.org/2579613003/diff/240001/webrtc/test/fuzzers/tran... webrtc/test/fuzzers/transport_feedback_packet_loss_tracker_fuzzer.cc:22: using TransportFeedback = rtcp::TransportFeedback; On 2017/01/23 14:04:36, kwiberg-webrtc wrote: > I'm counting just two uses of this alias. You can probably remove it. Done. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:176: RTC_DCHECK_GT(num_received_packets_, 0u); On 2017/01/23 14:04:36, kwiberg-webrtc wrote: > I think I've already told you this, but the "u" suffix is no longer needed. ah, yes. I forgot to take actions. apologies. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/01/23 14:04:36, kwiberg-webrtc wrote: > 2017! > > This probably applies to other files too. Done. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:48: // true if it is received. On 2017/01/23 14:04:36, kwiberg-webrtc wrote: > What does false mean? What does not present mean? updated the comment.
The CQ bit was checked by minyue@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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/17908) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/10185) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/10093) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/15452) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/22412) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, no build URL) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5513) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, no build URL)
https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.cc:176: RTC_DCHECK_GT(num_received_packets_, 0u); On 2017/01/23 14:29:38, minyue-webrtc wrote: > On 2017/01/23 14:04:36, kwiberg-webrtc wrote: > > I think I've already told you this, but the "u" suffix is no longer needed. > > ah, yes. I forgot to take actions. apologies. Acknowledged. https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... File webrtc/voice_engine/transport_feedback_packet_loss_tracker.h (right): https://codereview.webrtc.org/2579613003/diff/240001/webrtc/voice_engine/tran... webrtc/voice_engine/transport_feedback_packet_loss_tracker.h:48: // true if it is received. On 2017/01/23 14:29:38, minyue-webrtc wrote: > On 2017/01/23 14:04:36, kwiberg-webrtc wrote: > > What does false mean? What does not present mean? > > updated the comment. Excellent.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, danilchap@webrtc.org, kwiberg@webrtc.org, elad.alon@webrtc.org Link to the patchset: https://codereview.webrtc.org/2579613003/#ps260001 (title: "on comments")
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": 260001, "attempt_start_ts": 1485185382785540,
"parent_rev": "ed582f7e36773c50d359fc97525e17bbf3d1354c", "commit_rev":
"435ddf978d83794c16f842ba779e22b5d96ff161"}
Message was sent while issue was closed.
Description was changed from ========== Add TransportFeedbackPacketLossTracker. This CL is to calculate packet loss metrics from TransportFeedback. The outcome of this will be passed down to audio encoder. BUG=webrtc:6904 ========== to ========== Add TransportFeedbackPacketLossTracker. This CL is to calculate packet loss metrics from TransportFeedback. The outcome of this will be passed down to audio encoder. BUG=webrtc:6904 Review-Url: https://codereview.webrtc.org/2579613003 Cr-Commit-Position: refs/heads/master@{#16217} Committed: https://chromium.googlesource.com/external/webrtc/+/435ddf978d83794c16f842ba7... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001) as https://chromium.googlesource.com/external/webrtc/+/435ddf978d83794c16f842ba7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
