Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2107703002:
Updated comments and renaming of variables in ForwardErrorCorrection. (Closed)
|
Created:
4 years, 5 months ago by brandtr Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@flexfec-pt1b_use-unique_ptr-in-forward-error-correction Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdated comments and renaming of variables in ForwardErrorCorrection.
This CL should have no changes to functionality.
BUG=webrtc:5654
Committed: https://crrev.com/3e2d6ac0c06f5e133f7fe64fdc8dd6145a2e42a6
Cr-Commit-Position: refs/heads/master@{#13690}
Patch Set 1 : Initial. #Patch Set 2 : Rebase. #Patch Set 3 : Rebase on top of "1b". #
Total comments: 16
Patch Set 4 : Feedback response. #
Total comments: 2
Patch Set 5 : Switch to // comments. #Patch Set 6 : Rebase. #
Depends on Patchset: Messages
Total messages: 19 (9 generated)
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
Patchset #3 (id:40001) has been deleted
Description was changed from ========== FlexFEC pt. 1c: Renames and recommenting in ForwardErrorCorrection. Some variables have been renamed for consistency. Some comments have been updated. BUG=webrtc:5654 ========== to ========== Comments and renaming in ForwardErrorCorrection. Comments have been updated and variables have been renamed for consistency. BUG=webrtc:5654 ==========
Description was changed from ========== Comments and renaming in ForwardErrorCorrection. Comments have been updated and variables have been renamed for consistency. BUG=webrtc:5654 ========== to ========== Updated comments and renaming of variables in ForwardErrorCorrection. BUG=webrtc:5654 ==========
Rebased.
https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:457: // Create a recovered packet for this received media packet. word 'this' that has special meaning in c++ is misleading here. may be rephase to "Create a recovered packet for |received_packet|" (|name| is common way to refer to parameter in a comment) https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:121: * |Packet|. All packets must belong to the I think |something| is used to refer to parameter name, not type. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:146: * Output: fec_packets List of pointers to generated FEC packets, As a another refactoring step it might be better to have only input parameters and return std::list<> directly (it is a movable type, so it is cheap to return it) with empty list indicating an error. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:171: * Input: received_packets List of new received packets, of type with a detailed comment what function does, description of the individual parameters seems redundant. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:195: // Reset internal states from last frame. why remove note that |recovered_packets| are cleaned? (they still are) https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:261: // Inserts received packets into received FEC packet list or may be hint how this function decide which list to choose as target? when refer to parameter, enclose it into ||: Inserts |received_packets| into ... or into |recovered_packets|. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:266: // Inserts media packet into recovered packet list. We delete duplicates. Remove 'We'. e.g. Inserts |received_packet| into |recovered_packets|. Deletes duplicates. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:294: // Initializes packet recovery using the received FEC packet. |fec_packet| instead of FEC packet
Same here :) https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:457: // Create a recovered packet for this received media packet. On 2016/07/01 15:58:22, danilchap wrote: > word 'this' that has special meaning in c++ is misleading here. > may be rephase to "Create a recovered packet for |received_packet|" > (|name| is common way to refer to parameter in a comment) It's actually pretty clear what this code does, so I just removed the whole line. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:121: * |Packet|. All packets must belong to the On 2016/07/01 15:58:22, danilchap wrote: > I think |something| is used to refer to parameter name, not type. Done. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:146: * Output: fec_packets List of pointers to generated FEC packets, On 2016/07/01 15:58:23, danilchap wrote: > As a another refactoring step it might be better to have only input parameters > and return std::list<> directly (it is a movable type, so it is cheap to return > it) > with empty list indicating an error. Sounds like a good idea, but I'll leave that for future changes in this class. (These are coming later.) For now, I'll leave it as is, as there currently is a difference between success with no generated FEC packets and failure with no generated FEC packets. See https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:171: * Input: received_packets List of new received packets, of type On 2016/07/01 15:58:23, danilchap wrote: > with a detailed comment what function does, description of the individual > parameters seems redundant. Yes, but there is a little bit of extra information in the description of the parameters. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:195: // Reset internal states from last frame. On 2016/07/01 15:58:22, danilchap wrote: > why remove note that |recovered_packets| are cleaned? (they still are) Yep, but I thought it was redundant to write this and still keep the line below. I'll add it back to be clear. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:261: // Inserts received packets into received FEC packet list or On 2016/07/01 15:58:23, danilchap wrote: > may be hint how this function decide which list to choose as target? > when refer to parameter, enclose it into ||: > Inserts |received_packets| into ... or into |recovered_packets|. Done. https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:266: // Inserts media packet into recovered packet list. We delete duplicates. On 2016/07/01 15:58:23, danilchap wrote: > Remove 'We'. e.g. > Inserts |received_packet| into |recovered_packets|. Deletes duplicates. Done.
lgtm https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2107703002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:195: // Reset internal states from last frame. On 2016/07/04 13:57:39, brandtr wrote: > On 2016/07/01 15:58:22, danilchap wrote: > > why remove note that |recovered_packets| are cleaned? (they still are) > > Yep, but I thought it was redundant to write this and still keep the line below. > I'll add it back to be clear. Not that redundant: it is not obvious that content of the input parameter |recovered_packets| was allocated by this class.
lgtm https://codereview.webrtc.org/2107703002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2107703002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:117: /** It would be nice to change the comments to // style instead.
https://codereview.webrtc.org/2107703002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2107703002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:117: /** On 2016/07/05 17:21:38, stefan-webrtc (holmer) wrote: > It would be nice to change the comments to // style instead. Done.
Description was changed from ========== Updated comments and renaming of variables in ForwardErrorCorrection. BUG=webrtc:5654 ========== to ========== Updated comments and renaming of variables in ForwardErrorCorrection. This CL should have no changes to functionality. BUG=webrtc:5654 ==========
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2107703002/#ps120001 (title: "Rebase.")
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 ========== Updated comments and renaming of variables in ForwardErrorCorrection. This CL should have no changes to functionality. BUG=webrtc:5654 ========== to ========== Updated comments and renaming of variables in ForwardErrorCorrection. This CL should have no changes to functionality. BUG=webrtc:5654 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Updated comments and renaming of variables in ForwardErrorCorrection. This CL should have no changes to functionality. BUG=webrtc:5654 ========== to ========== Updated comments and renaming of variables in ForwardErrorCorrection. This CL should have no changes to functionality. BUG=webrtc:5654 Committed: https://crrev.com/3e2d6ac0c06f5e133f7fe64fdc8dd6145a2e42a6 Cr-Commit-Position: refs/heads/master@{#13690} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3e2d6ac0c06f5e133f7fe64fdc8dd6145a2e42a6 Cr-Commit-Position: refs/heads/master@{#13690} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
