Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7473021:
PulseAudio Sound Playback on Linux (Closed)
|
Created:
9 years, 5 months ago by slock Modified:
9 years, 4 months ago Reviewers:
vrk (LEFT CHROMIUM), Ami GONE FROM CHROMIUM, Sergey Ulanov, commit-bot: I haz the power, awong, Paweł Hajdan Jr., scherkus (not reviewing) 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 Visibility:
Public. |
DescriptionPulseAudio Sound Playback on Linux
This is the preliminary implementation of a PulseAudio sound backend for Chrome on Linux. At first, PulseAudio's mainloop, mainloop_api, and context constructs will be used instead of the message loop system used in alsa_output. This will be stereo only at first. Also, at first, PulseAudio will be dynamically linked in media.gyp as opposed to the final solution which will dynamically link PulseAudio in runtime if it is available.
BUG=32757
TEST=
Patch Set 1 #
Total comments: 19
Patch Set 2 : Opening a stream now works. #Patch Set 3 : "The disgusting hack I mentioned to prove audio output stream with a crude sine wave" #Patch Set 4 : "New helper methods" #Patch Set 5 : "Audio playback works, not quite synchronized" #Patch Set 6 : "Synchronized and clear audio to go with video." #Patch Set 7 : "Gyp and command line flag added, pausing/stopping/restarting works" #
Total comments: 82
Patch Set 8 : "Comment response etc." #
Total comments: 2
Patch Set 9 : "media.gyp fix" #
Total comments: 4
Patch Set 10 : "Preprocessor define added #
Total comments: 22
Patch Set 11 : "Uncommented thing I forgot to uncomment" #
Total comments: 20
Patch Set 12 : "Nits and more" #Patch Set 13 : "Added a MessageLoop so playback doesn't block the audio thread" #Patch Set 14 : "Volume adjustment works now" #
Total comments: 56
Patch Set 15 : "Comment response vrk" #
Total comments: 24
Patch Set 16 : "New buffering scheme, per offline with vrk" #
Total comments: 4
Patch Set 17 : "New runflow per offline with vrk" #
Total comments: 10
Patch Set 18 : "vrk comments response" #
Total comments: 16
Patch Set 19 : "Comment response vrk" #
Total comments: 2
Patch Set 20 : "Final nit" #Patch Set 21 : "alsa_output_unittest fix" #
Messages
Total messages: 41 (0 generated)
Just the bottom of the foundation. I'm hoping to flesh out this class and work through the few different iterations of the implementation of the feature we have planned while feedback comes in in parallel. I'm really excited to have an intern project of this importance.
Driveby style nits. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.cc File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:1: #include "media/audio/linux/pulse_output.h" Copyright header needed. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:25: // TODO: Sanity check input values. "TODO(slock): comment", here and elsewhere. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:73: uint32 max_size, Indentation is off. As the last arg won't fit, bring all the args (inc the first) to the next line, indenting by 4 spaces. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:84: AudioSourceCallback* callback) { Indent this 4 spaces from the begininning of the line. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.h File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:1: #ifndef MEDIA_AUDIO_LINUX_PULSE_OUTPUT_H_ You need a chromium copyright header here. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:25: // Implementation of AudioOutputStream End comments with a period. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:34: Spurious blank line. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:43: }; Blank line after the enum.
Drive-by. http://codereview.chromium.org/7473021/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7473021/diff/1/media/media.gyp#newcode231 media/media.gyp:231: '-lpulse', Pretty sure this will break on cros/arm where we don't carry pulse as a dependency (yet, at least).
Thanks! http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.cc File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:1: #include "media/audio/linux/pulse_output.h" I was going to copy-paste that in right before uploading and then forgot to do it. Done of course. On 2011/07/21 04:58:32, sjl wrote: > Copyright header needed. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:25: // TODO: Sanity check input values. On 2011/07/21 04:58:32, sjl wrote: > "TODO(slock): comment", here and elsewhere. Done. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:73: uint32 max_size, On 2011/07/21 04:58:32, sjl wrote: > Indentation is off. As the last arg won't fit, bring all the args (inc the > first) to the next line, indenting by 4 spaces. Done. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:84: AudioSourceCallback* callback) { On 2011/07/21 04:58:32, sjl wrote: > Indent this 4 spaces from the begininning of the line. Done. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.h File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:1: #ifndef MEDIA_AUDIO_LINUX_PULSE_OUTPUT_H_ On 2011/07/21 04:58:32, sjl wrote: > You need a chromium copyright header here. Done. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:25: // Implementation of AudioOutputStream On 2011/07/21 04:58:32, sjl wrote: > End comments with a period. Done. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:34: On 2011/07/21 04:58:32, sjl wrote: > Spurious blank line. Done. http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:43: }; I should fix this in alsa_output.h too then I take it? On 2011/07/21 04:58:32, sjl wrote: > Blank line after the enum. http://codereview.chromium.org/7473021/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7473021/diff/1/media/media.gyp#newcode231 media/media.gyp:231: '-lpulse', That's also my understanding. I was advised to link in pulse temporarily while I get it working, and move to dynamically linking it as a later step. Albert is helping me plan this out. On 2011/07/21 16:59:03, Ami Fischman wrote: > Pretty sure this will break on cros/arm where we don't carry pulse as a > dependency (yet, at least).
http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.cc File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/1/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:29: // TODO you know what's even better than TODO... NOTIMPLEMENTED() macro :)
This code successfully opens a "default" stream. It does play sound too. To make sure this worked, I wrote code (not shown here) that hijacked the Open() function to play a sample tone. Next I need to write Write(), BufferPacket(), WritePacket(), and ScheduleNextWrite() in order to play the sounds from the video through PulseAudio.
FYI, I'm not going to be able to look at this code soon. I can spot-check some areas if you tell me where to look, but Victoria, can I pass this off to you? -Albert On Mon, Jul 25, 2011 at 5:05 PM, <slock@chromium.org> wrote: > This code successfully opens a "default" stream. It does play sound too. > To > make sure this worked, I wrote code (not shown here) that hijacked the > Open() > function to play a sample tone. Next I need to write Write(), > BufferPacket(), > WritePacket(), and ScheduleNextWrite() in order to play the sounds from the > video through PulseAudio. > > > http://codereview.chromium.**org/7473021/<http://codereview.chromium.org/7473... >
On 2011/07/26 01:23:17, awong wrote: > FYI, I'm not going to be able to look at this code soon. I can spot-check > some areas if you tell me where to look, but Victoria, can I pass this off > to you? Whoops, yes! That's what was meant all along; sorry for not clarifying specifically! Steven or I will ping you directly if there's something specific we'd like you to take a look at. Until then feel free to ignore the CL! :)
CL looking good so far! I will refrain from line-by-line review until you have something more developed. Let me know if there are particular places in the code you'd like me to review. On 2011/07/26 00:05:35, Steven Lockhart wrote: > I wrote code (not shown here) that hijacked the Open() > function to play a sample tone. Why not upload it? Even if it's a hack, would be good progress to see! > Next I need to write Write(), BufferPacket(), > WritePacket(), and ScheduleNextWrite() in order to play the sounds from the > video through PulseAudio. Hmm, I noticed that you have kept the same private functions and fields from AlsaOutput. That's not necessarily a bad thing, but unless Alsa and PulseAudio are virtually identical, it seems strange to me that both objects would have the exact same private state and behavior. Instead of filling out these copied private functions, focus on implementing the functions from the AudioOutputStream interface. If in implementing this interface, you find it'd be useful and makes sense to have these same private functions, that's fine, but otherwise write whatever new functions and fields you need to make the AudioOutputStream interface functionality work for Pulse.
On 2011/07/26 18:26:34, Victoria Kirst wrote: > CL looking good so far! > > I will refrain from line-by-line review until you have something more developed. > Let me know if there are particular places in the code you'd like me to review. > > On 2011/07/26 00:05:35, Steven Lockhart wrote: > > I wrote code (not shown here) that hijacked the Open() > > function to play a sample tone. > > Why not upload it? Even if it's a hack, would be good progress to see! > > > Next I need to write Write(), BufferPacket(), > > WritePacket(), and ScheduleNextWrite() in order to play the sounds from the > > video through PulseAudio. > > Hmm, I noticed that you have kept the same private functions and fields from > AlsaOutput. That's not necessarily a bad thing, but unless Alsa and PulseAudio > are virtually identical, it seems strange to me that both objects would have the > exact same private state and behavior. > > Instead of filling out these copied private functions, focus on implementing the > functions from the AudioOutputStream interface. If in implementing this > interface, you find it'd be useful and makes sense to have these same private > functions, that's fine, but otherwise write whatever new functions and fields > you need to make the AudioOutputStream interface functionality work for Pulse. I was just trying to guess at what I might need and using the ALSA code as an example. The implementation of these methods are going to be different in PulseAudio-land (they already are, with what I've written so far) but I just figured that PulseAudio would still have things like InternalState and BufferPacket. I might be wrong though! This is what I'm moving into right now.
This plays sound from videos now. AudioManagerLinux is hardcoded to use my new PulseAudio path, but PulseAudio is also still linked in in compilation. Audio playback that I've heard is smooth and not choppy; no clipping. But it is not synchronized with the video. There is also sometimes static at the start. Lots of other important things, like cleanup/teardown, are also not implemented yet.
Audio output is clear and in sync the accompanying video now. This involved a little bit of a hack (making BufferPacketInClient public) but that won't be an issue once I abstract away the PulseAudio API behind a MessageLoop (hopefully). I need to rebase with Albert next.
Exciting progress so far!! One general question: How is seeking supposed to work with AudioOutputStream? Is it a call to Stop() then a call to Start() with a callback that loads data from the new location? I also wanted to confirm my understanding of what is going on in PulseAudioOutputStream::Start(). It appears to me this method essentially blocks until OnMoreData() returns 0: - pa_mainloop_iterate() with a nonzero 2nd parameter will block until it gets a new event to process. It is wrapped in a while loop that terminates when source_exhausted_ is true. - The only event pa_mainloop_iterate() will handle is WriteCallback, and that only sets source_exhausted_ to true when OnMoreData() returns 0. - AudioOutputStream API functions are always called from the same thread, as stated in alsa_output.h - (ISTM stop_stream_ is untouched in this and thus irrelevant to the termination of the ClientBufferLoop while loop.) It looks like OnMoreData() will write data into the buffer if the pipeline has downloaded and decoded enough to fulfill the request, and will return 0 if not (i.e. it will not block to receive+decode enough data). If my understanding above is correct, wouldn't it mean that if we have a fast enough network where we're always decoded enough to fulfill data requests, we block Start() until we've completely finished playing the audio of the video? If so, that should probably be fixed... my guess is seek/stop is working now because when Stop() is called by the other components of the pipeline, OnMoreData() return 0 and therefore stops blocking PulseAudioOutputStream::Start(), but PulseAudioOutputStream itself is not responding to the Stop(). Also it seems like the this impl will stop playing sound altogether after the first time OnMoreData() returns 0, which could happen in the middle of playback because of slow network. If the network stalls like this, will we be issued another call to Start()? Thanks! http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.h (right): http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.h:16: class PulseOutputStream; unused; remove http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:11: void PulseAudioStateCallback(pa_context* c, void* userdata) { All these callback methods should be static (to avoid cluttering up global namespace!) http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:11: void PulseAudioStateCallback(pa_context* c, void* userdata) { nit: more descriptive variable name than "c"? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:15: int* pa_context_ready = (int*)userdata; static_cast instead of C-style cast http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:16: state = pa_context_get_state(c); pa_context_state_t state = ... and remove line 14 http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:32: void WriteCallback(pa_stream* s, size_t length, void* userdata) { So actually, I think you can make this a static private member function of the PulseAudioOutputStream class. Then you could pass in "this" for the user data, and you should be able to make BufferPacketInClient() a private method? I *think* that'll work, try it and see! http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:38: !stream_ptr->source_exhausted_) nit: {} around body of loop (multi-line condition) http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:42: uint8 read_data[length]; variable-length arrays are no good! scoped_array instead http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:45: pa_stream_write(s, read_data, length, NULL, 0LL, PA_SEEK_RELATIVE); nit: more descriptive variable name than "s"? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:48: pa_sample_format_t BitsToFormat(int bits_per_sample) { static http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:99: // TODO(slock): Nothing to be done but state work. Delete comment Also, here you *do* want to free your resources, stop the stream, etc... http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:106: if (state() == kInError) delete; state transitions are not implemented http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:109: // TODO(slock): Implement state transitions. delete TODO http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:112: pa_mainloop_ = pa_mainloop_new(); Never deleted; needs a call to pa_mainloop_free http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:114: pa_context_ = pa_context_new(pa_mainloop_api_, "Chromium"); Never deleted; I believe you need to call pa_context_unref http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:121: while (pa_context_ready == 0 ){ nit: while (!pa_context_ready) and no {} http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:124: Handle error condition (pa_context_ready != PA_CONTEXT_READY) and return false? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:126: pa_sample_specs_ = new pa_sample_spec; probably better to stack-allocate instead of creating new http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:130: playback_handle_ = pa_stream_new(pa_context_, "Playback", Need to delete (I believe pa_stream_unref) http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:134: bytes_per_output_frame_ = bytes_per_frame_; bytes_per_output_frame_ is an unnecessary field; remove http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:139: pa_stream_set_write_callback(playback_handle_, WriteCallback, nit: put & before "WriteCallback" for clarity/consistency http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:140: this); nit: move "this" to line above http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:144: // values from PulseAudio's documentation for now. link to documentation where it gives these recommendations? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:145: pa_buffer_attributes_ = new pa_buffer_attr; stack-allocate instead of creating new http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:151: pa_stream_connect_playback(playback_handle_, NULL, indentation http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:159: // Finish initializing the stream if the device was opened successfully. Comment doesn't seem to match the block below? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:160: if (playback_handle_ == NULL) { nit: if (!playback_handle_) and no {} http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:161: stop_stream_ = true; What does this accomplish? Also, wouldn't you want to return false here? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:186: if (stop_stream_) { Is this ever reached? BufferPacketInClient() is only called by WriteCallback WriteCallback will be triggered synchronously from ClientBufferLoop()'s pa_mainloop_iterate stop_stream_ is never modified in between that time, I don't think. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:201: int negative; Is it okay for negative to be unused in hardware_delay calculation? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:203: uint32 hardware_delay = MicrosToBytes(pa_latency_micros, sample_rate_, nit: Change "Micros" to "Microseconds" http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:217: // WritePacket. TODO(slocK): Ensure that this is relevant here, it might "WritePacket" doesn't exist http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:228: } else nit: This time you *should* have {} around the else block :) http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:247: set_source_callback(callback); inline this method http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:258: // Nothing to be done because InternalState not implemented. Instead of trying to "port" the Alsa output stream behavior, you should read and implement the AudioOutputStream interface. The description for stop says: "Stops playing audio. Effect might not be instantaneous as the hardware might have locked audio data that is processing." This is relevant/necessary for your class, independent of state transitions. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:263: volume_ = static_cast<float>(volume); TODO make necessary calls to PulseAudio to actually set volume? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:297: size_t PulseAudioOutputStream::MicrosToBytes(uint32 micros, uint32 sample_rate, file-static function? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:37: void BufferPacketInClient(); As I note in the .cc file, I think you can make this static private http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:38: media::SeekableBuffer* client_buffer_; Make these fields private http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:38: media::SeekableBuffer* client_buffer_; scoped_ptr instead? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:43: enum InternalState { Please delete all unused/unimplemented private fields, enums, and functions in your CL.
A lot of smaller problems I have marked as 'Done.' were in fact washed away by larger changes that I made for the forthcoming patch. This should be a lot closer to ready to go. Summarizers: - PulseAudio linking got ripped out since the last patch, so my GYP variable is now the only thing that can dynamically link PulseAudio. This GYP variable (and the accompanying command line flag) are required to use pulse_output.cc in anyway. - Volume adjustment does not work because during playback PulseAudioOutputStream's thread is stuck in ClientBufferLoop. - Seeking, Stopping, Restarting, etc. all work. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.h (right): http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.h:16: class PulseOutputStream; On 2011/08/05 15:02:26, Victoria Kirst wrote: > unused; remove Done. Also removed line 14. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:11: void PulseAudioStateCallback(pa_context* c, void* userdata) { On 2011/08/05 15:02:26, Victoria Kirst wrote: > All these callback methods should be static (to avoid cluttering up global > namespace!) Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:11: void PulseAudioStateCallback(pa_context* c, void* userdata) { On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: more descriptive variable name than "c"? Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:15: int* pa_context_ready = (int*)userdata; On 2011/08/05 15:02:26, Victoria Kirst wrote: > static_cast instead of C-style cast Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:16: state = pa_context_get_state(c); On 2011/08/05 15:02:26, Victoria Kirst wrote: > pa_context_state_t state = ... > and remove line 14 Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:32: void WriteCallback(pa_stream* s, size_t length, void* userdata) { On 2011/08/05 15:02:26, Victoria Kirst wrote: > So actually, I think you can make this a static private member function of the > PulseAudioOutputStream class. > > Then you could pass in "this" for the user data, and you should be able to make > BufferPacketInClient() a private method? > > I *think* that'll work, try it and see! Done. That DID work, awesome. That's huge. Should I have known that that would work? http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:38: !stream_ptr->source_exhausted_) On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: {} around body of loop (multi-line condition) Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:42: uint8 read_data[length]; On 2011/08/05 15:02:26, Victoria Kirst wrote: > variable-length arrays are no good! scoped_array instead Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:45: pa_stream_write(s, read_data, length, NULL, 0LL, PA_SEEK_RELATIVE); On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: more descriptive variable name than "s"? Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:48: pa_sample_format_t BitsToFormat(int bits_per_sample) { On 2011/08/05 15:02:26, Victoria Kirst wrote: > static Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:99: // TODO(slock): Nothing to be done but state work. On 2011/08/05 15:02:26, Victoria Kirst wrote: > Delete comment > > Also, here you *do* want to free your resources, stop the stream, etc... Done. Note that AudioManagerLinux requires the runflow this::Close(), then AudioManagerLinux::ReleaseOutputStream(this), and finally this::~PulseAudioOutputStream. Therefore, resources are freed in Close() and the destructor is empty. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:106: if (state() == kInError) On 2011/08/05 15:02:26, Victoria Kirst wrote: > delete; state transitions are not implemented Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:109: // TODO(slock): Implement state transitions. On 2011/08/05 15:02:26, Victoria Kirst wrote: > delete TODO Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:112: pa_mainloop_ = pa_mainloop_new(); On 2011/08/05 15:02:26, Victoria Kirst wrote: > Never deleted; needs a call to pa_mainloop_free Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:114: pa_context_ = pa_context_new(pa_mainloop_api_, "Chromium"); On 2011/08/05 15:02:26, Victoria Kirst wrote: > Never deleted; I believe you need to call pa_context_unref Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:121: while (pa_context_ready == 0 ){ On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: while (!pa_context_ready) and no {} Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:124: On 2011/08/05 15:02:26, Victoria Kirst wrote: > Handle error condition (pa_context_ready != PA_CONTEXT_READY) and return false? Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:126: pa_sample_specs_ = new pa_sample_spec; On 2011/08/05 15:02:26, Victoria Kirst wrote: > probably better to stack-allocate instead of creating new Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:130: playback_handle_ = pa_stream_new(pa_context_, "Playback", On 2011/08/05 15:02:26, Victoria Kirst wrote: > Need to delete (I believe pa_stream_unref) Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:134: bytes_per_output_frame_ = bytes_per_frame_; On 2011/08/05 15:02:26, Victoria Kirst wrote: > bytes_per_output_frame_ is an unnecessary field; remove Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:139: pa_stream_set_write_callback(playback_handle_, WriteCallback, On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: put & before "WriteCallback" for clarity/consistency Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:140: this); On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: move "this" to line above Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:144: // values from PulseAudio's documentation for now. On 2011/08/05 15:02:26, Victoria Kirst wrote: > link to documentation where it gives these recommendations? Done, but the url plus the "//" and indentation is more than 80 characters. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:145: pa_buffer_attributes_ = new pa_buffer_attr; On 2011/08/05 15:02:26, Victoria Kirst wrote: > stack-allocate instead of creating new Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:151: pa_stream_connect_playback(playback_handle_, NULL, On 2011/08/05 15:02:26, Victoria Kirst wrote: > indentation Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:159: // Finish initializing the stream if the device was opened successfully. On 2011/08/05 15:02:26, Victoria Kirst wrote: > Comment doesn't seem to match the block below? Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:160: if (playback_handle_ == NULL) { On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: if (!playback_handle_) and no {} Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:161: stop_stream_ = true; On 2011/08/05 15:02:26, Victoria Kirst wrote: > What does this accomplish? Also, wouldn't you want to return false here? Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:186: if (stop_stream_) { On 2011/08/05 15:02:26, Victoria Kirst wrote: > Is this ever reached? > > BufferPacketInClient() is only called by WriteCallback > WriteCallback will be triggered synchronously from ClientBufferLoop()'s > pa_mainloop_iterate > > stop_stream_ is never modified in between that time, I don't think. I'll remove this for now, but this is part of the larger problem of Start()/ClientBufferLoop() blocking everything else. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:201: int negative; On 2011/08/05 15:02:26, Victoria Kirst wrote: > Is it okay for negative to be unused in hardware_delay calculation? No, but its never been negative that I know of. I'll need to decide what latency to use in this case. This will involve more research. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:203: uint32 hardware_delay = MicrosToBytes(pa_latency_micros, sample_rate_, On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: Change "Micros" to "Microseconds" Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:217: // WritePacket. TODO(slocK): Ensure that this is relevant here, it might On 2011/08/05 15:02:26, Victoria Kirst wrote: > "WritePacket" doesn't exist Done. This wasn't relevant anyway because I don't write by the packet. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:228: } else On 2011/08/05 15:02:26, Victoria Kirst wrote: > nit: This time you *should* have {} around the else block :) Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:247: set_source_callback(callback); On 2011/08/05 15:02:26, Victoria Kirst wrote: > inline this method Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:258: // Nothing to be done because InternalState not implemented. On 2011/08/05 15:02:26, Victoria Kirst wrote: > Instead of trying to "port" the Alsa output stream behavior, you should read and > implement the AudioOutputStream interface. The description for stop says: "Stops > playing audio. Effect might not be instantaneous as the hardware might have > locked audio data that is processing." > > This is relevant/necessary for your class, independent of state transitions. Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:263: volume_ = static_cast<float>(volume); On 2011/08/05 15:02:26, Victoria Kirst wrote: > TODO make necessary calls to PulseAudio to actually set volume? Done. I actually implemented this, but its not tested. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:297: size_t PulseAudioOutputStream::MicrosToBytes(uint32 micros, uint32 sample_rate, On 2011/08/05 15:02:26, Victoria Kirst wrote: > file-static function? Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:37: void BufferPacketInClient(); On 2011/08/05 15:02:26, Victoria Kirst wrote: > As I note in the .cc file, I think you can make this static private Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:38: media::SeekableBuffer* client_buffer_; On 2011/08/05 15:02:26, Victoria Kirst wrote: > scoped_ptr instead? Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:38: media::SeekableBuffer* client_buffer_; On 2011/08/05 15:02:26, Victoria Kirst wrote: > Make these fields private Done. http://codereview.chromium.org/7473021/diff/29001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:43: enum InternalState { On 2011/08/05 15:02:26, Victoria Kirst wrote: > Please delete all unused/unimplemented private fields, enums, and functions in > your CL. Done. Pretty sure I got them all.
http://codereview.chromium.org/7473021/diff/34002/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7473021/diff/34002/media/media.gyp#newcode242 media/media.gyp:242: ['use_pulse_audio == 1', { Have you tested this on a system without PA? I think you should also exclude PA-specific sources if use_pulse_audio==0.
http://codereview.chromium.org/7473021/diff/34002/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7473021/diff/34002/media/media.gyp#newcode242 media/media.gyp:242: ['use_pulse_audio == 1', { On 2011/08/08 20:43:40, Paweł Hajdan Jr. wrote: > Have you tested this on a system without PA? I think you should also exclude > PA-specific sources if use_pulse_audio==0. Done. Definitely, for the use-case of the person trying to compile on Linux without PA. There won't be any such system readily available around the office though as Goobuntu has PA built-in. Also of note, I considered excluding PA for FreeBSD and Solaris but decided not to for now (and probably won't in general). PA exists for/has been tested for those systems and there is easily available evidence of people actually using it on those systems (though with mixed results).
I think it's still broken with systems that don't have PA installed. http://codereview.chromium.org/7473021/diff/38001/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7473021/diff/38001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:19: #include "media/audio/linux/pulse_output.h" This header includes PA headers. audio_manager_linux.cc is always compiled, so this will still break on systems not using PA. http://codereview.chromium.org/7473021/diff/38001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:108: std::string device_name = AlsaPcmOutputStream::kAutoSelectDevice; Isn't this dead code? |pulse| is always true.
http://codereview.chromium.org/7473021/diff/38001/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7473021/diff/38001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:19: #include "media/audio/linux/pulse_output.h" On 2011/08/09 16:32:51, Paweł Hajdan Jr. wrote: > This header includes PA headers. audio_manager_linux.cc is always compiled, so > this will still break on systems not using PA. Done. http://codereview.chromium.org/7473021/diff/38001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:108: std::string device_name = AlsaPcmOutputStream::kAutoSelectDevice; On 2011/08/09 16:32:51, Paweł Hajdan Jr. wrote: > Isn't this dead code? |pulse| is always true. Yes it was, and it shouldn't have been there. I thought I had most of my Git wrinkles figured out, but I have no idea what happened there.
Just noticed that I forgot to uncomment something three patches ago and fixed it. Also, comments this week would be awesome (ping!) as I'm starting to work on other CLs that will depend on this one as part of my overall plan to break this project into smaller pieces/CLs.
Getting there! There's still the problem the Start() until OnMoreData() returns 0, and thus subsequent calls to Stop/Close/Set/GetVolume will stall until OnMoreData() has returned (presumably when the component controlling that callback has Stop()ed or if the internet connection is slow, but I'm not sure). Maybe that's okay for an initial check-in though? (If so, I'd have some additional suggestions to make the current situation more obvious.) ajwong: Could you give this a look? http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:44: int* context_ready = static_cast<int*>(userdata); Change int* to pa_context_state_t*, then just do *context_ready = pa_context_get_state(context) (no need for switch statement below) http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:98: // AudioManagerLinux::Release which deletes this object. DCHECK playback_handle_, pa_context_, pa_mainloop_ are all NULL http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:112: int pa_context_ready = 1; (See comment in ContextStateCallback) Change int to pa_context_state_t. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:117: if (pa_context_ready != 0) { if (pa_context_ready != PA_CONTEXT_READY) http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:118: stream_stopped_ = false; remove line http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:146: // Set volume nit: complete sentence http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:168: /* commented out? http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:170: if (playback_handle_) { Set playback_handle_, pa_context_, pa_mainloop_ to NULL after freeing each of them. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:227: // As long as the stream is active, we should be buffering packets if need Write comment outside of the while loop and reword. I think all you really need to mention is pa_mainloop_iterate blocks until PulseAudio is ready to write audio data, at which point WirteCallback is called (on the same thread in which PulseAudioOutputStream is created) http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:246: ClientBufferLoop(); inline function http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:48: uint32 RunDataCallback(uint8* dest, Can these parameters fit on one line?
drive-by, mostly style nits. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:46: switch(state) { nit: space after switch http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:47: default: nit: default is usually at the end of switch statement. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:50: *context_ready = 3; need break after that line. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:50: *context_ready = 3; nit: this should be indented 2 spaces relative to case. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:156: | PA_STREAM_AUTO_TIMING_UPDATE), nit: operator must be on the previous line
driveby nits http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:114: switches::kAlsaOutputDevice)) { nit: this should get indented by 4 more spaces (since it's a continuation of the HasSwitch( call http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:12: switch(bits_per_sample) { nit: space before ( http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:24: // Other cases: PA_SAMPLE_24_32LE (in LSBs of 32-bit field, little endian), align // http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:140: pa_buffer_attributes_.maxlength = (uint32_t)-1; static_cast<> instead of c-style cast http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:225: while(!stream_stopped_ && !source_exhausted_) { nit: space before (
For people in the audience: Talked to some of you offline and slock will update the CL such that Start() posts write tasks to the AudioThread's message loop (similar to the current Alsa implementation) to keep from blocking the thread.
The blocking-Start() fix Victoria mentioned will be the next patch, wanted to get all the comments on the last two patches in first. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:44: int* context_ready = static_cast<int*>(userdata); On 2011/08/10 16:34:51, Victoria Kirst wrote: > Change int* to pa_context_state_t*, then just do > > *context_ready = pa_context_get_state(context) (no need for switch statement > below) Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:98: // AudioManagerLinux::Release which deletes this object. On 2011/08/10 16:34:51, Victoria Kirst wrote: > DCHECK playback_handle_, pa_context_, pa_mainloop_ are all NULL Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:112: int pa_context_ready = 1; On 2011/08/10 16:34:51, Victoria Kirst wrote: > (See comment in ContextStateCallback) > > Change int to pa_context_state_t. Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:117: if (pa_context_ready != 0) { On 2011/08/10 16:34:51, Victoria Kirst wrote: > if (pa_context_ready != PA_CONTEXT_READY) Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:118: stream_stopped_ = false; On 2011/08/10 16:34:51, Victoria Kirst wrote: > remove line Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:146: // Set volume On 2011/08/10 16:34:51, Victoria Kirst wrote: > nit: complete sentence Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:168: /* On 2011/08/10 16:34:51, Victoria Kirst wrote: > commented out? Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:170: if (playback_handle_) { On 2011/08/10 16:34:51, Victoria Kirst wrote: > Set playback_handle_, pa_context_, pa_mainloop_ to NULL after freeing each of > them. Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:227: // As long as the stream is active, we should be buffering packets if need On 2011/08/10 16:34:51, Victoria Kirst wrote: > Write comment outside of the while loop and reword. I think all you really need > to mention is pa_mainloop_iterate blocks until PulseAudio is ready to write > audio data, at which point WirteCallback is called (on the same thread in which > PulseAudioOutputStream is created) Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:246: ClientBufferLoop(); On 2011/08/10 16:34:51, Victoria Kirst wrote: > inline function Done. http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/40001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:48: uint32 RunDataCallback(uint8* dest, On 2011/08/10 16:34:51, Victoria Kirst wrote: > Can these parameters fit on one line? Done. Well two of them can anyway. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:114: switches::kAlsaOutputDevice)) { On 2011/08/10 19:55:30, scherkus wrote: > nit: this should get indented by 4 more spaces (since it's a continuation of the > HasSwitch( call Done. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:12: switch(bits_per_sample) { On 2011/08/10 19:55:30, scherkus wrote: > nit: space before ( Done. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:24: // Other cases: PA_SAMPLE_24_32LE (in LSBs of 32-bit field, little endian), On 2011/08/10 19:55:30, scherkus wrote: > align // Done. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:46: switch(state) { On 2011/08/10 17:36:32, sergeyu wrote: > nit: space after switch This code no longer exists. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:47: default: On 2011/08/10 17:36:32, sergeyu wrote: > nit: default is usually at the end of switch statement. This code no longer exists. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:50: *context_ready = 3; On 2011/08/10 17:36:32, sergeyu wrote: > need break after that line. This code no longer exists. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:50: *context_ready = 3; On 2011/08/10 17:36:32, sergeyu wrote: > need break after that line. This code no longer exists. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:140: pa_buffer_attributes_.maxlength = (uint32_t)-1; On 2011/08/10 19:55:30, scherkus wrote: > static_cast<> instead of c-style cast Done. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:156: | PA_STREAM_AUTO_TIMING_UPDATE), On 2011/08/10 17:36:32, sergeyu wrote: > nit: operator must be on the previous line Done. http://codereview.chromium.org/7473021/diff/44001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:225: while(!stream_stopped_ && !source_exhausted_) { On 2011/08/10 19:55:30, scherkus wrote: > nit: space before ( Done.
Code I commented earlier LGTM, thank you.
This is a code review ping. I'd love to get this checked in soon (today?) because there are two other CLs for this project that will depend on this one. Thanks!
Sorry about that general ping, what I should have done is ask ask ajwong and vrk to PTAL now that the blocking problem is fixed as of a patch yesterday. This is only my second CL ever, my bad. Thanks!
deferring to vrk. On Thu, Aug 11, 2011 at 10:47 AM, <slock@chromium.org> wrote: > Sorry about that general ping, what I should have done is ask ask ajwong and > vrk > to PTAL now that the blocking problem is fixed as of a patch yesterday. > Â This is > only my second CL ever, my bad. > > Thanks! > > http://codereview.chromium.org/7473021/ >
Very close now... these comments are mostly nits but enough to make me withhold the OK this round! Probably one more iteration? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:43: void PulseAudioOutputStream::MainloopIterateTask() { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:43: void PulseAudioOutputStream::MainloopIterateTask() { Also nit: method name should be more descriptive. Perhaps WaitForWriteTask? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:50: // into |client_buffer_|. Comment with a "WARNING WARNING this blocks on Pulse! TODO fix this" http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:51: pa_write_has_calledback_ = false; nit: Change field name to "write_callback_handled_"? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:52: while (!pa_write_has_calledback_ && !stream_stopped_ && !source_exhausted_) { nit: no {} http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:58: void* userdata) { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:64: void* userdata) { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:78: stream_ptr->client_buffer_->Read(read_data.get(), length); OK, so there are two things here that aren't so ideal: (1) It looks like you're assuming you always have at least |length| amount of data in the buffer. (2) You're redundantly copying data out of your buffer into this read_data array, *and* pa_stream_write without a free_cb param will copy data into an internal buffer, so there are 2 extra copies going on here. To fix (1) is easy, just save the value of returned from Read() and use that value for the size from here on out. To fix (2) I think you can use GetCurrentChunk() instead of Read() on client_buffer_. You will then need to match this with a call to Seek() on client_buffer_ immediately after pa_stream_write(). (This only gets rid of 1 of 2 copies, but because of the way you're handling callbacks I think the PA internal copy is tricky to get rid of right now.) This will also get rid of the need to use a variable-length array! http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:99: stream_stopped_(false), Shouldn't this value be true? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:111: ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:112: // TODO(slock): Sanity check input values. DCHECK(manager_) http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:118: DCHECK(playback_handle_ == NULL); nit: instead of == NULL, !<field> here and the rest of these http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:124: bool PulseAudioOutputStream::Open() { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:130: pa_mainloop_api_ = pa_mainloop_get_api(pa_mainloop_); Actually, looks like pa_mainloop_api_ isn't used anywhere other than this function. Make it local instead of a field? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:142: stream_stopped_ = false; Why are you setting stream_stopped_ to false? If anything one would think you would set it to true... is there a reason why you need this? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:143: return false; When you return false here and below, you are likely leaking memory: the only way you wouldn't leak memory is if the client Close()es the stream even though the Open() function returned false. You'll need to delete the new'd stuff above before returning false... probably best to have a reset function now! In the reset function, you shouldn't include the call to manager_->ReleaseOutputStream(this). This means Close() should basically look like: void Close() { Reset(); manager_->ReleaseOutputStream(this); ) http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:148: pa_sample_specs_.format = sample_format_; Any reason why pa_sample_specs_ is a field instead of a local variable? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:165: pa_buffer_attributes_.maxlength = static_cast<uint32_t>(-1); Any reason why pa_buffer_attributes_ is a field instead of a local variable? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:188: void PulseAudioOutputStream::Close() { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:241: //volume_ = volume_ * 65536.0; //pa_sw_volume_from_linear(volume_); ? http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:244: ChannelLayoutToChannelCount(channel_layout_), Save ChannelLayoutToChannelCount(channel_layout_) result in a variable, then use that here and below instead of calling the function twice. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:248: Delete extra blank line. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:260: void PulseAudioOutputStream::Start(AudioSourceCallback* callback) { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:276: void PulseAudioOutputStream::Stop() { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:282: void PulseAudioOutputStream::SetVolume(double volume) { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:286: void PulseAudioOutputStream::GetVolume(double* volume) { DCHECK_EQ(message_loop_, MessageLoop::current()); http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:23: class PulseAudioOutputStream : public AudioOutputStream { Comment on what this class does. Things to include: - threading (basically, you assume all AudioOutputStream methods will be called on same thread that created this object) - BIG WARNING saying that this implementation blocks the audio thread to wait for PulseAudio callbacks. (It's just one Write callback instead of all callbacks, but still something that is worth warning about.) - Comment that you must Close() before delete http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:93: bool source_exhausted_; nit: comment this field and ones below (don't forget to mention ownership on pointers)
http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:43: void PulseAudioOutputStream::MainloopIterateTask() { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:43: void PulseAudioOutputStream::MainloopIterateTask() { On 2011/08/12 23:16:51, Victoria Kirst wrote: > Also nit: method name should be more descriptive. Perhaps WaitForWriteTask? Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:50: // into |client_buffer_|. On 2011/08/12 23:16:51, Victoria Kirst wrote: > Comment with a "WARNING WARNING this blocks on Pulse! TODO fix this" Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:51: pa_write_has_calledback_ = false; On 2011/08/12 23:16:51, Victoria Kirst wrote: > nit: Change field name to "write_callback_handled_"? Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:52: while (!pa_write_has_calledback_ && !stream_stopped_ && !source_exhausted_) { On 2011/08/12 23:16:51, Victoria Kirst wrote: > nit: no {} Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:58: void* userdata) { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Ignored per offline with vrk: can't check the message loop field here because this is a static method. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:64: void* userdata) { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:78: stream_ptr->client_buffer_->Read(read_data.get(), length); On 2011/08/12 23:16:51, Victoria Kirst wrote: > OK, so there are two things here that aren't so ideal: > (1) It looks like you're assuming you always have at least |length| amount of > data in the buffer. > (2) You're redundantly copying data out of your buffer into this read_data > array, *and* pa_stream_write without a free_cb param will copy data into an > internal buffer, so there are 2 extra copies going on here. > > To fix (1) is easy, just save the value of returned from Read() and use that > value for the size from here on out. > > To fix (2) I think you can use GetCurrentChunk() instead of Read() on > client_buffer_. You will then need to match this with a call to Seek() on > client_buffer_ immediately after pa_stream_write(). (This only gets rid of 1 of > 2 copies, but because of the way you're handling callbacks I think the PA > internal copy is tricky to get rid of right now.) > > This will also get rid of the need to use a variable-length array! Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:99: stream_stopped_(false), On 2011/08/12 23:16:51, Victoria Kirst wrote: > Shouldn't this value be true? Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:111: ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:112: // TODO(slock): Sanity check input values. On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK(manager_) Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:118: DCHECK(playback_handle_ == NULL); On 2011/08/12 23:16:51, Victoria Kirst wrote: > nit: instead of == NULL, !<field> here and the rest of these Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:124: bool PulseAudioOutputStream::Open() { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:130: pa_mainloop_api_ = pa_mainloop_get_api(pa_mainloop_); On 2011/08/12 23:16:51, Victoria Kirst wrote: > Actually, looks like pa_mainloop_api_ isn't used anywhere other than this > function. Make it local instead of a field? Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:142: stream_stopped_ = false; On 2011/08/12 23:16:51, Victoria Kirst wrote: > Why are you setting stream_stopped_ to false? If anything one would think you > would set it to true... is there a reason why you need this? Because I am dumb. That should definitely be false. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:143: return false; On 2011/08/12 23:16:51, Victoria Kirst wrote: > When you return false here and below, you are likely leaking memory: the only > way you wouldn't leak memory is if the client Close()es the stream even though > the Open() function returned false. > > You'll need to delete the new'd stuff above before returning false... probably > best to have a reset function now! > > In the reset function, you shouldn't include the call to > manager_->ReleaseOutputStream(this). This means Close() should basically look > like: > > void Close() { > Reset(); > manager_->ReleaseOutputStream(this); > ) Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:148: pa_sample_specs_.format = sample_format_; On 2011/08/12 23:16:51, Victoria Kirst wrote: > Any reason why pa_sample_specs_ is a field instead of a local variable? Done, not a field anymore. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:165: pa_buffer_attributes_.maxlength = static_cast<uint32_t>(-1); On 2011/08/12 23:16:51, Victoria Kirst wrote: > Any reason why pa_buffer_attributes_ is a field instead of a local variable? Done, not a field anymore. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:188: void PulseAudioOutputStream::Close() { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:241: //volume_ = volume_ * 65536.0; //pa_sw_volume_from_linear(volume_); On 2011/08/12 23:16:51, Victoria Kirst wrote: > ? Done. Deleted, was old code from an experiment. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:244: ChannelLayoutToChannelCount(channel_layout_), On 2011/08/12 23:16:51, Victoria Kirst wrote: > Save ChannelLayoutToChannelCount(channel_layout_) result in a variable, then use > that here and below instead of calling the function twice. Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:248: On 2011/08/12 23:16:51, Victoria Kirst wrote: > Delete extra blank line. Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:260: void PulseAudioOutputStream::Start(AudioSourceCallback* callback) { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:276: void PulseAudioOutputStream::Stop() { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:282: void PulseAudioOutputStream::SetVolume(double volume) { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:286: void PulseAudioOutputStream::GetVolume(double* volume) { On 2011/08/12 23:16:51, Victoria Kirst wrote: > DCHECK_EQ(message_loop_, MessageLoop::current()); Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:23: class PulseAudioOutputStream : public AudioOutputStream { On 2011/08/12 23:16:51, Victoria Kirst wrote: > Comment on what this class does. > > Things to include: > - threading (basically, you assume all AudioOutputStream methods will be called > on same thread that created this object) > - BIG WARNING saying that this implementation blocks the audio thread to wait > for PulseAudio callbacks. (It's just one Write callback instead of all > callbacks, but still something that is worth warning about.) > - Comment that you must Close() before delete Done. http://codereview.chromium.org/7473021/diff/53009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:93: bool source_exhausted_; On 2011/08/12 23:16:51, Victoria Kirst wrote: > nit: comment this field and ones below (don't forget to mention ownership on > pointers) Done.
Buffering is looking better! As in our offline conversation, next is to make sure the thread is not stalling/infinite looping when it doesn't have enough data to fulfill the write request. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:24: delete extra line http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:84: : channel_layout_(params.channel_layout), nit: Indentation: 4 spaces before ":" http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:104: delete line http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:113: DCHECK(!playback_handle_); nit: there is an extra space before each of these 3 lines http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:138: stream_stopped_ = true; Move "stream_stopped_ = true;" into Reset() as well and delete here? http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:180: stream_stopped_ = true; If you move stream_stopped_ = true; into Reset(), then delete here http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:206: // |pa_mainloop_api| is freed with |pa_mainloop_|. delete http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:228: // the audio sink. WriteCallback moves data from the |client_buffer_| to the nit: delete the comments from "WriteCallback moves..." up until the warning. (You'd really want to have those comments in WriteCallback, if you were to have them at all.) http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:242: nit: delete line http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:260: "Data source overran buffer."; nit: indentation http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:25: #include "base/message_loop.h" nit: includes in ABC order Well actually, just forward declare MessageLoop & include in cc http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:28: #include "media/audio/audio_parameters.h" Forward-declare AudioParameters instead of include http://codereview.chromium.org/7473021/diff/65001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/65001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:245: media::AdjustVolume(packet->GetWritableData(), I would think AdjustVolume should only be called if packet_size > 0? http://codereview.chromium.org/7473021/diff/65001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:260: while (client_buffer_->forward_bytes() < requested_bytes) Fix the stalling here if you don't have enough data. (This blocks forever if you never get enough data.)
This a major organizational change, but it should fix the blocking issue by using the MessageLoop much more, among other changes. I also did a lot of renaming and re-commenting to make things more clear. PTAL! http://codereview.chromium.org/7473021/diff/65001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/65001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:245: media::AdjustVolume(packet->GetWritableData(), On 2011/08/16 18:19:24, Victoria Kirst wrote: > I would think AdjustVolume should only be called if packet_size > 0? Done. http://codereview.chromium.org/7473021/diff/65001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:260: while (client_buffer_->forward_bytes() < requested_bytes) On 2011/08/16 18:19:24, Victoria Kirst wrote: > Fix the stalling here if you don't have enough data. (This blocks forever if you > never get enough data.) Done.
A few more comments... Also, it looks like you didn't address my comments from the last set; don't forget to address those when you take care of these. Thanks! http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:235: new media::DataBuffer(packet_size_); nit: indentation http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:241: if (packet_size > 0) { nit: a little nicer to say if (packet_size == 0) return false; then continue onward for the "good" case http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:258: if (!stream_stopped_) { nit: little cleaner to say if (stream_stopped_) return; then continue onward http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:259: if (client_buffer_->forward_bytes() >= requested_bytes) { Lines 259 - 282 can be simplified like follows: // Request more data from the source until we can fulfill the request or // fail to receive anymore data. bool buffering_successful = true; while (client_buffer_->forward_bytes() < requested_bytes && buffering_successful) buffering_successful = BufferPacketFromSource(); WriteToStream(requested_bytes); if (!buffering_successful) message_loop_->PostTask(...); http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:286: // This while loop will not stall because this function is only called when we " This while loop will not stall because this function is only called ..." Still dangerous! The goal is to write maintainable, easy-to-use and hard-to-misuse code. (You will not be the only reader/writer of this class, etc.) This function is very easy to misuse... Looks like GetCurrentChunk returns false when there is no data left in the buffer. Use this value to break out of the loop when you've exhausted your buffer.
PTYAL. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/audio_man... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/audio_man... media/audio/linux/audio_manager_linux.cc:24: On 2011/08/16 18:19:24, Victoria Kirst wrote: > delete extra line Done. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:84: : channel_layout_(params.channel_layout), On 2011/08/16 18:19:24, Victoria Kirst wrote: > nit: Indentation: 4 spaces before ":" Done. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:104: On 2011/08/16 18:19:24, Victoria Kirst wrote: > delete line Done. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:113: DCHECK(!playback_handle_); On 2011/08/16 18:19:24, Victoria Kirst wrote: > nit: there is an extra space before each of these 3 lines Done. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:138: stream_stopped_ = true; On 2011/08/16 18:19:24, Victoria Kirst wrote: > Move "stream_stopped_ = true;" into Reset() as well and delete here? Done. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:180: stream_stopped_ = true; On 2011/08/16 18:19:24, Victoria Kirst wrote: > If you move stream_stopped_ = true; into Reset(), then delete here Done. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:206: // |pa_mainloop_api| is freed with |pa_mainloop_|. On 2011/08/16 18:19:24, Victoria Kirst wrote: > delete Done. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:228: // the audio sink. WriteCallback moves data from the |client_buffer_| to the On 2011/08/16 18:19:24, Victoria Kirst wrote: > nit: delete the comments from "WriteCallback moves..." up until the warning. > > (You'd really want to have those comments in WriteCallback, if you were to have > them at all.) These comments are vastly different currently. PTAL? http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:242: On 2011/08/16 18:19:24, Victoria Kirst wrote: > nit: delete line This code isn't there anymore. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:260: "Data source overran buffer."; On 2011/08/16 18:19:24, Victoria Kirst wrote: > nit: indentation This code isn't there anymore. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:25: #include "base/message_loop.h" On 2011/08/16 18:19:24, Victoria Kirst wrote: > nit: includes in ABC order > > Well actually, just forward declare MessageLoop & include in cc Done. http://codereview.chromium.org/7473021/diff/57002/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:28: #include "media/audio/audio_parameters.h" On 2011/08/16 18:19:24, Victoria Kirst wrote: > Forward-declare AudioParameters instead of include Done. http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:235: new media::DataBuffer(packet_size_); On 2011/08/17 18:59:23, Victoria Kirst wrote: > nit: indentation Done. http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:241: if (packet_size > 0) { On 2011/08/17 18:59:23, Victoria Kirst wrote: > nit: a little nicer to say > if (packet_size == 0) > return false; > > then continue onward for the "good" case Done. http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:258: if (!stream_stopped_) { On 2011/08/17 18:59:23, Victoria Kirst wrote: > nit: little cleaner to say > if (stream_stopped_) > return; > > then continue onward Done. http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:259: if (client_buffer_->forward_bytes() >= requested_bytes) { On 2011/08/17 18:59:23, Victoria Kirst wrote: > Lines 259 - 282 can be simplified like follows: > > // Request more data from the source until we can fulfill the request or > // fail to receive anymore data. > bool buffering_successful = true; > while (client_buffer_->forward_bytes() < requested_bytes && > buffering_successful) > buffering_successful = BufferPacketFromSource(); > > WriteToStream(requested_bytes); > > if (!buffering_successful) > message_loop_->PostTask(...); I modified this slightly because it subtly implied a change of the behavior of WriteToStream(). You are correct in implying that users (and this code) needs to be able to call WriteToStream()num_bytes) without being under the guarantee that num_bytes can and will be written. But, if we are going to call WriteToStream(requested_bytes) without knowing that we will actually write exactly that many bytes, we need to know how many bytes it did write in order to correctly post another request for the remaining bytes. So, I made WriteToStream return how many bytes out of bytes_to_write it actually managed to write. I also only call WriteToStream if there are any bytes in the buffer to write. http://codereview.chromium.org/7473021/diff/71001/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:286: // This while loop will not stall because this function is only called when we On 2011/08/17 18:59:23, Victoria Kirst wrote: > " This while loop will not stall because this function is only called ..." > > Still dangerous! The goal is to write maintainable, easy-to-use and > hard-to-misuse code. (You will not be the only reader/writer of this class, > etc.) This function is very easy to misuse... > > Looks like GetCurrentChunk returns false when there is no data left in the > buffer. Use this value to break out of the loop when you've exhausted your > buffer. Done.
just about all nits this round! http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:9: #include "media/audio/audio_parameters.h" nit: abc order of includes http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:64: stream->message_loop_->PostTask( I believe this should be in the "else" block for if (bytes_written < requested_bytes) in FulfillWriteRequest. (If you haven't fulfilled the outstanding request, post task to get more data. Else, post task to process more PA callbacks.) http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:108: // TODO(slock): Possibly move most of this to a OpenPlaybackDevice function in nit: "to an" http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:144: this); nit: move this to line above? http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:271: // Try to fulfill the request by writing as many of the requested bytes to nit: Put the comment above the if statement, or put curly braces around the if statement (must have {} around multi-line blocks) http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:275: if (bytes_written < requested_bytes) nit: curly braces around multi-line blocks http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:301: buffer_exhausted = client_buffer_->GetCurrentChunk(&chunk, &chunk_size); logic is incorrect: should be buffer_exhausted = !client_buffer_->GetCurrentChunk(&chunk, &chunk_size); In fact, instead of using the buffer_exhausted variable, perhaps a cleaner way to write this loop would be: while (*bytes_written < bytes_to_write) { const uint8* chunk; size_t chunk_size; // Stop writing if there is no more data buffered. if (!client_buffer_->GetCurrentChunk(&chunk, &chunk_size)) break; ... } http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:14: // WARNING: This object blocks on internal PulseAudio calls after Start() is Also blocks on Open(), waiting for PulseAudio to be ready or to fail.
Iterate! Iterate! Iterate! http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:9: #include "media/audio/audio_parameters.h" On 2011/08/18 18:22:22, Victoria Kirst wrote: > nit: abc order of includes Done. http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:64: stream->message_loop_->PostTask( On 2011/08/18 18:22:22, Victoria Kirst wrote: > I believe this should be in the "else" block for if (bytes_written < > requested_bytes) in FulfillWriteRequest. > > (If you haven't fulfilled the outstanding request, post task to get more data. > Else, post task to process more PA callbacks.) Done. http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:108: // TODO(slock): Possibly move most of this to a OpenPlaybackDevice function in On 2011/08/18 18:22:22, Victoria Kirst wrote: > nit: "to an" Done. http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:144: this); On 2011/08/18 18:22:22, Victoria Kirst wrote: > nit: move this to line above? Done. http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:271: // Try to fulfill the request by writing as many of the requested bytes to On 2011/08/18 18:22:22, Victoria Kirst wrote: > nit: Put the comment above the if statement, or put curly braces around the if > statement (must have {} around multi-line blocks) Done. I didn't if 'multi-line' included comments and couldn't find it in the style-guide. http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:275: if (bytes_written < requested_bytes) On 2011/08/18 18:22:22, Victoria Kirst wrote: > nit: curly braces around multi-line blocks Done. http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:301: buffer_exhausted = client_buffer_->GetCurrentChunk(&chunk, &chunk_size); On 2011/08/18 18:22:22, Victoria Kirst wrote: > logic is incorrect: should be > buffer_exhausted = !client_buffer_->GetCurrentChunk(&chunk, &chunk_size); > > > In fact, instead of using the buffer_exhausted variable, perhaps a cleaner way > to write this loop would be: > > while (*bytes_written < bytes_to_write) { > const uint8* chunk; > size_t chunk_size; > // Stop writing if there is no more data buffered. > if (!client_buffer_->GetCurrentChunk(&chunk, &chunk_size)) > break; > ... > } Done. http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/7473021/diff/71003/media/audio/linux/pulse_out... media/audio/linux/pulse_output.h:14: // WARNING: This object blocks on internal PulseAudio calls after Start() is On 2011/08/18 18:22:22, Victoria Kirst wrote: > Also blocks on Open(), waiting for PulseAudio to be ready or to fail. Done.
LGTM!!! with one nit :) http://codereview.chromium.org/7473021/diff/75009/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/75009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:209: if (!write_callback_handled_ && !stream_stopped_) { Instead of checking && !stream_stopped_ here (stream will never be stopped during the handling of the write request), check for stopped stream at beginning: if (stream_stopped_) return; Also update comment to reflect this.
Committing now, thanks everyone! http://codereview.chromium.org/7473021/diff/75009/media/audio/linux/pulse_out... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/7473021/diff/75009/media/audio/linux/pulse_out... media/audio/linux/pulse_output.cc:209: if (!write_callback_handled_ && !stream_stopped_) { On 2011/08/18 20:00:48, Victoria Kirst wrote: > Instead of checking && !stream_stopped_ here (stream will never be stopped > during the handling of the write request), check for stopped stream at > beginning: > > if (stream_stopped_) > return; > > Also update comment to reflect this. Done.
This broke a unittest, lets try again.
Fix LGTM; try again!
Commit queue had an internal error. Something went really wrong, probably a crash, a hickup or simply the monkeys went out for diner. The relevant dude was pinged already. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
