Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533443002:
fallback to null sink in WebAudioSourceProvider::Initialize() + UMA stats for device status (Closed)
|
Created:
4 years ago by o1ka Modified:
4 years ago CC:
apacible+watch_chromium.org, asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFallback to null sink moved from WebMediaPlayer to WebAudioSourceProvider::Initialize(), so that we do it on media thread.
UMA stats for AudioOutputDevice status and for fallbacks to null sink.
BUG=668506, 668201, 663546, 626862, 673291
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
Committed: https://crrev.com/e4ba2ed8354d01411848143efbe8d17b41e1853b
Cr-Commit-Position: refs/heads/master@{#436614}
Patch Set 1 #
Total comments: 12
Patch Set 2 : review comments addressed #
Total comments: 1
Patch Set 3 : Moving fallback to null sink into WASP::Initialize() #Patch Set 4 : content unittest fix #
Total comments: 20
Patch Set 5 : review comments addressed #Messages
Total messages: 59 (31 generated)
Description was changed from ========== UMA stats for AudioOutputDevice status and for WebMediaPlayer fallbacks to null sink BUG=668506 ========== to ========== UMA stats for AudioOutputDevice status and for WebMediaPlayer fallbacks to null sink BUG=668506 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: + guidou@chromium.org
PTAL https://codereview.chromium.org/2533443002/diff/1/media/base/output_device_in... File media/base/output_device_info.h (right): https://codereview.chromium.org/2533443002/diff/1/media/base/output_device_in... media/base/output_device_info.h:26: OUTPUT_DEVICE_STATUS_MAX = OUTPUT_DEVICE_STATUS_ERROR_INTERNAL This is the only way to make presubmis pass :/ https://bugs.chromium.org/p/chromium/issues/detail?id=668527
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...
lgtm, take a look at the nit. https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:268: if (params.audio_renderer_sink().get()) { nit: Should a stat be collected if params.audio_renderer_sink() is null?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
On 2016/11/24 17:30:09, Guido Urdaneta wrote: > lgtm, take a look at the nit. > > https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... > media/blink/webmediaplayer_impl.cc:268: if (params.audio_renderer_sink().get()) > { > nit: Should a stat be collected if params.audio_renderer_sink() is null? Not sure - this won't give us any extra info on fallback reasons - and that's what we are investigating.
olka@chromium.org changed reviewers: + dalecurtis@chromium.org
Dale - PTAL
> > nit: Should a stat be collected if params.audio_renderer_sink() is null? > > Not sure - this won't give us any extra info on fallback reasons - and that's > what we are investigating. If params.audio_renderer_sink() is null that's another reason to fall back to NullAudioSink that you are not counting. If it is not supposed to be null, then don't check if it's null and let it crash if it's null by mistake and fix the bug when the crash reports come. But if it can be null, then I think you should count those cases too.
On 2016/11/24 18:26:04, Guido Urdaneta wrote: > > > nit: Should a stat be collected if params.audio_renderer_sink() is null? > > > > Not sure - this won't give us any extra info on fallback reasons - and that's > > what we are investigating. > > If params.audio_renderer_sink() is null that's another reason to fall back to > NullAudioSink that you are not counting. > If it is not supposed to be null, then don't check if it's null and let it crash > if it's null by mistake and fix the bug when the crash reports come. > But if it can be null, then I think you should count those cases too. Ok, I need to check where no sink may come from. I thought it was an intended normal behavior. But still, we are not really looking into muted playback because of no sink - it's been there for quite a while. We are looking into the reasons why the change to use null sink if output device has errors resulted in reduction of media errors, I.e. what errors we are making by null sink.
On 2016/11/24 18:34:59, o1ka wrote: > On 2016/11/24 18:26:04, Guido Urdaneta wrote: > > > > nit: Should a stat be collected if params.audio_renderer_sink() is null? > > > > > > Not sure - this won't give us any extra info on fallback reasons - and > that's > > > what we are investigating. > > > > If params.audio_renderer_sink() is null that's another reason to fall back to > > NullAudioSink that you are not counting. > > If it is not supposed to be null, then don't check if it's null and let it > crash > > if it's null by mistake and fix the bug when the crash reports come. > > But if it can be null, then I think you should count those cases too. > > Ok, I need to check where no sink may come from. I thought it was an intended > normal behavior. > But still, we are not really looking into muted playback because of no sink - > it's been there for quite a while. > We are looking into the reasons why the change to > use null sink if output device has errors resulted in reduction of media errors, > I.e. what errors we are making by null sink. The sink is initialized here https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?ty... So, the only case it's a nullptr is a custom audio device factory (I'm aware of its usage in unit tests only). Which means our metric cover all the interesting cases. WDYT?
On 2016/11/25 08:35:13, o1ka wrote: > On 2016/11/24 18:34:59, o1ka wrote: > > On 2016/11/24 18:26:04, Guido Urdaneta wrote: > > > > > nit: Should a stat be collected if params.audio_renderer_sink() is null? > > > > > > > > Not sure - this won't give us any extra info on fallback reasons - and > > that's > > > > what we are investigating. > > > > > > If params.audio_renderer_sink() is null that's another reason to fall back > to > > > NullAudioSink that you are not counting. > > > If it is not supposed to be null, then don't check if it's null and let it > > crash > > > if it's null by mistake and fix the bug when the crash reports come. > > > But if it can be null, then I think you should count those cases too. > > > > Ok, I need to check where no sink may come from. I thought it was an intended > > normal behavior. > > But still, we are not really looking into muted playback because of no sink - > > it's been there for quite a while. > > We are looking into the reasons why the change to > > use null sink if output device has errors resulted in reduction of media > errors, > > I.e. what errors we are making by null sink. > > The sink is initialized here > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?ty... > So, the only case it's a nullptr is a custom audio device factory (I'm aware of > its usage in unit tests only). Which means our metric cover all the interesting > cases. WDYT? SGTM
https://codereview.chromium.org/2533443002/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2533443002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:331: UMA_HISTOGRAM_ENUMERATION("Media.Audio.Render.OutputDeviceStatus", Isn't this always going to be timed out? https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:270: params.audio_renderer_sink()->GetOutputDeviceInfo().device_status(); In retrospect we probably shouldn't do this since it blocks the main render thread. Instead we shouldn't construct the audio_source_provider_ until we know if the sink is any good and that determination should be made on another thread. This is no worse than we've had in the past, but guidou@ did a lot of work to avoid us having this code run on the main thread, it's a shame to put it back. https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:271: UMA_HISTOGRAM_ENUMERATION("Media.AudioSinkStatus", device_status, Maybe Media.WebMediaPlayer.SinkStatus? https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:279: LOG(WARNING) << "Output device error, falling back to null sink"; This should be a MEDIA_LOG() so it shows up in chrome://media-internals
On a training today, don't have time to address comments. So, these are just answers to the questions. Thanks! https://codereview.chromium.org/2533443002/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2533443002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:331: UMA_HISTOGRAM_ENUMERATION("Media.Audio.Render.OutputDeviceStatus", On 2016/11/28 18:58:17, DaleCurtis wrote: > Isn't this always going to be timed out? No, we are here when it's the first time we received "DeviceAuthorized" - no matter timed out or not. https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:270: params.audio_renderer_sink()->GetOutputDeviceInfo().device_status(); On 2016/11/28 18:58:17, DaleCurtis wrote: > In retrospect we probably shouldn't do this since it blocks the main render > thread. Instead we shouldn't construct the audio_source_provider_ until we know > if the sink is any good and that determination should be made on another thread. > > This is no worse than we've had in the past, but guidou@ did a lot of work to > avoid us having this code run on the main thread, it's a shame to put it back. It's not new in this CL, we have already been doing it at l.272 (It's a change Guido introduced recently to fallback to null sink)
https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:270: params.audio_renderer_sink()->GetOutputDeviceInfo().device_status(); On 2016/11/29 at 13:55:58, o1ka wrote: > On 2016/11/28 18:58:17, DaleCurtis wrote: > > In retrospect we probably shouldn't do this since it blocks the main render > > thread. Instead we shouldn't construct the audio_source_provider_ until we know > > if the sink is any good and that determination should be made on another thread. > > > > This is no worse than we've had in the past, but guidou@ did a lot of work to > > avoid us having this code run on the main thread, it's a shame to put it back. > > It's not new in this CL, we have already been doing it at l.272 (It's a change Guido introduced recently to fallback to null sink) I know, sorry I wasn't clear, I was saying it's not great that we added it in the previous CL.
Description was changed from ========== UMA stats for AudioOutputDevice status and for WebMediaPlayer fallbacks to null sink BUG=668506 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 ========== UMA stats for AudioOutputDevice status and for WebMediaPlayer fallbacks to null sink BUG=668506,668201 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 ==========
Description was changed from ========== UMA stats for AudioOutputDevice status and for WebMediaPlayer fallbacks to null sink BUG=668506,668201 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 ========== UMA stats for AudioOutputDevice status and for WebMediaPlayer fallbacks to null sink BUG=668506,668201,663546 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 ==========
PTAL https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:270: params.audio_renderer_sink()->GetOutputDeviceInfo().device_status(); On 2016/11/29 20:32:02, DaleCurtis wrote: > On 2016/11/29 at 13:55:58, o1ka wrote: > > On 2016/11/28 18:58:17, DaleCurtis wrote: > > > In retrospect we probably shouldn't do this since it blocks the main render > > > thread. Instead we shouldn't construct the audio_source_provider_ until we > know > > > if the sink is any good and that determination should be made on another > thread. > > > > > > This is no worse than we've had in the past, but guidou@ did a lot of work > to > > > avoid us having this code run on the main thread, it's a shame to put it > back. > > > > It's not new in this CL, we have already been doing it at l.272 (It's a change > Guido introduced recently to fallback to null sink) > > I know, sorry I wasn't clear, I was saying it's not great that we added it in > the previous CL. Ah I see. We can probably defer fallback until WebAudioSourceProviderImpl::Initialize, but I'm not sure how it all will work if WebAudioSourceProviderImpl::GetOutputDeviceInfo() is called prior to initialization. We can remove it now, or figure out how to make it happen on media thread and address it in a separate CL. WDYT? https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:271: UMA_HISTOGRAM_ENUMERATION("Media.AudioSinkStatus", device_status, On 2016/11/28 18:58:17, DaleCurtis wrote: > Maybe Media.WebMediaPlayer.SinkStatus? Done. https://codereview.chromium.org/2533443002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:279: LOG(WARNING) << "Output device error, falling back to null sink"; On 2016/11/28 18:58:17, DaleCurtis wrote: > This should be a MEDIA_LOG() so it shows up in chrome://media-internals Done.
That's a good point, we can trivially handle this during Initialize(); we should go ahead and do that in this CL since otherwise this block of code will just be deleted. I'd do the following: 1. Remove the null-check here which injects the NullAudioSink(). 2. Change WASP::WASP() to generate the NullAudioSink() based on null parameter, log this as a Media.WebAudioSourceProvider.IsUsingNullAudioSink boolean. 3. Change WASP::Initialize() to check the sink status and switch to a NullAudioSink() if necessary. Also log this to the same boolean (avoid double-counting if it's already a null sink). Note: Also remember that Initialize() can be called many times now since we allow sinks to reinitialize. WDYT? https://codereview.chromium.org/2533443002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2533443002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:268: if (params.audio_renderer_sink().get()) { No need for .get().
On 2016/11/30 22:10:38, DaleCurtis wrote: > That's a good point, we can trivially handle this during Initialize(); we should > go ahead and do that in this CL since otherwise this block of code will just be > deleted. I'd do the following: > > 1. Remove the null-check here which injects the NullAudioSink(). > 2. Change WASP::WASP() to generate the NullAudioSink() based on null parameter, > log this as a Media.WebAudioSourceProvider.IsUsingNullAudioSink boolean. > 3. Change WASP::Initialize() to check the sink status and switch to a > NullAudioSink() if necessary. Also log this to the same boolean (avoid > double-counting if it's already a null sink). > > Note: Also remember that Initialize() can be called many times now since we > allow sinks to reinitialize. WDYT? > Sounds good. Unit tests are taking some time - will upload next patch tomorrow. > https://codereview.chromium.org/2533443002/diff/20001/media/blink/webmediapla... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2533443002/diff/20001/media/blink/webmediapla... > media/blink/webmediaplayer_impl.cc:268: if (params.audio_renderer_sink().get()) > { > No need for .get().
Description was changed from ========== UMA stats for AudioOutputDevice status and for WebMediaPlayer fallbacks to null sink BUG=668506,668201,663546 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 ========== Fallback to null sink moved from WebMediaPlayer to WebAudioSourceProvider::Initialize(), so that we do it on media thread. UMA stats for AudioOutputDevice status and for fallbacks to null sink. BUG=668506,668201,663546,626862 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...
PTAL
olka@chromium.org changed reviewers: + dcheng@chromium.org, holte@chromium.org
holte@ PTAL at histograms; dcheng@ PTAL at a trivial change in content/common/media/audio_messages.h caused by rename in media/base/output_device_info.h (done to satisfy UMA presubmit check, see https://bugs.chromium.org/p/chromium/issues/detail?id=668527)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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...
Fix for content unit test compilation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rs lgtm for ipc https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:108: media_log_(media_log), Nit: std::move()
lgtm, thanks for cleaning this up! https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:193: UMA_HISTOGRAM_ENUMERATION("Media.WebMediaPlayer.SinkStatus", device_status, WebAudioSourceProvider? https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:51: explicit WebAudioSourceProviderImpl( Drop explicit now. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:52: const scoped_refptr<SwitchableAudioRendererSink>& sink, Convert to non const& or flip media_log to const&? https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl_unittest.cc (right): https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:30: const scoped_refptr<SwitchableAudioRendererSink>& sink) non const& for new code. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:44: }; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:46: enum WaspSinkStatus { WASP_SINK_OK = 0, WASP_SINK_ERROR, WASP_SINK_NULL }; I prefer enum class these days (no = 0), but up to you. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:51: return new MockAudioRendererSink((status == WASP_SINK_OK) No uneccesary parens. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:71: expected_sink_((GetParam() == WASP_SINK_OK) Drop unnecessary parens. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:142: if (GetParam() == WASP_SINK_ERROR) { Don't need {} for single line if, but up to you.
histograms lgtm
Thanks everybody for the review! https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:108: media_log_(media_log), On 2016/12/02 18:25:03, dcheng wrote: > Nit: std::move() Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.cc:193: UMA_HISTOGRAM_ENUMERATION("Media.WebMediaPlayer.SinkStatus", device_status, On 2016/12/02 18:28:56, DaleCurtis wrote: > WebAudioSourceProvider? Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl.h (right): https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:51: explicit WebAudioSourceProviderImpl( On 2016/12/02 18:28:56, DaleCurtis wrote: > Drop explicit now. Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl.h:52: const scoped_refptr<SwitchableAudioRendererSink>& sink, On 2016/12/02 18:28:57, DaleCurtis wrote: > Convert to non const& or flip media_log to const&? Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... File media/blink/webaudiosourceprovider_impl_unittest.cc (right): https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:30: const scoped_refptr<SwitchableAudioRendererSink>& sink) On 2016/12/02 18:28:57, DaleCurtis wrote: > non const& for new code. Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:44: }; On 2016/12/02 18:28:57, DaleCurtis wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:46: enum WaspSinkStatus { WASP_SINK_OK = 0, WASP_SINK_ERROR, WASP_SINK_NULL }; On 2016/12/02 18:28:57, DaleCurtis wrote: > I prefer enum class these days (no = 0), but up to you. Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:51: return new MockAudioRendererSink((status == WASP_SINK_OK) On 2016/12/02 18:28:57, DaleCurtis wrote: > No uneccesary parens. Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:71: expected_sink_((GetParam() == WASP_SINK_OK) On 2016/12/02 18:28:57, DaleCurtis wrote: > Drop unnecessary parens. Done. https://codereview.chromium.org/2533443002/diff/60001/media/blink/webaudiosou... media/blink/webaudiosourceprovider_impl_unittest.cc:142: if (GetParam() == WASP_SINK_ERROR) { On 2016/12/02 18:28:57, DaleCurtis wrote: > Don't need {} for single line if, but up to you. Done.
guidou@ - still lgtm?
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...
still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, dalecurtis@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2533443002/#ps80001 (title: "review comments addressed")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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": 80001, "attempt_start_ts": 1481036525885750,
"parent_rev": "1c469387ecb7e490514ca605bd7be5100c566793", "commit_rev":
"6b67f1677ad563fb180bff145bfdf73c1701d6b5"}
Message was sent while issue was closed.
Description was changed from ========== Fallback to null sink moved from WebMediaPlayer to WebAudioSourceProvider::Initialize(), so that we do it on media thread. UMA stats for AudioOutputDevice status and for fallbacks to null sink. BUG=668506,668201,663546,626862 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 ========== Fallback to null sink moved from WebMediaPlayer to WebAudioSourceProvider::Initialize(), so that we do it on media thread. UMA stats for AudioOutputDevice status and for fallbacks to null sink. BUG=668506,668201,663546,626862 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 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fallback to null sink moved from WebMediaPlayer to WebAudioSourceProvider::Initialize(), so that we do it on media thread. UMA stats for AudioOutputDevice status and for fallbacks to null sink. BUG=668506,668201,663546,626862 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 ========== Fallback to null sink moved from WebMediaPlayer to WebAudioSourceProvider::Initialize(), so that we do it on media thread. UMA stats for AudioOutputDevice status and for fallbacks to null sink. BUG=668506,668201,663546,626862 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 Committed: https://crrev.com/e4ba2ed8354d01411848143efbe8d17b41e1853b Cr-Commit-Position: refs/heads/master@{#436614} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e4ba2ed8354d01411848143efbe8d17b41e1853b Cr-Commit-Position: refs/heads/master@{#436614}
Message was sent while issue was closed.
Description was changed from ========== Fallback to null sink moved from WebMediaPlayer to WebAudioSourceProvider::Initialize(), so that we do it on media thread. UMA stats for AudioOutputDevice status and for fallbacks to null sink. BUG=668506,668201,663546,626862 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 Committed: https://crrev.com/e4ba2ed8354d01411848143efbe8d17b41e1853b Cr-Commit-Position: refs/heads/master@{#436614} ========== to ========== Fallback to null sink moved from WebMediaPlayer to WebAudioSourceProvider::Initialize(), so that we do it on media thread. UMA stats for AudioOutputDevice status and for fallbacks to null sink. BUG=668506,668201,663546,626862,673291 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 Committed: https://crrev.com/e4ba2ed8354d01411848143efbe8d17b41e1853b Cr-Commit-Position: refs/heads/master@{#436614} ========== |
