WebRTC Code Reviews
Help | Chromium Project | Sign in
(194)

Issue 51119004: Miscellaneous cleanups in VCMReceiver and its unit tests. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 4 months ago by wtc
Modified:
4 years, 4 months ago
Reviewers:
Stefan
CC:
webrtc-reviews_webrtc.org, tterriberry, Stefan, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Miscellaneous cleanups in VCMReceiver and its unit tests. The most important change is to prevent a potential buffer overflow in NackList(). It cannot happen if the |size| argument passed to NackList() is consistent with the |max_nack_list_size| argument passed to SetNackSettings(), and there is an assertion to check that. But it is good to defend against this in the release build because assert() is compiled away in the release build. Remove the unused |master| parameter to the VCMReceiver constructor. Remove the unused State() getter method and the corresponding state_ member. Remove the declarations for the nonexistent GenerateReceiverId() method and the receiver_id_counter_ member. Remove the unneeded data_buffer_ member of TestVCMReceiver. It was assigned to packet.dataPtr and then immediately overwritten by stream_generator_->GetPacket() or stream_generator_->PopPacket(). R=stefan@webrtc.org BUG=none TEST=none Committed: https://crrev.com/92d94898814c2a027bda3993da6510340145720c Cr-Commit-Position: refs/heads/master@{#9318}

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -32 lines) Patch
M webrtc/modules/video_coding/main/source/receiver.h View 3 chunks +1 line, -14 lines 0 comments Download
M webrtc/modules/video_coding/main/source/receiver.cc View 4 chunks +4 lines, -9 lines 2 comments Download
M webrtc/modules/video_coding/main/source/receiver_unittest.cc View 3 chunks +2 lines, -8 lines 0 comments Download
M webrtc/modules/video_coding/main/source/video_receiver.cc View 1 chunk +1 line, -1 line 0 comments Download
Trybot results:
Project "webrtc" does not have a commit queue.

Messages

Total messages: 6 (1 generated)
wtc
https://webrtc-codereview.appspot.com/51119004/diff/1/webrtc/modules/video_coding/main/source/receiver.cc File webrtc/modules/video_coding/main/source/receiver.cc (right): https://webrtc-codereview.appspot.com/51119004/diff/1/webrtc/modules/video_coding/main/source/receiver.cc#newcode218 webrtc/modules/video_coding/main/source/receiver.cc:218: assert(*nack_list_length <= size); I verified that assert() is compiled ...
4 years, 4 months ago (2015-05-27 22:57:56 UTC) #2
Stefan
Thanks for the cleanup. I agree that the methods for generating nack lists are a ...
4 years, 4 months ago (2015-05-28 13:56:18 UTC) #3
Stefan
lgtm
4 years, 4 months ago (2015-05-28 13:56:40 UTC) #4
wtc
Committed patchset #1 (id:1) manually as 92d94898814c2a027bda3993da6510340145720c (presubmit successful).
4 years, 4 months ago (2015-05-28 20:36:27 UTC) #5
commit-bot
4 years, 4 months ago (2015-05-28 20:36:33 UTC) #6
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/92d94898814c2a027bda3993da6510340145720c
Cr-Commit-Position: refs/heads/master@{#9318}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted