Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3000713002:
Add audio_level member to RtpSource and set it from RtpReceiverImpl::IncomingRtpPacket. (Closed)
|
Created:
3 years, 4 months ago by Zach Stein Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, danilchap, zhuangzesen_agora.io, stefan-webrtc, tterriberry_mozilla.com, kwiberg-webrtc, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd audio_level member to RtpSource and set it from RtpReceiverImpl::IncomingRtpPacket.
BUG=webrtc:7987
Review-Url: https://codereview.webrtc.org/3000713002
Cr-Commit-Position: refs/heads/master@{#19503}
Committed: https://chromium.googlesource.com/external/webrtc/+/2b706343de6f79839f46d12008ec3c8062bb94bc
Patch Set 1 #
Total comments: 12
Patch Set 2 : Move audio level update to critical section protected method and address other feedback. #
Total comments: 2
Patch Set 3 : Move set_audio_level before RemoveOutdatedSources. #
Total comments: 11
Patch Set 4 : Minor fixes as suggested by pthatcher. #
Total comments: 7
Patch Set 5 : Style feedback from deadbeef #
Messages
Total messages: 34 (16 generated)
zstein@webrtc.org changed reviewers: + stefan@webrtc.org
PTAL
zstein@webrtc.org changed reviewers: + danilchap@webrtc.org - stefan@webrtc.org
PTAL
https://codereview.webrtc.org/3000713002/diff/1/webrtc/api/rtpreceiverinterfa... File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/3000713002/diff/1/webrtc/api/rtpreceiverinterfa... webrtc/api/rtpreceiverinterface.h:64: void update_audio_level(rtc::Optional<uint8_t> level) { write acessor usually has set_ prefix: set_audio_level (see sample at https://google.github.io/styleguide/cppguide.html#Function_Names) https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:168: auto source = ssrc_sources_.rbegin(); though it is not annotated, it seems ssrc_sources_ is guarded by critical_section_rtp_receiver_ it might be better to set audio_level inside UpdateSources function https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:169: source->update_audio_level( minor suggestion to use back() instead of rbegin(): ssrc_sources_.back().set_audio_level(... (feel free to ignore if you prefer rbegin) https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc (right): https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:273: EXPECT_THAT(sources, UnorderedElementsAre(RtpSource( ASSERT_THAT otherwise next line might crash if this expectation doesn't hold because sources is empty https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:275: ASSERT_EQ(rtc::Optional<uint8_t>(10), sources.begin()->audio_level()); there is a compare operator between optional<T> and T, i.e. you can write just EXPECT_EQ(10, sources.front().audio_level()); (here ASSERT might be EXPECT if you want to continue test should it fail) https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:307: may be add a test that absent of audio level extensions resets audio level
Thanks for the feedback - I think I addressed everything. https://codereview.webrtc.org/3000713002/diff/1/webrtc/api/rtpreceiverinterfa... File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/3000713002/diff/1/webrtc/api/rtpreceiverinterfa... webrtc/api/rtpreceiverinterface.h:64: void update_audio_level(rtc::Optional<uint8_t> level) { On 2017/08/15 08:18:29, danilchap wrote: > write acessor usually has set_ prefix: > set_audio_level > (see sample at https://google.github.io/styleguide/cppguide.html#Function_Names) I was following the style of this class (update_timestamp_ms), but I'll switch to set_. https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:168: auto source = ssrc_sources_.rbegin(); On 2017/08/15 08:18:29, danilchap wrote: > though it is not annotated, it seems ssrc_sources_ is guarded by > critical_section_rtp_receiver_ > it might be better to set audio_level inside UpdateSources function Done. https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:169: source->update_audio_level( On 2017/08/15 08:18:29, danilchap wrote: > minor suggestion to use back() instead of rbegin(): > ssrc_sources_.back().set_audio_level(... > (feel free to ignore if you prefer rbegin) Done. https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc (right): https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:273: EXPECT_THAT(sources, UnorderedElementsAre(RtpSource( On 2017/08/15 08:18:29, danilchap wrote: > ASSERT_THAT > otherwise next line might crash if this expectation doesn't hold because sources > is empty The following line is actually redundant with this EXPECT_THAT now (I wrote the following before I added the RtpSource constructor and operator== and forgot to delete it). I'll just rely on the EXPECT_THAT(..., UnorderedElementsAre(...)). https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:275: ASSERT_EQ(rtc::Optional<uint8_t>(10), sources.begin()->audio_level()); On 2017/08/15 08:18:29, danilchap wrote: > there is a compare operator between optional<T> and T, i.e. you can write just > EXPECT_EQ(10, sources.front().audio_level()); > (here ASSERT might be EXPECT if you want to continue test should it fail) Acknowledged. https://codereview.webrtc.org/3000713002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:307: On 2017/08/15 08:18:29, danilchap wrote: > may be add a test that absent of audio level extensions resets audio level Done.
lgtm https://codereview.webrtc.org/3000713002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/3000713002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:536: ssrc_sources_.back().set_audio_level(audio_level); this line probably looks better before RemoveOutdatedSources. then it is obvious why last element is updated and why it is safe to update. (as it is now need to ensure RemoveOutdatedSources do not remove last element or reorder them.)
The CQ bit was checked by zstein@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/3000713002/#ps40001 (title: "Move set_audio_level before RemoveOutdatedSources.")
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 zstein@webrtc.org
zstein@webrtc.org changed reviewers: + pthatcher@webrtc.org
@pthatcher can you take a quick look at the /api change? Thanks! https://codereview.webrtc.org/3000713002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/3000713002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:536: ssrc_sources_.back().set_audio_level(audio_level); On 2017/08/16 10:31:40, danilchap wrote: > this line probably looks better before RemoveOutdatedSources. > then it is obvious why last element is updated and why it is safe to update. (as > it is now need to ensure RemoveOutdatedSources do not remove last element or > reorder them.) Done.
pthatcher@google.com changed reviewers: + pthatcher@google.com
Just nits https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... webrtc/api/rtpreceiverinterface.h:41: audio_level_() {} Do you even need to do audio_level_() here? I thought rtc::Optional took care of that with adding it to the constructor. https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... webrtc/api/rtpreceiverinterface.h:50: audio_level_(audio_level) {} Is the other constructor (without an audio_level) even called by anything other than unit tests? If not, why not just have one constructor and update the unit tests to use this constructor (with the audio_level)? https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... webrtc/api/rtpreceiverinterface.h:65: void set_audio_level(rtc::Optional<uint8_t> level) { audio_level_ = level; } I think "const rtc::Optional<T>&" is usually how we pass them. https://codereview.webrtc.org/3000713002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/3000713002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:169: : rtc::Optional<uint8_t>(); Seems like rtp_header.extension.audioLevel should be an rtc::Optional. Out of scope for this CL, most likely, though. https://codereview.webrtc.org/3000713002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:507: void RtpReceiverImpl::UpdateSources(rtc::Optional<uint8_t> audio_level) { Again with const X& instead of X.
zhihuang@webrtc.org changed reviewers: + zhihuang@webrtc.org
https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... webrtc/api/rtpreceiverinterface.h:50: audio_level_(audio_level) {} On 2017/08/21 22:55:40, pthatcher wrote: > Is the other constructor (without an audio_level) even called by anything other > than unit tests? If not, why not just have one constructor and update the unit > tests to use this constructor (with the audio_level)? Some internal tests will call the other constructor, so we might need to keep both of them for now and leave a TODO to remove that.
https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... File webrtc/api/rtpreceiverinterface.h (right): https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... webrtc/api/rtpreceiverinterface.h:41: audio_level_() {} On 2017/08/21 22:55:39, pthatcher wrote: > Do you even need to do audio_level_() here? I thought rtc::Optional took care > of that with adding it to the constructor. I think the right thing will happen without this. I'll remove it. https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... webrtc/api/rtpreceiverinterface.h:50: audio_level_(audio_level) {} On 2017/08/22 00:01:19, Zhi Huang wrote: > On 2017/08/21 22:55:40, pthatcher wrote: > > Is the other constructor (without an audio_level) even called by anything > other > > than unit tests? If not, why not just have one constructor and update the > unit > > tests to use this constructor (with the audio_level)? > > Some internal tests will call the other constructor, so we might need to keep > both of them for now and leave a TODO to remove that. Yup, some external code relies on the existing constructor. https://codereview.webrtc.org/3000713002/diff/40001/webrtc/api/rtpreceiverint... webrtc/api/rtpreceiverinterface.h:65: void set_audio_level(rtc::Optional<uint8_t> level) { audio_level_ = level; } On 2017/08/21 22:55:39, pthatcher wrote: > I think "const rtc::Optional<T>&" is usually how we pass them. Done. https://codereview.webrtc.org/3000713002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/3000713002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:169: : rtc::Optional<uint8_t>(); On 2017/08/21 22:55:40, pthatcher wrote: > Seems like rtp_header.extension.audioLevel should be an rtc::Optional. Out of > scope for this CL, most likely, though. Acknowledged. https://codereview.webrtc.org/3000713002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:507: void RtpReceiverImpl::UpdateSources(rtc::Optional<uint8_t> audio_level) { On 2017/08/21 22:55:40, pthatcher wrote: > Again with const X& instead of X. Done.
The CQ bit was checked by zstein@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/3000713002/#ps60001 (title: "Minor fixes as suggested by pthatcher.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/20566)
@pthatcher lgty?
zstein@webrtc.org changed reviewers: + deadbeef@webrtc.org - pthatcher@google.com, pthatcher@webrtc.org
@deadbeef: Can you take a quick look at the api changes? Thanks!
lgtm with very minor nits https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:507: void RtpReceiverImpl::UpdateSources(const rtc::Optional<uint8_t>& audio_level) { nit: Since it's also possible to have CSRC-specific audio levels (just not supported yet), I'd call this "ssrc_audio_level" to make it clear what its scope is. https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc (right): https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:259: // RTPSource with the matching SSRC. nit: RtpSource https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:275: May help to have some comments for the different sections of this test. For example: "Receive a packet from a different SSRC with a different audio level, and ensure that both are remembered" https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:334: } Good tests!
https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc (right): https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:507: void RtpReceiverImpl::UpdateSources(const rtc::Optional<uint8_t>& audio_level) { On 2017/08/24 20:40:57, Taylor Brandstetter wrote: > nit: Since it's also possible to have CSRC-specific audio levels (just not > supported yet), I'd call this "ssrc_audio_level" to make it clear what its scope > is. Done. https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc (right): https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:259: // RTPSource with the matching SSRC. On 2017/08/24 20:40:57, Taylor Brandstetter wrote: > nit: RtpSource Done. https://codereview.webrtc.org/3000713002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_receiver_unittest.cc:275: On 2017/08/24 20:40:57, Taylor Brandstetter wrote: > May help to have some comments for the different sections of this test. For > example: "Receive a packet from a different SSRC with a different audio level, > and ensure that both are remembered" Done.
The CQ bit was checked by zstein@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/3000713002/#ps80001 (title: "Style feedback from deadbeef")
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": 80001, "attempt_start_ts": 1503609300919470,
"parent_rev": "0cab085157fb55f1b61e6700fe636895d9164e9a", "commit_rev":
"2b706343de6f79839f46d12008ec3c8062bb94bc"}
Message was sent while issue was closed.
Description was changed from ========== Add audio_level member to RtpSource and set it from RtpReceiverImpl::IncomingRtpPacket. BUG=webrtc:7987 ========== to ========== Add audio_level member to RtpSource and set it from RtpReceiverImpl::IncomingRtpPacket. BUG=webrtc:7987 Review-Url: https://codereview.webrtc.org/3000713002 Cr-Commit-Position: refs/heads/master@{#19503} Committed: https://chromium.googlesource.com/external/webrtc/+/2b706343de6f79839f46d1200... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/2b706343de6f79839f46d1200... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
