Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528933002:
Adding OnReceivedOverhead to AudioEncoder. (Closed)
|
Created:
4 years ago by minyue-webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding OnReceivedOverhead to AudioEncoder.
BUG=webrtc:6762
Committed: https://crrev.com/eca373f3baf5c0c8059fe038ec9a458483f5dfb5
Cr-Commit-Position: refs/heads/master@{#15457}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fixing and adding unittest #
Total comments: 3
Patch Set 3 : on Michael's comments #
Total comments: 6
Patch Set 4 : new touch #
Total comments: 16
Patch Set 5 : on Karl's comments #Patch Set 6 : adding static_cast #
Messages
Total messages: 33 (14 generated)
Description was changed from ========== Adding OnReceivedOverhead to AudioEncoder. BUG=webrtc:6762 ========== to ========== Adding OnReceivedOverhead to AudioEncoder. BUG=webrtc:6762 ==========
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
Hi Michael, PTAL at this CL. It removes overhead rate from target rate for audio. sister CL to https://codereview.webrtc.org/2517243005/ https://codereview.webrtc.org/2528933002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2528933002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:244: TEST(AudioEncoderOpusTest, InvokeAudioNetworkAdaptorOnReceivedUplinkBandwidth) { changed because the name was not very good.
https://codereview.webrtc.org/2528933002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2528933002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:96: 8 * 1000 * *overhead_bytes_per_packet_ / Num10MsFramesInNextPacket(); I think this calculation is wrong by a a factor of 10 :) . A unittest for this would be nice.
Hi Micheal, Thanks for the review! I fixed the problem and added unittests.
https://codereview.webrtc.org/2528933002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc (right): https://codereview.webrtc.org/2528933002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:19: using testing::_; testing::_ not used at the moment. https://codereview.webrtc.org/2528933002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:20: using testing::Invoke; testing::Invoke not used at the moment. https://codereview.webrtc.org/2528933002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:32: // method. I think i would prefer to add the mock function and invoke it. Other wise if we once add the mock function, this tests will fail.
On 2016/11/28 09:26:58, michaelt wrote: > https://codereview.webrtc.org/2528933002/diff/20001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc (right): > > https://codereview.webrtc.org/2528933002/diff/20001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:19: using > testing::_; > testing::_ not used at the moment. > > https://codereview.webrtc.org/2528933002/diff/20001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:20: using > testing::Invoke; > testing::Invoke not used at the moment. > > https://codereview.webrtc.org/2528933002/diff/20001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:32: // method. > I think i would prefer to add the mock function and invoke it. Other wise if we > once add the mock function, this tests will fail. Yes, very good suggesting. That was what I started with, so testing::invoke and _ remains there. I will go for it.
I modified the test so that it looks clearer that I am testing the implemented methods. But, to be honest, I am not totally satisfied with this since the delegation to the implemented methods are half controlled in the mocked class (ideally it should be fully controlled in the test) Any suggestions?
minyue@webrtc.org changed reviewers: + kwiberg@webrtc.org
Hi Karl, Micheal has seen this CL. Please also take a look. It would be great if you can suggest a better way to test the default implementation of AudioEncoder::OnReceivedOverhead. Thanks!
https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:92: // Ignore the target bitrate is overhead is unknown. Should the first "is" be "if"? https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:190: // for the payload. Please consider spending a few more words describing what this number means. Is it the number of bytes added to each packet the encoder emits? https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:204: rtc::Optional<size_t> overhead_bytes_per_packet_; Interface classes should not have data members! In this particular case, the reason you need a data member seems to be that the new default implementation of OnReceivedTargetAudioBitrate uses it when computing the value to pass to SetTargetBitrate. But the default implementation of SetTargetBitrate does nothing, so it would seem appropriate to let the default OnReceivedTargetAudioBitrate and OnReceivedOverhead do nothing as well, and let the classes that actually want to do something with the per-packet overhead implement this.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Hi Karl, I moved the implementation and unittests to Opus, since I find that only Opus override OnReceivedTargetAudioBitrate. This has been made clearer since https://codereview.webrtc.org/2538493006/ got landed. Find the new change in PS4. It would be even easier to review it independently from earlier patch sets. https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:92: // Ignore the target bitrate is overhead is unknown. On 2016/11/29 09:46:24, kwiberg-webrtc wrote: > Should the first "is" be "if"? I think we should also add log here. see new patch. https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:190: // for the payload. On 2016/11/29 09:46:24, kwiberg-webrtc wrote: > Please consider spending a few more words describing what this number means. Is > it the number of bytes added to each packet the encoder emits? Done. https://codereview.webrtc.org/2528933002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:204: rtc::Optional<size_t> overhead_bytes_per_packet_; On 2016/11/29 09:46:24, kwiberg-webrtc wrote: > Interface classes should not have data members! > > In this particular case, the reason you need a data member seems to be that the > new default implementation of OnReceivedTargetAudioBitrate uses it when > computing the value to pass to SetTargetBitrate. But the default implementation > of SetTargetBitrate does nothing, so it would seem appropriate to let the > default OnReceivedTargetAudioBitrate and OnReceivedOverhead do nothing as well, > and let the classes that actually want to do something with the per-packet > overhead implement this. Done.
https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:185: // bytes that will be add to each payload the encoder generates. With this "add" -> "added" And maybe "payload" -> "packet"? https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:188: // respectively, the encoder can decide its frame length and payload rate. Aren't the round-trip time and the packet loss rate relevant too? Maybe it's best to just say "together with the other run-time parameters", or perhaps drop this sentence altogether? https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:310: 8 * 100 * *overhead_bytes_per_packet_ / Num10MsFramesInNextPacket(); I had a hard time deciphering this calculation. Could you call the variable "overhead_bps" or something instead? Also, const. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:316: SetTargetBitrate(target_audio_bitrate_bps); I'd argue this would be easier to read if you removed the explicit returns and did if (...) { ... } else if (...) { ... } else { ... } https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:332: overhead_bytes_per_packet_ = rtc::Optional<size_t>(overhead_bytes_per_packet); if ... else ... instead of the explicit return. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:331: constexpr size_t kOverhead = 64; You don't have to use the kCamelCase naming convention for local variables. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:174: event_log, Is this just a reformatting? Remove it, in that case.
Patchset #5 (id:140001) has been deleted
Hi Karl, I tried to address your comments! PTAL again. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:185: // bytes that will be add to each payload the encoder generates. With this On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > "add" -> "added" > > And maybe "payload" -> "packet"? Done. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:188: // respectively, the encoder can decide its frame length and payload rate. On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > Aren't the round-trip time and the packet loss rate relevant too? Maybe it's > best to just say "together with the other run-time parameters", or perhaps drop > this sentence altogether? Round trip time is not taken into account right now, but yet, I like to make the description generalized. With that we need to say "together with the other run-time parameters, the encoder can decide its encoding mode." This is redundant given that we put "Provides overhead to this encoder to adapt" as the first sentence already. So dropping this sentence is best. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:310: 8 * 100 * *overhead_bytes_per_packet_ / Num10MsFramesInNextPacket(); On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > I had a hard time deciphering this calculation. Could you call the variable > "overhead_bps" or something instead? > > Also, const. Done. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:316: SetTargetBitrate(target_audio_bitrate_bps); On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > I'd argue this would be easier to read if you removed the explicit returns and > did > > if (...) { > ... > } else if (...) { > ... > } else { > ... > } Done. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:332: overhead_bytes_per_packet_ = rtc::Optional<size_t>(overhead_bytes_per_packet); On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > if ... else ... instead of the explicit return. Done. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:331: constexpr size_t kOverhead = 64; On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > You don't have to use the kCamelCase naming convention for local variables. ok. But if it is not a hard rule, I'd like to maintain consistency of this file. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/congestion_controller.cc (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/congestion_controller.cc:174: event_log, On 2016/12/03 00:30:57, kwiberg-webrtc wrote: > Is this just a reformatting? Remove it, in that case. oh, true. This CL was upstreamed on another branch, which might confused git cl format.
lgtm https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:188: // respectively, the encoder can decide its frame length and payload rate. On 2016/12/06 09:04:19, minyue-webrtc wrote: > On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > > Aren't the round-trip time and the packet loss rate relevant too? Maybe it's > > best to just say "together with the other run-time parameters", or perhaps > drop > > this sentence altogether? > > Round trip time is not taken into account right now, but yet, I like to make the > description generalized. With that we need to say "together with the other > run-time parameters, the encoder can decide its encoding mode." This is > redundant given that we put "Provides overhead to this encoder to adapt" as the > first sentence already. So dropping this sentence is best. Acknowledged. https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:331: constexpr size_t kOverhead = 64; On 2016/12/06 09:04:19, minyue-webrtc wrote: > On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > > You don't have to use the kCamelCase naming convention for local variables. > > ok. But if it is not a hard rule, I'd like to maintain consistency of this file. Acknowledged.
On 2016/12/06 09:43:25, kwiberg-webrtc wrote: > lgtm > > https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... > File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): > > https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/codecs/audio_encoder.h:188: // respectively, the > encoder can decide its frame length and payload rate. > On 2016/12/06 09:04:19, minyue-webrtc wrote: > > On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > > > Aren't the round-trip time and the packet loss rate relevant too? Maybe it's > > > best to just say "together with the other run-time parameters", or perhaps > > drop > > > this sentence altogether? > > > > Round trip time is not taken into account right now, but yet, I like to make > the > > description generalized. With that we need to say "together with the other > > run-time parameters, the encoder can decide its encoding mode." This is > > redundant given that we put "Provides overhead to this encoder to adapt" as > the > > first sentence already. So dropping this sentence is best. > > Acknowledged. > > https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... > File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc > (right): > > https://codereview.webrtc.org/2528933002/diff/120001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:331: > constexpr size_t kOverhead = 64; > On 2016/12/06 09:04:19, minyue-webrtc wrote: > > On 2016/12/03 00:30:56, kwiberg-webrtc wrote: > > > You don't have to use the kCamelCase naming convention for local variables. > > > > ok. But if it is not a hard rule, I'd like to maintain consistency of this > file. > > Acknowledged. Hi Michael, It would be good that you take a look at the final patch set too. A shift of method has been made since you reviewed.
lgtm
The CQ bit was checked by minyue@webrtc.org
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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/20432)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org, michaelt@webrtc.org Link to the patchset: https://codereview.webrtc.org/2528933002/#ps180001 (title: "adding static_cast")
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": 180001, "attempt_start_ts": 1481102629687690,
"parent_rev": "6121fbf925c61abefa6c26293f72e9be7e53a605", "commit_rev":
"2e3e70a4941549c3d71a10507a8e07e2b731f6df"}
Message was sent while issue was closed.
Description was changed from ========== Adding OnReceivedOverhead to AudioEncoder. BUG=webrtc:6762 ========== to ========== Adding OnReceivedOverhead to AudioEncoder. BUG=webrtc:6762 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Adding OnReceivedOverhead to AudioEncoder. BUG=webrtc:6762 ========== to ========== Adding OnReceivedOverhead to AudioEncoder. BUG=webrtc:6762 Committed: https://crrev.com/eca373f3baf5c0c8059fe038ec9a458483f5dfb5 Cr-Commit-Position: refs/heads/master@{#15457} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/eca373f3baf5c0c8059fe038ec9a458483f5dfb5 Cr-Commit-Position: refs/heads/master@{#15457} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
