Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8427031:
First unit tests for WebRTCAudioDevice. (Closed)
|
Created:
9 years, 1 month ago by tommi (sloooow) - chröme Modified:
9 years, 1 month ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFirst unit tests for WebRTCAudioDevice.
TEST=Run *WebRTCAudioDeviceTest* in content_unittests.
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109222
Patch Set 1 #Patch Set 2 : lint fixes #
Total comments: 16
Patch Set 3 : Addressed review comments #Patch Set 4 : Factored out the fixture class due to checkdeps #Patch Set 5 : Moved audio_util dependency out of content/test #Patch Set 6 : '' #
Total comments: 18
Patch Set 7 : Latest comments addressed #
Total comments: 22
Patch Set 8 : '' #Patch Set 9 : addressed review comments from henrik (also see prev patch) #Patch Set 10 : Added headless check #
Total comments: 2
Patch Set 11 : addressed review comments #Patch Set 12 : Disabled the tests by default until we've fixed flakiness in WebRTC's CpuWindows class #
Total comments: 10
Patch Set 13 : '' #
Messages
Total messages: 18 (0 generated)
Hey Henrik, please have a look. I'm not sure why the pcm files are showing up twice there. I guessing some local svn problem I'm having.
Drive-by with test comments. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... File content/renderer/media/webrtc_audio_device_unittest.cc (right): http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:12: nit: We're not doing gaps like this in the #includes. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:132: class SignalTask : public Task { We already have a class like this (chrome/test/base/signaling_task). Please move it to base/test or content/test and re-use instead of duplicating. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:207: IPC_BEGIN_MESSAGE_MAP(WebRTCAudioDeviceTest, message) Please use MESSAGE_MAP_EX to avoid an implicit DCHECK in MESSAGE_MAP. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:216: DLOG(WARNING) << "Unhandled IPC message"; Don't you want to make the test fail in that case? ADD_FAILURE could be what you really want. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:263: static void SetUpTestCase() { Shouldn't you have a corresponding TearDownTestCase? Also, why not just in SetUp? The operation doesn't seem to be expensive. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:278: content::GetContentClient()->set_renderer(&mock_content_renderer_client_); Is this undone in TearDown? Please use a scoped object to guarantee this. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:343: PathService::Get(content::DIR_TEST_DATA, &path); Check the return value. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:374: DISALLOW_COPY_AND_ASSIGN(WebRTCAudioDeviceTest); nit: No need for that in a test fixture.
http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... File content/renderer/media/webrtc_audio_device_unittest.cc (right): http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:12: On 2011/11/02 09:29:15, Paweł Hajdan Jr. wrote: > nit: We're not doing gaps like this in the #includes. Done. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:132: class SignalTask : public Task { On 2011/11/02 09:29:15, Paweł Hajdan Jr. wrote: > We already have a class like this (chrome/test/base/signaling_task). Please move > it to base/test or content/test and re-use instead of duplicating. http://codereview.chromium.org/8438034/ http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:207: IPC_BEGIN_MESSAGE_MAP(WebRTCAudioDeviceTest, message) On 2011/11/02 09:29:15, Paweł Hajdan Jr. wrote: > Please use MESSAGE_MAP_EX to avoid an implicit DCHECK in MESSAGE_MAP. Done. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:216: DLOG(WARNING) << "Unhandled IPC message"; On 2011/11/02 09:29:15, Paweł Hajdan Jr. wrote: > Don't you want to make the test fail in that case? ADD_FAILURE could be what you > really want. I added this comment: if (!handled) { // We don't do a NOTREACHED/NOTIMPLEMENTED here since this is FYI // for the developer in case the test stops working. If new IPC messages // are added but not handled in the test for some reason, the test might // break. This DLOG can save time in tracking down why. DLOG(WARNING) << "Unhandled IPC message"; } http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:263: static void SetUpTestCase() { On 2011/11/02 09:29:15, Paweł Hajdan Jr. wrote: > Shouldn't you have a corresponding TearDownTestCase? Also, why not just in > SetUp? The operation doesn't seem to be expensive. This fixture is based in parts on the AudioRendererImplTest fixture (see audio_renderer_impl_unittest.cc), where this part is done the same way. I think it's fine to just move this to SetUp though, so I've do that. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:278: content::GetContentClient()->set_renderer(&mock_content_renderer_client_); On 2011/11/02 09:29:15, Paweł Hajdan Jr. wrote: > Is this undone in TearDown? Please use a scoped object to guarantee this. This is another thing that is borrowed from AudioRendererImplTest. Do you mind taking a look at it? Perhaps there are some bugs we could fix there as well. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:343: PathService::Get(content::DIR_TEST_DATA, &path); On 2011/11/02 09:29:15, Paweł Hajdan Jr. wrote: > Check the return value. Done. http://codereview.chromium.org/8427031/diff/1009/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:374: DISALLOW_COPY_AND_ASSIGN(WebRTCAudioDeviceTest); On 2011/11/02 09:29:15, Paweł Hajdan Jr. wrote: > nit: No need for that in a test fixture. Done.
FYI - this patch isn't ready for formal reviews yet although I do welcome all comments. I need to work out a few dependency issues before I can go ahead with this and likely land a few other changes before I can land this one. Please let me know about any issues though if you see them.
http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:5: #include "content/test/webrtc_audio_device_test.h" Ooops, one thing I didn't initially notice is that this is placed in content/test . Please don't do that. It should only be used for testing infrastructure, and tests should live as close to code being tested as possible. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:62: LOG(WARNING) << __FUNCTION__; nit: Isn't this cluttering the logs too much? Ideally a passing test should generate no logs. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:77: DCHECK_EQ(AudioRendererImpl::kLowLatency, nit: I think ASSERT_EQ would fit better (not crashing entire test binary). http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:84: saved_content_renderer_ = content::GetContentClient()->renderer(); Could you create a scoped object for this and put it in content/test? Restoring is better than not doing it, but a scoper is even harder to misuse. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:161: DLOG_IF(WARNING, !audio_util_callback_) << "Can't get input sample rate"; Should that possibly be EXCEPT_TRUE? I generally don't like "warnings" that don't make the test fails. Either this condition means things are broken, or it's possibly not really a warning. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:186: bool message_is_ok = true; Now could you EXPECT_TRUE(message_is_ok) after END_MESSAGE_MAP_EX? http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:196: // We don't do a NOTREACHED/NOTIMPLEMENTED here since this is FYI I think it'd be much simpler and cleaner to just say EXPECT_TRUE(handled) here. A log that is only done in Debug mode might be missed by someone working in Release mode, and if the test produces lots of other output it's quite easy to miss anyway. If you still want to make it non-fatal, consider converting DLOG -> LOG, and please make the comment shorter (no more than 1-2 lines), like "Unhandled messages are not fatal, but may be indication of problems.". Also, maybe (D)LOG_IF would be slightly shorter. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.h (right): http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.h:106: explicit SetupTask(WebRTCAudioDeviceTest* test) : test_(test) {} nit: Ah, it's probably useful to DCHECK here (if test is NULL, things will crash later anyway, just more mysteriously). This is an exception from avoiding DCHECK in test code, but in this case it'd crash anyway.
http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:5: #include "content/test/webrtc_audio_device_test.h" On 2011/11/02 18:00:24, Paweł Hajdan Jr. wrote: > Ooops, one thing I didn't initially notice is that this is placed in > content/test . Please don't do that. It should only be used for testing > infrastructure, and tests should live as close to code being tested as possible. You didn't notice before because this file wasn't here the first time around :) Due to deps problems I split up the test file to three files. A .cc/.h pair in content/test that can include from content/browser and a _unittest.cc file that has (or will have) the unit tests themselves in the same directory as the code being tested. When I ran into the deps problem, I figured I had two options: a) Modify the DEPS files (or create a new one) to allow me to include from content/browser. b) Split the files up like I did. I discussed these with other engs and we found other examples of doing it this way. Besides, I'm not familiar enough with the background behind the content/ include rules. Some of the files I depend on might belong in content/test actually, like mock_media_observer.*, but not others like resource_context.h How do you propose we solve this? http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:62: LOG(WARNING) << __FUNCTION__; On 2011/11/02 18:00:24, Paweł Hajdan Jr. wrote: > nit: Isn't this cluttering the logs too much? Ideally a passing test should > generate no logs. This has been removed. It was there just for my benefit while debugging. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:77: DCHECK_EQ(AudioRendererImpl::kLowLatency, On 2011/11/02 18:00:24, Paweł Hajdan Jr. wrote: > nit: I think ASSERT_EQ would fit better (not crashing entire test binary). agreed. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:84: saved_content_renderer_ = content::GetContentClient()->renderer(); On 2011/11/02 18:00:24, Paweł Hajdan Jr. wrote: > Could you create a scoped object for this and put it in content/test? Restoring > is better than not doing it, but a scoper is even harder to misuse. Will do. (I'm at home right now, will post an update later) http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:161: DLOG_IF(WARNING, !audio_util_callback_) << "Can't get input sample rate"; On 2011/11/02 18:00:24, Paweł Hajdan Jr. wrote: > Should that possibly be EXCEPT_TRUE? I generally don't like "warnings" that > don't make the test fails. Either this condition means things are broken, or > it's possibly not really a warning. Yes, that makes sense as things are right now. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:186: bool message_is_ok = true; On 2011/11/02 18:00:24, Paweł Hajdan Jr. wrote: > Now could you EXPECT_TRUE(message_is_ok) after END_MESSAGE_MAP_EX? Sure http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:196: // We don't do a NOTREACHED/NOTIMPLEMENTED here since this is FYI On 2011/11/02 18:00:24, Paweł Hajdan Jr. wrote: > I think it'd be much simpler and cleaner to just say EXPECT_TRUE(handled) here. > > A log that is only done in Debug mode might be missed by someone working in > Release mode, and if the test produces lots of other output it's quite easy to > miss anyway. > > If you still want to make it non-fatal, consider converting DLOG -> LOG, and > please make the comment shorter (no more than 1-2 lines), like "Unhandled > messages are not fatal, but may be indication of problems.". > > Also, maybe (D)LOG_IF would be slightly shorter. Right now there are 1-2 such cases for each unit test but that's fine. I would like to avoid entries in the release output log that could indicate an error while the test is succeeding. I'll change it to DLOG_IF
Latest changes uploaded.
http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:5: #include "content/test/webrtc_audio_device_test.h" On 2011/11/03 07:56:52, tommi wrote: > Due to deps problems I split up the test file to three files. A .cc/.h pair in > content/test that can include from content/browser and a _unittest.cc file that > has (or will have) the unit tests themselves in the same directory as the code > being tested. Ah, I see. Do you expect more tests to use webrtc_audio_device_test.h though? As an alternative you could create content/renderer/test and put the test there and add a DEPS file. I think that's how some renderer_host test deps were solved.
http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:5: #include "content/test/webrtc_audio_device_test.h" On 2011/11/04 07:51:32, Paweł Hajdan Jr. wrote: > On 2011/11/03 07:56:52, tommi wrote: > > Due to deps problems I split up the test file to three files. A .cc/.h pair > in > > content/test that can include from content/browser and a _unittest.cc file > that > > has (or will have) the unit tests themselves in the same directory as the code > > being tested. > > Ah, I see. Do you expect more tests to use webrtc_audio_device_test.h though? As > an alternative you could create content/renderer/test and put the test there and > add a DEPS file. I think that's how some renderer_host test deps were solved. More tests are going to be added but I don't know if there will be more files than the _unittest file in this cl that will use it. Hmm.. actually, some of this code could be shared with the audio_renderer tests. http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.h (right): http://codereview.chromium.org/8427031/diff/8002/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.h:106: explicit SetupTask(WebRTCAudioDeviceTest* test) : test_(test) {} On 2011/11/02 18:00:24, Paweł Hajdan Jr. wrote: > nit: Ah, it's probably useful to DCHECK here (if test is NULL, things will crash > later anyway, just more mysteriously). > > This is an exception from avoiding DCHECK in test code, but in this case it'd > crash anyway. Done.
Great work! I'll be happy to start adding more tests using your new framework. I am so glad that you were able to build this up for us. My comments are mainly related to the fact that you are using parts in the tests that can (and should as I see it) be removed. I would prefer more focused test cases. If the test says "PlayFile" only the required APIs should be called, nothing else. http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/audio... File content/renderer/media/audio_renderer_impl.h (right): http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/audio... content/renderer/media/audio_renderer_impl.h:109: friend class WebRTCAudioDeviceTest; May I ask why? And, does the comment above apply to this friend as well? http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... File content/renderer/media/webrtc_audio_device_unittest.cc (right): http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:42: set_audio_util_callback(&audio_util); Where can I find the definition of set_audio_util_callback()? Are you missing a file in this CL? http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:47: WebRTCAutoDelete<webrtc::VoiceEngine> engine(webrtc::VoiceEngine::Create()); Why not ensure that engine is valid (not NULL) here? http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:59: PlayLocalFile(0); I have never seen this before where a unit test calls test functions that are not in the same file as the actual unit test. Guess you have a good reason for it. http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:59: PlayLocalFile(0); Add comment about meaning of '0' here. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:52: class ReplaceContentClientRenderer { Comment? http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:87: // Set low latency mode, as it soon would be on by default. Don't think you need this part at all, right? http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:262: ScopedWebRTCPtr<webrtc::VoEAudioProcessing> audio_processing(engine.get()); An AGC is not needed to play out a file. I understand that the base for this section is taken from my existing test but I feel that we should clean up as much as possible and make the test more focused. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:269: ScopedWebRTCPtr<webrtc::VoENetwork> network(engine.get()); webrtc::VoENetwork is not needed either since we are only playing out here. Only the base->StartPlayout(ch) is required. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:305: int WebRTCTransportImpl::SendPacket(int channel, const void* data, int len) { Can be removed. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.h (right): http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.h:166: RenderThreadImpl* render_thread_; // owned by mock_process_ nit. You mix initial captial letter in short comments. Compare owned and Weak. Same for period (.).
http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/audio... File content/renderer/media/audio_renderer_impl.h (right): http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/audio... content/renderer/media/audio_renderer_impl.h:109: friend class WebRTCAudioDeviceTest; On 2011/11/04 10:35:12, henrika1 wrote: > May I ask why? > > And, does the comment above apply to this friend as well? Removed. This isn't needed anymore. http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... File content/renderer/media/webrtc_audio_device_unittest.cc (right): http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:42: set_audio_util_callback(&audio_util); On 2011/11/04 10:35:12, henrika1 wrote: > Where can I find the definition of set_audio_util_callback()? Are you missing a > file in this CL? webrtc_audio_device_test.h@130 http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:47: WebRTCAutoDelete<webrtc::VoiceEngine> engine(webrtc::VoiceEngine::Create()); On 2011/11/04 10:35:12, henrika1 wrote: > Why not ensure that engine is valid (not NULL) here? Done. http://codereview.chromium.org/8427031/diff/7017/content/renderer/media/webrt... content/renderer/media/webrtc_audio_device_unittest.cc:59: PlayLocalFile(0); On 2011/11/04 10:35:12, henrika1 wrote: > I have never seen this before where a unit test calls test functions that are > not in the same file as the actual unit test. Guess you have a good reason for > it. The reason is because two tests use the same code. However, I think I'm just going to make one test that uses TestTimeouts::action_timeout_ms() and move all the code to this test. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:52: class ReplaceContentClientRenderer { On 2011/11/04 10:35:12, henrika1 wrote: > Comment? The comment was in the header file but I moved it here. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:87: // Set low latency mode, as it soon would be on by default. On 2011/11/04 10:35:12, henrika1 wrote: > Don't think you need this part at all, right? Right - and thanks for catching. Removed. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:262: ScopedWebRTCPtr<webrtc::VoEAudioProcessing> audio_processing(engine.get()); On 2011/11/04 10:35:12, henrika1 wrote: > An AGC is not needed to play out a file. I understand that the base for this > section is taken from my existing test but I feel that we should clean up as > much as possible and make the test more focused. Done. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:269: ScopedWebRTCPtr<webrtc::VoENetwork> network(engine.get()); On 2011/11/04 10:35:12, henrika1 wrote: > webrtc::VoENetwork is not needed either since we are only playing out here. > Only the base->StartPlayout(ch) is required. Done. http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:305: int WebRTCTransportImpl::SendPacket(int channel, const void* data, int len) { On 2011/11/04 10:35:12, henrika1 wrote: > Can be removed. It's a pure virtual method that must be implemented in order to compile. How about adding an assert here to ensure that this never gets called? http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.h (right): http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.h:166: RenderThreadImpl* render_thread_; // owned by mock_process_ On 2011/11/04 10:35:12, henrika1 wrote: > nit. You mix initial captial letter in short comments. Compare owned and Weak. > Same for period (.). Done.
LGTM with comments. http://codereview.chromium.org/8427031/diff/21001/content/test/webrtc_audio_d... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/21001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:242: NOTREACHED(); Did you mean ADD_FAILURE here? NOTREACHED() will crash entire test binary, but only in Debug mode. In release mode it won't do anything. ADD_FAILURE will make the test fail regardless of the mode.
Please add explicit call to VoEBase::Terminate(). Otherwise, LGTM. Thanks!
http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/7017/content/test/webrtc_audio_de... content/test/webrtc_audio_device_test.cc:305: int WebRTCTransportImpl::SendPacket(int channel, const void* data, int len) { Fine with me.
Also added explicit calls to Terminate() at the end of the tests. http://codereview.chromium.org/8427031/diff/21001/content/test/webrtc_audio_d... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/21001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:242: NOTREACHED(); On 2011/11/07 13:50:53, Paweł Hajdan Jr. wrote: > Did you mean ADD_FAILURE here? > > NOTREACHED() will crash entire test binary, but only in Debug mode. In release > mode it won't do anything. ADD_FAILURE will make the test fail regardless of the > mode. Changed to ADD_FAILURE
FYI - the test pcm files have been submitted. For some reason they were confusing Rietveld.
LGTM w/ nits also please de-camel-case singleUserDemo_mono_16kHz.pcm chromium files are unix_hacker_style.pcm http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:45: const gfx::Rect& rect) { return NULL; } want to align 2nd param w/ TransportDIB** then drop body of method to next line? http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:87: : render_thread_(NULL), event_(false, false), audio_util_callback_(NULL) {} nit: not a one-liner, drop } to next line http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:145: void WebRTCAudioDeviceTest::CreateChannel(const char* name, param indentation is unusual: want to drop 1st param to next line and align w/ 2nd param? http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:238: WebRTCTransportImpl::~WebRTCTransportImpl() { nit: close one-liners to {} http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... File content/test/webrtc_audio_device_test.h (right): http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.h:100: // Implemented and defined the cc file. s/defined the cc/defined in the cc/
http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... File content/test/webrtc_audio_device_test.cc (right): http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:45: const gfx::Rect& rect) { return NULL; } On 2011/11/08 22:24:46, scherkus wrote: > want to align 2nd param w/ TransportDIB** then drop body of method to next line? Done. http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:87: : render_thread_(NULL), event_(false, false), audio_util_callback_(NULL) {} On 2011/11/08 22:24:46, scherkus wrote: > nit: not a one-liner, drop } to next line Done. http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:145: void WebRTCAudioDeviceTest::CreateChannel(const char* name, On 2011/11/08 22:24:46, scherkus wrote: > param indentation is unusual: want to drop 1st param to next line and align w/ > 2nd param? Done. http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.cc:238: WebRTCTransportImpl::~WebRTCTransportImpl() { On 2011/11/08 22:24:46, scherkus wrote: > nit: close one-liners to {} Done. http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... File content/test/webrtc_audio_device_test.h (right): http://codereview.chromium.org/8427031/diff/24001/content/test/webrtc_audio_d... content/test/webrtc_audio_device_test.h:100: // Implemented and defined the cc file. On 2011/11/08 22:24:46, scherkus wrote: > s/defined the cc/defined in the cc/ Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
