Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043883005:
Implementing AudioOutputDevice authorization timeout (Closed)
|
Created:
4 years, 6 months ago by o1ka Modified:
4 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Henrik Grunell, vanellope-cl_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplementing AudioOutputDevice authorization timeout
by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call.
BUG=615589
Committed: https://crrev.com/698458b0a4cd539c3945d1cb988d9a4a9a96d64d
Cr-Commit-Position: refs/heads/master@{#399685}
Patch Set 1 #
Total comments: 18
Patch Set 2 : "Review comments addrests; UMA stats and unit test added" #
Total comments: 11
Patch Set 3 : "Review comments addressed; auth timeout as AOD constructor parameter" #Patch Set 4 : "moving ProcessDeviceAuthorizationOnIOThread() to the proper place" #
Total comments: 4
Patch Set 5 : guidou@'s comments addressed #
Total comments: 10
Patch Set 6 : Dale's comments addressed: code simplified, comments fixed. #
Messages
Total messages: 44 (10 generated)
Description was changed from ========== Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 ========== to ========== Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 ==========
olka@chromium.org changed reviewers: + dalecurtis@chromium.org, guidou@chromium.org
Dale, Guido - WDYT of this approach? (I'm not sure it's actually bug-free: don't have time to test it today) https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:23: constexpr int kAuthorizationTimeoutMs = 1000; Not sure about the value https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:313: void AudioOutputDevice::ProcessDeviceAuthorizationOnIOThread( Will move it to an appropriate place, now it's here to make diff simpler.
Why do we want this versus just using TimedWait() instead of Wait() in the various places where we block today?
On 2016/06/08 18:06:15, DaleCurtis wrote: > Why do we want this versus just using TimedWait() instead of Wait() in the > various places where we block today? Access to device status and audio parameters is synchronized via authorization event: they are never accessed before authorization is signalled, and their values are guaranteed to be updated only before it is signalled. If we just use timed wait, that it can be the race in case we finally receive authorization => we need som synchronization. Also, when we receive authorization event after timeout, state machine gets confused, as well as the users: there is no way to signal state update, but the state will be updated if we leave the state machine as it is now. The proposed approach looks pretty consistent to me, and I think it's better to avoid making the state machine more complex.
On 2016/06/08 at 18:44:27, olka wrote: > On 2016/06/08 18:06:15, DaleCurtis wrote: > > Why do we want this versus just using TimedWait() instead of Wait() in the > > various places where we block today? > > Access to device status and audio parameters is synchronized via authorization event: they are never accessed before authorization is signalled, and their values are guaranteed to be updated only before it is signalled. > If we just use timed wait, that it can be the race in case we finally receive authorization => we need som synchronization. Also, when we receive authorization event after timeout, state machine gets confused, as well as the users: there is no way to signal state update, but the state will be updated if we leave the state machine as it is now. > > The proposed approach looks pretty consistent to me, and I think it's better to avoid making the state machine more complex. How is that different than what this is doing? If the TimedWait expires you'd just set some flag that the process has failed. Just like you're doing here, you'd ignore authorization after that point.
Flag protected by the lock? (It will be accessed from IP thread as well) And we won't shut down IPC? (Here IPC gets shut down and state changes from AUTHORIZING to IPC CLOSED)? On Jun 8, 2016 20:53, <dalecurtis@chromium.org> wrote: > On 2016/06/08 at 18:44:27, olka wrote: > > On 2016/06/08 18:06:15, DaleCurtis wrote: > > > Why do we want this versus just using TimedWait() instead of Wait() in > the > > > various places where we block today? > > > > Access to device status and audio parameters is synchronized via > authorization > event: they are never accessed before authorization is signalled, and their > values are guaranteed to be updated only before it is signalled. > > If we just use timed wait, that it can be the race in case we finally > receive > authorization => we need som synchronization. Also, when we receive > authorization event after timeout, state machine gets confused, as well as > the > users: there is no way to signal state update, but the state will be > updated if > we leave the state machine as it is now. > > > > The proposed approach looks pretty consistent to me, and I think it's > better > to avoid making the state machine more complex. > > How is that different than what this is doing? If the TimedWait expires > you'd > just set some flag that the process has failed. Just like you're doing > here, > you'd ignore authorization after that point. > > https://codereview.chromium.org/2043883005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/08 19:11:44, chromium-reviews wrote: > Flag protected by the lock? (It will be accessed from IP thread as well) > And we won't shut down IPC? (Here IPC gets shut down and state changes from > AUTHORIZING to IPC CLOSED)? > On Jun 8, 2016 20:53, <mailto:dalecurtis@chromium.org> wrote: > > > On 2016/06/08 at 18:44:27, olka wrote: > > > On 2016/06/08 18:06:15, DaleCurtis wrote: > > > > Why do we want this versus just using TimedWait() instead of Wait() in > > the > > > > various places where we block today? > > > > > > Access to device status and audio parameters is synchronized via > > authorization > > event: they are never accessed before authorization is signalled, and their > > values are guaranteed to be updated only before it is signalled. > > > If we just use timed wait, that it can be the race in case we finally > > receive > > authorization => we need som synchronization. Also, when we receive > > authorization event after timeout, state machine gets confused, as well as > > the > > users: there is no way to signal state update, but the state will be > > updated if > > we leave the state machine as it is now. > > > > > > The proposed approach looks pretty consistent to me, and I think it's > > better > > to avoid making the state machine more complex. > > > > How is that different than what this is doing? If the TimedWait expires > > you'd > > just set some flag that the process has failed. Just like you're doing > > here, > > you'd ignore authorization after that point. > > > > https://codereview.chromium.org/2043883005/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ok, I'll make an atomic flag and see what to do with IPC.
Ah, I forgot this will span threads a simple flag would need a lock. I guess my only worry with this approach is that once you're in the Wait() call this will not unblock it. You need to add a Signal() somewhere to wake up outstanding waiters... but if that's on the same thread that your timeout is delivered on, how does it work?
On 2016/06/08 20:41:25, DaleCurtis wrote: > Ah, I forgot this will span threads a simple flag would need a lock. I guess my > only worry with this approach is that once you're in the Wait() call this will > not unblock it. You need to add a Signal() somewhere to wake up outstanding > waiters... but if that's on the same thread that your timeout is delivered on, > how does it work? I'm not quite sure I follow. If by "this approach" you mean the currently implemented one, then: * The only place we wait for authorization is GetOutputDeviceInfo(), which must not be called on IO thread, and there is a check for that (https://cs-staging.chromium.org/chromium/src/media/audio/audio_output_device....) * Authorization event is always signalled on IO thread. * IO thread is never blocked waiting anything from browser (AFAIK). * The only two places where authorization is signalled are OnDeviceAuthorized() (https://cs-staging.chromium.org/chromium/src/media/audio/audio_output_device....) and OnIPCClosed() (https://cs-staging.chromium.org/chromium/src/media/audio/audio_output_device....), Both of them are called (on IO thread) when a corresponding message is received from the browser side or if we invoke them manually. * Upon a timeout we call ProcessDeviceAuthorizationOnIOThread() and execute OnIPCClosed() (l.377) - just the same way we do if the device is not authorized => event is signalled and all GetOutputDeviceInfo() calls are unblocked and return the device info reporting internal error. A pro of this approach is we just simulate "not authorized" reply from the browser and do not introduce any additional state variables/transitions.
Now I tested it by delaying browser side authorization responses by 2 seconds. Seems to work fine so far.
I like olka's approach. Upon timeout just simulate a "not authorized" reply from the browser. Question: In case of a late authorization from the browser, how do we remove the authorization from the browser?
On 2016/06/08 20:41:25, DaleCurtis wrote: > Ah, I forgot this will span threads a simple flag would need a lock. (I also looked a bit closer into using the flag: we can't get away with atomic ops, because OnDeviceAuthorized() requires a wider scope of the lock. And extra locking in OnDeviceAuthorized on IO thread does not quite appeal to me.)
On 2016/06/09 09:37:47, Guido Urdaneta wrote: > I like olka's approach. Upon timeout just simulate a "not authorized" reply from > the browser. > Question: In case of a late authorization from the browser, how do we remove the > authorization from the browser? My understanding is that ipc_->CloseStream() (l.376) takes care of that: it sends AudioHostMsg_CloseStream to the browser (https://cs-staging.chromium.org/chromium/src/content/renderer/media/audio_mes...), which is queued on BrowserThread::IO (i.e. guaranteed to be processed _after_ authorization request), and authorization is removed in AudioRendererHost::OnCloseStream() (https://cs-staging.chromium.org/chromium/src/content/browser/renderer_host/me...)
https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:23: constexpr int kAuthorizationTimeoutMs = 1000; On 2016/06/08 15:46:06, o1ka wrote: > Not sure about the value Looks like renderer hang timeout is defined by content::kHungRendererDelayMs (https://cs-staging.chromium.org/chromium/src/content/common/content_constants...), which is (for now) defined as 5000 for android / 30000 otherwise (https://cs-staging.chromium.org/chromium/src/content/common/content_constants...). We can probably use something like content::kHungRendererDelayMs/2 as authorization timeout, or stick with min(3000, kHungRendererDelayMs). Not sure which strategy is more reasonable: is it normal to wait for 15 seconds?
On 2016/06/09 11:19:53, o1ka wrote: > On 2016/06/09 09:37:47, Guido Urdaneta wrote: > > I like olka's approach. Upon timeout just simulate a "not authorized" reply > from > > the browser. > > Question: In case of a late authorization from the browser, how do we remove > the > > authorization from the browser? > > My understanding is that ipc_->CloseStream() (l.376) takes care of that: > it sends AudioHostMsg_CloseStream to the browser > (https://cs-staging.chromium.org/chromium/src/content/renderer/media/audio_mes...), > which is queued on BrowserThread::IO (i.e. guaranteed to be processed _after_ > authorization request), > and authorization is removed in AudioRendererHost::OnCloseStream() > (https://cs-staging.chromium.org/chromium/src/content/browser/renderer_host/me...) Now I see that CloseStream must have been called before in case a late authorization arrives. Thanks :)
just a couple of nits https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:317: bool responce_received) { s/responce/response or maybe use a more explicit name like |did_time_out|. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:334: if (!responce_received) { else?
https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:334: if (!responce_received) { On 2016/06/09 12:09:49, Guido Urdaneta wrote: > else? Yes, I'll need "else" for UMA stats (those will be in the next patch if all the reviewers approve the approach).
https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:23: constexpr int kAuthorizationTimeoutMs = 1000; On 2016/06/09 at 12:02:04, o1ka wrote: > On 2016/06/08 15:46:06, o1ka wrote: > > Not sure about the value > > Looks like renderer hang timeout is defined by content::kHungRendererDelayMs (https://cs-staging.chromium.org/chromium/src/content/common/content_constants...), > > which is (for now) defined as 5000 for android / 30000 otherwise (https://cs-staging.chromium.org/chromium/src/content/common/content_constants...). > > We can probably use something like content::kHungRendererDelayMs/2 as authorization timeout, or stick with min(3000, kHungRendererDelayMs). Not sure which strategy is more reasonable: is it normal to wait for 15 seconds? I don't think we want to block for anymore than 5 seconds, but we should definitely have a UMA to figure out how often this is ticking as well as one which includes when the response actually comes in if possible. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:160: if (auth_timeout_action_) Instead, if you use a base::OneShotTimer here you can replace all of this with auth_timeout_action_.Start(FROM_HERE, delay, base::Bind(...)); on l.173. It'll take care of posting the task, doing nothing when the timer isn't running, and canceling it if it is running (as well as cleaning up the timer on destruction). https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:249: if (auth_timeout_action_) { No need for this with the timer (or cancellable really), it'll auto drop on destruction. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:321: if (auth_timeout_action_) Would just be .Stop() with timer. You can always call it outside of the nest here and simplifyy these nested conditionals to a single line w/ return. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.h:154: bool responce_received); response
PTAL: added UMA and a unit test, addressed the comments. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:23: constexpr int kAuthorizationTimeoutMs = 1000; On 2016/06/09 18:25:34, DaleCurtis wrote: > On 2016/06/09 at 12:02:04, o1ka wrote: > > On 2016/06/08 15:46:06, o1ka wrote: > > > Not sure about the value > > > > Looks like renderer hang timeout is defined by content::kHungRendererDelayMs > (https://cs-staging.chromium.org/chromium/src/content/common/content_constants...), > > > > which is (for now) defined as 5000 for android / 30000 otherwise > (https://cs-staging.chromium.org/chromium/src/content/common/content_constants...). > > > > We can probably use something like content::kHungRendererDelayMs/2 as > authorization timeout, or stick with min(3000, kHungRendererDelayMs). Not sure > which strategy is more reasonable: is it normal to wait for 15 seconds? > > I don't think we want to block for anymore than 5 seconds, but we should > definitely have a UMA to figure out how often this is ticking as well as one > which includes when the response actually comes in if possible. Ok. I'm not sure if we should wait for 5 seconds on Android - I think we should be a little below the threshold. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:160: if (auth_timeout_action_) On 2016/06/09 18:25:34, DaleCurtis wrote: > Instead, if you use a base::OneShotTimer here you can replace all of this with > > auth_timeout_action_.Start(FROM_HERE, delay, base::Bind(...)); > > on l.173. It'll take care of posting the task, doing nothing when the timer > isn't running, and canceling it if it is running (as well as cleaning up the > timer on destruction). Thanks! Done. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:249: if (auth_timeout_action_) { On 2016/06/09 18:25:34, DaleCurtis wrote: > No need for this with the timer (or cancellable really), it'll auto drop on > destruction. I guess we do need this, because both cancellable and timer are not thread-safe and should be destroyed on the thread they are used on. For example: https://cs-staging.chromium.org/chromium/src/base/timer/timer.h?sq=package:ch... https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:317: bool responce_received) { On 2016/06/09 12:09:49, Guido Urdaneta wrote: > s/responce/response > > or maybe use a more explicit name like |did_time_out|. Done. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:321: if (auth_timeout_action_) On 2016/06/09 18:25:34, DaleCurtis wrote: > Would just be .Stop() with timer. You can always call it outside of the nest > here and simplifyy these nested conditionals to a single line w/ return. Done. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:334: if (!responce_received) { On 2016/06/09 12:38:28, o1ka wrote: > On 2016/06/09 12:09:49, Guido Urdaneta wrote: > > else? > > Yes, I'll need "else" for UMA stats (those will be in the next patch if all the > reviewers approve the approach). Done. https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/2043883005/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.h:154: bool responce_received); On 2016/06/09 18:25:35, DaleCurtis wrote: > response Done. https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:26: constexpr int kAuthorizationTimeoutMs = 4000; Since content::kHungRendererDelayMs is obviously in content, we can't rely on it here, so using an absolute value. https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:333: "Media.Audio.Render.OutputDeviceAuthorizationTimeoutExceededMs", I'm not sure if we want to have this metric. Since we close IPC when authorization request times out, we'll log only a little fraction which is (probably) received right after the timeout fired. The values should be within just several milliseconds. And we won't receive any later authorizations as soon as IPC is closed. https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:333: TEST_P(AudioOutputDeviceTest, AuthorizationTimedOut) { Since it takes 4 seconds (= auth timeout value), probably enable it on Windows only?
https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:330: // upgrade device information after |did_receive_auth_| is signalled. signaled. https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:333: "Media.Audio.Render.OutputDeviceAuthorizationTimeoutExceededMs", On 2016/06/10 at 14:21:09, o1ka wrote: > I'm not sure if we want to have this metric. Since we close IPC when authorization request times out, we'll log only a little fraction which is (probably) received right after the timeout fired. The values should be within just several milliseconds. And we won't receive any later authorizations as soon as IPC is closed. Yeah, I don't think this is useful. https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:339: UMA_HISTOGRAM_BOOLEAN( Just have this in one spot outside the conditional: UMA_HISTOGRAM_BOOLEAN("Media.Audio.Render.OutputDeviceAuthorizationTimedOut", timed_out); https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:354: // Runs the loop and waits for |thread| to call event's closure. Don't use real time if you want to add a test like this. You'll need to inject a fake timer or have a SetTimeoutForTesting() to something much smaller.
Comments addressed, PTAL https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:330: // upgrade device information after |did_receive_auth_| is signalled. On 2016/06/10 18:29:18, DaleCurtis wrote: > signaled. Acknowledged. https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:333: "Media.Audio.Render.OutputDeviceAuthorizationTimeoutExceededMs", On 2016/06/10 18:29:18, DaleCurtis wrote: > On 2016/06/10 at 14:21:09, o1ka wrote: > > I'm not sure if we want to have this metric. Since we close IPC when > authorization request times out, we'll log only a little fraction which is > (probably) received right after the timeout fired. The values should be within > just several milliseconds. And we won't receive any later authorizations as soon > as IPC is closed. > > Yeah, I don't think this is useful. Removed. https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:339: UMA_HISTOGRAM_BOOLEAN( On 2016/06/10 18:29:18, DaleCurtis wrote: > Just have this in one spot outside the conditional: > > UMA_HISTOGRAM_BOOLEAN("Media.Audio.Render.OutputDeviceAuthorizationTimedOut", > timed_out); Done. https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/2043883005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:354: // Runs the loop and waits for |thread| to call event's closure. On 2016/06/10 18:29:19, DaleCurtis wrote: > Don't use real time if you want to add a test like this. You'll need to inject a > fake timer or have a SetTimeoutForTesting() to something much smaller. Introduced a timeout parameter in AOD constructor. It also allows to align the timeout value to make sure it is under kHungRendererDelayMs.
olka@chromium.org changed reviewers: + holte@chromium.org, perkj@chromium.org
perkj@chromium.org: Please review changes in audio_device_factory
holte@chromium.org: please review UMA
https://codereview.chromium.org/2043883005/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/60001/media/audio/audio_outpu... media/audio/audio_output_device.cc:279: auth_timeout_action_->Stop(); Should you set |auth_timeout_action_| to nullptr after Stop() here? https://codereview.chromium.org/2043883005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2043883005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:20835: + Wheather audio output device timed out waiting for authorization reply from s/Wheather/Whether
https://codereview.chromium.org/2043883005/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/60001/media/audio/audio_outpu... media/audio/audio_output_device.cc:279: auth_timeout_action_->Stop(); On 2016/06/13 14:39:39, Guido Urdaneta wrote: > Should you set |auth_timeout_action_| to nullptr after Stop() here? It does not really matter, but it's cleaner. Done. https://codereview.chromium.org/2043883005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2043883005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:20835: + Wheather audio output device timed out waiting for authorization reply from On 2016/06/13 14:39:39, Guido Urdaneta wrote: > s/Wheather/Whether Done.
Patchset #5 (id:70001) has been deleted
lgtm % comment fixes and code simplification. https://codereview.chromium.org/2043883005/diff/90001/content/renderer/media/... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/2043883005/diff/90001/content/renderer/media/... content/renderer/media/audio_device_factory.cc:38: std::min(kHungRendererDelayMs * 8 / 10, kMaxAuthorizationTimeout)))); Needs explanation. https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... media/audio/audio_output_device.cc:171: OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, media::AudioParameters(), This is fine with me, but have you considered adding an OUTPUT_DEVICE_STATUS_ERROR_TIMED_OUT to avoid needing the ProcessXXX() trampoline method? https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... media/audio/audio_output_device.cc:247: auth_timeout_action_.reset(nullptr); reset(nullptr) is unnecessary. just .reset() https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... media/audio/audio_output_device.cc:277: if (auth_timeout_action_) { Just auth_timeout_action_.reset() is sufficient. https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:47: const int kAuthTimeoutForTesting = 500; Ms? Secs?
histograms lgtm
On 2016/06/13 14:17:47, o1ka wrote: > mailto:perkj@chromium.org: Please review changes in audio_device_factory I dont know anything about the audio_device_factory. Is there a better reviewer?
lgtm provided you address dalecurtis' comments.
olka@chromium.org changed reviewers: - perkj@chromium.org
On 2016/06/14 06:27:53, perkj_chrome wrote: > On 2016/06/13 14:17:47, o1ka wrote: > > mailto:perkj@chromium.org: Please review changes in audio_device_factory > > I dont know anything about the audio_device_factory. Is there a better > reviewer? -perkj@chromium.org: as I was pointed out, dalecurtis@ is an owner audio_device_factory (fancy reviewer highlighter tool misinformed me).
Addressed dalecurtis@'s comments. https://codereview.chromium.org/2043883005/diff/90001/content/renderer/media/... File content/renderer/media/audio_device_factory.cc (right): https://codereview.chromium.org/2043883005/diff/90001/content/renderer/media/... content/renderer/media/audio_device_factory.cc:38: std::min(kHungRendererDelayMs * 8 / 10, kMaxAuthorizationTimeout)))); On 2016/06/13 18:30:09, DaleCurtis wrote: > Needs explanation. Done. https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... media/audio/audio_output_device.cc:171: OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, media::AudioParameters(), On 2016/06/13 18:30:09, DaleCurtis wrote: > This is fine with me, but have you considered adding an > OUTPUT_DEVICE_STATUS_ERROR_TIMED_OUT to avoid needing the ProcessXXX() > trampoline method? Good idea! Done. https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... media/audio/audio_output_device.cc:247: auth_timeout_action_.reset(nullptr); On 2016/06/13 18:30:09, DaleCurtis wrote: > reset(nullptr) is unnecessary. just .reset() Done. https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... media/audio/audio_output_device.cc:277: if (auth_timeout_action_) { On 2016/06/13 18:30:09, DaleCurtis wrote: > Just auth_timeout_action_.reset() is sufficient. Done. https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/2043883005/diff/90001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:47: const int kAuthTimeoutForTesting = 500; On 2016/06/13 18:30:09, DaleCurtis wrote: > Ms? Secs? Done.
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org, dalecurtis@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2043883005/#ps110001 (title: "Dale's comments addressed: code simplified, comments fixed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043883005/110001
Message was sent while issue was closed.
Description was changed from ========== Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 ========== to ========== Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:110001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 ========== to ========== Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 Committed: https://crrev.com/698458b0a4cd539c3945d1cb988d9a4a9a96d64d Cr-Commit-Position: refs/heads/master@{#399685} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/698458b0a4cd539c3945d1cb988d9a4a9a96d64d Cr-Commit-Position: refs/heads/master@{#399685}
Message was sent while issue was closed.
Description was changed from ========== Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 Committed: https://crrev.com/698458b0a4cd539c3945d1cb988d9a4a9a96d64d Cr-Commit-Position: refs/heads/master@{#399685} ========== to ========== Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 Committed: https://crrev.com/698458b0a4cd539c3945d1cb988d9a4a9a96d64d Cr-Commit-Position: refs/heads/master@{#399685} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
