Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8591028:
Change the way we are sending audio data to driver when using WaveOut API. (Closed)
|
Created:
9 years, 1 month ago by enal Modified:
9 years ago Reviewers:
tommi (sloooow) - chröme, Raymond Toy (Google), cpu_(ooo_6.6-7.5), henrika_dont_use, enal1, henrika (OOO until Aug 14) CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, acolwell+watch_chromium.org, annacc+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionChange the way we are sending audio data to driver when using WaveOut API.
Before, we were "feeding" the driver in the callback when OS returned audio
buffer to us. MSDN disallows that, but We were avoiding deadlock when stopping
the stream by using some clever tricks. Unfortunately, exactly the same
deadlock happens when Windows were draining audio stream when switching
the device, and our tricks did not help, as we were not controlling exact timing.
Fix is to separate receiving freed buffer and "feeding" audio driver. Now we are
using CALLBACK_EVENT wave out mode in which Windows signal event when buffer runs
out of data, and using RegisterWaitForSingleObject() so our callback is called by
one of thread pool threads.
Stopping of playback became slower because now it is synchronous. If that ever
become a problem we can use singleton that keeps track of the playing audio
streams, than stopping can become instaneous, at a cost of more complex checks
in the callback.
Unfortunately, no automatic test, as there is no documented way to programmatically
switch default audio device on Windows. People were able to figure it out (see
http://www.daveamenta.com/2011-05/programmatically-or-command-line-change-the-default-sound-playback-device-in-windows-7/),
but there is no guarantee it will continue to work in Win8, or even in next Service Pack
for Win7.
BUG=41036
TEST=Play any HTML5 audio/video on Windows and change default audio device
TEST=using "Control Panel | Sound". There should be no hang, you should hear
TEST=audio from the newly chosen device.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112147
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112484
Patch Set 1 #
Total comments: 35
Patch Set 2 : '' #
Total comments: 11
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 23
Patch Set 6 : '' #
Total comments: 20
Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 4
Patch Set 9 : '' #
Messages
Total messages: 52 (0 generated)
I believe I finally have code ready...
Great work Eugene. After thinking about this while skimming through the code (I left the comments in below just fyi), I wonder if we can't come up with a solution that does not require singletons. What about reference counting either the stream object, or have a reference counted state object that has a pointer back to the stream. The reference count of the state, whether it belongs to the stream class or not, would be: 1 when not playing 2 while playing 1 after Stop has been called but callbacks are still expected. 0 when Stop has been called and no further callbacks expected. I think this would also be good for unit tests where singletons are a nuisance. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:23: PCMWaveOutAudioOutputStream::g_streams_map_; All these objects should belong to one singleton object - assuming we cannot live without a singleton. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:157: delete[] buffers_; use scoped_array? http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:226: // (a) get object lock why do we need the object lock while we hold the other lock? The state_ variable (which isn't protected, hence I'm assuming single threadedness for these functions) should make sure that Start() isn't called while we're stopping. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:259: } it's ok to keep the empty line below this one imo. same for the scope above for separation. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:341: // exactly happens. Are we doing this check to avoid touching an object that might be deleted? If not, then could we move all of this function into a non-static member function of PCMWaveOutAudioOutputStream and skip all stream-> parts? http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:349: !stream->lock_.Try()); Maybe add a comment that here is where you acquire the object's lock. I missed it the first time around. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:351: if (stopped) what about checking the state_ variable for != PCMA_PLAYING? http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:372: // from here, have to schedule it for the separate therad. therad -> thread http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... File media/audio/win/waveout_output_win.h (right): http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:9: #include <map> os includes first http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:59: WAVEHDR* GetBuffer(int n) const { nice. you could move this to the cc file to avoid including logging.h in the header. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:74: DWORD CbBuffer() const { CbBuffer -> BufferSize Also, return size_t? http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:75: return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<DWORD>(-16); instead of static_cast<DWORD>(-16), can you just use 0xFFFFFFF0? imho that's more readable. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:93: // Allocates and prepares the memory that will be used for playback. add an empty line above this one. same throughout. Generally when functions or member variables have a comment, we have have a single empty line above the comment for separation. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:97: // Globals-related functions. This comment isn't very useful. Can you document which globals we need instead? http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:107: static base::Lock g_lock_; We should avoid global objects that require construction. If we absolutely must have global state, we should group it all in a separate class and use one of the singleton classes from base/ http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:111: // by looking stream address and its ordinal in the map of all streams. by looking stream address -> by looking at the stream address http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:112: // Stream is not touched before check is complete. This way we don't "Stream is not touched before check is complete" -> "The stream is not be touched before the check is complete." http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:163: // chunk. Stream owns them. I prefer "This object owns it" to "Stream owns them".
Eugene, I have applied the patch and can confirm that it does avoid the current hang we have seen in combination with device switches. Also, the audio is "moved" from the disabled device to the new device. Nice! Next, to the details. Given the rather extensive set of comments from Tommi I rest my case the first round. Would be great to sort out the big picture first.
On 2011/11/18 12:24:21, henrika1 wrote: > Eugene, > > I have applied the patch and can confirm that it does avoid the current hang we > have seen in combination with device switches. Also, the audio is "moved" from > the disabled device to the new device. > > Nice! > > Next, to the details. > > Given the rather extensive set of comments from Tommi I rest my case the first > round. Would be great to sort out the big picture first. I am build sheriff till Monday, looking at the broken tests on Linux right now. Will address the issues in the background.
Major comment was "do not use singleton that keeps track of playing audio streams, use aux ref-counted object". Fixed it and all small issues as well. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:23: PCMWaveOutAudioOutputStream::g_streams_map_; On 2011/11/18 09:53:21, tommi wrote: > All these objects should belong to one singleton object - assuming we cannot > live without a singleton. Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:157: delete[] buffers_; On 2011/11/18 09:53:21, tommi wrote: > use scoped_array? Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:226: // (a) get object lock On 2011/11/18 09:53:21, tommi wrote: > why do we need the object lock while we hold the other lock? > The state_ variable (which isn't protected, hence I'm assuming single > threadedness for these functions) should make sure that Start() isn't called > while we're stopping. Got rid of other lock. Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:259: } On 2011/11/18 09:53:21, tommi wrote: > it's ok to keep the empty line below this one imo. same for the scope above for > separation. Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:341: // exactly happens. On 2011/11/18 09:53:21, tommi wrote: > Are we doing this check to avoid touching an object that might be deleted? > > If not, then could we move all of this function into a non-static member > function of PCMWaveOutAudioOutputStream and skip all stream-> parts? Yes, we had very complicated checking to avoid touching potentially deleted object. Not necessary with aux object anymore. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:349: !stream->lock_.Try()); On 2011/11/18 09:53:21, tommi wrote: > Maybe add a comment that here is where you acquire the object's lock. I missed > it the first time around. Code totally rewritten. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:351: if (stopped) On 2011/11/18 09:53:21, tommi wrote: > what about checking the state_ variable for != PCMA_PLAYING? Again, could not do that because object could be deleted. Not necessary anymore. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.cc:372: // from here, have to schedule it for the separate therad. On 2011/11/18 09:53:21, tommi wrote: > therad -> thread Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... File media/audio/win/waveout_output_win.h (right): http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:9: #include <map> On 2011/11/18 09:53:21, tommi wrote: > os includes first Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:59: WAVEHDR* GetBuffer(int n) const { On 2011/11/18 09:53:21, tommi wrote: > nice. you could move this to the cc file to avoid including logging.h in the > header. Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:74: DWORD CbBuffer() const { On 2011/11/18 09:53:21, tommi wrote: > CbBuffer -> BufferSize > Also, return size_t? Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:75: return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<DWORD>(-16); On 2011/11/18 09:53:21, tommi wrote: > instead of static_cast<DWORD>(-16), can you just use 0xFFFFFFF0? imho that's > more readable. In general, 0xFFFFFFF0 is bad -- it can become a problem on 64-bit platform. You are right, in code as it was 0xFFFFFFF0 is probably better, but not anymore after I switch to size_t. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:93: // Allocates and prepares the memory that will be used for playback. On 2011/11/18 09:53:21, tommi wrote: > add an empty line above this one. same throughout. Generally when functions or > member variables have a comment, we have have a single empty line above the > comment for separation. Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:97: // Globals-related functions. On 2011/11/18 09:53:21, tommi wrote: > This comment isn't very useful. Can you document which globals we need instead? Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:107: static base::Lock g_lock_; On 2011/11/18 09:53:21, tommi wrote: > We should avoid global objects that require construction. If we absolutely must > have global state, we should group it all in a separate class and use one of the > singleton classes from base/ Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:111: // by looking stream address and its ordinal in the map of all streams. On 2011/11/18 09:53:21, tommi wrote: > by looking stream address -> by looking at the stream address Done. http://codereview.chromium.org/8591028/diff/1/media/audio/win/waveout_output_... media/audio/win/waveout_output_win.h:112: // Stream is not touched before check is complete. This way we don't On 2011/11/18 09:53:21, tommi wrote: > "Stream is not touched before check is complete" -> > "The stream is not be touched before the check is complete." Done.
excellent! I have one more suggestion below and a couple of nits. Almost there. http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.cc:71: return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<size_t>(~15); nit: instead of 15 I find hex better to read for bit operations, so 0xF. using ~ is a nice touch. http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.cc:234: playing_object_ = NULL; While this correctly releases the object, it also sets the pointer to NULL. So if there are pending callbacks, they will crash when trying to Release the reference for those pending callbacks. So, the callbacks basically can't use the same member variable to access the playing_object_ as the stream object uses. Instead of sending a pointer to the stream object to the callback, can we send a pointer to the playing object directly? Perhaps it would also be a good idea to let the playing object have a pointer to the 'manager' and own the buffer in case the thread that calls Stop() also deletes the stream object straight away or calls Start() again, in which case a new playing object would be used. Basically I think it would be good if the playing object doesn't have a pointer back to the stream. http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.cc:370: AddRef(); maybe a comment on where this reference is released. http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... File media/audio/win/waveout_output_win.h (right): http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.h:77: return stream_ != NULL; should we lock lock_ here and in stop_playing() or DCHECK that the lock is already held? http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.h:83: PlayingObject(PCMWaveOutAudioOutputStream *stream); PCMWaveOutAudioOutputStream* stream http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.h:94: PCMWaveOutAudioOutputStream *stream_; PCMWaveOutAudioOutputStream* stream_; http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.h:159: // Infamous aux object. hah!
noticed one more thing. http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.cc:370: AddRef(); Actually, I think this AddRef should not be bere. It should be where you call PostTask. The reason is that we must guarantee that the object lives while a pointer to it lives in the message queue. Alternatively, and even simpler, you can skip calling Release inside WaveCallback when playing: if (playing_object->playing()) { playing_object->message_loop()->PostTask(...); // no Release here. } else { playing_object->Release(); }
http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.cc:312: // from here, have to schedule it for the separate thread. I don't agree with this approach. We have added another thread to this operation, the one calling this callback is already a real-time thread. The right approach is to use CALLBACK_EVENT mode, and remove this overhead. http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.cc:357: if (!lock_.Try()) I don't completely follow why we need to take this lock anymore.
> The right approach is to use CALLBACK_EVENT mode, and remove this > overhead. I would be happy to follow the advice, but I don't see how to easily integrate waiting for Windows events into Chrome message loops, especially when I try to reuse existing thread. Do you have any example I can look at? What will happen if we cannot handle the event before Windows is ready to release next buffer -- will we lose the event? I.e should we iterate through all buffers, filling all released ones? > I don't completely follow why we need to take this lock anymore. There is potential conflict between stopping the stream (and potentially closing the stream and deleting the object) and filling the buffer in the audio manager thread. I.e. we have checks on the entry to callback, but not later. Simplest way to avoid conflict is to use the lock. Thanks, Eugene On Sun, Nov 20, 2011 at 5:18 PM, <cpu@chromium.org> wrote: > > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... > File media/audio/win/waveout_output_win.cc (right): > > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... > media/audio/win/waveout_output_win.cc:312: // from here, have to > schedule it for the separate thread. > I don't agree with this approach. We have added another thread to this > operation, the one calling this callback is already a real-time thread. > > The right approach is to use CALLBACK_EVENT mode, and remove this > overhead. > > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... > media/audio/win/waveout_output_win.cc:357: if (!lock_.Try()) > I don't completely follow why we need to take this lock anymore. > > http://codereview.chromium.org/8591028/ >
Carlos, I read your input in the issue tracker about the initial work you did on comparing CALLBACK and EVENT mode. Based on your experience, do you feel that the existing issue is "strong enough" to start making more fundamental changes to the code? My point is that if Eugene starts working in this new direction, there might be a risk that he later encounter other types of issues? Or, is the EVENT mode the solution you would go for today if you would rewrite from scratch?
On 2011/11/21 08:31:51, henrika1 wrote: > Carlos, > > I read your input in the issue tracker about the initial work you did on > comparing CALLBACK and EVENT mode. Based on your experience, do you feel that > the existing issue is "strong enough" to start making more fundamental changes > to the code? My point is that if Eugene starts working in this new direction, > there might be a risk that he later encounter other types of issues? Or, is the > EVENT mode the solution you would go for today if you would rewrite from > scratch? (Re-typing, my previous attempt to reply was lost with "Invalid XSRF token" or some similar message). Carlos, I can switch to CALLBACK_EVENT and directly use RegisterWaitForSingleObject(), wrapper ObjectWatcher is very inconvenient because it should start watching in the same message loop where callback should later run. RegisterWaitForSingleObject() would use one of the threads from the thread pool, more or less what CALLBACK_FUNCTION is doing. I verified that when using CALLBACK_FUNCTION we have exactly one thread calling all the callbacks in the process, i.e. there is one thread when we have several audio objects in JS game. Priority of that thread is -3, between THREAD_PRIORITY_IDLE (-15) and THREAD_PRIORITY_LOWEST (-2). So if you prefer I can go this way but I don't think it would be much better. Thanks, Eugene
http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... media/audio/win/waveout_output_win.cc:312: // from here, have to schedule it for the separate thread. On 2011/11/21 01:18:43, cpu wrote: > I don't agree with this approach. We have added another thread to this > operation, the one calling this callback is already a real-time thread. > > The right approach is to use CALLBACK_EVENT mode, and remove this overhead. Hey Carlos - If we use the event model, won't the runtime properties be similar to this approach? Presumably we would be waiting for the event on our own thread and the event signalled on another. In both cases there are two threads in the picture although in one case the signaling thread doesn't enter our code... right? Or is what we're saving, several context switches? I'm guessing, switch from kernel to user for callback and back, then later to the mq thread to process the message?
On 2011/11/22 08:21:45, tommi wrote: > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... > File media/audio/win/waveout_output_win.cc (right): > > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... > media/audio/win/waveout_output_win.cc:312: // from here, have to schedule it for > the separate thread. > On 2011/11/21 01:18:43, cpu wrote: > > I don't agree with this approach. We have added another thread to this > > operation, the one calling this callback is already a real-time thread. > > > > The right approach is to use CALLBACK_EVENT mode, and remove this overhead. > > Hey Carlos - If we use the event model, won't the runtime properties be similar > to this approach? Presumably we would be waiting for the event on our own > thread and the event signalled on another. In both cases there are two threads > in the picture although in one case the signaling thread doesn't enter our > code... right? > Or is what we're saving, several context switches? I'm guessing, switch from > kernel to user for callback and back, then later to the mq thread to process the > message? I see how we can use RegisterWaitForSingleObject() and buffer data directly from callback, without using message loops. Idea is very compelling, we would use only Windows mechanisms, without mixing them with Chrome ones. Code would be simple and elegant. Unfortunately there are drawbacks as well. We still have to use locks to avoid callbacks conflicting with each other -- Richter specifically says there is no guarantee 2 callbacks cannot be called simultaneously. And I don't see easy way to quickly stop playback, most likely we would have to use synchronous mode in UnregisterWaitForSingleObject() and wait for all callbacks to finish. Most likely we would use slightly less resources, but startup latency would not be lower. We would also lost the ability to post delayed tasks. If possible I'd prefer to use current code...
In my accounting we save one thread. That is, before this change there are two threads created by windows, lets call them A and B. The A thread seems to be mixing and the B thread is the one that you get the callback on. B is always in some form of real-time priority level and can be killed at the whim of windows. The A thread is longer lived and more mysterious. In the event mode B disappears and the event is signaled by either the driver, the A thread or maybe the kmixer (the master kernel mixer thread). On 2011/11/22 08:21:45, tommi wrote: > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... > File media/audio/win/waveout_output_win.cc (right): > > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... > media/audio/win/waveout_output_win.cc:312: // from here, have to schedule it for > the separate thread. > On 2011/11/21 01:18:43, cpu wrote: > > I don't agree with this approach. We have added another thread to this > > operation, the one calling this callback is already a real-time thread. > > > > The right approach is to use CALLBACK_EVENT mode, and remove this overhead. > > Hey Carlos - If we use the event model, won't the runtime properties be similar > to this approach? Presumably we would be waiting for the event on our own > thread and the event signalled on another. In both cases there are two threads > in the picture although in one case the signaling thread doesn't enter our > code... right? > Or is what we're saving, several context switches? I'm guessing, switch from > kernel to user for callback and back, then later to the mq thread to process the > message?
I already wrote -- thread B is not real time. Its priority is -3, that is between THREAD_PRIORITY_IDLE (-15) and THREAD_PRIORITY_LOWEST (-2). In case of event some thread still have to wait. That can be one of our threads, or if we use RegisterWaitForSingleObject() that would be one of threadpool waiting threads. When getting the event it will use some form of queue to schedule execution of callback on some other threadpool thread. Thanks, Eugene On Tue, Nov 22, 2011 at 9:41 AM, <cpu@chromium.org> wrote: > In my accounting we save one thread. That is, before this change there are > two > threads created by windows, lets call them A and B. The A thread seems to be > mixing and the B thread is the one that you get the callback on. B is always > in > some form of real-time priority level and can be killed at the whim of > windows. > The A thread is longer lived and more mysterious. > > In the event mode B disappears and the event is signaled by either the > driver, > the A thread or maybe the kmixer (the master kernel mixer thread). > > On 2011/11/22 08:21:45, tommi wrote: > > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... >> >> File media/audio/win/waveout_output_win.cc (right): > > > http://codereview.chromium.org/8591028/diff/7001/media/audio/win/waveout_outp... >> >> media/audio/win/waveout_output_win.cc:312: // from here, have to schedule >> it > > for >> >> the separate thread. >> On 2011/11/21 01:18:43, cpu wrote: >> > I don't agree with this approach. We have added another thread to this >> > operation, the one calling this callback is already a real-time thread. >> > >> > The right approach is to use CALLBACK_EVENT mode, and remove this >> > overhead. > >> Hey Carlos - If we use the event model, won't the runtime properties be > > similar >> >> to this approach? Presumably we would be waiting for the event on our own >> thread and the event signalled on another. In both cases there are two > > threads >> >> in the picture although in one case the signaling thread doesn't enter our >> code... right? >> Or is what we're saving, several context switches? I'm guessing, switch >> from >> kernel to user for callback and back, then later to the mq thread to >> process > > the >> >> message? > > > > http://codereview.chromium.org/8591028/ >
(hit send too soon) Yes the point is to keep the number of context switches to the minimum. Lots of effort has gone into that, for example the low-latency audio path skips the IO threads (of both browser and renderer) and uses a dedicated pipe/socket to signal the availability of buffers. > I see how we can use RegisterWaitForSingleObject() You can use the same approach of ObjectWatcher (src/base/win/object_watcher.cc) > Richter specifically says there is no > guarantee 2 callbacks cannot be called simultaneously. Not sure I understand what you mean.
> You can use the same approach of ObjectWatcher> (src/base/win/object_watcher.cc) I believe I already answered that earlier in the thread -- ObjectWatcher internally uses RegisterWaitForSingleObject, and it is not very convenient for us to use ObjectWatcher, it expects it to be started on the same thread that will later call the callbacks. And using ObjectWatcher would not reduce number of context switches... On Tue, Nov 22, 2011 at 10:19 AM, <cpu@chromium.org> wrote: > (hit send too soon) > > Yes the point is to keep the number of context switches to the minimum. Lots > of > effort has gone into that, for example the low-latency audio path skips the > IO > threads (of both browser and renderer) and uses a dedicated pipe/socket to > signal the availability of buffers. > >> I see how we can use RegisterWaitForSingleObject() > > You can use the same approach of ObjectWatcher > (src/base/win/object_watcher.cc) > >> Richter specifically says there is no >> guarantee 2 callbacks cannot be called simultaneously. > > Not sure I understand what you mean. > > > > > > http://codereview.chromium.org/8591028/ >
> In case of event some thread still have to wait. Maybe I haven't been clear. Let me try again. An event is always in the picture, in whatever mode waveOut is operated. In callback mode a windows thread waits on it and when that is signaled the thread awakes and enters our callback function. This change introduces yet another thread, yet another context switch, the callback now posts a task which in this case (I think post and I/O completion packet) to a chrome thread.
Rewrote the code to use event, not callback. Stopping is currently synchronous. Added comment how it can be speed up if that ever become a problem.
Mainly questions and comments first round. Will check more details next time. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:63: inline size_t PCMWaveOutAudioOutputStream::BufferSize() const { Why is this alignment needed? Benefits? http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:116: // Create buffer event. Empty line above comment. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:121: // Open the device. Empty line above comment. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:140: for (int ix = 0; ix != num_buffers_; ++ix) { Can you explain some more on why you have modified the buffer handling and removed the "circular list structure"? http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:336: // Windows call us back in this function when buffer_event_ is signalled. Are you saying that we now can set a buffer size of say 10ms and get events every 20ms and ask for 2 times 10ms each time? My point is that that it looks like you have modified the "callback timing" to the client given a certain buffer size. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:346: // Lock the stream. Two callbacks can be called simultaneously. Please elaborate. Not sure if I understand why this lock is needed.
http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (left): http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:38: static const uint32 kMaxOpenBufferSize = 1024 * 1024 * 64; the limits were added after a couple of security reviews. I don't think its wise to remove them. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:140: for (int ix = 0; ix != num_buffers_; ++ix) { On 2011/11/23 07:12:50, henrika1 wrote: > Can you explain some more on why you have modified the buffer handling and > removed the "circular list structure"? +1 http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:172: RegisterWaitForSingleObject(&waiting_handle, use :: for windows calls. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:177: WT_EXECUTEDEFAULT); People complain in the internet about the some lag on scheduling the callback using the default flag, with the workarround of passing the 'execute long' one. I don't know if the claim is true.
http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:117: buffer_event_.Set(::CreateEvent(NULL, FALSE, FALSE, NULL)); please add in the comment that this is an auto-reset event. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:368: } looks like if the event is signaled twice while a callback is in flight (slow) then it is possible that this loop here finds no buffer available, in this case we should sleep here?
Some more general input/questions: Has it been verified that this new implementation does in fact solve the original issue which started this work? I.e., switch of device while streaming? Also, given that WebAudio will use the same parts (for Wave), you should sync up with Ray so he can verify that the timing is OK for the WebAudio demos. I suggest that you CC both Ray and Chris on this CL. One would have to make some hacks to force the manager to use the old Wave parts while testing on Windows 7. On Thu, Nov 24, 2011 at 12:04 AM, <cpu@chromium.org> wrote: > > http://codereview.chromium.**org/8591028/diff/29001/media/** > audio/win/waveout_output_win.**cc<http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_output_win.cc> > File media/audio/win/waveout_**output_win.cc (right): > > http://codereview.chromium.**org/8591028/diff/29001/media/** > audio/win/waveout_output_win.**cc#newcode117<http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_output_win.cc#newcode117> > media/audio/win/waveout_**output_win.cc:117: > buffer_event_.Set(::**CreateEvent(NULL, FALSE, FALSE, NULL)); > please add in the comment that this is an auto-reset event. > > http://codereview.chromium.**org/8591028/diff/29001/media/** > audio/win/waveout_output_win.**cc#newcode368<http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_output_win.cc#newcode368> > media/audio/win/waveout_**output_win.cc:368: } > looks like if the event is signaled twice while a callback is in flight > (slow) then it is possible that this loop here finds no buffer > available, in this case we should sleep here? > > http://codereview.chromium.**org/8591028/<http://codereview.chromium.org/8591... > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
I believe I addressed all in-line comments. Yes, I verified that change fixes the problem -- I am able to change default audio device while stream is playing, and device really switches. (All 3 versions of code reviewed so far fix the problem) I tried web audio Drum Machine demo on both my dev Win7 machine (modifying the code to make wave out default one on any version of Windows) and 9-years old XP notebook (933MHz single-core CPU, 512Mb RAM): * There was bug in auto_utils.cc when WASAPI code path was called on XP (web audio only, HTML5 worked Ok) -- fixed. * Web audio does not work well on both Win7 and XP, with and without my changes. On Win7 there are constant clicks while you can clearly hear the music (both implementations). On XP two implementations sound somewhat differently, but sound is totally broken. New one is probably slightly better -- visual effects are more visible as web page renders more often (most likely because there is less real-time threads), but both sound badly, you have to guess music in both cases. It is very hard to say which way is worse... HTML5 audio works well on XP, most likely because it is less latency sensitive. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (left): http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:38: static const uint32 kMaxOpenBufferSize = 1024 * 1024 * 64; On 2011/11/23 22:39:56, cpu wrote: > the limits were added after a couple of security reviews. I don't think its wise > to remove them. Constant was not used anywhere. Check was removed by http://codereview.chromium.org/4661001. Added similar check back. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:63: inline size_t PCMWaveOutAudioOutputStream::BufferSize() const { On 2011/11/23 07:12:50, henrika1 wrote: > Why is this alignment needed? Benefits? If data is properly aligned compiler/libraries can use fast XMM instructions for memcpy() or vectorized load/store. VS2012 finally contains vectorizer, and I suspect it can vectorize loop in Crossfade(). http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:116: // Create buffer event. On 2011/11/23 07:12:50, henrika1 wrote: > Empty line above comment. Done. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:117: buffer_event_.Set(::CreateEvent(NULL, FALSE, FALSE, NULL)); On 2011/11/23 23:04:17, cpu wrote: > please add in the comment that this is an auto-reset event. Done. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:121: // Open the device. On 2011/11/23 07:12:50, henrika1 wrote: > Empty line above comment. Done. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:140: for (int ix = 0; ix != num_buffers_; ++ix) { On 2011/11/23 22:39:56, cpu wrote: > On 2011/11/23 07:12:50, henrika1 wrote: > > Can you explain some more on why you have modified the buffer handling and > > removed the "circular list structure"? > > +1 For historical reasons :-) It is 3rd major rewrite of the code. In the 1st one I need buffer->dwUser for some other data, so I got rid of pointers that use the same field. Later I did not need buffer->dwUser anymore, but I decided array is slightly cleaner, because (1) initialization code does not have to set up all the pointers, and (2) we can use scoped_array<> instead of explicit free[] call. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:172: RegisterWaitForSingleObject(&waiting_handle, On 2011/11/23 22:39:56, cpu wrote: > use :: for windows calls. Done. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:177: WT_EXECUTEDEFAULT); On 2011/11/23 22:39:56, cpu wrote: > People complain in the internet about the some lag on scheduling the callback > using the default flag, with the workarround of passing the 'execute long' one. > I don't know if the claim is true. I also don't know. MSDN says 'execute long' hints it's necessary to increase number of threads in the thread pool, not sure we want that. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:336: // Windows call us back in this function when buffer_event_ is signalled. On 2011/11/23 07:12:50, henrika1 wrote: > Are you saying that we now can set a buffer size of say 10ms and get events > every 20ms and ask for 2 times 10ms each time? My point is that that it looks > like you have modified the "callback timing" to the client given a certain > buffer size. Modified comment. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:346: // Lock the stream. Two callbacks can be called simultaneously. On 2011/11/23 07:12:50, henrika1 wrote: > Please elaborate. Not sure if I understand why this lock is needed. Done. http://codereview.chromium.org/8591028/diff/29001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:368: } On 2011/11/23 23:04:17, cpu wrote: > looks like if the event is signaled twice while a callback is in flight (slow) > then it is possible that this loop here finds no buffer available, in this case > we should sleep here? I doubt we can hit such a problem. We always have freed buffer if callback was called. I feared different problem, remember my questions about "events loss" when we discussed switching to events instead of explicit callbacks? I feared that event can be signaled 2nd time before wait thread would get it and event auto-switched to non-signaled state. To handle this I tried not to stop the loop after finding the first freed buffer, but that caused lot of audible clicks when rapidly stopping/starting the stream several times, most likely because we were asking for data too soon. Adding "return" fixed all of them. So I hope Windows audio API waits for event to become non-signaled before it signals the event, otherwise CALLBACK_EVENT node is (almost) unusable.
+rtoy Thanks for all your comments and changes Eugene. I've added Raymond on the CC list since you will need to cooperate with home to resolve the WebAudio issues. I don't have access to an XP machine but I think I can state that the existing buffer size is not correct and that is why it sounds bad on XP. As I mention in my comments, the added check for XP is OK but the buffer size must be modified given the new timing and the new WebKit changes for buffer handling done by Raymond. http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc#n... media/audio/audio_util.cc:296: if (base::win::GetVersion() <= base::win::VERSION_XP) { This will most likely not work! It is correct that the VERSION_XP check is needed. I had missed that in my last patch. BUT, the buffer size must be modified manually and tweaked by Raymond. It used to be 2048 for Windows but since then the WebKit part in WebAudio has changed. Again, you must ensure that Raymond helps you determine the best value here. It will depend on the new timing.
http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc#n... media/audio/audio_util.cc:296: if (base::win::GetVersion() <= base::win::VERSION_XP) { On 2011/11/29 12:20:42, henrika1 wrote: > This will most likely not work! > > It is correct that the VERSION_XP check is needed. I had missed that in my last > patch. > > BUT, the buffer size must be modified manually and tweaked by Raymond. It used > to be 2048 for Windows but since then the WebKit part in WebAudio has changed. > > Again, you must ensure that Raymond helps you determine the best value here. It > will depend on the new timing. This will mostly likely need to be set to 2048 again instead of 480. I do not have an XP machine to test with, so I will have to fake it somehow to test that the waveout method still works. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:107: format_.Samples.wValidBitsPerSample = params.bits_per_sample; In Open, there's a check for num_buffers_ between 2 and 5. Should there be a check here to clip the value 3 or 4? http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.h (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.h:130: base::Lock lock_; The previous version used a Windows-specific lock. There is no need to do that in this version?
http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:68: return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<size_t>(~15); Instead of static_cast<size_t>(~15), can this be just ~15u?
http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc#n... media/audio/audio_util.cc:299: return 480; I tested this on my Windows 7 machine by adjusting the checks to pretend this is a XP machine. Webaudio is horrible with this, so please change the 480 back to 2048.
Addressed all the issues. I believe I now know how to make code bullet-proof and safe in case of event losses, will send new version for review later today. Meanwhile please look at the code... I also believe fixing web audio on wave out should be done separately, it is totally broken now. http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc#n... media/audio/audio_util.cc:299: return 480; On 2011/11/29 18:45:29, rtoy wrote: > I tested this on my Windows 7 machine by adjusting the checks to pretend this is > a XP machine. Webaudio is horrible with this, so please change the 480 back to > 2048. 2048 does not help with either new or old implementation on both Win7 and XP. Increasing number of buffers to 3 does not help either. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:68: return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<size_t>(~15); On 2011/11/29 18:18:40, rtoy wrote: > Instead of static_cast<size_t>(~15), can this be just ~15u? Results are different on platform where sizeof(int) != sizeof(size_t), e.g. on Win64 -- in one case we sign extend 0xFFFFFFF0, in other zero-extend it. I know Chrome is currently 32-bit only, but I'd prefer safe code. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:107: format_.Samples.wValidBitsPerSample = params.bits_per_sample; On 2011/11/29 18:13:57, rtoy wrote: > In Open, there's a check for num_buffers_ between 2 and 5. Should there be a > check here to clip the value 3 or 4? I'd prefer leaving it this way. Actually, in the future I may want to return here and (1) change startup code to start playing after feeding first 2 buffers and feed all remaining ones, and (2) using more buffers, this way we would have fast startup and better behavior on busy systems. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.h (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.h:130: base::Lock lock_; On 2011/11/29 18:13:57, rtoy wrote: > The previous version used a Windows-specific lock. There is no need to do that > in this version? We changed wave out mode from CALLBACK_FUNCTION to CALLBACK_EVENT, so now there are no restrictions what code can use. Using Chrome lock is somewhat cleaner.
1) Increased size of web audio buffer on XP to 2048. That does not help, web audio still sounds horrible with both new and old code. I suggest to move it into separate issue. 2) Made code bullet-proof even in case of event loss. To do that, (a) got rid of "return" in the loop after finding 1st freed buffer, so one callback can fill all buffers, even if events for those buffers were lost, and (b) added extra check for stopped stream before feeding data to Windows -- this way we avoid click when stopping playback by not feeding extra data.
http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc#n... media/audio/audio_util.cc:299: return 480; On 2011/11/29 18:57:44, enal wrote: > On 2011/11/29 18:45:29, rtoy wrote: > > I tested this on my Windows 7 machine by adjusting the checks to pretend this > is > > a XP machine. Webaudio is horrible with this, so please change the 480 back > to > > 2048. > > 2048 does not help with either new or old implementation on both Win7 and XP. This seems not right. How did you test this? 2048 sounds fine for me on my Win7 machine with the XP tests changed to pretend I'm on an XP machine. 2048 was the value before Henrik added WASAPI, and that sounded fine on my machine. Do you have an example that sounds bad? I just ran the drumkit demo (and a few random other webaudio demos). http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:68: return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<size_t>(~15); On 2011/11/29 18:57:44, enal wrote: > On 2011/11/29 18:18:40, rtoy wrote: > > Instead of static_cast<size_t>(~15), can this be just ~15u? > > Results are different on platform where sizeof(int) != sizeof(size_t), e.g. on > Win64 -- in one case we sign extend 0xFFFFFFF0, in other zero-extend it. I know > Chrome is currently 32-bit only, but I'd prefer safe code. Ok. But won't ~15lu work in all cases then? Leave the static_cast if you want, though. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:107: format_.Samples.wValidBitsPerSample = params.bits_per_sample; On 2011/11/29 18:57:44, enal wrote: > On 2011/11/29 18:13:57, rtoy wrote: > > In Open, there's a check for num_buffers_ between 2 and 5. Should there be a > > check here to clip the value 3 or 4? > > I'd prefer leaving it this way. Actually, in the future I may want to return > here and (1) change startup code to start playing after feeding first 2 buffers > and feed all remaining ones, and (2) using more buffers, this way we would have > fast startup and better behavior on busy systems. Ok. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.h (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.h:130: base::Lock lock_; On 2011/11/29 18:57:44, enal wrote: > On 2011/11/29 18:13:57, rtoy wrote: > > The previous version used a Windows-specific lock. There is no need to do > that > > in this version? > > We changed wave out mode from CALLBACK_FUNCTION to CALLBACK_EVENT, so now there > are no restrictions what code can use. Using Chrome lock is somewhat cleaner. Ok.
Answered. http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc#n... media/audio/audio_util.cc:299: return 480; On 2011/11/29 20:01:29, rtoy wrote: > On 2011/11/29 18:57:44, enal wrote: > > On 2011/11/29 18:45:29, rtoy wrote: > > > I tested this on my Windows 7 machine by adjusting the checks to pretend > this > > is > > > a XP machine. Webaudio is horrible with this, so please change the 480 back > > to > > > 2048. > > > > 2048 does not help with either new or old implementation on both Win7 and XP. > > This seems not right. How did you test this? 2048 sounds fine for me on my > Win7 machine with the XP tests changed to pretend I'm on an XP machine. > > 2048 was the value before Henrik added WASAPI, and that sounded fine on my > machine. > > Do you have an example that sounds bad? I just ran the drumkit demo (and a few > random other webaudio demos). > I figured out what my problem was when testing on Win7. I forced audio manager to use wave out, but did not modify the code to set buffer size to 2k, so it used one for WASAPI because it is running on Win7. When I forced buffer size to 2k on Win7 everything became fine. Still broken on my XP, both old and new code, maybe because it has only 512Mb RAM. I ordered extra 512Mb, for testing in the future. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:68: return (sizeof(WAVEHDR) + buffer_size_ + 15u) & static_cast<size_t>(~15); On 2011/11/29 20:01:29, rtoy wrote: > On 2011/11/29 18:57:44, enal wrote: > > On 2011/11/29 18:18:40, rtoy wrote: > > > Instead of static_cast<size_t>(~15), can this be just ~15u? > > > > Results are different on platform where sizeof(int) != sizeof(size_t), e.g. on > > Win64 -- in one case we sign extend 0xFFFFFFF0, in other zero-extend it. I > know > > Chrome is currently 32-bit only, but I'd prefer safe code. > > Ok. But won't ~15lu work in all cases then? Leave the static_cast if you want, > though. No, ~15lu will not work, because sizeof(long) == 4 as well. You have to use ~15ULL (I prefer uppercase 'L', lowercase 'l' resembles '1' too much). Problem is that average developer never saw ULL in all his/her life, and with static_cast<> it's obvious what happens...
> So I hope Windows audio API waits for event to become non-signaled before it > signals the event, otherwise CALLBACK_EVENT node is (almost) unusable. It is an auto-reset event, it should not have that issue. In my experiments, windows XP could not handle buffers less than 40ms long. There are ways but you need special drivers and specific soundcards.
I am pretty much lgtm with your change. Hopefully things don't regress. The only concern at this point is that of using the win2k thread pool (latency), but it is the right thing to try. If that sucks we can consider something custom made. I also must mention that there is an interesting side effect, if the audio callback blocks for longer than a buffer long (tens of milliseconds) wave will signal the event again and a second thread will also do the callback, which media code might not expect. I don't recall if I added a test that does this, that is basically Sleep() in the callack. It will be worth doing that just to make sure we don't explode.
Code I sent today does not depend on that assumption. It can handle "event losses". On Tue, Nov 29, 2011 at 4:32 PM, <cpu@chromium.org> wrote: >> So I hope Windows audio API waits for event to become non-signaled before >> it >> signals the event, otherwise CALLBACK_EVENT node is (almost) unusable. > > > It is an auto-reset event, it should not have that issue. > > In my experiments, windows XP could not handle buffers less than 40ms long. > There are ways but you need special drivers and specific soundcards. > > > http://codereview.chromium.org/8591028/
Again, code I sent today would handle such case. It goes through all buffers, looking for freed ones, fills them, and give to Windows. If there are no free buffers, fine. On Tue, Nov 29, 2011 at 4:43 PM, <cpu@chromium.org> wrote: > I am pretty much lgtm with your change. Hopefully things don't regress. > > The only concern at this point is that of using the win2k thread pool > (latency), > but it is the right thing to try. If that sucks we can consider something > custom > made. > > I also must mention that there is an interesting side effect, if the audio > callback blocks for longer than a buffer long (tens of milliseconds) wave > will > signal the event again and a second thread will also do the callback, which > media code might not expect. > > I don't recall if I added a test that does this, that is basically Sleep() > in > the callack. It will be worth doing that just to make sure we don't explode. > > > > http://codereview.chromium.org/8591028/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enal@chromium.org/8591028/35005
Change committed as 112147
Eugene, nice work. Thanks for doing this! However, this patch breaks the media_unittests on Win7. Most likely some expected timing which has been modified. media_unittests --gtest_filter=WinAudioTest* fails.
Improved filter: media_unittests --gtest_filter=WinAudioTest*PCMWaveStreamPendingBytes* On Wed, Nov 30, 2011 at 9:05 AM, <henrika@chromium.org> wrote: > Eugene, > > nice work. Thanks for doing this! > > However, this patch breaks the media_unittests on Win7. Most likely some > expected timing which has been modified. > > media_unittests --gtest_filter=WinAudioTest* > > fails. > > http://codereview.chromium.**org/8591028/<http://codereview.chromium.org/8591... > -- Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 74
On 2011/11/29 21:02:45, enal wrote: > Answered. > > http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc > File media/audio/audio_util.cc (right): > > http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc#n... > media/audio/audio_util.cc:299: return 480; > On 2011/11/29 20:01:29, rtoy wrote: > > On 2011/11/29 18:57:44, enal wrote: > > > On 2011/11/29 18:45:29, rtoy wrote: > > > > I tested this on my Windows 7 machine by adjusting the checks to pretend > > this > > > is > > > > a XP machine. Webaudio is horrible with this, so please change the 480 > back > > > to > > > > 2048. > > > > > > 2048 does not help with either new or old implementation on both Win7 and > XP. > > > > This seems not right. How did you test this? 2048 sounds fine for me on my > > Win7 machine with the XP tests changed to pretend I'm on an XP machine. > > > > 2048 was the value before Henrik added WASAPI, and that sounded fine on my > > machine. > > > > Do you have an example that sounds bad? I just ran the drumkit demo (and a > few > > random other webaudio demos). > > > > I figured out what my problem was when testing on Win7. I forced audio manager > to use wave out, but did not modify the code to set buffer size to 2k, so it > used one for WASAPI because it is running on Win7. When I forced buffer size to > 2k on Win7 everything became fine. > > Still broken on my XP, both old and new code, maybe because it has only 512Mb > RAM. I ordered extra 512Mb, for testing in the future. I ran this build on my XP machine at home. It's a 2.2 GHz Core 2 Duo with 1 GB of memory. I would think this machine should do reasonably well, but the drumkit demo is absolutely horrid. You can hardly tell it's the drumkit running. But the chrome stable build runs the drumkit just fine, mostly. There are a few clicks when I move the window around, but nothing really bad. I would like to do a test without your change, but I won't be able to do that until tomorrow evening. Perhaps it's bad because of how I packaged my chrome build. I just zipped up the build/Release directory and copied it to my XP box at home and unzipped it and ran it. Perhaps that's not right. In fact, something is missing because about:tracing doesn't work.
I'm a bit surprised to see that this was submitted. Only 1 of 5 reviewers has given consent and I'd even say that the one lg*m wasn't that strong. I'm going to assume that some offline discussion took place and it was decided to land the change as is. Can anyone confirm? http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:186: static_cast<void*>(this), nit: no cast should be needed.
Sorry, it was my decision, and it looks I was wrong. I would revert it. Thanks, Euegen On Wed, Nov 30, 2011 at 2:33 AM, <tommi@chromium.org> wrote: > I'm a bit surprised to see that this was submitted. Only 1 of 5 reviewers > has > given consent and I'd even say that the one lg*m wasn't that strong. > I'm going to assume that some offline discussion took place and it was > decided > to land the change as is. Can anyone confirm? > > > http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... > File media/audio/win/waveout_output_win.cc (right): > > http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... > media/audio/win/waveout_output_win.cc:186: static_cast<void*>(this), > nit: no cast should be needed. > > http://codereview.chromium.org/8591028/
On 2011/11/30 15:56:52, enal1 wrote: > Sorry, it was my decision, and it looks I was wrong. I would revert it. > > Thanks, > Euegen > > On Wed, Nov 30, 2011 at 2:33 AM, <mailto:tommi@chromium.org> wrote: > > I'm a bit surprised to see that this was submitted. Only 1 of 5 reviewers > > has > > given consent and I'd even say that the one lg*m wasn't that strong. > > I'm going to assume that some offline discussion took place and it was > > decided > > to land the change as is. Can anyone confirm? > > > > > > > http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... > > File media/audio/win/waveout_output_win.cc (right): > > > > > http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... > > media/audio/win/waveout_output_win.cc:186: static_cast<void*>(this), > > nit: no cast should be needed. > > > > http://codereview.chromium.org/8591028/ Reverted. http://codereview.chromium.org/8753001/ Waiting for LGTM from all reviewers. Again, it was bad decision on my part -- yesterday I felt lot of pressure because of incoming bugs and my scheduled work being late, sorry...
Thank you, exact failure varies between runs on my system. Sometimes there is no failure... Looking how to make expectations timing-safe... On Wed, Nov 30, 2011 at 12:11 AM, Henrik Andreasson <henrika@google.com> wrote: > Improved filter: > > media_unittests --gtest_filter=WinAudioTest*PCMWaveStreamPendingBytes* > > On Wed, Nov 30, 2011 at 9:05 AM, <henrika@chromium.org> wrote: >> >> Eugene, >> >> nice work. Thanks for doing this! >> >> However, this patch breaks the media_unittests on Win7. Most likely some >> expected timing which has been modified. >> >> media_unittests --gtest_filter=WinAudioTest* >> >> fails. >> >> http://codereview.chromium.org/8591028/ > > > > > -- > > Henrik Andreasson | Software Engineer | henrika@google.com | +46 70 376 91 > 74 >
Addressed nit, made unit test less sensitive to exact timing. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:186: static_cast<void*>(this), On 2011/11/30 10:33:50, tommi wrote: > nit: no cast should be needed. Done.
lgtm and thanks for waiting. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:127: )); nit: close parenthesis on the preceding line. http://codereview.chromium.org/8591028/diff/34001/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:128: if (!buffer_event_.Get()) { nit: no {} http://codereview.chromium.org/8591028/diff/49002/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/49002/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:353: void NTAPI PCMWaveOutAudioOutputStream::BufferCallback(PVOID lpParameter, nit: remove hungarian and PVOID->void*
Did not verify that the new unit test works. LGTM w/ nits.
http://codereview.chromium.org/8591028/diff/49002/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.cc (right): http://codereview.chromium.org/8591028/diff/49002/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:123: buffer_event_.Set(::CreateEvent(NULL, // Security attributes nit (add period '.')? http://codereview.chromium.org/8591028/diff/49002/media/audio/win/waveout_out... media/audio/win/waveout_output_win.cc:360: // Lock the stream so 2 callbacks do not interfere with each other. two? http://codereview.chromium.org/8591028/diff/49002/media/audio/win/waveout_out... File media/audio/win/waveout_output_win.h (right): http://codereview.chromium.org/8591028/diff/49002/media/audio/win/waveout_out... media/audio/win/waveout_output_win.h:70: static void NTAPI BufferCallback(PVOID lpParameter, BOOLEAN); variable name missing
On 2011/11/30 08:31:49, rtoy wrote: > On 2011/11/29 21:02:45, enal wrote: > > Answered. > > > > http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc > > File media/audio/audio_util.cc (right): > > > > > http://codereview.chromium.org/8591028/diff/34001/media/audio/audio_util.cc#n... > > media/audio/audio_util.cc:299: return 480; > > On 2011/11/29 20:01:29, rtoy wrote: > > > On 2011/11/29 18:57:44, enal wrote: > > > > On 2011/11/29 18:45:29, rtoy wrote: > > > > > I tested this on my Windows 7 machine by adjusting the checks to pretend > > > this > > > > is > > > > > a XP machine. Webaudio is horrible with this, so please change the 480 > > back > > > > to > > > > > 2048. > > > > > > > > 2048 does not help with either new or old implementation on both Win7 and > > XP. > > > > > > This seems not right. How did you test this? 2048 sounds fine for me on my > > > Win7 machine with the XP tests changed to pretend I'm on an XP machine. > > > > > > 2048 was the value before Henrik added WASAPI, and that sounded fine on my > > > machine. > > > > > > Do you have an example that sounds bad? I just ran the drumkit demo (and a > > few > > > random other webaudio demos). > > > > > > > I figured out what my problem was when testing on Win7. I forced audio manager > > to use wave out, but did not modify the code to set buffer size to 2k, so it > > used one for WASAPI because it is running on Win7. When I forced buffer size > to > > 2k on Win7 everything became fine. > > > > Still broken on my XP, both old and new code, maybe because it has only 512Mb > > RAM. I ordered extra 512Mb, for testing in the future. > > I ran this build on my XP machine at home. It's a 2.2 GHz Core 2 Duo with 1 GB > of memory. I would think this machine should do reasonably well, but the > drumkit demo is absolutely horrid. You can hardly tell it's the drumkit > running. But the chrome stable build runs the drumkit just fine, mostly. There > are a few clicks when I move the window around, but nothing really bad. > > I would like to do a test without your change, but I won't be able to do that > until tomorrow evening. Ok. I removed your patch and rebuilt and tested this on my XP machine. The audio is bad on the drumkit. I don't know why. I would investigate this some more, but... I applied your patch and built again. This works fine on my XP machine. It sounds good, except for a few glitches, but the stable chrome build behaves the same. I am happy with this change now. Sorry for the false alarm. I don't know what happened with my first test.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enal@chromium.org/8591028/57002
Change committed as 112484 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
