Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668693003:
Change opus min bitrate. (Closed)
DescriptionChange opus min bitrate.
BUG=webrtc:7087
Review-Url: https://codereview.webrtc.org/2668693003
Cr-Commit-Position: refs/heads/master@{#16383}
Committed: https://chromium.googlesource.com/external/webrtc/+/54340d8e7569633f15c79a6687babdebcf9cc989
Patch Set 1 #Patch Set 2 : Fix for AudioDecoderOpusTest.SetTargetBitrate #
Total comments: 5
Messages
Total messages: 30 (19 generated)
Description was changed from ========== Change opus min bitrate. BUG=webrtc:7087 ========== to ========== Change opus min bitrate. BUG=webrtc:7087 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org
Hi minyue, Here the promised CL to change the min bit-rate for opus.
minyue@webrtc.org changed reviewers: + ossu@webrtc.org
On 2017/01/31 14:54:50, michaelt wrote: > Hi minyue, > Here the promised CL to change the min bit-rate for opus. I say lgtm. But I added Oskar to double check.
Looks fine! Good to know what changes are coming up since I'm also touching that code.
Oh, might as well lgtm so my name turns green. :)
The CQ bit was checked by michaelt@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: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
The CQ bit was checked by michaelt@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: This issue passed the CQ dry run.
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2668693003/#ps20001 (title: "Fix for AudioDecoderOpusTest.SetTargetBitrate")
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": 20001, "attempt_start_ts": 1485882236155760,
"parent_rev": "cf34fdea193d453c3158bd43acd040ba3a9f6ff3", "commit_rev":
"54340d8e7569633f15c79a6687babdebcf9cc989"}
Message was sent while issue was closed.
Description was changed from ========== Change opus min bitrate. BUG=webrtc:7087 ========== to ========== Change opus min bitrate. BUG=webrtc:7087 Review-Url: https://codereview.webrtc.org/2668693003 Cr-Commit-Position: refs/heads/master@{#16383} Committed: https://chromium.googlesource.com/external/webrtc/+/54340d8e7569633f15c79a668... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/54340d8e7569633f15c79a668...
Message was sent while issue was closed.
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:128: RTC_DCHECK(speech_data->Init( This is wrong for two reasons: 1. It won't call Init when the test is run in Release mode. 2. Don't use (D)CHECKs in test code. Use EXPECT or ASSERT.
Message was sent while issue was closed.
Description was changed from ========== Change opus min bitrate. BUG=webrtc:7087 Review-Url: https://codereview.webrtc.org/2668693003 Cr-Commit-Position: refs/heads/master@{#16383} Committed: https://chromium.googlesource.com/external/webrtc/+/54340d8e7569633f15c79a668... ========== to ========== Change opus min bitrate. BUG=webrtc:7087 Review-Url: https://codereview.webrtc.org/2668693003 Cr-Commit-Position: refs/heads/master@{#16383} Committed: https://chromium.googlesource.com/external/webrtc/+/54340d8e7569633f15c79a668... ==========
Message was sent while issue was closed.
henrik.lundin@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:128: RTC_DCHECK(speech_data->Init( On 2017/03/03 14:32:59, hlundin-webrtc wrote: > This is wrong for two reasons: > 1. It won't call Init when the test is run in Release mode. Right, thanks! Weird that the test still passes. > 2. Don't use (D)CHECKs in test code. Use EXPECT or ASSERT. I think it was my fault to introduce D(CHECKs) in tests. What I wanted was to let D(CHECKs) capture wrong test implementation, and let EXPECT to capture wrong product implementation. But, that may not be necessary.
Message was sent while issue was closed.
https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:128: RTC_DCHECK(speech_data->Init( In general i like the idea of separation of test's and test setup (Open files ...) by using (D)CHECKs and use of EXPECT or ASSERT. For situations like this one we have a additional problem. In this case we should use ASSERT, but we are not allowed to use ASSERT in functions which are called by the test. So it seams best to me to just replace the RTC_DCHECK by a a RTC_CHECK.
Message was sent while issue was closed.
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:128: RTC_DCHECK(speech_data->Init( On 2017/03/06 10:09:52, michaelt wrote: > In general i like the idea of separation of test's and test setup (Open files > ...) by using (D)CHECKs and use of EXPECT or ASSERT. > > For situations like this one we have a additional problem. > In this case we should use ASSERT, but we are not allowed to use ASSERT in > functions which are called by the test. > > So it seams best to me to just replace the RTC_DCHECK by a a RTC_CHECK. > The test still passes on the bots (Debug and Release alike) because they always run with DCHECK_ALWAYS_ON set. The reason I regularly dislike (D)CHECKs in tests is that the same binary runs many test cases, and if the (D)CHECK triggers, you get no results at all from the ones that did not have a chance to run yet. The problem with ASSERT_* is that, due to how it is implemented, it can only be used in void-returning functions. What you can do is have this method return an empty result (nullptr) if init fails, and then let the test itself ASSERT_TRUE on that.
Message was sent while issue was closed.
https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2668693003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:128: RTC_DCHECK(speech_data->Init( On 2017/03/06 10:49:13, hlundin-webrtc wrote: > On 2017/03/06 10:09:52, michaelt wrote: > > In general i like the idea of separation of test's and test setup (Open files > > ...) by using (D)CHECKs and use of EXPECT or ASSERT. > > > > For situations like this one we have a additional problem. > > In this case we should use ASSERT, but we are not allowed to use ASSERT in > > functions which are called by the test. > > > > So it seams best to me to just replace the RTC_DCHECK by a a RTC_CHECK. > > > > The test still passes on the bots (Debug and Release alike) because they always > run with DCHECK_ALWAYS_ON set. > > The reason I regularly dislike (D)CHECKs in tests is that the same binary runs > many test cases, and if the (D)CHECK triggers, you get no results at all from > the ones that did not have a chance to run yet. > > The problem with ASSERT_* is that, due to how it is implemented, it can only be > used in void-returning functions. What you can do is have this method return an > empty result (nullptr) if init fails, and then let the test itself ASSERT_TRUE > on that. Acknowledged. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
