Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692203003:
Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one. (Closed)
|
Created:
3 years, 10 months ago by o1ka Modified:
3 years, 9 months ago CC:
chromium-reviews, creis+watch_chromium.org, posciak+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org, Max Morin, ossu, audio-mojo-cl_google.com Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one.
BUG=672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2692203003
Cr-Commit-Position: refs/heads/master@{#450939}
Committed: https://chromium.googlesource.com/chromium/src/+/04ff52bb66284467ccb43d90800013b89ee8db75
Patch Set 1 #
Total comments: 14
Patch Set 2 : AudioSystem comments updated according to discussion with tommi@ #
Total comments: 4
Messages
Total messages: 35 (15 generated)
Description was changed from ========== Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one. BUG=672468 ========== to ========== Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
olka@chromium.org changed reviewers: + guido@google.com, tommi@chromium.org
guidou@ PTAL at content/browser/renderer_host/media and my questions, tommi@ PTAL at media/audio as an owner. https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system.h File media/audio/audio_system.h (right): https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system.h#... media/audio/audio_system.h:35: // Callback will receive invalid parameters if the device (default or not) is guidou@: AudioOutputAuthorizationHandler will fall back to UnavailableDeviceParameters in TryToFixAudioParameters(), so the behavior is not changed for authorization. Do we probably want to change it in the future to notify that the device is not found? And when we can actually hit it? (Looks like it's if the device disappeared after enumeration?) https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:43: return media::AudioDeviceDescription::IsDefaultDevice(device_id) guidou@: it seems that all the usecases make this check anyways, so I decided to combine the two cases into one interface method. Are you ok with it, or may it lead to a confusion?
https://codereview.chromium.org/2692203003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (left): https://codereview.chromium.org/2692203003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_output_authorization_handler.cc:37: media::AudioParameters GetDeviceParametersOnDeviceThread( Very nice that this code goes away. It never belonged here anyways :)
https://codereview.chromium.org/2692203003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (left): https://codereview.chromium.org/2692203003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_output_authorization_handler.cc:37: media::AudioParameters GetDeviceParametersOnDeviceThread( On 2017/02/14 11:45:45, Max Morin wrote: > Very nice that this code goes away. It never belonged here anyways :) Well, not quite. It's just collapsed to one call. I'm not altering authorization sequence in this CL.
https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:39: // the platforms. This covers a particular set of cases but not when a specific device is being asked for. As an example, I've seen calls make it to the AudioManager layer where the device_id is actually an id for tab audio capture. So, in this case, even if HasAudioOutputDevices() returns true, it still could be that the device_id is invalid or points to a device that isn't available. In those cases, we should also be returning invalid audio parameters. https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:43: return media::AudioDeviceDescription::IsDefaultDevice(device_id) On 2017/02/14 11:37:03, o1ka wrote: > guidou@: it seems that all the usecases make this check anyways, so I decided to > combine the two cases into one interface method. Are you ok with it, or may it > lead to a confusion? Doesn't GetOutputStreamParameters already handle the default device too? One method at the audiomanager level (for now), makes sense to me instead of making this level have to check the id.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:39: // the platforms. On 2017/02/14 12:14:26, tommi (krómíum) wrote: > This covers a particular set of cases but not when a specific device is being > asked for. > As an example, I've seen calls make it to the AudioManager layer where the > device_id is actually an id for tab audio capture. > > So, in this case, even if HasAudioOutputDevices() returns true, it still could > be that the device_id is invalid or points to a device that isn't available. In > those cases, we should also be returning invalid audio parameters. TabAudioCapture are input devices, not output ones. Same if it is just a random string. This is up to per-platform AudioManager implementations to handle it, and it can't be addressed in this CL. https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:43: return media::AudioDeviceDescription::IsDefaultDevice(device_id) On 2017/02/14 12:14:26, tommi (krómíum) wrote: > On 2017/02/14 11:37:03, o1ka wrote: > > guidou@: it seems that all the usecases make this check anyways, so I decided > to > > combine the two cases into one interface method. Are you ok with it, or may it > > lead to a confusion? > > Doesn't GetOutputStreamParameters already handle the default device too? No, it does not. One > method at the audiomanager level (for now), makes sense to me instead of making > this level have to check the id. The goal is to introduce the correct interface. This particular implementation is just what it was before the CL - it's just in a different place now, hidden by the interface. Modifications of AudioManager within this CL will lead to a much larger diff (just check all the occurrences of AudioManager::GetDefaultOutputStreamParameters). If we want to modify AudioManager, it can be addressed in a separate CL after all the clients are switch to AudioSystem. This way we will have better locality of changes and a smaller diff. Overall, for Audio Process work we want to avoid AudioManager modifications as much as possible, because each platform-specific implementation requires a separate attention, and we are very limited in resources. AudioManager interface is not nice at all, but we will deal with it later - now it's enough to hide it.
https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:39: // the platforms. On 2017/02/14 15:00:49, o1ka wrote: > On 2017/02/14 12:14:26, tommi (krómíum) wrote: > > This covers a particular set of cases but not when a specific device is being > > asked for. > > As an example, I've seen calls make it to the AudioManager layer where the > > device_id is actually an id for tab audio capture. > > > > So, in this case, even if HasAudioOutputDevices() returns true, it still could > > be that the device_id is invalid or points to a device that isn't available. > In > > those cases, we should also be returning invalid audio parameters. > > TabAudioCapture are input devices, not output ones. I am aware of that and I didn't say it was output. In my comment, I mention it as an example of a request going to a layer it should not go to, unchecked. The pattern being used here, is the same pattern as being used a couple of lines up, in GetInputParametersOnDeviceThread. That means that both have the same limitation. > Same if it is just a random string. This is up to per-platform AudioManager > implementations to handle it, and it can't be addressed in this CL. I didn't say that it should. However, this CL attempts to handle it partially but not completely and checks are being done in two places. - Which is OK as a temporary situation. The comment however says "remove this when [...] works this way on all the platforms". I'm pointing out that a proper check needs to do more than the error check being added here. So in this CL, it would be good to at least update the comment. https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:43: return media::AudioDeviceDescription::IsDefaultDevice(device_id) On 2017/02/14 15:00:49, o1ka wrote: > On 2017/02/14 12:14:26, tommi (krómíum) wrote: > > On 2017/02/14 11:37:03, o1ka wrote: > > > guidou@: it seems that all the usecases make this check anyways, so I > decided > > to > > > combine the two cases into one interface method. Are you ok with it, or may > it > > > lead to a confusion? > > > > Doesn't GetOutputStreamParameters already handle the default device too? > No, it does not. Uhm, ok, time for fact checking! :) On Mac and Windows they actually do and those are the most widely used implementations. > One > > method at the audiomanager level (for now), makes sense to me instead of > making > > this level have to check the id. > > The goal is to introduce the correct interface. This particular implementation > is just what it was before the CL - it's just in a different place now, hidden > by the interface. Modifications of AudioManager within this CL will lead to a > much larger diff (just check all the occurrences of > AudioManager::GetDefaultOutputStreamParameters). If we want to modify > AudioManager, it can be addressed in a separate CL after all the clients are > switch to AudioSystem. This way we will have better locality of changes and a > smaller diff. > > Overall, for Audio Process work we want to avoid AudioManager modifications as > much as possible, because each platform-specific implementation requires a > separate attention, and we are very limited in resources. AudioManager interface > is not nice at all, but we will deal with it later - now it's enough to hide it. I understand, I'm one of those resources and I'm not trying to block the cl.
https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:39: // the platforms. On 2017/02/14 15:30:32, tommi (krómíum) wrote: > On 2017/02/14 15:00:49, o1ka wrote: > > On 2017/02/14 12:14:26, tommi (krómíum) wrote: > > > This covers a particular set of cases but not when a specific device is > being > > > asked for. > > > As an example, I've seen calls make it to the AudioManager layer where the > > > device_id is actually an id for tab audio capture. > > > > > > So, in this case, even if HasAudioOutputDevices() returns true, it still > could > > > be that the device_id is invalid or points to a device that isn't available. > > > In > > > those cases, we should also be returning invalid audio parameters. > > > > TabAudioCapture are input devices, not output ones. > > I am aware of that and I didn't say it was output. > You haven't said that you meant both Input and Output methods either :) > In my comment, I mention it as an example of a request going to a layer it > should not go to, unchecked. The pattern being used here, is the same pattern > as being used a couple of lines up, in GetInputParametersOnDeviceThread. That > means that both have the same limitation. > > > Same if it is just a random string. This is up to per-platform AudioManager > > implementations to handle it, and it can't be addressed in this CL. > > I didn't say that it should. However, this CL attempts to handle it partially > but not completely and checks are being done in two places. - Which is OK as a > temporary situation. > > The comment however says "remove this when [...] works this way on all the > platforms". I'm pointing out that a proper check needs to do more than the > error check being added here. So in this CL, it would be good to at least > update the comment. I'm not sure I completely follow you. AudioManager should return invalid parameters if device is not found. When it (all its implementations) is fixed to do so, we can remove HasDevices check. Right? That's what I meant by the comment. I'll update the header with more precise description of when invalid parameters are returned. https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:43: return media::AudioDeviceDescription::IsDefaultDevice(device_id) On 2017/02/14 15:30:32, tommi (krómíum) wrote: > On 2017/02/14 15:00:49, o1ka wrote: > > On 2017/02/14 12:14:26, tommi (krómíum) wrote: > > > On 2017/02/14 11:37:03, o1ka wrote: > > > > guidou@: it seems that all the usecases make this check anyways, so I > > decided > > > to > > > > combine the two cases into one interface method. Are you ok with it, or > may > > it > > > > lead to a confusion? > > > > > > Doesn't GetOutputStreamParameters already handle the default device too? > > No, it does not. > > Uhm, ok, time for fact checking! :) > > On Mac and Windows they actually do and those are the most widely used > implementations. No, it does not :) Take a look here: AudioParameters AudioManagerBase::GetDefaultOutputStreamParameters() { return GetPreferredOutputStreamParameters(GetDefaultOutputDeviceID(), AudioParameters()); } We need to pass a result of a non-public AudioManager::GetDefaultOutputDeviceID() into AudioManager::GetOutputStreamParameters(), to get the parameters for the default device. With the proposed AudioSystem interface, we are passing "default" and getting the default parameters. > > > One > > > method at the audiomanager level (for now), makes sense to me instead of > > making > > > this level have to check the id. > > > > The goal is to introduce the correct interface. This particular implementation > > is just what it was before the CL - it's just in a different place now, hidden > > by the interface. Modifications of AudioManager within this CL will lead to a > > much larger diff (just check all the occurrences of > > AudioManager::GetDefaultOutputStreamParameters). If we want to modify > > AudioManager, it can be addressed in a separate CL after all the clients are > > switch to AudioSystem. This way we will have better locality of changes and a > > smaller diff. > > > > Overall, for Audio Process work we want to avoid AudioManager modifications as > > much as possible, because each platform-specific implementation requires a > > separate attention, and we are very limited in resources. AudioManager > interface > > is not nice at all, but we will deal with it later - now it's enough to hide > it. > > I understand, I'm one of those resources and I'm not trying to block the cl. I'm glad that we are on the same page :)
https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:39: // the platforms. On 2017/02/14 15:47:12, o1ka wrote: > On 2017/02/14 15:30:32, tommi (krómíum) wrote: > > On 2017/02/14 15:00:49, o1ka wrote: > > > On 2017/02/14 12:14:26, tommi (krómíum) wrote: > > > > This covers a particular set of cases but not when a specific device is > > being > > > > asked for. > > > > As an example, I've seen calls make it to the AudioManager layer where the > > > > device_id is actually an id for tab audio capture. > > > > > > > > So, in this case, even if HasAudioOutputDevices() returns true, it still > > could > > > > be that the device_id is invalid or points to a device that isn't > available. > > > > > In > > > > those cases, we should also be returning invalid audio parameters. > > > > > > TabAudioCapture are input devices, not output ones. > > > > I am aware of that and I didn't say it was output. > > > > You haven't said that you meant both Input and Output methods either :) > I wasn't talking specifically about either input or output. I was talking about this layer vs the AudioManager layer. Applicable to both input and output. Can you read the comment again and perhaps tell me how I should have phrased them? > > In my comment, I mention it as an example of a request going to a layer it > > should not go to, unchecked. The pattern being used here, is the same pattern > > as being used a couple of lines up, in GetInputParametersOnDeviceThread. That > > means that both have the same limitation. > > > > > Same if it is just a random string. This is up to per-platform AudioManager > > > implementations to handle it, and it can't be addressed in this CL. > > > > I didn't say that it should. However, this CL attempts to handle it partially > > but not completely and checks are being done in two places. - Which is OK as a > > temporary situation. > > > > The comment however says "remove this when [...] works this way on all the > > platforms". I'm pointing out that a proper check needs to do more than the > > error check being added here. So in this CL, it would be good to at least > > update the comment. > > I'm not sure I completely follow you. AudioManager should return invalid > parameters if device is not found. When it (all its implementations) is fixed to > do so, we can remove HasDevices check. Right? That's what I meant by the > comment. I'll update the header with more precise description of when invalid > parameters are returned. That's correct. The HasDevices check can be removed when all implementations do proper error handling. Just the HasDevices() check, is not enough. It only does something useful if there are no devices at all. https://codereview.chromium.org/2692203003/diff/1/media/audio/audio_system_im... media/audio/audio_system_impl.cc:43: return media::AudioDeviceDescription::IsDefaultDevice(device_id) On 2017/02/14 15:47:12, o1ka wrote: > On 2017/02/14 15:30:32, tommi (krómíum) wrote: > > On 2017/02/14 15:00:49, o1ka wrote: > > > On 2017/02/14 12:14:26, tommi (krómíum) wrote: > > > > On 2017/02/14 11:37:03, o1ka wrote: > > > > > guidou@: it seems that all the usecases make this check anyways, so I > > > decided > > > > to > > > > > combine the two cases into one interface method. Are you ok with it, or > > may > > > it > > > > > lead to a confusion? > > > > > > > > Doesn't GetOutputStreamParameters already handle the default device too? > > > No, it does not. > > > > Uhm, ok, time for fact checking! :) > > > > On Mac and Windows they actually do and those are the most widely used > > implementations. > > No, it does not :) Take a look here: > AudioParameters AudioManagerBase::GetDefaultOutputStreamParameters() { > return GetPreferredOutputStreamParameters(GetDefaultOutputDeviceID(), > AudioParameters()); > } > > We need to pass a result of a non-public > AudioManager::GetDefaultOutputDeviceID() into > AudioManager::GetOutputStreamParameters(), to get the parameters for the default > device. > > With the proposed AudioSystem interface, we are passing "default" and getting > the default parameters. Windows: https://cs.chromium.org/chromium/src/media/audio/win/core_audio_util_win.cc?r... Mac: https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?rcl... That's where GetOutputStreamParameters("default") would end up (from AudioManagerBase::GetOutputStreamParameters to platform specific GetPreferredOutputStreamParameters). That's all I'm saying. > > > > > One > > > > method at the audiomanager level (for now), makes sense to me instead of > > > making > > > > this level have to check the id. > > > > > > The goal is to introduce the correct interface. This particular > implementation > > > is just what it was before the CL - it's just in a different place now, > hidden > > > by the interface. Modifications of AudioManager within this CL will lead to > a > > > much larger diff (just check all the occurrences of > > > AudioManager::GetDefaultOutputStreamParameters). If we want to modify > > > AudioManager, it can be addressed in a separate CL after all the clients are > > > switch to AudioSystem. This way we will have better locality of changes and > a > > > smaller diff. > > > > > > Overall, for Audio Process work we want to avoid AudioManager modifications > as > > > much as possible, because each platform-specific implementation requires a > > > separate attention, and we are very limited in resources. AudioManager > > interface > > > is not nice at all, but we will deal with it later - now it's enough to hide > > it. > > > > I understand, I'm one of those resources and I'm not trying to block the cl. > > I'm glad that we are on the same page :)
guidou@chromium.org changed reviewers: + guidou@chromium.org - guido@google.com
> > I wasn't talking specifically about either input or output. I was talking about > this layer vs the AudioManager layer. Applicable to both input and output. Can > you read the comment again and perhaps tell me how I should have phrased them? > The comment was confusing for me, because it was for a specific line of the code, which usually means the context for the comment (otherwise, it is useful to mention "here and ...") Also, input was already reviewed by you, so it was hard for me to guess that the comment was applicable to both. Specifically stating "Applicable to both input and output" would help to better understand what you meant. > That's correct. The HasDevices check can be removed when all implementations do > proper error handling. > Just the HasDevices() check, is not enough. It only does something useful if > there are no devices at all. Not much we can do outside of AudioManager. I clarified the comments - WDYT? > > > Windows: > https://cs.chromium.org/chromium/src/media/audio/win/core_audio_util_win.cc?r... > Mac: > https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?rcl... > > That's where GetOutputStreamParameters("default") would end up (from > AudioManagerBase::GetOutputStreamParameters to platform specific > GetPreferredOutputStreamParameters). > That's all I'm saying. > Aha, now I see what you meant. This is weird, taking into account this code on Windows [https://cs.chromium.org/chromium/src/media/audio/win/audio_manager_win.cc?type=cs&q=GetPreferredAudioParameters&l=403)] and the above-mentioned AudioManagerBase call. All in all, it does not help us with this CL.
https://codereview.chromium.org/2692203003/diff/20001/media/audio/audio_syste... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/20001/media/audio/audio_syste... media/audio/audio_system_impl.cc:41: return AudioParameters(); Should this be UnavailableDeviceParams()?
https://codereview.chromium.org/2692203003/diff/20001/media/audio/audio_syste... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/20001/media/audio/audio_syste... media/audio/audio_system_impl.cc:41: return AudioParameters(); On 2017/02/15 15:40:06, Guido Urdaneta wrote: > Should this be UnavailableDeviceParams()? The client usually needs to check for parameter validity anyways. Also, AudioManager returns invalid parameters in one case on Windows now, so returning invalid parameters looks natural. Whatever we decide here, Input needs to be symmetrical; and Tommi wanted to switch AudioManager to return invalid parameters for input. WDYT?
lgtm https://codereview.chromium.org/2692203003/diff/20001/media/audio/audio_syste... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2692203003/diff/20001/media/audio/audio_syste... media/audio/audio_system_impl.cc:41: return AudioParameters(); On 2017/02/15 15:45:07, o1ka wrote: > On 2017/02/15 15:40:06, Guido Urdaneta wrote: > > Should this be UnavailableDeviceParams()? > > The client usually needs to check for parameter validity anyways. Also, > AudioManager returns invalid parameters in one case on Windows now, so returning > invalid parameters looks natural. > Whatever we decide here, Input needs to be symmetrical; and Tommi wanted to > switch AudioManager to return invalid parameters for input. > WDYT? Makes sense.
olka@chromium.org changed reviewers: + avi@chromium.org
avi@chromium.org: Please review changes in content/browser/renderer_host/render_process_host_impl.cc Thanks!
lgtm
lgtm https://codereview.chromium.org/2692203003/diff/20001/media/audio/audio_system.h File media/audio/audio_system.h (right): https://codereview.chromium.org/2692203003/diff/20001/media/audio/audio_syste... media/audio/audio_system.h:31: // TODO(olka,tommi): fix all AudioManager implementations to return invalid w00t - I getz todo too! Thanks for updating the comments
The CQ bit was checked by olka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by olka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487245114720190,
"parent_rev": "68dc18612e07009d613d3bbe4b22900e7d913f7e", "commit_rev":
"04ff52bb66284467ccb43d90800013b89ee8db75"}
Message was sent while issue was closed.
Description was changed from ========== Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2692203003 Cr-Commit-Position: refs/heads/master@{#450939} Committed: https://chromium.googlesource.com/chromium/src/+/04ff52bb66284467ccb43d908000... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/04ff52bb66284467ccb43d908000...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2696253004/ by perkj@chromium.org. The reason for reverting is: Seems to break PPAPINaClPNaClTest.MediaStreamAudioTrack on Mac. https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/13657.
Message was sent while issue was closed.
Patchset #3 (id:40001) has been deleted |
