Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23296008:
Adding audio unit tests for Android (Closed)
|
Created:
7 years, 4 months ago by henrika (OOO until Aug 14) Modified:
7 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Raymond Toy Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdding audio unit tests for Android.
This CL adds support for a new set of audio unit tests intended for Android. Some of them are suitable for trybots but some are mainly intended for offline benchmarks/testing on different devices. The idea is that we shall use these tests to check if a certain device has potential of providing low-latency, high-quality audio in full duplex or not.
BUG=none
TEST=media_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222779
Patch Set 1 : First round #
Total comments: 44
Patch Set 2 : tommi@-#1 #
Total comments: 4
Patch Set 3 : @wjia #1 #Patch Set 4 : Fixed test error #Patch Set 5 : Now supports Start,Stop,Start #
Total comments: 14
Patch Set 6 : Feedback from tommi@ #
Total comments: 8
Patch Set 7 : nits #
Total comments: 3
Patch Set 8 : now thread safe #Patch Set 9 : First attempt to make output side thread safe as well #Patch Set 10 : rebased #
Total comments: 11
Patch Set 11 : tommi@ #Patch Set 12 : rebased again #Patch Set 13 : OVERRIDE #Patch Set 14 : fixed includes #
Total comments: 27
Patch Set 15 : Removed Start/Stop support #Patch Set 16 : dalecurtis@ #
Total comments: 18
Patch Set 17 : wjia@ #
Total comments: 2
Patch Set 18 : Now uses gmock #
Total comments: 20
Patch Set 19 : joi@ #
Total comments: 4
Patch Set 20 : rebased #Patch Set 21 : reverted change in Windows unit test #Patch Set 22 : Nit #
Messages
Total messages: 54 (0 generated)
This CL is very large but the main part is in the actual unit test which I hope can be reviewed separately since it is still under development. During the development of these unit tests I've found that we have racing conditions. I have done changes in the OpenSLESInputStream class and would appreciate a review of these parts initially. We might have similar problems on the output part but I have not been able to trigger any deadlocks with my tests. I can take look at the output as well later. The issues I found for input was that sequences like streaming, Stop(),Close() could deadlock in Close(). With the changes I've done, no deadlock has been found. I always find it tricky to make the "perfect" lock solution and I might have done some bad choices in this initial patch. Any input is highly appreciated.
Thanks for fixing the racing condition! opensl output also has racing condition. I'd suggest you check in the fix for both input and output racing in a separate patch, since the test is new and will iterate several rounds. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:53: // DVLOG(1) << "##" << thread_id; please remove unused code before checking in. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:84: } It seems that these lines of code (line 75 through 84) is same as those in OnData. Any chance to cut dupped code? https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:171: } This won't work well when tests are run in parallel. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:174: audio_bus->frames() * audio_bus->channels() * kBitsPerSample / 8; nit: indent by 4. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:179: if (pos_ + static_cast<int>(max_size) > file_size()) { |max_size| is "int". https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:354: (kBitsPerSample / 8) * dest->frames() * dest->channels(); nit: indent by 4. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:354: (kBitsPerSample / 8) * dest->frames() * dest->channels(); Do you need kBitsPerSample here? params_ has bits_per_sample_. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:374: buffer_.get(), dest->frames(), kBitsPerSample / 8); ditto for kBitsPerSample. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:424: return input && output; I am not sure if I understand the logic here. This test will be built into a apk file and installed on an android device attached to the build machine. Do you mean the android device attached to build bots could have no audio input or output? https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:438: return std::string(); use switch? https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:453: return std::string("CHANNEL_LAYOUT_UNSUPPORTED"); ditto. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:478: double TimeBetweenCallbacks(AudioParameters params) const { It's better to call this "ExpectedTimeBetweenCallbacks", since it calculates the elapsed time based on # of samples, not real callbacks. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:479: return (base::TimeDelta::FromMicroseconds( nit: indent. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:515: 1.25 * time_between_callbacks_ms); Is 25% margin good enough for one second audio input/output?
https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:52: DCHECK(thread_checker_.CalledOnValidThread()); nice to have the thread checker in here. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:80: if (started_) this needs to be checked under the lock https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:93: err = (*simple_buffer_queue_)->Enqueue( As is, it looks to me that you need to hold the lock whenever you use the simple_buffer_queue_ variable. It's being used from the worker thread as well as here and it's cleared in Close(). If however, there is a way to guarantee that the worker thread is never running when the value of simple_buffer_queue_ changes, then the lock is not required. If that's true though, I would like to see that documented at least to prevent regressions. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:112: if (!started_) need to hold lock. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:146: engine_object_.Reset(); If I understand things correctly, resetting the engine should be done last since it actually deletes the object that implements the interfaces. As is, you've here deleted the engine object but simple_buffer_queue_ is still non-NULL but points to a deleted object (i.e. use after free problem). https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:149: simple_buffer_queue_ = NULL; here you actually do grab the lock before changing the value... this doesn't mean anything though unless you do the same in all other places that use the pointer. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:182: DCHECK(thread_checker_.CalledOnValidThread()); We should also dcheck that all interface and object pointers are NULL when this is called. If this is considered part of the object's construction sequence, it is OK to not grab the lock (since no other threads should have access to them) but DCHECK that all member variables are in their newly-constructed state.
Wei, it feels more efficient to use this CL as base since I can then verify that my changes actually solve the problems I can see with the tests. Hope that is OK. Made changes based on input from tommi@ related to threading. tommi@: PTAL https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:52: DCHECK(thread_checker_.CalledOnValidThread()); Thanks. It was a really smart guy who suggested it ;-) https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:80: if (started_) On 2013/08/29 09:56:27, tommi wrote: > this needs to be checked under the lock Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:93: err = (*simple_buffer_queue_)->Enqueue( I find it difficult to guarantee that the worker thread is dead at all changes and therefore extend the scope of the lock somewhat. Can't see that it wort hurt us. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:112: if (!started_) On 2013/08/29 09:56:27, tommi wrote: > need to hold lock. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:146: engine_object_.Reset(); Good point, thanks. Analyzed how things are created and rewrote. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:149: simple_buffer_queue_ = NULL; On 2013/08/29 09:56:27, tommi wrote: > here you actually do grab the lock before changing the value... this doesn't > mean anything though unless you do the same in all other places that use the > pointer. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/opens... media/audio/android/opensles_input.cc:182: DCHECK(thread_checker_.CalledOnValidThread()); On 2013/08/29 09:56:27, tommi wrote: > We should also dcheck that all interface and object pointers are NULL when this > is called. > > If this is considered part of the object's construction sequence, it is OK to > not grab the lock (since no other threads should have access to them) but DCHECK > that all member variables are in their newly-constructed state. Done.
Wei, thanks for checking the unit tests. I did not expect you to do that already now but your input is appreciated. I have done all you asked for. Again, regarding the race parts. My plan was to fix those parts in this CL. Waiting for reply from tommi@. Will make similar fixes on the output side when input is done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/08/28 22:23:16, wjia wrote: > 2013. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:53: // DVLOG(1) << "##" << thread_id; On 2013/08/28 22:23:16, wjia wrote: > please remove unused code before checking in. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:84: } I rewrote by creating arrays of size 2, an enumerator for IN and OUT and one common method. OK? https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:171: } Can you elaborate? Not sure if I understand. Note that I will most likely have to add DISABLED_ for all tests that requires manual interaction (like this one). The idea is that not all of theses tests should run on bots. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:174: audio_bus->frames() * audio_bus->channels() * kBitsPerSample / 8; On 2013/08/28 22:23:16, wjia wrote: > nit: indent by 4. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:179: if (pos_ + static_cast<int>(max_size) > file_size()) { On 2013/08/28 22:23:16, wjia wrote: > |max_size| is "int". Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:354: (kBitsPerSample / 8) * dest->frames() * dest->channels(); On 2013/08/28 22:23:16, wjia wrote: > Do you need kBitsPerSample here? params_ has bits_per_sample_. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:354: (kBitsPerSample / 8) * dest->frames() * dest->channels(); On 2013/08/28 22:23:16, wjia wrote: > nit: indent by 4. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:374: buffer_.get(), dest->frames(), kBitsPerSample / 8); On 2013/08/28 22:23:16, wjia wrote: > ditto for kBitsPerSample. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:424: return input && output; Really good point. Did not think of that. Makes sense. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:438: return std::string(); On 2013/08/28 22:23:16, wjia wrote: > use switch? Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:453: return std::string("CHANNEL_LAYOUT_UNSUPPORTED"); On 2013/08/28 22:23:16, wjia wrote: > ditto. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:478: double TimeBetweenCallbacks(AudioParameters params) const { On 2013/08/28 22:23:16, wjia wrote: > It's better to call this "ExpectedTimeBetweenCallbacks", since it calculates the > elapsed time based on # of samples, not real callbacks. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:479: return (base::TimeDelta::FromMicroseconds( On 2013/08/28 22:23:16, wjia wrote: > nit: indent. Done. https://codereview.chromium.org/23296008/diff/28001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:515: 1.25 * time_between_callbacks_ms); Good point. So far so good but I've been close on some devices. Makes sense to tweak. Extended measurement time.
https://codereview.chromium.org/23296008/diff/39001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/39001/media/audio/android/opens... media/audio/android/opensles_input.cc:84: if (started_) No need to guard this check since this is the only thread on which |started_| could be modified. https://codereview.chromium.org/23296008/diff/39001/media/audio/android/opens... media/audio/android/opensles_input.cc:116: if (!started_) ditto. https://codereview.chromium.org/23296008/diff/39001/media/audio/android/opens... media/audio/android/opensles_input.cc:140: Stop(); |lock_| is not needed after this point, since |started_| is already false and opensl callback thread will never use other members. https://codereview.chromium.org/23296008/diff/39001/media/audio/android/opens... media/audio/android/opensles_input.cc:353: void OpenSLESInputStream::HandleError(SLresult error) { This function should be called HandleErrorWithLock. Then you will find some caller doesn't acquire lock before calling it.
Wei, as a general reply to your comments: Do you know why we check start_ at all in the callback? It should be possible to remove that check and rely on Stop() to call the worker thread, right?
On 2013/08/29 16:04:00, henrika wrote: > Wei, > > as a general reply to your comments: Do you know why we check start_ at all in > the callback? > It should be possible to remove that check and rely on Stop() to call the worker > thread, right? Stop() can be called anytime (on AudioManager thread) when opensl is still possibly delivering data on opensl thread (worker thread). The caller of Stop() would expect Stop() is an synchronous call, which means no audio data will be delivered to the caller after Stop() is returned. opensl thread is not a chrome thread.
Why is there a check in the callback? If it is needed, the code is racy and guards are needed wherever started_ is used. If it is not needed, then we should remove it and it will be clear from the code that started_ is only ever referenced from the same thread. On Aug 29, 2013 7:17 PM, <wjia@chromium.org> wrote: > On 2013/08/29 16:04:00, henrika wrote: > >> Wei, >> > > as a general reply to your comments: Do you know why we check start_ at >> all in >> the callback? >> It should be possible to remove that check and rely on Stop() to call the >> > worker > >> thread, right? >> > > Stop() can be called anytime (on AudioManager thread) when opensl is still > possibly delivering data on opensl thread (worker thread). The caller of > Stop() > would expect Stop() is an synchronous call, which means no audio data will > be > delivered to the caller after Stop() is returned. > > opensl thread is not a chrome thread. > > > https://codereview.chromium.**org/23296008/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Let me try it again. |started_| is accessed on 2 threads, AudioManager thread and opensl thread. It's modified on only AudioManager thread. So there is no need to use lock when trying to read |started_| on AudioManager thread. To clarify my previous comment: inside Close(), no need to use lock after Stop() is called, because |started_| has been set to false so that opensl callback will not access other members of OpenSLESInputStream except |started_|. On 2013/08/29 17:40:29, tommi wrote: > Why is there a check in the callback? > > If it is needed, the code is racy and guards are needed wherever started_ > is used. If it is not needed, then we should remove it and it will be clear > from the code that started_ is only ever referenced from the same thread. > On Aug 29, 2013 7:17 PM, <mailto:wjia@chromium.org> wrote: > > > On 2013/08/29 16:04:00, henrika wrote: > > > >> Wei, > >> > > > > as a general reply to your comments: Do you know why we check start_ at > >> all in > >> the callback? > >> It should be possible to remove that check and rely on Stop() to call the > >> > > worker > > > >> thread, right? > >> > > > > Stop() can be called anytime (on AudioManager thread) when opensl is still > > possibly delivering data on opensl thread (worker thread). The caller of > > Stop() > > would expect Stop() is an synchronous call, which means no audio data will > > be > > delivered to the caller after Stop() is returned. > > > > opensl thread is not a chrome thread. > > > > > > > https://codereview.chromium.**org/23296008/%3Chttps://codereview.chromium.org...> > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Ok, I thought you were saying that the lock isn't needed to check it at all. But in those places the lock is needed anyway straight away (right?) so aren't we then just optimizing the case where Start and Stop etc are called unnecessarily? Is it worth it? I worry that it could lead to regressions since the checks wouldn't be uniformly guarded. On Aug 29, 2013 7:52 PM, <wjia@chromium.org> wrote: > Let me try it again. |started_| is accessed on 2 threads, AudioManager > thread > and opensl thread. It's modified on only AudioManager thread. So there is > no > need to use lock when trying to read |started_| on AudioManager thread. > > To clarify my previous comment: inside Close(), no need to use lock after > Stop() > is called, because |started_| has been set to false so that opensl > callback will > not access other members of OpenSLESInputStream except |started_|. > > > > On 2013/08/29 17:40:29, tommi wrote: > >> Why is there a check in the callback? >> > > If it is needed, the code is racy and guards are needed wherever started_ >> is used. If it is not needed, then we should remove it and it will be >> clear >> from the code that started_ is only ever referenced from the same thread. >> On Aug 29, 2013 7:17 PM, <mailto:wjia@chromium.org> wrote: >> > > > On 2013/08/29 16:04:00, henrika wrote: >> > >> >> Wei, >> >> >> > >> > as a general reply to your comments: Do you know why we check start_ at >> >> all in >> >> the callback? >> >> It should be possible to remove that check and rely on Stop() to call >> the >> >> >> > worker >> > >> >> thread, right? >> >> >> > >> > Stop() can be called anytime (on AudioManager thread) when opensl is >> still >> > possibly delivering data on opensl thread (worker thread). The caller of >> > Stop() >> > would expect Stop() is an synchronous call, which means no audio data >> will >> > be >> > delivered to the caller after Stop() is returned. >> > >> > opensl thread is not a chrome thread. >> > >> > >> > >> > > https://codereview.chromium.****org/23296008/%3Chttps://codere** > view.chromium.org/23296008/ <http://codereview.chromium.org/23296008/>> > >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org> >> . >> > > > https://codereview.chromium.**org/23296008/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Henrik and I just discussed this and I think that if locking is needed anywhere, it should be consistent everywhere. Otherwise it can be misleading to the reader. There's furthermore no benefit in delaying the locking for the normal case in the Start/Stop/Close methods. However, what do you guys think about this suggestion: * Don't check started_ inside the callback. (thus no locking will be required to check it) * Check the recorded state from the recorder_ object. This object is thread safe and the state of it is what we really use to stop it.
Thanks Tommi. I added such a method and also logs if it is hit. Have tried on several devices and it does not seem to be needed actually but we might as well keep it. I've added a new unit test to really test "threading" and I was hit by SL_RESULT_BUFFER_INSUFFICIENT when I called Enque in second Start. It is evident that the drivers does not do as the OpenSLES doc says since all buffers should be cleared when Clear is called in Stop. But they are *not* (I check the state after Clear). So, to allow Start,Stop,Start... calls (Yes, I think we should allow it just in case), I ensure that we don't queue up new buffers in the second Start call. It works well even if (in theory) a new sequence can start with some crack if the previous session ended exactly when audible audio was recorded. I don't think it matters but like the idea that we can at least support Start,Stop,Start better now than before. PTAL
thanks, that approach sgtm https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:82: if (started_) now that you don't touch started_ in the callback, it doesn't need to be protected by the lock. https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:86: callback_ = callback; should we dcheck that callback_ is NULL before assigning the new value? https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:95: HandleError(err); set started_ to false? alternatively don't set start_ to true until everything has succeeded https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:130: base::AutoLock lock(lock_); same here https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:325: HandleError(err); missing return? https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:327: DLOG(WARNING) << "Invalid callback in non-recording state"; nit: s/Invalid/Received https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:346: active_queue_ = (active_queue_ + 1) % kNumOfQueuesInBuffer; if Enqueue failed, is this still the right thing to do?
Thanks Tommi, new version coming up. https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:82: if (started_) oops, thanks https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:86: callback_ = callback; On 2013/08/30 12:43:34, tommi wrote: > should we dcheck that callback_ is NULL before assigning the new value? Done. https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:95: HandleError(err); Moved it to the end since we don't check it in the callback anyhow. https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:130: base::AutoLock lock(lock_); On 2013/08/30 12:43:34, tommi wrote: > same here Done. https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:325: HandleError(err); On 2013/08/30 12:43:34, tommi wrote: > missing return? Done. https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:327: DLOG(WARNING) << "Invalid callback in non-recording state"; On 2013/08/30 12:43:34, tommi wrote: > nit: s/Invalid/Received Done. https://codereview.chromium.org/23296008/diff/56001/media/audio/android/opens... media/audio/android/opensles_input.cc:346: active_queue_ = (active_queue_ + 1) % kNumOfQueuesInBuffer; Really good comment. I rewrote Start to ensure that active_queue_ is maintained in Start,Stop,Start sequences. Only reset in Open. Should work now. Works as before in the default case.
Think I've covered all input from tommi@. Did some trivial name changes in last patch as well. Feel that it improves readability.
lgtm for the opensles input files % nits https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... media/audio/android/opensles_input.cc:79: DCHECK(!callback_); nit: keep the dcheck behind the lock https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... media/audio/android/opensles_input.cc:123: started_ = true; if we failed to set the state to 'recording' I don't think we should flip this switch. Stop() basically just tries to flip the switch back, which won't work if we didn't succeed in the first place. https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... media/audio/android/opensles_input.cc:341: err = (*simple_buffer_queue_)->Enqueue( I just noticed that a buffer_queue pointer is supplied to the callback. Would it make sense to dcheck that it's the same pointer as we use here or should we use the pointer that's given to us?
Done. Wei: PTAL https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... media/audio/android/opensles_input.cc:79: DCHECK(!callback_); On 2013/08/30 14:37:13, tommi wrote: > nit: keep the dcheck behind the lock Done. https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... media/audio/android/opensles_input.cc:123: started_ = true; On 2013/08/30 14:37:13, tommi wrote: > if we failed to set the state to 'recording' I don't think we should flip this > switch. > Stop() basically just tries to flip the switch back, which won't work if we > didn't succeed in the first place. Done. https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... media/audio/android/opensles_input.cc:341: err = (*simple_buffer_queue_)->Enqueue( Offline discussions => we decided that I could ignore this comment.
My plan was to: - Wait for Wei. - Repeat input style in output parts as well. - Discuss with Tommi if we can submit the tests as well (will have to DISABLE_ some since they are intended for manual testing)
https://codereview.chromium.org/23296008/diff/64001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/64001/media/audio/android/opens... media/audio/android/opensles_input.cc:324: SLresult err = (*recorder_)->GetRecordState(recorder_, &state); This is a regression from last patch. If you don't check |started_|, you need check |recorder_| before dereferencing it. Think about the following case: 1. Right before ReadBufferQueue() acquires lock_, opensl thread is preempted. 2. Stop() and Close() are called on AudioManager thread. 3. opensl thread is resumed. At this time, recorder_ is NULL!
https://codereview.chromium.org/23296008/diff/64001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/64001/media/audio/android/opens... media/audio/android/opensles_input.cc:324: SLresult err = (*recorder_)->GetRecordState(recorder_, &state); On 2013/08/30 16:25:18, wjia wrote: > This is a regression from last patch. If you don't check |started_|, you need > check |recorder_| before dereferencing it. Think about the following case: > 1. Right before ReadBufferQueue() acquires lock_, opensl thread is preempted. > 2. Stop() and Close() are called on AudioManager thread. > 3. opensl thread is resumed. > > At this time, recorder_ is NULL! good spot - multi-threading is hard ;)
Nice catch Wei. Are you OK with the other changes? https://codereview.chromium.org/23296008/diff/64001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/64001/media/audio/android/opens... media/audio/android/opensles_input.cc:324: SLresult err = (*recorder_)->GetRecordState(recorder_, &state); Thx, so I assume that a NULL check is all we need here, right?
Added check proposed by Wei. Had to remove DCHECK of callback in Start() since we don't reset in in Stop() but use it in Close(). PTAL. Will start adding similar fixes for output next. https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... media/audio/android/opensles_input.cc:79: DCHECK(!callback_); Actually, this will not work since we don't reset callback in Stop. The same is true for all other platforms as well and the reason is that callback is used in Close to signal OnClose and cant be cleared in Stop. Hope it is OK if I don't add this check here now.
https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/60001/media/audio/android/opens... media/audio/android/opensles_input.cc:79: DCHECK(!callback_); On 2013/09/02 09:36:15, henrika wrote: > Actually, this will not work since we don't reset callback in Stop. The same is > true for all other platforms as well and the reason is that callback is used in > Close to signal OnClose and cant be cleared in Stop. Hope it is OK if I don't > add this check here now. That's either a design flaw or we simply have bugs on all platforms. If the callback is set in Start, it should be cleared in Stop (assuming an implementation bug). Alternatively, the interface could be changed so that the callback is set in Open() and then cleared in Close(). Given that the callback has an OnClose method, I'm leaning towards design flaw. As a temporary workaround, I'd like the DCHECK to then be to check: DCHECK(NULL || callback_ == callback); As is, a call sequence of Open()/Start()/Stop()/Start()/Stop()/Close() would potentially only call OnClose() on the second callback, not the first.
make that: DCHECK(callback_ == NULL || callback_ == callback);
Added similar parts for the output side. One note: the Clear() call on the buffer queue does have the expected effect on the output side (all counters are cleared as they should). Hence I did not have to add an extra fixes to compensate for an invalid functionality as was the case on the input side.
lgtm for input and output implementations. great job. https://codereview.chromium.org/23296008/diff/38001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:166: printf("Reading from file: %s\n", file_path.value().c_str()); printf? should this be LOG()? https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... media/audio/android/opensles_input.cc:243: static_cast<SLuint32>(kMaxNumOfBuffersInQueue) // Number of buffers. nit: comment not necessary https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... File media/audio/android/opensles_input.h (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... media/audio/android/opensles_input.h:91: int active_buffer_; nit: active_buffer_index_ https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... File media/audio/android/opensles_output.h (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... media/audio/android/opensles_output.h:68: // TODO(henrika): add comments.... ping https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... media/audio/android/opensles_output.h:91: int active_buffer_; nit: active_buffer_index_
Thanks Tommi. Wei, would you mind checking again as well? Sorry for all the test code but it is mainly parts that we already use on other platforms but somewhat tweaked for Android. Also, several tests are DISABLED_ today and should not run on bots; only as manual tests requiring manual user interaction. PTAL https://codereview.chromium.org/23296008/diff/38001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:166: printf("Reading from file: %s\n", file_path.value().c_str()); I actually prefer printf since LOG adds so much overhead (file name etc.). https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... media/audio/android/opensles_input.cc:243: static_cast<SLuint32>(kMaxNumOfBuffersInQueue) // Number of buffers. On 2013/09/03 12:15:09, tommi wrote: > nit: comment not necessary Done. https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... File media/audio/android/opensles_input.h (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... media/audio/android/opensles_input.h:91: int active_buffer_; On 2013/09/03 12:15:09, tommi wrote: > nit: active_buffer_index_ Done. https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... File media/audio/android/opensles_output.h (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... media/audio/android/opensles_output.h:68: // TODO(henrika): add comments.... On 2013/09/03 12:15:09, tommi wrote: > ping Done. https://codereview.chromium.org/23296008/diff/38001/media/audio/android/opens... media/audio/android/opensles_output.h:91: int active_buffer_; On 2013/09/03 12:15:09, tommi wrote: > nit: active_buffer_index_ Done.
https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:86: DCHECK(callback_ == NULL || callback_ == callback); It's more appropriate to clear |callback_| in Stop() and DCHECK(!callback_) here. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:106: for (int i = 0; i < num_free_buffers; ++i) { num_free_buffers is used incorrectly here, because: 1. Stop() has cleared simple_buffer_queue_. There should be no buffer held on OpenSL side. 2. Even though some buffers are held at OpenSL side, it's possible they are audio_data_[0 ...], not always audio_data_[num_free_buffers ... kMaxNumOfBuffersInQueue]. It's better to just clear simple_buffer_queue_ when Stop() is called. That's much simpler. We'd DCHECK_EQ(buffer_queue_state.count, 0). https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:323: if (!recorder_) |recorder_| is cleared in Closed(). That means after Stop() is called, we will still send data back to client. That's incorrect. Therefore, the desired behavior is to clear |callback_| in Stop() and check |callback_| here.
Only comments to input from Wei. Tommi and I gave discussed the usage of callback_ earlier in this CL and come to the conclusion that we can't clear it in Stop() since it is used in Close(). And it does not feel like the proper time to make a design change for that now. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:86: DCHECK(callback_ == NULL || callback_ == callback); Actually, No. Since the current design is that the callback is used to generate OnClose() in Close(). It does not seem to be used on any platform but I don't think it is OK to break this pattern only on Andrtoid. All other platforms calls OnClose() in Close(). PS Tommi had the same comment but we landed in the existing compromise. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:106: for (int i = 0; i < num_free_buffers; ++i) { I have verified this scheme on four different devices and we need it since: Clear() does not clear simple_buffer_queue_ and buffer_queue_state.count is *not* set to 0, hence the proposed DCHECK would always fail. My tests shows that buffer_queue_state.count is always 2 in Stop(). i.e., 2 buffers are ready to be filled. num_free_buffers can only take two different values, 2 or 0 (see DCHECK above). It will always be 2 if we have done Start/Stop and then call Start again, and always 0 at the first Start. Also, I don't reset active_buffer_index_ in Start, which ensures that the next buffer which is queued is the correct one even at the second Start call. Note that, for the output side, Clear() does clear the buffer but not for the input side. I think that the reason is that OpenSLES expects that one should get some extra/remaining callbacks even after Stop() is called and after that the queue is cleared. I tried to do this but it does not seems to be possible using a simple queue. It could work with the default Open SLES queue but I think we are fine as is. To summarize: the new code does not make any changes to the default Start/Stop/Close call but it adds the possibility to do Start/Stop/Start... The old code failed for Start/Stop/Start since we could not call Enqueue the second time. Hence, I am solving an existing problem here. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:323: if (!recorder_) Same comment as above. |callback_| must be used in Close(). The complete API must be redesigned before we can change that. Best long-term solution is probably to move the callback to Open() instead. The current check was added to deal with the special case you mentioned earlier where we could crash.
Wei, it would be so nice if you had time to take another look. Thanks.
Dale, I realize that I have now owner for the media.gyp file. Care to take a look? You are of course more than welcome to comment on the rest. Sorry for inviting you so late. Main idea here is to add more unit tests on Android. Initially mainly intended for manual tests where we can determine which devices are OK and which are not. The tests can also be used to learn more about the basic performance. We have already used them to find a bug in OpenSLES and resolved some threading issues.
This seems to be moving away from the platform agnostic unit tests we've talked about before, but doesn't add anything that looks specific to android. Is there a reason these need to be android specific? Tests which printf() stuff are generally frowned upon. Do you need all this data printed? If it's just for debugging then you could put them under DVLOGs. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:38: class MockAudioInputOutputCallbacks This seems like it could all be done much more cleanly using gmock w/ expectations and actions instead of a custom mock implementation. Is there a reason you went this way instead of using gmock? https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:160: // Reads a test file from media/test/data directory and stores it in Indent it way off. I'd run clang-format on the file. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:166: printf("Reading from file: %s\n", file_path.value().c_str()); Generally we avoid visible log messages in unittests. Do you really need this? https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:182: printf("."); Ditto. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:504: #define START_STREAM_AND_WAIT_FOR_EVENT(stream, dir) \ Seems you could just make this a templated function instead? https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:546: #define MULTIPLE_START_STREAM_AND_WAIT_FOR_EVENT(stream, dir) \ Ditto. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:606: low_latency ? printf("Low latency output is supported\n") : Weird style and use of printf. Why not just if (xxx) LOG(xxx), else LOG(XXX).
"This seems to be moving away from the platform agnostic unit tests we've talked about before, but doesn't add anything that looks specific to android. Is there a reason these need to be android specific?" For Android, we need a set of unit tests now which tests both input and output and also adds some parts that might not be suitable for automated testing (DISABLED_). The platform agnostic unit tests is still in my list but it was intended for output only and should also test fake device, low-latency and high-latency and that was a bit out of scope for what we need for Android right now. I simply felt that I did not know Android well enough to start with the agnostic test. Now when I know it better I can write a better agnostic test for output. Also, we needed a unit test for Android ASAP and I felt that I'd rather spend time on a more extensive loopback test for Android now than to solve issues on all other platforms for output only. Again, my plan is to make the test you want as well. Hence, I am not moving away, only making something in parallel. And, I have used printf using your benchmark unit tests as reference. The idea is that our test team shall be able to run this test on a device an get a nice overview of its performance and features. That was the plan at least. Will check your comments tomorrow.
https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:106: for (int i = 0; i < num_free_buffers; ++i) { On 2013/09/04 07:44:57, henrika wrote: > I have verified this scheme on four different devices and we need it since: > > Clear() does not clear simple_buffer_queue_ and buffer_queue_state.count is > *not* set to 0, hence the proposed DCHECK would always fail. My tests shows that > buffer_queue_state.count is always 2 in Stop(). i.e., 2 buffers are ready to be > filled. > > num_free_buffers can only take two different values, 2 or 0 (see DCHECK above). > It will always be 2 if we have done Start/Stop and then call Start again, and > always 0 at the first Start. > > Also, I don't reset active_buffer_index_ in Start, which ensures that the next > buffer which is queued is the correct one even at the second Start call. > > Note that, for the output side, Clear() does clear the buffer but not for the > input side. I think that the reason is that OpenSLES expects that one should get > some extra/remaining callbacks even after Stop() is called and after that the > queue is cleared. I tried to do this but it does not seems to be possible using > a simple queue. It could work with the default Open SLES queue but I think we > are fine as is. > > To summarize: the new code does not make any changes to the default > Start/Stop/Close call but it adds the possibility to do Start/Stop/Start... The > old code failed for Start/Stop/Start since we could not call Enqueue the second > time. Hence, I am solving an existing problem here. The essential problem is that Clear() doesn't work as expected. But buffer_queue_state.count is unnecessarily always be either 0 or 2, because there is another rare case in which buffer_queue_state.count could be any value between 0 and kMaxNumOfBuffersInQueue. Just think about a similar case to what I mentioned in comment #20. When OpenSL delivers a buffer back, right before it acquires the lock, it's preempted. Stop() is called. When OpenSL thread resumes, it will return immediately without delivering buffer to client, and OpenSLESInputStream will not send the buffer back to OpenSL either. That means buffer_queue_state.count could be 1 now (at least, it's not 2). Then Start() is called. The options are: 1. don't support start/stop/start/... 2. support start/stop/start, need a cleaner workaround. For example, keep track of which buffers are on OpenSLESInputStream side. That will allow you to send correct buffers to OpenSL when restarting it. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:323: if (!recorder_) On 2013/09/04 07:44:57, henrika wrote: > Same comment as above. |callback_| must be used in Close(). The complete API > must be redesigned before we can change that. Best long-term solution is > probably to move the callback to Open() instead. > > The current check was added to deal with the special case you mentioned earlier > where we could crash. You can use |started_|, instead of |recorder_|, since neither |recorder_| or |callback_| can be used as "stop" signal. Then the following check is not needed.
https://codereview.chromium.org/23296008/diff/38001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/38001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:166: printf("Reading from file: %s\n", file_path.value().c_str()); On 2013/09/03 12:48:32, henrika wrote: > I actually prefer printf since LOG adds so much overhead (file name etc.). Sorry, I didn't see your reply to this but as you see from Dale's comment, we don't use printf.
Tommi, any input on how I can remove support for Start/Stop/Start and let the user know that second Start failed? https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:106: for (int i = 0; i < num_free_buffers; ++i) { I understand your concern but note that the current implementation fails directly when you do Start/Stop/Start and I have so far not been able to make the new version fail. I am a bit tempted to remove support for Start/Stop/Start and then try to reinsert it once the OpenSLES bug is solved. Not sure though how to signal to the user that the second Start failed (since it returns void).
I guess we can use the OnError callback. So, I will remove the support and disable the unit test for now but keep it if we want to support Start/Stop/Start when the bug is resolved.
Wei, I have now removed support for Start/Stop/Start, removed the unit test that checked the same sequence and check start_ in the callback as you suggested. Dale, will look at you comments next. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:323: if (!recorder_) Thanks. Will think again and probably restore the original solution.
Thanks Dale. Added comments and started out removing printfs. Did not finalize. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:38: class MockAudioInputOutputCallbacks I feel that this approach gives me more flexibility and e.g. the file read/write parts would have been messy in combination with gmock. Also, I was not sure how to use gmock when I want to mock two interfaces in one. To be honest, I find it somewhat cumbersome to read and maintain gmock tests and actually prefer "manual mocks" since I find them easier to read and to debug. It also gives me the possibility to add logs while debugging. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:160: // Reads a test file from media/test/data directory and stores it in Thanks. Did it on the complete CL. Thanks for the tip. Did not know we had it. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:166: printf("Reading from file: %s\n", file_path.value().c_str()); I will remove it. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:504: #define START_STREAM_AND_WAIT_FOR_EVENT(stream, dir) \ Guess I could also have done it as a separate function taking dir as input. Are you OK with the current version as is? Can't see any obvious advantages by changing it to a templated function. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:546: #define MULTIPLE_START_STREAM_AND_WAIT_FOR_EVENT(stream, dir) \ Now removed. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:606: low_latency ? printf("Low latency output is supported\n") : On 2013/09/05 20:35:23, DaleCurtis wrote: > Weird style and use of printf. Why not just if (xxx) LOG(xxx), else LOG(XXX). Done. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/opens... media/audio/android/opensles_input.cc:323: if (!recorder_) Ignore my previous comment. Should be Done.
Dale, looks like clang-format resulted in several changes. Hope that is OK. Not sure if it is mandatory or not to use it.
I don't want to block this for another time zone cycle, so media.gyp lgtm, I defer to tommi@ for final approval though. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:38: class MockAudioInputOutputCallbacks On 2013/09/06 15:59:51, henrika wrote: > I feel that this approach gives me more flexibility and e.g. the file read/write > parts would have been messy in combination with gmock. Also, I was not sure how > to use gmock when I want to mock two interfaces in one. > > To be honest, I find it somewhat cumbersome to read and maintain gmock tests and > actually prefer "manual mocks" since I find them easier to read and to debug. It > also gives me the possibility to add logs while debugging. By choosing to manual mock objects you're increasing the readability complexity for others. Chrome has an established mocking tool which makes it easier for others to come into the code and figure out what's going on. E.g., this approach is much less clear to me because I need to spend extra time figuring out how you've mapped your mocking concepts. https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:504: #define START_STREAM_AND_WAIT_FOR_EVENT(stream, dir) \ On 2013/09/06 15:59:51, henrika wrote: > Guess I could also have done it as a separate function taking dir as input. Are > you OK with the current version as is? Can't see any obvious advantages by > changing it to a templated function. Chrome is a C++ based project, so we try to avoid #define unless absolutely necessary. It's much easier to read a templated function than a macro with "\" everywhere. Please change. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. No (c) in 2013 license. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:96: ; ; is unnecessary, which is why clang-format moved it around. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:107: ; Ditto. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:194: int callbacks_[2]; It'd probably be clearer if you just put all these values in a struct and then had an array of them. Reset is just a memset then. Up to you though. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:231: printf("."); There are still a lot of printf's all over. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:317: fwrite(chunk, 1, chunk_size, binary_file_); This should probably be using some piece of file_util:: instead.
nice work! we are almost there. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... media/audio/android/opensles_input.cc:147: callback_->OnClose(this); I don't quite understand why callback_ is used here, since callback_ is passed in at Start(), not Open(). https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... media/audio/android/opensles_input.cc:309: } The above check is not needed. OpenSLESInputStream should listen to ObjectCallback or RecordCallback to get notification of any error. You can do that in a separate patch. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... File media/audio/android/opensles_output.cc (right): https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... media/audio/android/opensles_output.cc:286: if (!player_) |started_| is a better flag used here than |player_|, since started_ is cleared in Stop() and player_ is reset in Close().
https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/92001/media/audio/android/audio... media/audio/android/audio_android_unittest.cc:504: #define START_STREAM_AND_WAIT_FOR_EVENT(stream, dir) \ On 2013/09/06 22:05:07, DaleCurtis wrote: > On 2013/09/06 15:59:51, henrika wrote: > > Guess I could also have done it as a separate function taking dir as input. > Are > > you OK with the current version as is? Can't see any obvious advantages by > > changing it to a templated function. > > Chrome is a C++ based project, so we try to avoid #define unless absolutely > necessary. It's much easier to read a templated function than a macro with "\" > everywhere. Please change. Agree. As an additional thing to think about, these macros are unsearchable and source code indexing tools like sublime or visual assist will not pick up the macros as references to the functions they call. Instead I find the approach of having homebrewn counters in the "mock" class a little awkward. Wouldn't it me easier to just use gmock and set the expected nr of callbacks in an EXPECT_CALL?
Think I've covered your latest feedback Wei. PTAL. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... media/audio/android/opensles_input.cc:147: callback_->OnClose(this); Neither do I but that is how it is implemented on all other platforms and we have many tests which checks that OnClose is called at Close. It does not feel like the best solution to either avoid calling OnClose or to move to Open() in this CL sine it would lead to a rather messy refactoring job for all platforms. Hope we agree here. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... media/audio/android/opensles_input.cc:309: } On 2013/09/07 17:09:36, wjia wrote: > The above check is not needed. OpenSLESInputStream should listen to > ObjectCallback or RecordCallback to get notification of any error. You can do > that in a separate patch. Done. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... File media/audio/android/opensles_output.cc (right): https://codereview.chromium.org/23296008/diff/127001/media/audio/android/open... media/audio/android/opensles_output.cc:286: if (!player_) On 2013/09/07 17:09:36, wjia wrote: > |started_| is a better flag used here than |player_|, since started_ is cleared > in Stop() and player_ is reset in Close(). Done.
Added comments to review from Dale. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/09/06 22:05:07, DaleCurtis wrote: > No (c) in 2013 license. Done. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:96: ; On 2013/09/06 22:05:07, DaleCurtis wrote: > ; is unnecessary, which is why clang-format moved it around. Done. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:107: ; On 2013/09/06 22:05:07, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:194: int callbacks_[2]; Will replace all with gmock. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:231: printf("."); Will fix. https://codereview.chromium.org/23296008/diff/127001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:317: fwrite(chunk, 1, chunk_size, binary_file_); Added TODO. Will see if I can find something useful.
Thanks, Henrik! LGTM (on opensles_input, opensles_output and audiomanager) with one comment. I didn't spend much time on the unit test since other reviewers are on it. https://codereview.chromium.org/23296008/diff/140001/media/audio/android/open... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/140001/media/audio/android/open... media/audio/android/opensles_input.cc:147: callback_->OnClose(this); Since none of us knows why callback is passed in at Start() and used at Close(), please put a TODO for this so that we won't forget this needs to be refactored later.
Thanks all for great review input. I have now modified one concrete callback implementation into gmock with some Invoke() magic (thanks Jói). Joí; care to take a look at the test? https://codereview.chromium.org/23296008/diff/140001/media/audio/android/open... File media/audio/android/opensles_input.cc (right): https://codereview.chromium.org/23296008/diff/140001/media/audio/android/open... media/audio/android/opensles_input.cc:147: callback_->OnClose(this); On 2013/09/10 16:33:35, wjia wrote: > Since none of us knows why callback is passed in at Start() and used at Close(), > please put a TODO for this so that we won't forget this needs to be refactored > later. Done.
Hey, I looked only at the audio_android_unittest.cc file, see comments below. Cheers, Jói https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:71: case CHANNEL_LAYOUT_UNSUPPORTED: Could move this to just before default and fall through to the default implementation. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:85: static_cast<float>(params.sample_rate()))).InMillisecondsF(); The function returns double, not float, is that intentional? https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:89: os << std::endl << "format: " << FormatToString(params.format()) << std::endl I guess this is an artifact of git cl format, but this would really be less confusing to read if the std::endl were always at the end or the front, or if they don't fit that way, always on separate lines. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:203: // audio data to a local output file. Would put a note here saying this is only used for manually invoked and evaluated tests, and that is why you're writing a file without cleaning it up. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:304: DVLOG(1) << extra_fio_delay; extra_fio_delay -> extra_fifo_delay https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:313: // Append new data to the FIFO and extend the size if the mac capacity mac -> max https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:370: static_cast<float>(params_.sample_rate()))).InMillisecondsF(); Again float vs. double, is it intentional? https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:409: return ((end_time_ - start_time_) / (num_callbacks - 1)).InMillisecondsF(); Looks like the calculation occurs in integer land, is that intentional? Or is there perhaps some serious operator overloading for TimeTicks that I'm missing? It is then converted to milliseconds float and finally to double for the return - is that conversion intentional? https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:504: LOG(INFO) << params; For all of the tests that are enabled to run without supervision, I would make all logs VLOGs that can be easily turned on when doing manual inspection of the tests but don't litter automated test logs. This is just my preference though - no need to change any of the logging if the convention in media code is different. I understand the motivation behind using LOG(INFO) for the tests and related code that are meant to run under human supervision. Those I have no problem with using LOG(INFO). https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:593: // the callback sequence is sane. The only changed we make in this test is to changed -> change
Thanks Jói. PTAL. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:71: case CHANNEL_LAYOUT_UNSUPPORTED: On 2013/09/11 15:42:58, Jói wrote: > Could move this to just before default and fall through to the default > implementation. Done. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:85: static_cast<float>(params.sample_rate()))).InMillisecondsF(); Good point. Fixed. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:89: os << std::endl << "format: " << FormatToString(params.format()) << std::endl Added local using namespace std. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:203: // audio data to a local output file. On 2013/09/11 15:42:58, Jói wrote: > Would put a note here saying this is only used for manually invoked and > evaluated tests, and that is why you're writing a file without cleaning it up. Done. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:304: DVLOG(1) << extra_fio_delay; On 2013/09/11 15:42:58, Jói wrote: > extra_fio_delay -> extra_fifo_delay Done. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:313: // Append new data to the FIFO and extend the size if the mac capacity On 2013/09/11 15:42:58, Jói wrote: > mac -> max Done. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:370: static_cast<float>(params_.sample_rate()))).InMillisecondsF(); On 2013/09/11 15:42:58, Jói wrote: > Again float vs. double, is it intentional? Done. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:409: return ((end_time_ - start_time_) / (num_callbacks - 1)).InMillisecondsF(); Thanks. Please note that InMillisecondsF() does return double. Odd name I must say. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:504: LOG(INFO) << params; On 2013/09/11 15:42:58, Jói wrote: > For all of the tests that are enabled to run without supervision, I would make > all logs VLOGs that can be easily turned on when doing manual inspection of the > tests but don't litter automated test logs. > > This is just my preference though - no need to change any of the logging if the > convention in media code is different. > > I understand the motivation behind using LOG(INFO) for the tests and related > code that are meant to run under human supervision. Those I have no problem with > using LOG(INFO). Done. https://codereview.chromium.org/23296008/diff/146001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:593: // the callback sequence is sane. The only changed we make in this test is to On 2013/09/11 15:42:58, Jói wrote: > changed -> change Done.
LGTM with a couple of tiny nits. Looked only at the unit test. https://codereview.chromium.org/23296008/diff/150001/media/audio/android/audi... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/150001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:204: // only be used for manually invoked and evaluated tests, hence the created created -> created file https://codereview.chromium.org/23296008/diff/150001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:205: // will not be destroyed after the test is done since the intention that it intention that -> intention is that
Thanks. Looks like I have OK from everyone now. Will ensure that the try bots cycle green as well. https://codereview.chromium.org/23296008/diff/150001/media/audio/android/audi... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/23296008/diff/150001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:204: // only be used for manually invoked and evaluated tests, hence the created On 2013/09/12 09:50:53, Jói wrote: > created -> created file Done. https://codereview.chromium.org/23296008/diff/150001/media/audio/android/audi... media/audio/android/audio_android_unittest.cc:205: // will not be destroyed after the test is done since the intention that it On 2013/09/12 09:50:53, Jói wrote: > intention that -> intention is that Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/23296008/161001
Message was sent while issue was closed.
Change committed as 222779 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
