Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1542103002:
Handle invalid hardware output parameters in AudioRendererHost. (Closed)
|
Created:
5 years ago by Guido Urdaneta Modified:
4 years, 11 months ago Reviewers:
DaleCurtis CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle invalid hardware output parameters in AudioRendererHost.
It may happen that the audio parameters returned by a hardware device are invalid for Chromium.
If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels.
BUG=568860
Committed: https://crrev.com/2d0a4f0bf564bbef1b001004a0d54b9845bc21ad
Cr-Commit-Position: refs/heads/master@{#368316}
Patch Set 1 #Patch Set 2 : return fake audio path and status OK on invalid hardware parameters #Patch Set 3 : Handle AUDIO_FAKE in ARI and ARMM #
Total comments: 2
Patch Set 4 : Remove unnecesary conditions from test #Patch Set 5 : Minor style change #
Messages
Total messages: 35 (14 generated)
Description was changed from ========== Correctly handle invalid audio output parameters in AudioRendererHost. It may happen that the audio parameters returned by a hardware device are invalid for Chromium. In this case, an error status should be returned to the renderer. If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels. BUG=568860 ========== to ========== Handle invalid hardware output parameters in AudioRendererHost. It may happen that the audio parameters returned by a hardware device are invalid for Chromium. In this case, an error status should be returned to the renderer. If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels. BUG=568860 ==========
guidou@chromium.org changed reviewers: + xhwang@chromium.org
Hi, PTAL
xhwang@chromium.org changed reviewers: + dalecurtis@chromium.org
xhwang -> dalecurtis, who is more familiar with audio stuff...
Won't this cause playback to break when bad audio hardware is present? I.e. we'll shutdown a video stream because we can't play audio? Today we intentionally fallback to the fake path so that video can continue to play. When I tested this in the past, it was important for remote desktop/citrix type situations where the audio device is bugged/unavailable.
On 2016/01/05 21:23:44, DaleCurtis wrote: > Won't this cause playback to break when bad audio hardware is present? I.e. > we'll shutdown a video stream because we can't play audio? Today we > intentionally fallback to the fake path so that video can continue to play. > When I tested this in the past, it was important for remote desktop/citrix type > situations where the audio device is bugged/unavailable. I see, so we should return OUTPUT_DEVICE_STATUS_OK with valid parameters with the fake audio path in those cases?
This update returns status OK with dummy (but valid) params and fake audio path in case of unfixable invalid hardware params.
Hmm, I'm still not sure this does what we want, since this code path will now trigger: https://code.google.com/p/chromium/codesearch#chromium/src/media/renderers/au... If you go this route I think we need to modify that section to also use passthrough when a fake is used. Probably a similar change should be done in the ARMM.
On 2016/01/06 21:12:47, DaleCurtis wrote: > Hmm, I'm still not sure this does what we want, since this code path will now > trigger: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/renderers/au... > > If you go this route I think we need to modify that section to also use > passthrough when a fake is used. Probably a similar change should be done in the > ARMM. That particular code will continue to work the same because ARI is reading hardware parameters with GetAudioHardwareConfig(), which uses sync IPC and works as in pre-M47. However, I think we should make the changes you suggest anyway. Perhaps in a separate CL that also changes GetAudioHardwareConfig() to return AUDIO_FAKE instead of invalid parameters so that it is consistent with ARH? Also, is it intentional that invalid audio parameters are serializable and delivered with sync IPC, but not with async IPC?
No, I don't think that's intentional and I'm not sure why it would be the case. I would have thought the param traits is invoked in both cases.
I'm torn on the AUDIO_FAKE idea too. Invalid parameters fail fast, while accidentally using fake parameters can silently inject resampling and other performance impacts. Also it's possible the sync IPC isn't serialized either and the fact that the renderer assumes an invalid AudioParameters is what allows the sync set to work correctly.
On 2016/01/06 23:33:58, DaleCurtis wrote: > I'm torn on the AUDIO_FAKE idea too. Invalid parameters fail fast, while > accidentally using fake parameters can silently inject resampling and other > performance impacts. > > Also it's possible the sync IPC isn't serialized either and the fact that the > renderer assumes an invalid AudioParameters is what allows the sync set to work > correctly. I just reconfirmed that invalid AudioParameters are definitely transferred and delivered with sync IPC (GetAudioHardwareConfig()). I don't know why this is the case, but it is the reason apollo devices worked before M47. These devices report more than 32 channels, which is considered invalid in Chrome. In m47 the hardware parameters are sent from the browser using async IPC, not delivered, and the renderer is left hanging. In m46 sync IPC is used, the invalid parameters are delivered, and everything works as expected. For the apollo devices this is no problem, as the hardware channels are ignored in the renderer and the other parameters are actually valid. How should we fix this, given that it worked in m46 probably due to a bug in sync IPC.
I guess the fake is as good as we can do without adding another type. Lets go with fake and then add the fake checks to the relevant spots (even if they aren't necessary) in this CL.
Description was changed from ========== Handle invalid hardware output parameters in AudioRendererHost. It may happen that the audio parameters returned by a hardware device are invalid for Chromium. In this case, an error status should be returned to the renderer. If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels. BUG=568860 ========== to ========== Handle invalid hardware output parameters in AudioRendererHost. It may happen that the audio parameters returned by a hardware device are invalid for Chromium. If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels. BUG=568860 ==========
guidou@chromium.org changed reviewers: + tommi@chromium.org
guidou@chromium.org changed reviewers: - tommi@chromium.org
On 2016/01/07 01:23:37, DaleCurtis wrote: > I guess the fake is as good as we can do without adding another type. Lets go > with fake and then add the fake checks to the relevant spots (even if they > aren't necessary) in this CL. What do you think about first having another CL increasing media::limits::kMaxChannels to, say, 128 or some other practical number in order to properly support apollo-like devices?
The latest update includes handling of hardware params with AUDIO_FAKE in ARMM and ARI. However, I have some questions: * Should we change GetAudioHardwareConfig() or the AudioManager implementations to return AUDIO_FAKE when parameters are invalid or when there is no device? In this CL? * Should we do anything about some GetAudioHardwareConfig() call sites that assume that hardware parameters are always valid? In those cases there is no obvious thing to do as in ARMM and ARI. * Should we increase media::limits::kMaxChannels to a number large enough that covers all expected hardware and remove the "fixing" of the number of channels currently implemented in MaybeFixAudioParameters()? In a separate CL?
xhwang@chromium.org changed reviewers: - xhwang@chromium.org
lgtm https://codereview.chromium.org/1542103002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1542103002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:142: if (!params->IsValid() && Seems these first two conditions aren't necessary? just check channel count?
On 2016/01/07 13:21:55, Guido Urdaneta wrote: > The latest update includes handling of hardware params with AUDIO_FAKE in ARMM > and ARI. > > However, I have some questions: > * Should we change GetAudioHardwareConfig() or the AudioManager implementations > to return AUDIO_FAKE when parameters are invalid or when there is no device? In > this CL? Most clients still need to check the IsValid flag I think, so can we actually clean that up? > * Should we do anything about some GetAudioHardwareConfig() call sites that > assume that hardware parameters are always valid? In those cases there is no > obvious thing to do as in ARMM and ARI. I think I fixed these a while back? Do you see any other broken ones? > * Should we increase media::limits::kMaxChannels to a number large enough that > covers all expected hardware and remove the "fixing" of the number of channels > currently implemented in MaybeFixAudioParameters()? In a separate CL? sgtm if it works and doesn't blwo anything up.
On 2016/01/07 19:16:01, DaleCurtis wrote: > On 2016/01/07 13:21:55, Guido Urdaneta wrote: > > The latest update includes handling of hardware params with AUDIO_FAKE in ARMM > > and ARI. > > > > However, I have some questions: > > * Should we change GetAudioHardwareConfig() or the AudioManager > implementations > > to return AUDIO_FAKE when parameters are invalid or when there is no device? > In > > this CL? > > Most clients still need to check the IsValid flag I think, so can we actually > clean that up? > > > * Should we do anything about some GetAudioHardwareConfig() call sites that > > assume that hardware parameters are always valid? In those cases there is no > > obvious thing to do as in ARMM and ARI. > > I think I fixed these a while back? Do you see any other broken ones? > It's used also by blink and pepper wrappers. They don't check IsValid(), but I think they handle invalid values for specific parameters (e.g., zero sample rates) separately so it's probably not cause for concern. > > * Should we increase media::limits::kMaxChannels to a number large enough that > > covers all expected hardware and remove the "fixing" of the number of channels > > currently implemented in MaybeFixAudioParameters()? In a separate CL? > > sgtm if it works and doesn't blwo anything up. I'll look into that separately, then.
https://codereview.chromium.org/1542103002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/1542103002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:142: if (!params->IsValid() && On 2016/01/07 19:14:51, DaleCurtis wrote: > Seems these first two conditions aren't necessary? just check channel count? Done.
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1542103002/#ps60001 (title: "Remove unnecesary conditions from test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1542103002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1542103002/60001
The CQ bit was unchecked by guidou@chromium.org
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1542103002/#ps80001 (title: "Minor style change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1542103002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1542103002/80001
Message was sent while issue was closed.
Description was changed from ========== Handle invalid hardware output parameters in AudioRendererHost. It may happen that the audio parameters returned by a hardware device are invalid for Chromium. If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels. BUG=568860 ========== to ========== Handle invalid hardware output parameters in AudioRendererHost. It may happen that the audio parameters returned by a hardware device are invalid for Chromium. If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels. BUG=568860 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Handle invalid hardware output parameters in AudioRendererHost. It may happen that the audio parameters returned by a hardware device are invalid for Chromium. If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels. BUG=568860 ========== to ========== Handle invalid hardware output parameters in AudioRendererHost. It may happen that the audio parameters returned by a hardware device are invalid for Chromium. If the invalidity is caused only by having too many output channels, return the maximum allowed channels to make sure there are no serialization problems. This does not matter in practice, as streams are created using the input channels. BUG=568860 Committed: https://crrev.com/2d0a4f0bf564bbef1b001004a0d54b9845bc21ad Cr-Commit-Position: refs/heads/master@{#368316} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2d0a4f0bf564bbef1b001004a0d54b9845bc21ad Cr-Commit-Position: refs/heads/master@{#368316} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
