Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2836293002:
Introduce AudioRendererSink::IsOptimizedForHardwareParameters (Closed)
|
Created:
3 years, 8 months ago by flim-chromium Modified:
3 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIndicates whether it's preferable to use a source's hardware params. In most cases, the default value is true.
If WebAudio is attached, this is set to false such that the renderer instead uses the native sample rate, channel count, and layout without rebuffering.
BUG=693745
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2836293002
Cr-Commit-Position: refs/heads/master@{#473600}
Committed: https://chromium.googlesource.com/chromium/src/+/083e943d8ea4952a225355d1a40d945aa1e1c3f6
Patch Set 1 #
Total comments: 6
Patch Set 2 : Handle null sink and fix chromecast build #Patch Set 3 : Add comment to GetOutputDeviceInfo #Patch Set 4 : Revise comment #
Total comments: 2
Patch Set 5 : revert preferred_params and add OptimizedForHardwareParameters #
Total comments: 12
Patch Set 6 : Rename to IsOptimizedForHardwareParameters and set some to false #
Total comments: 2
Patch Set 7 : Lock |sink_lock_| before accessing |client_| #Patch Set 8 : rebase #Messages
Total messages: 68 (32 generated)
Description was changed from ========== Extend media::GetOutputDeviceInfo(AudioParameters& preferred_params) This is a proxy to having MediaElement provide channel count to WebAudio based on the number of channels in the media. When WebAudio isn't attached, preferred_params is ignored. Otherwise, the native sample rate, channel count, and layout is used without rebuffering. More background is available at https://codereview.chromium.org/2752323002/. BUG=693745 ========== to ========== Extend media::GetOutputDeviceInfo(AudioParameters& preferred_params) This is a proxy to having MediaElement provide channel count to WebAudio based on the number of channels in the media. When WebAudio isn't attached, preferred_params is ignored. Otherwise, the native sample rate, channel count, and layout is used without rebuffering. More background is available at https://codereview.chromium.org/2752323002/. BUG=693745 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
flim@chromium.org changed reviewers: + dalecurtis@chromium.org, olka@chromium.org
Hi Dale, Olka, could you PTAL? Thanks!
dalecurtis@chromium.org changed required reviewers: + olka@chromium.org
lgtm since it was my idea, but would like olka@ to do the final sign off.
The CQ bit was checked by flim@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... File media/base/audio_renderer_sink.h (right): https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... media/base/audio_renderer_sink.h:75: const AudioParameters& preferred_params = AudioParameters()) = 0; I understand why you are doing this for https://codereview.chromium.org/2752323002/. But what is the meaning of |preferred_params| in general? All the implementations except WebAudioSourceProviderImpl just ignore the parameters. So how would you describe the contract of this method? Also, what is expected to be returned if "the sink is not associated with any output device"? https://codereview.chromium.org/2836293002/diff/1/media/blink/webaudiosourcep... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2836293002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.cc:259: OutputDeviceInfo sink_info = sink_->GetOutputDeviceInfo(); What if |sink_| is nullptr?
https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... File media/base/audio_renderer_sink.h (right): https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... media/base/audio_renderer_sink.h:75: const AudioParameters& preferred_params = AudioParameters()) = 0; On 2017/04/26 14:27:54, o1ka wrote: > I understand why you are doing this for > https://codereview.chromium.org/2752323002/. > But what is the meaning of |preferred_params| in general? > All the implementations except WebAudioSourceProviderImpl just ignore the > parameters. So how would you describe the contract of this method? |preferred_params| would be the native parameters from the stream, which the implementation can choose to use instead of the sink params. If that makes sense, I can add this to the comments? > Also, what is > expected to be returned if "the sink is not associated with any output device"? It should return OUTPUT_DEVICE_STATUS_ERROR_INTERNAL - I've now fixed the case in WebAudioSourceProviderImpl. https://codereview.chromium.org/2836293002/diff/1/media/blink/webaudiosourcep... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2836293002/diff/1/media/blink/webaudiosourcep... media/blink/webaudiosourceprovider_impl.cc:259: OutputDeviceInfo sink_info = sink_->GetOutputDeviceInfo(); On 2017/04/26 14:27:54, o1ka wrote: > What if |sink_| is nullptr? Fixed, thanks for spotting this!
https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... File media/base/audio_renderer_sink.h (right): https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... media/base/audio_renderer_sink.h:75: const AudioParameters& preferred_params = AudioParameters()) = 0; On 2017/04/26 16:24:09, flim-chromium wrote: > On 2017/04/26 14:27:54, o1ka wrote: > > I understand why you are doing this for > > https://codereview.chromium.org/2752323002/. > > But what is the meaning of |preferred_params| in general? > > All the implementations except WebAudioSourceProviderImpl just ignore the > > parameters. So how would you describe the contract of this method? > > |preferred_params| would be the native parameters from the stream, which the > implementation can choose to use instead of the sink params. If that makes > sense, I can add this to the comments? This does not quite make sense to me in the context of AudioRendererSink interface. Sink implementation does not really choose to use anything, it uses parameters provided by the user in Initialize() method. And it's up to the user to render using native parameters or sink parameters, or whatever. Also, WebAudioSourceProviderImpl has SetClient() method, it can switch from using |sink_| to |client_| and back. Which makes GetOutputDeviceInfo() return rather a random result, doesn't it? > > > Also, what is > > expected to be returned if "the sink is not associated with any output > device"? > > It should return OUTPUT_DEVICE_STATUS_ERROR_INTERNAL - I've now fixed the case > in WebAudioSourceProviderImpl.
https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... File media/base/audio_renderer_sink.h (right): https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... media/base/audio_renderer_sink.h:75: const AudioParameters& preferred_params = AudioParameters()) = 0; On 2017/04/26 at 17:40:29, o1ka wrote: > On 2017/04/26 16:24:09, flim-chromium wrote: > > On 2017/04/26 14:27:54, o1ka wrote: > > > I understand why you are doing this for > > > https://codereview.chromium.org/2752323002/. > > > But what is the meaning of |preferred_params| in general? > > > All the implementations except WebAudioSourceProviderImpl just ignore the > > > parameters. So how would you describe the contract of this method? > > > > |preferred_params| would be the native parameters from the stream, which the > > implementation can choose to use instead of the sink params. If that makes > > sense, I can add this to the comments? > > This does not quite make sense to me in the context of AudioRendererSink interface. Sink implementation does not really choose to use anything, it uses parameters provided by the user in Initialize() method. And it's up to the user to render using native parameters or sink parameters, or whatever. > > Also, WebAudioSourceProviderImpl has SetClient() method, it can switch from using |sink_| to |client_| and back. Which makes GetOutputDeviceInfo() return rather a random result, doesn't it? > > > > > > Also, what is > > > expected to be returned if "the sink is not associated with any output > > device"? > > > > It should return OUTPUT_DEVICE_STATUS_ERROR_INTERNAL - I've now fixed the case > > in WebAudioSourceProviderImpl. That should be okay, we'll just end using the browser side conversion process instead then. Mostly we want the common case work well. I.e. attach WebAudio, then set src=. If we ever get around to implementing a device change API in the renderer we could use that here.
On 2017/04/26 18:25:59, DaleCurtis wrote: > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > File media/base/audio_renderer_sink.h (right): > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > media/base/audio_renderer_sink.h:75: const AudioParameters& preferred_params = > AudioParameters()) = 0; > On 2017/04/26 at 17:40:29, o1ka wrote: > > On 2017/04/26 16:24:09, flim-chromium wrote: > > > On 2017/04/26 14:27:54, o1ka wrote: > > > > I understand why you are doing this for > > > > https://codereview.chromium.org/2752323002/. > > > > But what is the meaning of |preferred_params| in general? > > > > All the implementations except WebAudioSourceProviderImpl just ignore the > > > > parameters. So how would you describe the contract of this method? > > > > > > |preferred_params| would be the native parameters from the stream, which the > > > implementation can choose to use instead of the sink params. If that makes > > > sense, I can add this to the comments? > > > > This does not quite make sense to me in the context of AudioRendererSink > interface. Sink implementation does not really choose to use anything, it uses > parameters provided by the user in Initialize() method. And it's up to the user > to render using native parameters or sink parameters, or whatever. Sorry, you're right the implementation itself still uses whatever params specified at Initialize(). It's just GetOutputDeviceInfo() that can choose which params to return, depending on whether the client_ is attached. > > > > Also, WebAudioSourceProviderImpl has SetClient() method, it can switch from > using |sink_| to |client_| and back. Which makes GetOutputDeviceInfo() return > rather a random result, doesn't it? > > > > > > > > > Also, what is > > > > expected to be returned if "the sink is not associated with any output > > > device"? > > > > > > It should return OUTPUT_DEVICE_STATUS_ERROR_INTERNAL - I've now fixed the > case > > > in WebAudioSourceProviderImpl. > > That should be okay, we'll just end using the browser side conversion process > instead then. Mostly we want the common case work well. I.e. attach WebAudio, > then set src=. If we ever get around to implementing a device change API in the > renderer we could use that here.
On 2017/04/27 12:04:39, flim-chromium wrote: > On 2017/04/26 18:25:59, DaleCurtis wrote: > > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > > File media/base/audio_renderer_sink.h (right): > > > > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > > media/base/audio_renderer_sink.h:75: const AudioParameters& preferred_params = > > AudioParameters()) = 0; > > On 2017/04/26 at 17:40:29, o1ka wrote: > > > On 2017/04/26 16:24:09, flim-chromium wrote: > > > > On 2017/04/26 14:27:54, o1ka wrote: > > > > > I understand why you are doing this for > > > > > https://codereview.chromium.org/2752323002/. > > > > > But what is the meaning of |preferred_params| in general? > > > > > All the implementations except WebAudioSourceProviderImpl just ignore > the > > > > > parameters. So how would you describe the contract of this method? > > > > > > > > |preferred_params| would be the native parameters from the stream, which > the > > > > implementation can choose to use instead of the sink params. If that makes > > > > sense, I can add this to the comments? > > > > > > This does not quite make sense to me in the context of AudioRendererSink > > interface. Sink implementation does not really choose to use anything, it uses > > parameters provided by the user in Initialize() method. And it's up to the > user > > to render using native parameters or sink parameters, or whatever. > > Sorry, you're right the implementation itself still uses whatever params > specified at Initialize(). It's just GetOutputDeviceInfo() that can choose which > params to return, depending on whether the client_ is attached. > Ok, let me sum up my thought here: (1) AudioRendererSink promise to render a stream with any parameters provided in Initialize(). This is a contract. (2) Depending on implementation, it still may be optimal for ARS to render a stream with some specific parameters. (3) As of now, physical device parameters are considered to be optimal for any sink implementation. (4) This physical device parameters are returned in GetOutputDeviceInfo(). (5) An ARS client can choose to optimize its code for ARS optimal parameters, or to make ARS to render at whatever parameters the client wants. --- (6) It turns out (3) is not always valid for WebAudioSourceProviderImpl implementation of ARS. In particular, when it’s rendering through WebAudio, any stream parameters are optimal, so we don’t want optimization on the client side. (7) However we do want to have optimization on the client side if WebAudioSourceProviderImpl renders directly to a sink. (8) So, we are changing OutputDeviceInfo returned in (4) into a mix of device id, device status and optimal parameters the sink would perform the best at (as opposite to output device parameters); and thus altering the meaning of ARS::GetOutputDeviceInfo(). --- Questions: Is (6) correct? Is (7) correct? And if yes - why? Is (8) what we want to do? If so, could you update GetOutputDeviceInfo() comment accordingly?
On 2017/04/26 18:25:59, DaleCurtis wrote: > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > File media/base/audio_renderer_sink.h (right): > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > media/base/audio_renderer_sink.h:75: const AudioParameters& preferred_params = > AudioParameters()) = 0; > On 2017/04/26 at 17:40:29, o1ka wrote: > > On 2017/04/26 16:24:09, flim-chromium wrote: > > > On 2017/04/26 14:27:54, o1ka wrote: > > > > I understand why you are doing this for > > > > https://codereview.chromium.org/2752323002/. > > > > But what is the meaning of |preferred_params| in general? > > > > All the implementations except WebAudioSourceProviderImpl just ignore the > > > > parameters. So how would you describe the contract of this method? > > > > > > |preferred_params| would be the native parameters from the stream, which the > > > implementation can choose to use instead of the sink params. If that makes > > > sense, I can add this to the comments? > > > > This does not quite make sense to me in the context of AudioRendererSink > interface. Sink implementation does not really choose to use anything, it uses > parameters provided by the user in Initialize() method. And it's up to the user > to render using native parameters or sink parameters, or whatever. > > > > Also, WebAudioSourceProviderImpl has SetClient() method, it can switch from > using |sink_| to |client_| and back. Which makes GetOutputDeviceInfo() return > rather a random result, doesn't it? > > > > > > > > > Also, what is > > > > expected to be returned if "the sink is not associated with any output > > > device"? > > > > > > It should return OUTPUT_DEVICE_STATUS_ERROR_INTERNAL - I've now fixed the > case > > > in WebAudioSourceProviderImpl. > > That should be okay, we'll just end using the browser side conversion process > instead then. Mostly we want the common case work well. I.e. attach WebAudio, > then set src=. If we ever get around to implementing a device change API in the > renderer we could use that here. Do you have any sense of how often it may happen? Why not to always use browser side conversion (or WebAudio conversion is case audio is directed through it)? - i.e. initialize the sink with the source parameters no matter what?
On 2017/04/27 13:09:38, o1ka wrote: > On 2017/04/27 12:04:39, flim-chromium wrote: > > On 2017/04/26 18:25:59, DaleCurtis wrote: > > > > > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > > > File media/base/audio_renderer_sink.h (right): > > > > > > > > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > > > media/base/audio_renderer_sink.h:75: const AudioParameters& preferred_params > = > > > AudioParameters()) = 0; > > > On 2017/04/26 at 17:40:29, o1ka wrote: > > > > On 2017/04/26 16:24:09, flim-chromium wrote: > > > > > On 2017/04/26 14:27:54, o1ka wrote: > > > > > > I understand why you are doing this for > > > > > > https://codereview.chromium.org/2752323002/. > > > > > > But what is the meaning of |preferred_params| in general? > > > > > > All the implementations except WebAudioSourceProviderImpl just ignore > > the > > > > > > parameters. So how would you describe the contract of this method? > > > > > > > > > > |preferred_params| would be the native parameters from the stream, which > > the > > > > > implementation can choose to use instead of the sink params. If that > makes > > > > > sense, I can add this to the comments? > > > > > > > > This does not quite make sense to me in the context of AudioRendererSink > > > interface. Sink implementation does not really choose to use anything, it > uses > > > parameters provided by the user in Initialize() method. And it's up to the > > user > > > to render using native parameters or sink parameters, or whatever. > > > > Sorry, you're right the implementation itself still uses whatever params > > specified at Initialize(). It's just GetOutputDeviceInfo() that can choose > which > > params to return, depending on whether the client_ is attached. > > > > > Ok, let me sum up my thought here: > > (1) AudioRendererSink promise to render a stream with any parameters provided in > Initialize(). This is a contract. > > (2) Depending on implementation, it still may be optimal for ARS to render a > stream with some specific parameters. > > (3) As of now, physical device parameters are considered to be optimal for any > sink implementation. > > (4) This physical device parameters are returned in GetOutputDeviceInfo(). > > (5) An ARS client can choose to optimize its code for ARS optimal parameters, or > to make ARS to render at whatever parameters the client wants. > > --- > > (6) It turns out (3) is not always valid for WebAudioSourceProviderImpl > implementation of ARS. In particular, when it’s rendering through WebAudio, any > stream parameters are optimal, so we don’t want optimization on the client side. > > (7) However we do want to have optimization on the client side if > WebAudioSourceProviderImpl renders directly to a sink. > > (8) So, we are changing OutputDeviceInfo returned in (4) into a mix of device > id, device status and optimal parameters the sink would perform the best at (as > opposite to output device parameters); and thus altering the meaning of > ARS::GetOutputDeviceInfo(). > > > --- Questions: > > Is (6) correct? Yes. For example, ambisonics files may have {9,11,16+} channels; we want to pass all of them to WebAudio, which will then take care of rendering them to e.g. stereo/whatever based on the user-defined graph. > Is (7) correct? And if yes - why? Yes, because that is the default behaviour today, e.g. there wouldn't be a WebAudio graph to handle rendering mismatching input/output channel counts. I'm not familiar with all the nuances of the default case, Dale can probably add more. > Is (8) what we want to do? Yes, I think (pls correct me if I'm wrong) |client_| and |sink_| are both valid "output" devices from the view of the renderer, so the broader meaning of ARS::GetOutputDeviceInfo() still holds even with this change. Ideally, WebAudio and WebAudioSourceProviderImpl can communicate the optimal params directly, such that this GetOutputDeviceInfo(preferred_params) proxy isn't needed. But apparently something like this would involve a spec change as per the discussions in the related CL. > If so, could you update GetOutputDeviceInfo() comment accordingly? Done, pls let me know if you think it needs more clarity.
On 2017/04/27 14:25:57, flim-chromium wrote: > On 2017/04/27 13:09:38, o1ka wrote: > > On 2017/04/27 12:04:39, flim-chromium wrote: > > > On 2017/04/26 18:25:59, DaleCurtis wrote: > > > > > > > > > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > > > > File media/base/audio_renderer_sink.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > > > > media/base/audio_renderer_sink.h:75: const AudioParameters& > preferred_params > > = > > > > AudioParameters()) = 0; > > > > On 2017/04/26 at 17:40:29, o1ka wrote: > > > > > On 2017/04/26 16:24:09, flim-chromium wrote: > > > > > > On 2017/04/26 14:27:54, o1ka wrote: > > > > > > > I understand why you are doing this for > > > > > > > https://codereview.chromium.org/2752323002/. > > > > > > > But what is the meaning of |preferred_params| in general? > > > > > > > All the implementations except WebAudioSourceProviderImpl just > ignore > > > the > > > > > > > parameters. So how would you describe the contract of this method? > > > > > > > > > > > > |preferred_params| would be the native parameters from the stream, > which > > > the > > > > > > implementation can choose to use instead of the sink params. If that > > makes > > > > > > sense, I can add this to the comments? > > > > > > > > > > This does not quite make sense to me in the context of AudioRendererSink > > > > interface. Sink implementation does not really choose to use anything, it > > uses > > > > parameters provided by the user in Initialize() method. And it's up to the > > > user > > > > to render using native parameters or sink parameters, or whatever. > > > > > > Sorry, you're right the implementation itself still uses whatever params > > > specified at Initialize(). It's just GetOutputDeviceInfo() that can choose > > which > > > params to return, depending on whether the client_ is attached. > > > > > > > > > Ok, let me sum up my thought here: > > > > (1) AudioRendererSink promise to render a stream with any parameters provided > in > > Initialize(). This is a contract. > > > > (2) Depending on implementation, it still may be optimal for ARS to render a > > stream with some specific parameters. > > > > (3) As of now, physical device parameters are considered to be optimal for any > > sink implementation. > > > > (4) This physical device parameters are returned in GetOutputDeviceInfo(). > > > > (5) An ARS client can choose to optimize its code for ARS optimal parameters, > or > > to make ARS to render at whatever parameters the client wants. > > > > --- > > > > (6) It turns out (3) is not always valid for WebAudioSourceProviderImpl > > implementation of ARS. In particular, when it’s rendering through WebAudio, > any > > stream parameters are optimal, so we don’t want optimization on the client > side. > > > > (7) However we do want to have optimization on the client side if > > WebAudioSourceProviderImpl renders directly to a sink. > > > > (8) So, we are changing OutputDeviceInfo returned in (4) into a mix of device > > id, device status and optimal parameters the sink would perform the best at > (as > > opposite to output device parameters); and thus altering the meaning of > > ARS::GetOutputDeviceInfo(). > > > > > > --- Questions: > > > > Is (6) correct? > Yes. For example, ambisonics files may have {9,11,16+} channels; we want to pass > all of them to WebAudio, which will then take care of rendering them to e.g. > stereo/whatever based on the user-defined graph. > > > Is (7) correct? And if yes - why? > Yes, because that is the default behaviour today, e.g. there wouldn't be a > WebAudio graph to handle rendering mismatching input/output channel counts. I'm > not familiar with all the nuances of the default case, Dale can probably add > more. > > > Is (8) what we want to do? > Yes, I think (pls correct me if I'm wrong) |client_| and |sink_| are both valid > "output" devices from the view of the renderer, so the broader meaning of > ARS::GetOutputDeviceInfo() still holds even with this change. Ideally, WebAudio > and WebAudioSourceProviderImpl can communicate the optimal params directly, such > that this GetOutputDeviceInfo(preferred_params) proxy isn't needed. But > apparently something like this would involve a spec change as per the > discussions in the related CL. "Output device" is a physical device audio will be rendered to, it has id and status. And "sink" is a node in the rendering graph. That's why this new version of GetOutputParameters looks a bit confusing to me: the meaning of the return result has changed. > > > If so, could you update GetOutputDeviceInfo() comment accordingly? > Done, pls let me know if you think it needs more clarity. I don't think ARS interface should mention WebAudio or any other implementation detail. It should explain how the client can interpret the result. In out case, the returned OutputDeviceInfo will contain "optimal parameters the sink would perform the best at", or something like that.
On 2017/04/27 17:32:03, o1ka wrote: > On 2017/04/27 14:25:57, flim-chromium wrote: > > On 2017/04/27 13:09:38, o1ka wrote: > > > On 2017/04/27 12:04:39, flim-chromium wrote: > > > > On 2017/04/26 18:25:59, DaleCurtis wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > > > > > File media/base/audio_renderer_sink.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2836293002/diff/1/media/base/audio_renderer_s... > > > > > media/base/audio_renderer_sink.h:75: const AudioParameters& > > preferred_params > > > = > > > > > AudioParameters()) = 0; > > > > > On 2017/04/26 at 17:40:29, o1ka wrote: > > > > > > On 2017/04/26 16:24:09, flim-chromium wrote: > > > > > > > On 2017/04/26 14:27:54, o1ka wrote: > > > > > > > > I understand why you are doing this for > > > > > > > > https://codereview.chromium.org/2752323002/. > > > > > > > > But what is the meaning of |preferred_params| in general? > > > > > > > > All the implementations except WebAudioSourceProviderImpl just > > ignore > > > > the > > > > > > > > parameters. So how would you describe the contract of this method? > > > > > > > > > > > > > > > |preferred_params| would be the native parameters from the stream, > > which > > > > the > > > > > > > implementation can choose to use instead of the sink params. If that > > > makes > > > > > > > sense, I can add this to the comments? > > > > > > > > > > > > This does not quite make sense to me in the context of > AudioRendererSink > > > > > interface. Sink implementation does not really choose to use anything, > it > > > uses > > > > > parameters provided by the user in Initialize() method. And it's up to > the > > > > user > > > > > to render using native parameters or sink parameters, or whatever. > > > > > > > > Sorry, you're right the implementation itself still uses whatever params > > > > specified at Initialize(). It's just GetOutputDeviceInfo() that can choose > > > which > > > > params to return, depending on whether the client_ is attached. > > > > > > > > > > > > > Ok, let me sum up my thought here: > > > > > > (1) AudioRendererSink promise to render a stream with any parameters > provided > > in > > > Initialize(). This is a contract. > > > > > > (2) Depending on implementation, it still may be optimal for ARS to render a > > > stream with some specific parameters. > > > > > > (3) As of now, physical device parameters are considered to be optimal for > any > > > sink implementation. > > > > > > (4) This physical device parameters are returned in GetOutputDeviceInfo(). > > > > > > (5) An ARS client can choose to optimize its code for ARS optimal > parameters, > > or > > > to make ARS to render at whatever parameters the client wants. > > > > > > --- > > > > > > (6) It turns out (3) is not always valid for WebAudioSourceProviderImpl > > > implementation of ARS. In particular, when it’s rendering through WebAudio, > > any > > > stream parameters are optimal, so we don’t want optimization on the client > > side. > > > > > > (7) However we do want to have optimization on the client side if > > > WebAudioSourceProviderImpl renders directly to a sink. > > > > > > (8) So, we are changing OutputDeviceInfo returned in (4) into a mix of > device > > > id, device status and optimal parameters the sink would perform the best at > > (as > > > opposite to output device parameters); and thus altering the meaning of > > > ARS::GetOutputDeviceInfo(). > > > > > > > > > --- Questions: > > > > > > Is (6) correct? > > Yes. For example, ambisonics files may have {9,11,16+} channels; we want to > pass > > all of them to WebAudio, which will then take care of rendering them to e.g. > > stereo/whatever based on the user-defined graph. > > > > > Is (7) correct? And if yes - why? > > Yes, because that is the default behaviour today, e.g. there wouldn't be a > > WebAudio graph to handle rendering mismatching input/output channel counts. > I'm > > not familiar with all the nuances of the default case, Dale can probably add > > more. > > > > > Is (8) what we want to do? > > Yes, I think (pls correct me if I'm wrong) |client_| and |sink_| are both > valid > > "output" devices from the view of the renderer, so the broader meaning of > > ARS::GetOutputDeviceInfo() still holds even with this change. Ideally, > WebAudio > > and WebAudioSourceProviderImpl can communicate the optimal params directly, > such > > that this GetOutputDeviceInfo(preferred_params) proxy isn't needed. But > > apparently something like this would involve a spec change as per the > > discussions in the related CL. > > "Output device" is a physical device audio will be rendered to, it has id and > status. > And "sink" is a node in the rendering graph. That's why this new version of > GetOutputParameters looks a bit confusing to me: the meaning of the return > result has changed. > > > > > > If so, could you update GetOutputDeviceInfo() comment accordingly? > > Done, pls let me know if you think it needs more clarity. > > I don't think ARS interface should mention WebAudio or any other implementation > detail. It should explain how the client can interpret the result. > In out case, the returned OutputDeviceInfo will contain "optimal parameters the > sink would perform the best at", or something like that. Ah ok, thanks for the explanation, I've revised the comment to try explain it better - wdyt?
Should we rename the method GetPreferredDeviceInfo?
On 2017/04/27 23:30:30, DaleCurtis wrote: > Should we rename the method GetPreferredDeviceInfo? It's not really about renaming. I'm sorry, I know it's an easy change on the way to https://codereview.chromium.org/2752323002/, and it looks neat at a first glance, and we've done such things before (though they are quite confusing and add to code complexity). But when I look closer it's falling apart for me: I don't see a good contract. Throughout the whole codebase we interpret ARS::GetOutputDeviceInfo result as device/hardware parameters. For example https://cs.chromium.org/chromium/src/content/renderer/media/track_audio_rende... And we also build other interfaces on top of it, with the same interpretation, for example: https://cs.chromium.org/chromium/src/content/public/renderer/media_stream_aud... And here is the long call stack implying we are getting physical device parameters: https://cs.chromium.org/chromium/src/content/renderer/media/audio_device_fact... So we need to be able to get those hardware parameters. The last description Felicia proposed ("Returns current output device id and status with the optimal parameters given the sink's internal state. These may be the output device's parameters or the given |preferred_params|") would work around this problem (though an explanation is still needed regarding empty parameters passed to it), but this contract is hard to track and it will be violated sooner or later: 1) Every implementation needs to check if empty params are passed in. (BTW WebAudioSourceProviderImpl::GetOutputDeviceInfo already does not do it - a bug in this CL which all of us missed). 2) Another example: SinkA ownes |sink_|, and does rebuffering before rendering, so it has some |output_params_| for that: OutputDeviceInfo SinkA::GetOutputDeviceInfo(const AudioParameters& params) { return sink_->GetOutputDeviceInfo(output_params_); } - the contract is violated, but it's really hard to spot in the review. Dale, could you answer my questions in #15 and #16?
(I referred this issues in the comment above) https://codereview.chromium.org/2836293002/diff/60001/media/base/audio_render... File media/base/audio_renderer_sink.h (right): https://codereview.chromium.org/2836293002/diff/60001/media/base/audio_render... media/base/audio_renderer_sink.h:77: const AudioParameters& preferred_params = AudioParameters()) = 0; What happens in the default case? What if |preferred_params| are not default AudioParameters(), but invalid? https://codereview.chromium.org/2836293002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2836293002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:266: preferred_params); What if preferred_params are empty/invalid?
Another example of a problem with changing GetOutputDeviceInfo meaning: https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.h?ty...
WDYT of introducing something like bool ARS::OptimizedForHardwareParameters() // Returns true if a source with HW parameters is preferable. with default implementation returning true. and modifying renderer to make decision basing on its result? (and we need to change the meaning of https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.h?ty...)
On 2017/04/28 at 10:14:58, olka wrote: > WDYT of introducing something like > > bool ARS::OptimizedForHardwareParameters() // Returns true if a source with HW parameters is preferable. > with default implementation returning true. > > and modifying renderer to make decision basing on its result? > > (and we need to change the meaning of https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.h?ty...) I think this sgtm and is simpler to implement. Good suggestion! We don't need the full flexibility that we do for preferred on browser side. Also, that comment is actually just wrong, so we can change it :)
The CQ bit was checked by flim@chromium.org to run a CQ dry run
On 2017/04/28 18:29:53, DaleCurtis wrote: > On 2017/04/28 at 10:14:58, olka wrote: > > WDYT of introducing something like > > > > bool ARS::OptimizedForHardwareParameters() // Returns true if a source with HW > parameters is preferable. > > with default implementation returning true. > > > > and modifying renderer to make decision basing on its result? > > > > (and we need to change the meaning of > https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.h?ty...) > > I think this sgtm and is simpler to implement. Good suggestion! We don't need > the full flexibility that we do for preferred on browser side. Also, that > comment is actually just wrong, so we can change it :) Thanks, that makes a lot of sense. I've implemented the change, could you please take another look?
Dry run: 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
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm % naming https://codereview.chromium.org/2836293002/diff/80001/chromecast/media/servic... File chromecast/media/service/cast_mojo_media_client.cc (right): https://codereview.chromium.org/2836293002/diff/80001/chromecast/media/servic... chromecast/media/service/cast_mojo_media_client.cc:45: bool OptimizedForHardwareParameter() final { Line above and below. https://codereview.chromium.org/2836293002/diff/80001/media/audio/clockless_a... File media/audio/clockless_audio_sink.cc (right): https://codereview.chromium.org/2836293002/diff/80001/media/audio/clockless_a... media/audio/clockless_audio_sink.cc:137: bool ClocklessAudioSink::OptimizedForHardwareParameters() { This is actually false; we don't care. https://codereview.chromium.org/2836293002/diff/80001/media/audio/null_audio_... File media/audio/null_audio_sink.cc (right): https://codereview.chromium.org/2836293002/diff/80001/media/audio/null_audio_... media/audio/null_audio_sink.cc:83: return true; Ditto, false. https://codereview.chromium.org/2836293002/diff/80001/media/base/audio_render... File media/base/audio_renderer_sink.h (right): https://codereview.chromium.org/2836293002/diff/80001/media/base/audio_render... media/base/audio_renderer_sink.h:77: virtual bool OptimizedForHardwareParameters() = 0; Sorry, this should be IsOptimizedForHardwareParameters()? https://codereview.chromium.org/2836293002/diff/80001/media/base/fake_audio_r... File media/base/fake_audio_renderer_sink.cc (right): https://codereview.chromium.org/2836293002/diff/80001/media/base/fake_audio_r... media/base/fake_audio_renderer_sink.cc:73: return true; false. https://codereview.chromium.org/2836293002/diff/80001/media/base/mock_audio_r... File media/base/mock_audio_renderer_sink.cc (right): https://codereview.chromium.org/2836293002/diff/80001/media/base/mock_audio_r... media/base/mock_audio_renderer_sink.cc:52: return true; false?
The CQ bit was checked by flim@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #6 (id:100001) has been deleted
Description was changed from ========== Extend media::GetOutputDeviceInfo(AudioParameters& preferred_params) This is a proxy to having MediaElement provide channel count to WebAudio based on the number of channels in the media. When WebAudio isn't attached, preferred_params is ignored. Otherwise, the native sample rate, channel count, and layout is used without rebuffering. More background is available at https://codereview.chromium.org/2752323002/. BUG=693745 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Indicates whether it's preferable to use a source's hardware params. The default value is true. If WebAudio is attached, this is set to false such that the renderer instead uses the native sample rate, channel count, and layout without rebuffering. BUG=693745 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
Description was changed from ========== Indicates whether it's preferable to use a source's hardware params. The default value is true. If WebAudio is attached, this is set to false such that the renderer instead uses the native sample rate, channel count, and layout without rebuffering. BUG=693745 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Indicates whether it's preferable to use a source's hardware params. In most cases, the default value is true. If WebAudio is attached, this is set to false such that the renderer instead uses the native sample rate, channel count, and layout without rebuffering. BUG=693745 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
Thanks for taking a look! I've addressed the comments and also updated the CL subject/description. https://codereview.chromium.org/2836293002/diff/80001/chromecast/media/servic... File chromecast/media/service/cast_mojo_media_client.cc (right): https://codereview.chromium.org/2836293002/diff/80001/chromecast/media/servic... chromecast/media/service/cast_mojo_media_client.cc:45: bool OptimizedForHardwareParameter() final { On 2017/05/01 22:55:08, DaleCurtis_OOO_May_5_To_May23 wrote: > Line above and below. Done. https://codereview.chromium.org/2836293002/diff/80001/media/audio/clockless_a... File media/audio/clockless_audio_sink.cc (right): https://codereview.chromium.org/2836293002/diff/80001/media/audio/clockless_a... media/audio/clockless_audio_sink.cc:137: bool ClocklessAudioSink::OptimizedForHardwareParameters() { On 2017/05/01 22:55:08, DaleCurtis_OOO_May_5_To_May23 wrote: > This is actually false; we don't care. Done. https://codereview.chromium.org/2836293002/diff/80001/media/audio/null_audio_... File media/audio/null_audio_sink.cc (right): https://codereview.chromium.org/2836293002/diff/80001/media/audio/null_audio_... media/audio/null_audio_sink.cc:83: return true; On 2017/05/01 22:55:08, DaleCurtis_OOO_May_5_To_May23 wrote: > Ditto, false. Done. https://codereview.chromium.org/2836293002/diff/80001/media/base/audio_render... File media/base/audio_renderer_sink.h (right): https://codereview.chromium.org/2836293002/diff/80001/media/base/audio_render... media/base/audio_renderer_sink.h:77: virtual bool OptimizedForHardwareParameters() = 0; On 2017/05/01 22:55:08, DaleCurtis_OOO_May_5_To_May23 wrote: > Sorry, this should be IsOptimizedForHardwareParameters()? Done. https://codereview.chromium.org/2836293002/diff/80001/media/base/fake_audio_r... File media/base/fake_audio_renderer_sink.cc (right): https://codereview.chromium.org/2836293002/diff/80001/media/base/fake_audio_r... media/base/fake_audio_renderer_sink.cc:73: return true; On 2017/05/01 22:55:08, DaleCurtis_OOO_May_5_To_May23 wrote: > false. This needs to be true for several tests in AudioRendererImplTest else the wrong path in the amended media/renderers/audio_renderer_impl.cc will be taken. https://codereview.chromium.org/2836293002/diff/80001/media/base/mock_audio_r... File media/base/mock_audio_renderer_sink.cc (right): https://codereview.chromium.org/2836293002/diff/80001/media/base/mock_audio_r... media/base/mock_audio_renderer_sink.cc:52: return true; On 2017/05/01 22:55:08, DaleCurtis_OOO_May_5_To_May23 wrote: > false? Done.
The CQ bit was checked by flim@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good, one question on locking: https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.cc:262: return client_ ? false : true; Access to |client_| is guarded everywhere by |sink_lock_|. Is data race intentional here? Or is it not possible? I would prefer to avoid locking here if we can, because we've had troubles with that lock before (around l.119). But then we need a comment explaining what's going on.
https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.cc:262: return client_ ? false : true; On 2017/05/16 10:31:19, o1ka wrote: > Access to |client_| is guarded everywhere by |sink_lock_|. > Is data race intentional here? Or is it not possible? > I would prefer to avoid locking here if we can, because we've had troubles with > that lock before (around l.119). But then we need a comment explaining what's > going on. Thanks for spotting this! I think it would actually be safer to add the lock; I can't see a guarantee that there won't be a race with SetClient. If that also makes sense to you, I'll upload the change.
On 2017/05/16 11:04:05, flim-chromium wrote: > https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... > File media/blink/webaudiosourceprovider_impl.cc (right): > > https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... > media/blink/webaudiosourceprovider_impl.cc:262: return client_ ? false : true; > On 2017/05/16 10:31:19, o1ka wrote: > > Access to |client_| is guarded everywhere by |sink_lock_|. > > Is data race intentional here? Or is it not possible? > > I would prefer to avoid locking here if we can, because we've had troubles > with > > that lock before (around l.119). But then we need a comment explaining what's > > going on. > > Thanks for spotting this! I think it would actually be safer to add the lock; I > can't see a guarantee that there won't be a race with SetClient. If that also > makes sense to you, I'll upload the change. sgtm
On 2017/05/16 14:28:49, o1ka wrote: > On 2017/05/16 11:04:05, flim-chromium wrote: > > > https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... > > File media/blink/webaudiosourceprovider_impl.cc (right): > > > > > https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... > > media/blink/webaudiosourceprovider_impl.cc:262: return client_ ? false : true; > > On 2017/05/16 10:31:19, o1ka wrote: > > > Access to |client_| is guarded everywhere by |sink_lock_|. > > > Is data race intentional here? Or is it not possible? > > > I would prefer to avoid locking here if we can, because we've had troubles > > with > > > that lock before (around l.119). But then we need a comment explaining > what's > > > going on. > > > > Thanks for spotting this! I think it would actually be safer to add the lock; > I > > can't see a guarantee that there won't be a race with SetClient. If that also > > makes sense to you, I'll upload the change. > > sgtm done
On 2017/05/16 14:31:49, flim-chromium wrote: > On 2017/05/16 14:28:49, o1ka wrote: > > On 2017/05/16 11:04:05, flim-chromium wrote: > > > > > > https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... > > > File media/blink/webaudiosourceprovider_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... > > > media/blink/webaudiosourceprovider_impl.cc:262: return client_ ? false : > true; > > > On 2017/05/16 10:31:19, o1ka wrote: > > > > Access to |client_| is guarded everywhere by |sink_lock_|. > > > > Is data race intentional here? Or is it not possible? > > > > I would prefer to avoid locking here if we can, because we've had troubles > > > with > > > > that lock before (around l.119). But then we need a comment explaining > > what's > > > > going on. > > > > > > Thanks for spotting this! I think it would actually be safer to add the > lock; > > I > > > can't see a guarantee that there won't be a race with SetClient. If that > also > > > makes sense to you, I'll upload the change. > > > > sgtm > > done lgtm - thanks!
On 2017/05/16 14:50:58, o1ka wrote: > On 2017/05/16 14:31:49, flim-chromium wrote: > > On 2017/05/16 14:28:49, o1ka wrote: > > > On 2017/05/16 11:04:05, flim-chromium wrote: > > > > > > > > > > https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... > > > > File media/blink/webaudiosourceprovider_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2836293002/diff/120001/media/blink/webaudioso... > > > > media/blink/webaudiosourceprovider_impl.cc:262: return client_ ? false : > > true; > > > > On 2017/05/16 10:31:19, o1ka wrote: > > > > > Access to |client_| is guarded everywhere by |sink_lock_|. > > > > > Is data race intentional here? Or is it not possible? > > > > > I would prefer to avoid locking here if we can, because we've had > troubles > > > > with > > > > > that lock before (around l.119). But then we need a comment explaining > > > what's > > > > > going on. > > > > > > > > Thanks for spotting this! I think it would actually be safer to add the > > lock; > > > I > > > > can't see a guarantee that there won't be a race with SetClient. If that > > also > > > > makes sense to you, I'll upload the change. > > > > > > sgtm > > > > done > > lgtm - thanks! Thanks for the review!
The CQ bit was checked by flim@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/2836293002/#ps140001 (title: "Lock |sink_lock_| before accessing |client_|")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
flim@chromium.org changed reviewers: + slan@chromium.org
+slan for chromecast OWNERS review; could you PTAL?
On 2017/05/16 15:22:28, flim-chromium wrote: > +slan for chromecast OWNERS review; could you PTAL? friendly ping :)
On 2017/05/18 20:17:47, flim-chromium wrote: > On 2017/05/16 15:22:28, flim-chromium wrote: > > +slan for chromecast OWNERS review; could you PTAL? > > friendly ping :) cast lgtm!
The CQ bit was checked by flim@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 flim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org, dalecurtis@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2836293002/#ps160001 (title: "rebase")
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": 160001, "attempt_start_ts": 1495466096899000,
"parent_rev": "226bebaa36af71678e3a8f236258c769b868f490", "commit_rev":
"083e943d8ea4952a225355d1a40d945aa1e1c3f6"}
Message was sent while issue was closed.
Description was changed from ========== Indicates whether it's preferable to use a source's hardware params. In most cases, the default value is true. If WebAudio is attached, this is set to false such that the renderer instead uses the native sample rate, channel count, and layout without rebuffering. BUG=693745 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Indicates whether it's preferable to use a source's hardware params. In most cases, the default value is true. If WebAudio is attached, this is set to false such that the renderer instead uses the native sample rate, channel count, and layout without rebuffering. BUG=693745 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2836293002 Cr-Commit-Position: refs/heads/master@{#473600} Committed: https://chromium.googlesource.com/chromium/src/+/083e943d8ea4952a225355d1a40d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/083e943d8ea4952a225355d1a40d... |
