Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1711823004:
Let default device in PulseAudio be the system default device (Closed)
DescriptionUse the the system default device instead of the application default when opening the default device in PulseAudio.
BUG=343839
NOTRY=true
Committed: https://crrev.com/254c78a58a9da3a320c5ce8b1d84c0843fe3b8bc
Cr-Commit-Position: refs/heads/master@{#383945}
Patch Set 1 #Patch Set 2 : remove space #
Total comments: 6
Patch Set 3 : fix issues #
Total comments: 5
Patch Set 4 : move to pulseaudioinput/outputstream #Patch Set 5 : remove headers #Patch Set 6 : #
Total comments: 52
Patch Set 7 : rebase #Patch Set 8 : fix #Patch Set 9 : fix #
Total comments: 30
Patch Set 10 : latest #
Total comments: 32
Patch Set 11 : latest #
Total comments: 8
Patch Set 12 : latest #
Total comments: 4
Patch Set 13 : last #Patch Set 14 : rebase #
Total comments: 2
Patch Set 15 : last #
Total comments: 6
Patch Set 16 : #Patch Set 17 : #
Total comments: 8
Patch Set 18 : #Patch Set 19 : #
Total comments: 2
Patch Set 20 : #
Messages
Total messages: 70 (13 generated)
Description was changed from ========== Let default device in PulseAudio be the system default device Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 ========== to ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 ==========
Description was changed from ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 ========== to ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 ==========
rchtara@chromium.org changed reviewers: + grunell@chromium.org
Hello, Could you please have a look to this cl? Thanks Riadh
Hello, Could you please have a look to this cl? Thanks Riadh
rchtara@chromium.org changed reviewers: + tommi@chromium.org
Hello tommi, I'm a new returning intern and grunell@chromium.org is my host. Nice to meet. Could you please have a look to this cl? Thanks a lot Riadh
How does this work wrt to the device selected by the user in the permission UI as being the default?
On 2016/02/22 17:26:43, tommi-chromium wrote: > How does this work wrt to the device selected by the user in the permission UI > as being the default? Riadh, I've been ooo today and can take a look tomorrow.
On 2016/02/22 17:26:43, tommi-chromium wrote: > How does this work wrt to the device selected by the user in the permission UI > as being the default? Hi, Sorry, I'm a newbie, so I'm not exactly sure about what you mean by your questions. What this patch is doing is just replacing the NULL dev argument of pa_stream_connect_record and _stream_connect_playback with the system default device id. However, I think that when a device is selected by the user in the permission UI as being the default: in this case, device_id is different than AudioManagerBase::kDefaultDeviceId so we are going to use device_id for output/input. Thanks a lot Riadh
On 2016/02/22 17:41:04, Henrik Grunell wrote: > On 2016/02/22 17:26:43, tommi-chromium wrote: > > How does this work wrt to the device selected by the user in the permission UI > > as being the default? > > Riadh, I've been ooo today and can take a look tomorrow. Hello, Sorry, for annoying you. And thanks in advance for the review. Riadh
https://codereview.chromium.org/1711823004/diff/20001/media/audio/pulse/pulse... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:167: void SystemDefaultDeviceCallback(pa_context* context, Put these functions in the anonymous namespace above since they're only used in this file. https://codereview.chromium.org/1711823004/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:167: void SystemDefaultDeviceCallback(pa_context* context, Add comments with brief description for these two functions. https://codereview.chromium.org/1711823004/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:171: system_default_input_device = info->default_source_name; Store the system default device names in the stream objects instead. See e.g. https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/pulse/... Seems like that callback doesn't signal, but others do. Can you look into why and if needed here?
On 2016/02/22 17:26:43, tommi-chromium wrote: > How does this work wrt to the device selected by the user in the permission UI > as being the default? Hmm, there is no selection of device in the UI, right? We have either permission or not. And for output, we always allow playing to the default device. For both input and output the system default or "application default" (i.e. as chosen by the PA server, typically based on user settings for streams within that application) can be considered a default device.
Could you please have a second look to the cl? Thanks a lot https://codereview.chromium.org/1711823004/diff/20001/media/audio/pulse/pulse... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:167: void SystemDefaultDeviceCallback(pa_context* context, On 2016/02/23 11:13:24, Henrik Grunell wrote: > Add comments with brief description for these two functions. Done. https://codereview.chromium.org/1711823004/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:167: void SystemDefaultDeviceCallback(pa_context* context, I changed the cl architecture to use stream objects for storing device names. I cannot move now the new functions I introduced to an anonymous name space because the new functions need to be friends with the PulseAudioInputStream so we can access the private attributes of PulseAudioInputStream from the new functions. https://codereview.chromium.org/1711823004/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:171: system_default_input_device = info->default_source_name; On 2016/02/23 11:13:24, Henrik Grunell wrote: > Store the system default device names in the stream objects instead. See e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/pulse/... Done > Seems like that callback doesn't signal, but others do. Can you look into why > and if needed here? In the callback, pa_threaded_mainloop_signal is called. so the callback is signaling.
https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:171: stream->device_name_ = info->default_source_name; This will overwrite the device name provided in the stream ctor, which will break how things should work. If the default device should be used, |stream->device_name_| will be "default". When overwritten that device will be used going forward, even if the default changes. Looking closer at this, instead move getting the system default device name into PulseAudioInputStream::Open(). There first check if |device_name_| == AudioManagerBase::kDefaultDeviceId. If yes get the system default device name and use that when calling CreateInputStream(). If no, use |device_name_|. These two functions go there as ordinary functions in the cc file. Same for output. https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:250: *stream, device_id == AudioManagerBase::kDefaultDeviceId Then here, always use |device_id|. And below for input too.
On 2016/02/24 15:54:53, Henrik Grunell wrote: > https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... > File media/audio/pulse/pulse_util.cc (right): > > https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... > media/audio/pulse/pulse_util.cc:171: stream->device_name_ = > info->default_source_name; > This will overwrite the device name provided in the stream ctor, which will > break how things should work. If the default device should be used, > |stream->device_name_| will be "default". When overwritten that device will be > used going forward, even if the default changes. > > Looking closer at this, instead move getting the system default device name into > PulseAudioInputStream::Open(). There first check if |device_name_| == > AudioManagerBase::kDefaultDeviceId. If yes get the system default device name > and use that when calling CreateInputStream(). If no, use |device_name_|. These > two functions go there as ordinary functions in the cc file. > > Same for output. For void PulseAudioOutputStream::Open(), pa_mainloop_ and pa_context_ are not created there yet.(they are created in CreateOutputStream) and they are required for SystemDefaultOutputDevice. Do you recommend that i move the pa_mainloop_ and pa_context_ initialization to PulseAudioOutputStream::Open()
Could you please have a look to the new patch? Thanks Riadh https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:171: stream->device_name_ = info->default_source_name; On 2016/02/24 15:54:53, Henrik Grunell wrote: > This will overwrite the device name provided in the stream ctor, which will > break how things should work. If the default device should be used, > |stream->device_name_| will be "default". When overwritten that device will be > used going forward, even if the default changes. > > Looking closer at this, instead move getting the system default device name into > PulseAudioInputStream::Open(). There first check if |device_name_| == > AudioManagerBase::kDefaultDeviceId. If yes get the system default device name > and use that when calling CreateInputStream(). If no, use |device_name_|. These > two functions go there as ordinary functions in the cc file. > > Same for output. Done. https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:250: *stream, device_id == AudioManagerBase::kDefaultDeviceId On 2016/02/24 15:54:52, Henrik Grunell wrote: > Then here, always use |device_id|. > > And below for input too. Done. https://codereview.chromium.org/1711823004/diff/40001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:250: *stream, device_id == AudioManagerBase::kDefaultDeviceId On 2016/02/24 15:54:52, Henrik Grunell wrote: > Then here, always use |device_id|. > > And below for input too. Done.
Sorry for the delay. Move creating mainloop and context to the output class is good. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:52: void PulseAudioInputStream::UseSystemDefaultInputDeviceCallback( Since this function is only used in this cc file, remove from class and make it an ordinary function under anonymous namespace. Same for output. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:58: stream->device_name_ = info->default_source_name; Same here as for output (see that comment), add a new member variable and don't change |devcie_name_|. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:78: UseSystemDefaultInputDevice(); Should this function be called under the lock? https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:18: #include "media/audio/pulse/pulse_util.h" Is this needed? https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:74: std::string device_name_; Change this to const. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:96: DCHECK(!pa_mainloop_); Put the moved code in a new function, InitializeMainloopAndContext(). Add thread check. I wonder why the functions CreateOutput/InputStream() are in pulse_util.cc at all? It's only used by this and the corresponding input function and could be member functions. I don't think you need to move them now though. Another interesting note is that in the audio manager we create the input mainloop and context and hand it to the input class. In the manager, it's also used for device enumeration, both input and output. Same here, no need to change that. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:114: AutoPulseLock auto_lock(pa_mainloop_); Scope the code that should execute under this lock (until after the while loop I believe), i.e. with { }. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:137: DCHECK(thread_checker_.CalledOnValidThread()); Put thread checker first in function. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:75: void UseSystemDefaultOutputDevice(); Rename to GetSystem...(). https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:88: std::string device_id_; You need to keep const std::string device_id_; and add a new std::string system_default_device_id_; since we need to know if the default was chosen, (|device_id_| == "default"). We can't overwrite that. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:164: bool CreateInputStream(pa_threaded_mainloop* mainloop, Add a todo comment in the header file for this and the output function that they should be moved to the input and output classes. (Since they're only used there.) https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:213: RETURN_ON_FAILURE( Add DCHECK that device_id != AudioManagerBase::kDefaultDeviceId. Ditto for output. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:231: bool CreateOutputStream(pa_threaded_mainloop** mainloop, Change to take pa_threaded_mainloop* and pa_context*. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:243: // Set sample specifications. Do we need to grab the lock here? The old code held on to it throughout the function, that may or may not have been necessary. The old comment only says "Lock the main loop while setting up the context." Can you figure out if the lock is need here?
CC Olga FYI.
Another thing, make sure that you test thoroughly manually. There's no unit tests unfortunately for this code.
On 2016/03/07 11:49:17, Henrik Grunell wrote: > Another thing, make sure that you test thoroughly manually. There's no unit > tests unfortunately for this code. Thanks a lot for the review. No worries, I didn't have time yesterday. Unfortunately, I still wasn't able to figure about for the locks if there are needed or not. For more details, have a look please to my replies.
https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:52: void PulseAudioInputStream::UseSystemDefaultInputDeviceCallback( On 2016/03/07 11:37:17, Henrik Grunell wrote: > Since this function is only used in this cc file, remove from class and make it > an ordinary function under anonymous namespace. Same for output. I tried to do that by making it friend with PulseAudioInputStream but unfortunately it's not possible : the function is declared in the anonymous namespace of pulse_input.cc, so can't be used from pulse_input.cc, As the function is in the anonymous NS in the pulse_input.cc, it's not possible to refer to it in pulse_input.h. There many ways to solve this issue: we can for example make it a part of media NS. However this is going to work but make it accessible for anyone, which is worst than the current situation as it's currently private and accessible from PulseAudioInputStream. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:58: stream->device_name_ = info->default_source_name; On 2016/03/07 11:37:17, Henrik Grunell wrote: > Same here as for output (see that comment), add a new member variable and don't > change |devcie_name_|. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:78: UseSystemDefaultInputDevice(); On 2016/03/07 11:37:17, Henrik Grunell wrote: > Should this function be called under the lock? I'm not sure, it works even the lock is completely removed. But I'm dont feel comfortable about removing it as it's used in start,stop... https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:18: #include "media/audio/pulse/pulse_util.h" On 2016/03/07 11:37:17, Henrik Grunell wrote: > Is this needed? Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:74: std::string device_name_; On 2016/03/07 11:37:17, Henrik Grunell wrote: > Change this to const. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:96: DCHECK(!pa_mainloop_); On 2016/03/07 11:37:17, Henrik Grunell wrote: > Put the moved code in a new function, InitializeMainloopAndContext(). Add thread > check. > > I wonder why the functions CreateOutput/InputStream() are in pulse_util.cc at > all? It's only used by this and the corresponding input function and could be > member functions. I don't think you need to move them now though. > > Another interesting note is that in the audio manager we create the input > mainloop and context and hand it to the input class. In the manager, it's also > used for device enumeration, both input and output. Same here, no need to change > that. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:114: AutoPulseLock auto_lock(pa_mainloop_); On 2016/03/07 11:37:17, Henrik Grunell wrote: > Scope the code that should execute under this lock (until after the while loop I > believe), i.e. with { }. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:137: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/03/07 11:37:17, Henrik Grunell wrote: > Put thread checker first in function. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:75: void UseSystemDefaultOutputDevice(); On 2016/03/07 11:37:17, Henrik Grunell wrote: > Rename to GetSystem...(). Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:88: std::string device_id_; On 2016/03/07 11:37:17, Henrik Grunell wrote: > You need to keep > const std::string device_id_; > and add a new > std::string system_default_device_id_; > since we need to know if the default was chosen, (|device_id_| == "default"). We > can't overwrite that. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:164: bool CreateInputStream(pa_threaded_mainloop* mainloop, On 2016/03/07 11:37:18, Henrik Grunell wrote: > Add a todo comment in the header file for this and the output function that they > should be moved to the input and output classes. (Since they're only used > there.) Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:213: RETURN_ON_FAILURE( On 2016/03/07 11:37:17, Henrik Grunell wrote: > Add DCHECK that device_id != AudioManagerBase::kDefaultDeviceId. Ditto for > output. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:231: bool CreateOutputStream(pa_threaded_mainloop** mainloop, On 2016/03/07 11:37:17, Henrik Grunell wrote: > Change to take pa_threaded_mainloop* and pa_context*. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:243: // Set sample specifications. No sure about this too. The last version is working. And the lock is held only in InitializeMainloopAndContext
Some comments on input code applies to output as well. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:52: void PulseAudioInputStream::UseSystemDefaultInputDeviceCallback( On 2016/03/08 17:28:16, rchtara wrote: > On 2016/03/07 11:37:17, Henrik Grunell wrote: > > Since this function is only used in this cc file, remove from class and make > it > > an ordinary function under anonymous namespace. Same for output. > > I tried to do that by making it friend with PulseAudioInputStream but > unfortunately it's not possible : the function is declared in the anonymous > namespace of pulse_input.cc, so can't be used from pulse_input.cc, > As the function is in the anonymous NS in the pulse_input.cc, it's not possible > to refer to it in pulse_input.h. > There many ways to solve this issue: we can for example make it a part of media > NS. However this is going to work but make it accessible for anyone, which is > worst than the current situation as it's currently private and accessible from > PulseAudioInputStream. I don't understand what the problem is. Is it accessing the member variables? See next comment. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:243: // Set sample specifications. On 2016/03/08 17:28:17, rchtara wrote: > No sure about this too. > The last version is working. And the lock is held only in > InitializeMainloopAndContext Is it guaranteed that we can never get any concurrency issues if the lock isn't held here? I'm sure the code works, but that's no guarantee. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:58: stream->default_system_device_name_ = info->default_source_name; Add setter function for |default_system_device_name_| and getter for |pa_mainloop_|. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:60: static_cast<pa_threaded_mainloop*>(stream->pa_mainloop_); It doesn't look like casting is needed here. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:76: std::string device_to_use_ = device_name_; "device_to_use_" -> "device_name_to_use". (No trailing "_".) https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:56: // GetSystemDefaultInputDevice to set |device_name_| to the |device_name_| -> |system_device_name_| https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:62: // Get default system input device for the input stream Nit: Add "." in end. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:74: // The name of the system default device. Returned Change "Returned" -> "Set". Also add that it's set if |device_name_| is set to be default device. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:75: // by GetSystemDefaultOutputDeviceCallback. Nit: "by" fits on first line. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:77: AudioParameters params_; Empty line before params. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:101: app_name = AudioManager::GetGlobalAppName(); Oh, I see, the parameters are outputs. This function should only initialize mainloop and context, i.e. include code up until right after the while loop. So remove the parameters. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:139: device_to_use_ = device_id_; Move this code to Open(). https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:74: bool InitializeMainloopAndContext(std::string& app_name, Add comment with description. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:74: bool InitializeMainloopAndContext(std::string& app_name, const std::string& (both arguments). https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:75: std::string& device_to_use_); Remove trailing "_". https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:173: DCHECK(device_id != AudioManagerBase::kDefaultDeviceId); I'm not sure id DCHECK_NE() works for std::string. Can you try if it compiles? https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.h (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_util.h:55: // TODO: Move this function to PulseAudioInputStream class since it's only "TODO:" -> "TODO(grunell):". Ditto below.
Could you please have a look have a look to this patch? Thanks a lot Riadh https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:52: void PulseAudioInputStream::UseSystemDefaultInputDeviceCallback( If we put the function UseSystemDefaultInputDeviceCallback in the anonymous namespace of pulse_input.cc, it's not visible outside this file. so we cannot access it from the header file pulse_input.h and so we cannot make it friends with PulseAudioInputStream. So, in this case,it's not possible to make them friends. As solution we can make some setters or getters for anything needed in UseSystemDefaultInputDeviceCallback or we can make the function private static member of PulseAudioInputStream... https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:243: // Set sample specifications. So let's keep the lock. it safer https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:58: stream->default_system_device_name_ = info->default_source_name; On 2016/03/09 00:49:17, Henrik Grunell wrote: > Add setter function for |default_system_device_name_| and getter for > |pa_mainloop_|. Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:60: static_cast<pa_threaded_mainloop*>(stream->pa_mainloop_); On 2016/03/09 00:49:17, Henrik Grunell wrote: > It doesn't look like casting is needed here. Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:76: std::string device_to_use_ = device_name_; On 2016/03/09 00:49:17, Henrik Grunell wrote: > "device_to_use_" -> "device_name_to_use". (No trailing "_".) Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:56: // GetSystemDefaultInputDevice to set |device_name_| to the On 2016/03/09 00:49:17, Henrik Grunell wrote: > |device_name_| -> |system_device_name_| Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:62: // Get default system input device for the input stream On 2016/03/09 00:49:17, Henrik Grunell wrote: > Nit: Add "." in end. Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:74: // The name of the system default device. Returned On 2016/03/09 00:49:17, Henrik Grunell wrote: > Change "Returned" -> "Set". Also add that it's set if |device_name_| is set to > be default device. Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:75: // by GetSystemDefaultOutputDeviceCallback. On 2016/03/09 00:49:17, Henrik Grunell wrote: > Nit: "by" fits on first line. Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:77: AudioParameters params_; On 2016/03/09 00:49:17, Henrik Grunell wrote: > Empty line before params. Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:101: app_name = AudioManager::GetGlobalAppName(); On 2016/03/09 00:49:17, Henrik Grunell wrote: > Oh, I see, the parameters are outputs. This function should only initialize > mainloop and context, i.e. include code up until right after the while loop. So > remove the parameters. Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:139: device_to_use_ = device_id_; On 2016/03/09 00:49:17, Henrik Grunell wrote: > Move this code to Open(). Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:74: bool InitializeMainloopAndContext(std::string& app_name, On 2016/03/09 00:49:17, Henrik Grunell wrote: > const std::string& (both arguments). Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:74: bool InitializeMainloopAndContext(std::string& app_name, On 2016/03/09 00:49:17, Henrik Grunell wrote: > Add comment with description. Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:75: std::string& device_to_use_); On 2016/03/09 00:49:17, Henrik Grunell wrote: > Remove trailing "_". Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:173: DCHECK(device_id != AudioManagerBase::kDefaultDeviceId); On 2016/03/09 00:49:17, Henrik Grunell wrote: > I'm not sure id DCHECK_NE() works for std::string. Can you try if it compiles? Done. https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.h (right): https://codereview.chromium.org/1711823004/diff/160001/media/audio/pulse/puls... media/audio/pulse/pulse_util.h:55: // TODO: Move this function to PulseAudioInputStream class since it's only On 2016/03/09 00:49:17, Henrik Grunell wrote: > "TODO:" -> "TODO(grunell):". Ditto below. Done.
https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:13: namespace { Put the anonymous namespace inside the media namespace. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:27: } } } // namespace https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:237: default_system_device_name_ = name; Either add a thread check or lock. Seems like we can't use a thread checker, right? I think the callback that in turn calls this function is on another thread than Open() is called on, is that correct? https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:43: pa_threaded_mainloop* GetPAMainloop(); Empty line before since it's not part of AudioInputStream implementation. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:26: pa_threaded_mainloop_signal(pa_mainloop, 0); You could just use stream->GetPAMainloop() directly here, and remove the above line. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:101: std::string app_name = AudioManager::GetGlobalAppName(); Move this line to just before where it's used (pa_context_ = ...). https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:149: device_name_to_use = device_id_; std::string device_name_to_use = device_id_; Then remove line above. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:151: GetSystemDefaultOutputDevice(); I wonder if we should have the lock when calling this. On the input side we do. I'm not sure about this. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:157: app_name, &StreamNotifyCallback, &StreamRequestCallback, this); Use AudioManager::GetGlobalAppName() here, remove first line in function. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:326: pa_threaded_mainloop* PulseAudioOutputStream::GetPAMainloop() { Put the function definitions in the same order as the declaration order in the header. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:57: pa_threaded_mainloop* GetPAMainloop(); Empty line before. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:68: // Initialize |pa_mainloop_| and |pa_context_| and prepare them for creating Empty line before the comment. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:244: AutoPulseLock auto_lock(mainloop); For input we already have the lock when calling CreateInputStream(). We should do the same way for both: either the lock should be grabbed before or not.
Could you have a look please to the patch again? Thanks a lot Riadh https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:13: namespace { On 2016/03/10 00:57:07, Henrik Grunell wrote: > Put the anonymous namespace inside the media namespace. Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:27: } On 2016/03/10 00:57:07, Henrik Grunell wrote: > } > > } // namespace Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:237: default_system_device_name_ = name; DCHECK(stream->thread_checker_.CalledOnValidThread()) on the callback thread causes a crash so they 're different threads. Using a lock doesn't work either as we are not in the right thread: Assertion '!m->thread || !pa_thread_is_running(m->thread) || !in_worker(m)' failed at pulse/thread-mainloop.c:169, function pa_threaded_mainloop_lock(). Aborting https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.h:43: pa_threaded_mainloop* GetPAMainloop(); On 2016/03/10 00:57:07, Henrik Grunell wrote: > Empty line before since it's not part of AudioInputStream implementation. Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:26: pa_threaded_mainloop_signal(pa_mainloop, 0); On 2016/03/10 00:57:07, Henrik Grunell wrote: > You could just use stream->GetPAMainloop() directly here, and remove the above > line. Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:101: std::string app_name = AudioManager::GetGlobalAppName(); On 2016/03/10 00:57:07, Henrik Grunell wrote: > Move this line to just before where it's used (pa_context_ = ...). Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:149: device_name_to_use = device_id_; On 2016/03/10 00:57:07, Henrik Grunell wrote: > std::string device_name_to_use = device_id_; > > Then remove line above. Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:151: GetSystemDefaultOutputDevice(); I tied to use a lock before calling it but the browser crashes. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:157: app_name, &StreamNotifyCallback, &StreamRequestCallback, this); On 2016/03/10 00:57:07, Henrik Grunell wrote: > Use AudioManager::GetGlobalAppName() here, remove first line in function. Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:326: pa_threaded_mainloop* PulseAudioOutputStream::GetPAMainloop() { They're not ordered as in the header. But I'm doing my best. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:57: pa_threaded_mainloop* GetPAMainloop(); On 2016/03/10 00:57:07, Henrik Grunell wrote: > Empty line before. Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.h:68: // Initialize |pa_mainloop_| and |pa_context_| and prepare them for creating On 2016/03/10 00:57:07, Henrik Grunell wrote: > Empty line before the comment. Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:244: AutoPulseLock auto_lock(mainloop); On 2016/03/10 00:57:07, Henrik Grunell wrote: > For input we already have the lock when calling CreateInputStream(). We should > do the same way for both: either the lock should be grabbed before or not. Done.
Thanks for addressing all comments patiently. Getting closer. :) https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:237: default_system_device_name_ = name; On 2016/03/10 13:20:34, rchtara wrote: > DCHECK(stream->thread_checker_.CalledOnValidThread()) on the callback thread > causes a crash so they 're different threads. > Using a lock doesn't work either as we are not in the right thread: > Assertion '!m->thread || !pa_thread_is_running(m->thread) || !in_worker(m)' > failed at pulse/thread-mainloop.c:169, function pa_threaded_mainloop_lock(). > Aborting Seems you were using the pulse auto lock. Instead add a new ordinary lock (base::Lock) as a member, then use base::AutoLock to acquire and auto release. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:151: GetSystemDefaultOutputDevice(); On 2016/03/10 13:20:35, rchtara wrote: > I tied to use a lock before calling it but the browser crashes. OK, what was the crash? And why doesn't it crash on the input side where we have the lock when getting the default device? https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:326: pa_threaded_mainloop* PulseAudioOutputStream::GetPAMainloop() { On 2016/03/10 13:20:35, rchtara wrote: > They're not ordered as in the header. > But I'm doing my best. Ah, OK. Well, best effort is fine then. https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:224: return pa_mainloop_; Could this function be called at the same time as setting the variable in Open()? I don't think so, but can you verify that not possible? https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:152: if (!InitializeMainloopAndContext()) { InitializeMainloopAndContext() should only be called once as it is written now.
Thanks a lot for the review. could you please have a last look to it? Thanks https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:237: default_system_device_name_ = name; On 2016/03/10 20:07:34, Henrik Grunell wrote: > On 2016/03/10 13:20:34, rchtara wrote: > > DCHECK(stream->thread_checker_.CalledOnValidThread()) on the callback thread > > causes a crash so they 're different threads. > > Using a lock doesn't work either as we are not in the right thread: > > Assertion '!m->thread || !pa_thread_is_running(m->thread) || !in_worker(m)' > > failed at pulse/thread-mainloop.c:169, function pa_threaded_mainloop_lock(). > > Aborting > > Seems you were using the pulse auto lock. Instead add a new ordinary lock > (base::Lock) as a member, then use base::AutoLock to acquire and auto release. Done. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:151: GetSystemDefaultOutputDevice(); I rechecked it, and now it's working : the crash was due to another issue that was fixed. https://codereview.chromium.org/1711823004/diff/180001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:326: pa_threaded_mainloop* PulseAudioOutputStream::GetPAMainloop() { On 2016/03/10 20:07:34, Henrik Grunell wrote: > On 2016/03/10 13:20:35, rchtara wrote: > > They're not ordered as in the header. > > But I'm doing my best. > > Ah, OK. Well, best effort is fine then. Done. https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:224: return pa_mainloop_; PulseAudioInputStream::Open is going to lock the main loop until the stream is open (which includes calling: PulseAudioInputStream::GetPAMainloop). In case a concurrent call to Open happens, it's going to wait until the first one has finished. so this function could not be called at the same time as setting the variable in Open() https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:152: if (!InitializeMainloopAndContext()) { On 2016/03/10 20:07:34, Henrik Grunell wrote: > InitializeMainloopAndContext() should only be called once as it is written now. Done.
https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:224: return pa_mainloop_; On 2016/03/11 10:13:09, rchtara wrote: > PulseAudioInputStream::Open is going to lock the main loop until the stream is > open (which includes calling: PulseAudioInputStream::GetPAMainloop). > In case a concurrent call to Open happens, it's going to wait until the first > one has finished. > so this function could not be called at the same time as setting the variable in > Open() Acknowledged. https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:152: if (!InitializeMainloopAndContext()) { On 2016/03/11 10:13:09, rchtara wrote: > On 2016/03/10 20:07:34, Henrik Grunell wrote: > > InitializeMainloopAndContext() should only be called once as it is written > now. > > Done. Sorry for not being clear with what I meant. If Open is called again during the lifetime of this object, InitializeMainloopAndContext() will be too, and we shouldn't do that. I noticed now that there are DCHECKs in that function, which asserts that in debug builds. Assuming tests cover various enough scenarios, that should be fine. Remove the comment. https://codereview.chromium.org/1711823004/diff/220001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/220001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:229: base::Lock lock; The lock must be a member variable, and you must lock also where |default_system_device_name_| is accessed. But reading your comment about GetPAMainloop(), isn't it the same for this function? When we call it we will be waiting in Open(), and after done, we read it there. So there's no chance of concurrency issues. Yu can remove the lock. https://codereview.chromium.org/1711823004/diff/220001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/220001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:340: #undef RETURN_ON_FAILURE This isn't necessary. Remove.
Could please review this patch? https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:224: return pa_mainloop_; On 2016/03/11 18:54:41, Henrik Grunell wrote: > On 2016/03/11 10:13:09, rchtara wrote: > > PulseAudioInputStream::Open is going to lock the main loop until the stream is > > open (which includes calling: PulseAudioInputStream::GetPAMainloop). > > In case a concurrent call to Open happens, it's going to wait until the first > > one has finished. > > so this function could not be called at the same time as setting the variable > in > > Open() > > Acknowledged. Done. https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/200001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:152: if (!InitializeMainloopAndContext()) { On 2016/03/11 18:54:41, Henrik Grunell wrote: > On 2016/03/11 10:13:09, rchtara wrote: > > On 2016/03/10 20:07:34, Henrik Grunell wrote: > > > InitializeMainloopAndContext() should only be called once as it is written > > now. > > > > Done. > > Sorry for not being clear with what I meant. If Open is called again during the > lifetime of this object, InitializeMainloopAndContext() will be too, and we > shouldn't do that. > > I noticed now that there are DCHECKs in that function, which asserts that in > debug builds. Assuming tests cover various enough scenarios, that should be > fine. > > Remove the comment. Done. https://codereview.chromium.org/1711823004/diff/220001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/220001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:229: base::Lock lock; On 2016/03/11 18:54:41, Henrik Grunell wrote: > The lock must be a member variable, and you must lock also where > |default_system_device_name_| is accessed. > > But reading your comment about GetPAMainloop(), isn't it the same for this > function? When we call it we will be waiting in Open(), and after done, we read > it there. So there's no chance of concurrency issues. Yu can remove the lock. Done. https://codereview.chromium.org/1711823004/diff/220001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/220001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:340: #undef RETURN_ON_FAILURE On 2016/03/11 18:54:41, Henrik Grunell wrote: > This isn't necessary. Remove. Done.
Just a final change now, or revert of a change I wanted before... https://codereview.chromium.org/1711823004/diff/260001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/260001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:20: void GetSystemDefaultInputDeviceCallback(pa_context* context, Really sorry, but I didn't see until now that the existing callbacks (e.g. VolumeCallback) are static functions that access members without setters and getters. I think it's better to follow the existing style. So change back to that and remove the setter and getter for default system device name and PA mainloop. Again, sorry for the extra work.
No worries. Could you please have a look to my last patch? https://codereview.chromium.org/1711823004/diff/260001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/260001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:20: void GetSystemDefaultInputDeviceCallback(pa_context* context, No worries :) Done!
lgtm Tommi: ptal.
On 2016/03/14 14:42:33, Henrik Grunell wrote: > lgtm > > Tommi: ptal.
Thanks a lot Henrik for the quick review.
https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:20: #define RETURN_ON_FAILURE(expression, message) \ we try to avoid macros as much as we can. Even when we do use them, they shouldn't hide a return statement. I'd rather have C++ code inline https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:80: stream->device_id_ = info->default_sink_name; if device_id_ was set to "default" will this perhaps change it to be something else and potentially then cause checks for the default device (assuming we have those somewhere), fail? https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:92: WaitForOperationCompletion(pa_mainloop_, operation); are we on the audiomanager thread? If we can avoid waiting synchronously like this, then that would be preferable. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:125: while (true) { same here https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:284: reinterpret_cast<media::PulseAudioInputStream*>(user_data); nit: typically we use static_cast<> for upcasting a void pointer to a more specific pointer type. https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:20: #define RETURN_ON_FAILURE(expression, message) \ macros are discouraged and especially ones that have a return statement. Can you think of a way to do this without it? https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:83: pa_mainloop_api, app_name.empty() ? "Chromium" : app_name.c_str()); When do we get an empty string from GetGlobalAppName?
On 2016/03/16 15:21:48, tommi-chromium wrote: > https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... > File media/audio/pulse/pulse_output.cc (right): > > https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... > media/audio/pulse/pulse_output.cc:20: #define RETURN_ON_FAILURE(expression, > message) \ > we try to avoid macros as much as we can. Even when we do use them, they > shouldn't hide a return statement. I'd rather have C++ code inline > > https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... > media/audio/pulse/pulse_output.cc:80: stream->device_id_ = > info->default_sink_name; > if device_id_ was set to "default" will this perhaps change it to be something > else and potentially then cause checks for the default device (assuming we have > those somewhere), fail? > > https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... > media/audio/pulse/pulse_output.cc:92: WaitForOperationCompletion(pa_mainloop_, > operation); > are we on the audiomanager thread? If we can avoid waiting synchronously like > this, then that would be preferable. > > https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... > media/audio/pulse/pulse_output.cc:125: while (true) { > same here > > https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... > File media/audio/pulse/pulse_input.cc (right): > > https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... > media/audio/pulse/pulse_input.cc:284: > reinterpret_cast<media::PulseAudioInputStream*>(user_data); > nit: typically we use static_cast<> for upcasting a void pointer to a more > specific pointer type. > > https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... > File media/audio/pulse/pulse_output.cc (right): > > https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... > media/audio/pulse/pulse_output.cc:20: #define RETURN_ON_FAILURE(expression, > message) \ > macros are discouraged and especially ones that have a return statement. Can you > think of a way to do this without it? > > https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... > media/audio/pulse/pulse_output.cc:83: pa_mainloop_api, app_name.empty() ? > "Chromium" : app_name.c_str()); > When do we get an empty string from GetGlobalAppName? Apparantly I had some comments in draft that got sent now. Sorry about the duplicates.
Thanks a lot for the review. I will have a look tomorrow.
Hi Tommi, Sorry for the delay, I preferred to continue working on my intern project last week. Could you have another look please, there are some issues that were not solved and I will need some one more experienced to help me with them?(I will ask Henrik Grunell for help) Thanks a lot Riadh https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:20: #define RETURN_ON_FAILURE(expression, message) \ On 2016/03/16 15:21:47, tommi-chromium wrote: > we try to avoid macros as much as we can. Even when we do use them, they > shouldn't hide a return statement. I'd rather have C++ code inline Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:80: stream->device_id_ = info->default_sink_name; A new attribute of PulseAudioOutputStream called |default_system_device_name_| is going to be used instead https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:92: WaitForOperationCompletion(pa_mainloop_, operation); I'm not sure about that. I don't know how we could do that. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:125: while (true) { On 2016/03/16 15:21:47, tommi-chromium wrote: > same here Not sure here too. https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... media/audio/pulse/pulse_input.cc:284: reinterpret_cast<media::PulseAudioInputStream*>(user_data); On 2016/03/16 15:21:47, tommi-chromium wrote: > nit: typically we use static_cast<> for upcasting a void pointer to a more > specific pointer type. Done. https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:20: #define RETURN_ON_FAILURE(expression, message) \ On 2016/03/16 15:21:47, tommi-chromium wrote: > macros are discouraged and especially ones that have a return statement. Can you > think of a way to do this without it? Done. https://codereview.chromium.org/1711823004/diff/280001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:83: pa_mainloop_api, app_name.empty() ? "Chromium" : app_name.c_str()); I'm just moving some code from CreateOutputStream to here and I'm not sure if that could happen or not. So, this why I prefer keeping things as they were to avoid bugs. GetGlobalAppName says that it returns the app name or an empty string if it is not set. So it's probably possible for it to return empty string. https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... Using the app name was introduced here by the way. https://codereview.chromium.org/1317033006/
some more comments but almost there. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:92: WaitForOperationCompletion(pa_mainloop_, operation); On 2016/03/19 12:07:29, rchtara wrote: > I'm not sure about that. I don't know how we could do that. You can see it from where this function is called. Looking at it now, I see that it's called from Open() below, that function has a thread check, so you can deduce that this method is called on that same thread. Would be good to also have DCHECK(thread_checker_.CalledOnValidThread()); at the top of this function. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:125: while (true) { On 2016/03/19 12:07:30, rchtara wrote: > On 2016/03/16 15:21:47, tommi-chromium wrote: > > same here > > Not sure here too. See thread check below (which should be at the top like it was). https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:243: // Set sample specifications. On 2016/03/09 16:16:09, rchtara wrote: > So let's keep the lock. it safer > We also don't want inefficiency :) Best is to understand exactly how the code works. Having a lock is either going to be needed or not. We don't want to grab the lock here if we don't need it. A way to understand how things work and to make sure the threading requirements are met, is to use a thread checker. Is this function always called on the same thread as other methods that touch the same data? https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:23: do { Since this is now a function and not a macro, the do/while trick isn't needed. The variable name 'expression' doesn't represent an expression anymore. The function basically always returns the value of |expression|, so technically the caller is providing the return value that it then checks. What remains is basically the same thing that can be accomplished by calling DLOG_IF(). https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:82: if (!LogOnFailure(pa_mainloop_, "Failed to create PulseAudio main loop.")) { It's a little misleading to check the return value of LogOnFailure. It reads as LogOnFailure having failed, but kind of the opposite just happened. Instead of having LogOnFailure at all, I would just write this out as: if (!pa_mainloop_) { DLOG(ERROR) << "Failed to create PulseAudio main loop."; return false; } Same below. As is, the error string is present in official shipping builds, that don't include DLOG code, so it's just bloat. https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.h (left): https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... media/audio/pulse/pulse_util.h:51: keep this empty line around for consistency and readability.
Tommi, it looks like you commented on a pretty old patch set a few days ago. Latest round was on latest code though. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:20: #define RETURN_ON_FAILURE(expression, message) \ On 2016/03/19 12:07:29, rchtara wrote: > On 2016/03/16 15:21:47, tommi-chromium wrote: > > we try to avoid macros as much as we can. Even when we do use them, they > > shouldn't hide a return statement. I'd rather have C++ code inline > > Done. As background, the macro came along with the code below that was moved from pulse_util.cc. Agree though we shouldn't use it. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:92: WaitForOperationCompletion(pa_mainloop_, operation); On 2016/03/19 13:44:17, tommi-chromium wrote: > On 2016/03/19 12:07:29, rchtara wrote: > > I'm not sure about that. I don't know how we could do that. > > You can see it from where this function is called. Looking at it now, I see > that it's called from Open() below, that function has a thread check, so you can > deduce that this method is called on that same thread. Would be good to also > have DCHECK(thread_checker_.CalledOnValidThread()); at the top of this function. Add DCHECK here is good. We'd have to wait though since we need the device name when opening the device. Right now Open() returns success or failure, so we'd have to change that to being asynchronous. Maybe we could test measure the time it takes to get the device name to get an understanding of the size of the cost. Wdyt? https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:125: while (true) { On 2016/03/19 13:44:17, tommi-chromium wrote: > On 2016/03/19 12:07:30, rchtara wrote: > > On 2016/03/16 15:21:47, tommi-chromium wrote: > > > same here > > > > Not sure here too. > > See thread check below (which should be at the top like it was). Tommi, did you perhaps not review the latest patch set? I think some of the comments were already addressed. If you just want the thread check on top of the functions, that's already done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:243: // Set sample specifications. On 2016/03/19 13:44:17, tommi-chromium wrote: > On 2016/03/09 16:16:09, rchtara wrote: > > So let's keep the lock. it safer > > > > We also don't want inefficiency :) Best is to understand exactly how the code > works. Having a lock is either going to be needed or not. We don't want to > grab the lock here if we don't need it. A way to understand how things work and > to make sure the threading requirements are met, is to use a thread checker. Is > this function always called on the same thread as other methods that touch the > same data? Is the lock anyhow related to PulseAudio internals? Or is it just Chrome code related? I'm thinking that maybe PA accesses it, and we don't have control over when.
https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (left): https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:173: Keep the empty line after last DCHECK.
On 2016/03/21 09:44:18, Henrik Grunell wrote: > Tommi, it looks like you commented on a pretty old patch set a few days ago. > Latest round was on latest code though. I looked at the latest (320001). I had comments in draft for a previous patch set, that got sent out as well.
https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:20: #define RETURN_ON_FAILURE(expression, message) \ On 2016/03/21 09:44:18, Henrik Grunell wrote: > On 2016/03/19 12:07:29, rchtara wrote: > > On 2016/03/16 15:21:47, tommi-chromium wrote: > > > we try to avoid macros as much as we can. Even when we do use them, they > > > shouldn't hide a return statement. I'd rather have C++ code inline > > > > Done. > > As background, the macro came along with the code below that was moved from > pulse_util.cc. Agree though we shouldn't use it. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:92: WaitForOperationCompletion(pa_mainloop_, operation); On 2016/03/19 13:44:17, tommi-chromium wrote: > On 2016/03/19 12:07:29, rchtara wrote: > > I'm not sure about that. I don't know how we could do that. > > You can see it from where this function is called. Looking at it now, I see > that it's called from Open() below, that function has a thread check, so you can > deduce that this method is called on that same thread. Would be good to also > have DCHECK(thread_checker_.CalledOnValidThread()); at the top of this function. Done. https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:125: while (true) { On 2016/03/19 13:44:17, tommi-chromium wrote: > On 2016/03/19 12:07:30, rchtara wrote: > > On 2016/03/16 15:21:47, tommi-chromium wrote: > > > same here > > > > Not sure here too. > > See thread check below (which should be at the top like it was). Done. https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:23: do { On 2016/03/19 13:44:17, tommi-chromium wrote: > Since this is now a function and not a macro, the do/while trick isn't needed. > The variable name 'expression' doesn't represent an expression anymore. > The function basically always returns the value of |expression|, so technically > the caller is providing the return value that it then checks. > What remains is basically the same thing that can be accomplished by calling > DLOG_IF(). Done. https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:82: if (!LogOnFailure(pa_mainloop_, "Failed to create PulseAudio main loop.")) { On 2016/03/19 13:44:17, tommi-chromium wrote: > It's a little misleading to check the return value of LogOnFailure. It reads as > LogOnFailure having failed, but kind of the opposite just happened. Instead of > having LogOnFailure at all, I would just write this out as: > > if (!pa_mainloop_) { > DLOG(ERROR) << "Failed to create PulseAudio main loop."; > return false; > } > > Same below. As is, the error string is present in official shipping builds, > that don't include DLOG code, so it's just bloat. Done. https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (left): https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:173: On 2016/03/21 09:46:14, Henrik Grunell wrote: > Keep the empty line after last DCHECK. Done. https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.h (left): https://codereview.chromium.org/1711823004/diff/320001/media/audio/pulse/puls... media/audio/pulse/pulse_util.h:51: On 2016/03/19 13:44:17, tommi-chromium wrote: > keep this empty line around for consistency and readability. Done.
I updated the patch, could you guys have look to it. Thanks a lot Riadh
https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_output.cc:92: WaitForOperationCompletion(pa_mainloop_, operation); So, I created a timer to get the duration needed: it was: 0.000147 s 0.00032 s 0.000908 s So, It's probably not that slow.
On 2016/03/21 09:59:39, tommi-chromium wrote: > On 2016/03/21 09:44:18, Henrik Grunell wrote: > > Tommi, it looks like you commented on a pretty old patch set a few days ago. > > Latest round was on latest code though. > > I looked at the latest (320001). I had comments in draft for a previous patch > set, that got sent out as well. Ah, I see.
On 2016/03/22 16:56:25, rchtara wrote: > https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... > File media/audio/pulse/pulse_output.cc (right): > > https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... > media/audio/pulse/pulse_output.cc:92: WaitForOperationCompletion(pa_mainloop_, > operation); > So, I created a timer to get the duration needed: it was: > 0.000147 s > 0.00032 s > 0.000908 s > > So, It's probably not that slow. Less that 1 ms for these cases. This could maybe occasionally be much larger. Let's ping Dale and see if he knows PulseAudio details.
https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:243: // Set sample specifications. On 2016/03/21 09:44:18, Henrik Grunell wrote: > On 2016/03/19 13:44:17, tommi-chromium wrote: > > On 2016/03/09 16:16:09, rchtara wrote: > > > So let's keep the lock. it safer > > > > > > > We also don't want inefficiency :) Best is to understand exactly how the code > > works. Having a lock is either going to be needed or not. We don't want to > > grab the lock here if we don't need it. A way to understand how things work > and > > to make sure the threading requirements are met, is to use a thread checker. > Is > > this function always called on the same thread as other methods that touch the > > same data? > > Is the lock anyhow related to PulseAudio internals? Or is it just Chrome code > related? I'm thinking that maybe PA accesses it, and we don't have control over > when. Also, while we should sure not grab a lock if we don't need it, in this case we're just keeping the locking as it was before. I think that's fine and we could add a todo and bug to look at improvements later. https://codereview.chromium.org/1711823004/diff/360001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/360001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:174: DCHECK_NE(device_id, AudioManagerBase::kDefaultDeviceId); Nit: No empty line between the DCHECKs, instead empty line after the last DCHECK.
Dale (cc), are you familiar with PulseAudio? What's the risk that the blocking time in https://codereview.chromium.org/1711823004/diff/100001/media/audio/pulse/puls... would be occasionally large. Three measurements Riadh did was < 1 ms. Make Open asynchronous would be a larger change.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Pulse unfortunately requires a lot of those blocking operations. I don't think there's anyway around it in some cases. If you need the value in a synchronous manner you'll have to wait for the operation to complete. We also had some issue with deadlock by not waiting on one of these operations before, but I can't recall the details.
On 2016/03/23 17:46:20, DaleCurtis wrote: > Pulse unfortunately requires a lot of those blocking operations. I don't think > there's anyway around it in some cases. If you need the value in a synchronous > manner you'll have to wait for the operation to complete. We also had some issue > with deadlock by not waiting on one of these operations before, but I can't > recall the details. Thanks. And you're not aware of if the time these operations (or this operation in particular) block can vary a lot? I.e. how much can we trust the three samples Riadh recorded?
Those times are inline with what I saw in the past. In general it's not the end of the world to very rarely block the audio thread for even a second or so (so long as it's not on OSX) -- since mostly that thread is just a control channel for play/pause/create events which are relatively infrequent. Only tab capture / fake audio devices make use of the audio thread in a high frequency manner.
On 2016/03/23 18:29:52, DaleCurtis wrote: > Those times are inline with what I saw in the past. In general it's not the end > of the world to very rarely block the audio thread for even a second or so (so > long as it's not on OSX) -- since mostly that thread is just a control channel > for play/pause/create events which are relatively infrequent. Only tab capture / > fake audio devices make use of the audio thread in a high frequency manner. Thanks. lgtm. Tommi?
lgtm
Thanks a lot guys for the reviews Riadh https://codereview.chromium.org/1711823004/diff/360001/media/audio/pulse/puls... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/1711823004/diff/360001/media/audio/pulse/puls... media/audio/pulse/pulse_util.cc:174: DCHECK_NE(device_id, AudioManagerBase::kDefaultDeviceId); On 2016/03/23 12:51:27, Henrik Grunell wrote: > Nit: No empty line between the DCHECKs, instead empty line after the last > DCHECK. Done.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1711823004/#ps380001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1711823004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711823004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 ========== to ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 NOTRY=true ==========
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1711823004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711823004/380001
Message was sent while issue was closed.
Description was changed from ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 NOTRY=true ========== to ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 NOTRY=true ========== to ========== Use the the system default device instead of the application default when opening the default device in PulseAudio. BUG=343839 NOTRY=true Committed: https://crrev.com/254c78a58a9da3a320c5ce8b1d84c0843fe3b8bc Cr-Commit-Position: refs/heads/master@{#383945} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/254c78a58a9da3a320c5ce8b1d84c0843fe3b8bc Cr-Commit-Position: refs/heads/master@{#383945} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
