|
|
Created:
3 years, 7 months ago by ossu-chromium Modified:
3 years, 6 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, xjz+watch_chromium.org, mfoltz+watch_chromium.org, miu+watch_chromium.org, Henrik Grunell Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded initial set of logging for AudioOutputController.
BUG=chromium:714119
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/2900043002
Cr-Commit-Position: refs/heads/master@{#476257}
Committed: https://chromium.googlesource.com/chromium/src/+/9e1250518374b610d8a23e8b87c44a717a8c919a
Patch Set 1 #Patch Set 2 : Improved printouts a bit using StringPrintf. #
Total comments: 3
Patch Set 3 : Turned DoLog static, so we can log things even when delegate_ has disappeared. #
Total comments: 10
Patch Set 4 : Bind -> BindOnce; DoLog now a lambda; removed extra blank lines. #
Total comments: 6
Patch Set 5 : Rebase #Patch Set 6 : Put logging directly in OnLog. Provide stream_id explicitly. #
Total comments: 1
Patch Set 7 : impliciticity #
Messages
Total messages: 38 (21 generated)
Description was changed from ========== Added initial set of logging for AudioOutputController. BUG=chromium:714119 ========== to ========== Added initial set of logging for AudioOutputController. BUG=chromium:714119 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 ==========
The CQ bit was checked by ossu@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.
ossu@chromium.org changed reviewers: + maxmorin@chromium.org
I've put in some initial logging for AudioOutputController and it SeemsToWork(tm), but my local Chrome build has been exploding a lot (with these changes and without), so I haven't been able to 100% confirm it's alright. PTAL and see if this is roughly how you'd imagine I'd delegate the logging etc.
On 2017/05/24 17:19:41, ossu-chromium wrote: > I've put in some initial logging for AudioOutputController and it > SeemsToWork(tm), but my local Chrome build has been exploding a lot (with these > changes and without), so I haven't been able to 100% confirm it's alright. > > PTAL and see if this is roughly how you'd imagine I'd delegate the logging etc. Yes, this LGTM.
https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_outpu... media/audio/audio_output_controller.cc:111: handler_->OnLog(base::StringPrintf("AOC::DoCreate (for device change: %s)", I think we should consider having a common logging prefix for all audio things. This makes it easier to grep it in logs and also makes it more forward compatible (e.g. we don't have to worry about the prefix if we move the log message to another class).
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Added initial set of logging for AudioOutputController. BUG=chromium:714119 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 ========== Added initial set of logging for AudioOutputController. BUG=chromium:714119 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 ==========
ossu@chromium.org changed reviewers: + liberato@chromium.org
The CQ bit was checked by ossu@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...
+liberato@ for owner review. I've had to change DoLog to be static and the event handler to retain the stream_id on creation, so that OnClose could be logged properly. Before that, since AudioOutputController is called in AudioOutputDelegateImpl's destructor, the delegate had disappeared once the OnClose message was to be logged, and the task was dropped. Oh, and I've been able to test it properly, locally now. Turns out I had some dbus issues on my machine that are now resolved. https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_outpu... media/audio/audio_output_controller.cc:111: handler_->OnLog(base::StringPrintf("AOC::DoCreate (for device change: %s)", On 2017/05/29 08:12:53, Max Morin wrote: > I think we should consider having a common logging prefix for all audio things. > This makes it easier to grep it in logs and also makes it more forward > compatible (e.g. we don't have to worry about the prefix if we move the log > message to another class). It should be possible to grep for these lines from AudioInputController and AudioOutputController by grepping for A[IO]C for now. We can bike-shed a bit what exactly the common prefix should be and which files it should be applied to, and I can fix that in a separate CL, if that's alright?
https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2900043002/diff/20001/media/audio/audio_outpu... media/audio/audio_output_controller.cc:111: handler_->OnLog(base::StringPrintf("AOC::DoCreate (for device change: %s)", On 2017/05/29 14:19:43, ossu-chromium wrote: > On 2017/05/29 08:12:53, Max Morin wrote: > > I think we should consider having a common logging prefix for all audio > things. > > This makes it easier to grep it in logs and also makes it more forward > > compatible (e.g. we don't have to worry about the prefix if we move the log > > message to another class). > > It should be possible to grep for these lines from AudioInputController and > AudioOutputController by grepping for A[IO]C for now. We can bike-shed a bit > what exactly the common prefix should be and which files it should be applied > to, and I can fix that in a separate CL, if that's alright? Sounds good. https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:79: base::Bind(&AudioOutputDelegateImpl::DoLog, stream_id_, message)); I just realized this should be BindOnce, like in all the other methods here.
https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:79: base::Bind(&AudioOutputDelegateImpl::DoLog, stream_id_, message)); On 2017/05/29 14:25:44, Max Morin wrote: > I just realized this should be BindOnce, like in all the other methods here. I think I've been a bit hazy on the significance of Bind vs. BindOnce. I guess that since each of these tasks will only be called once, BindOnce is the proper one? Will fix!
https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:79: base::Bind(&AudioOutputDelegateImpl::DoLog, stream_id_, message)); On 2017/05/29 14:35:48, ossu-chromium wrote: > On 2017/05/29 14:25:44, Max Morin wrote: > > I just realized this should be BindOnce, like in all the other methods here. > > I think I've been a bit hazy on the significance of Bind vs. BindOnce. I guess > that since each of these tasks will only be called once, BindOnce is the proper > one? Will fix! Yes, it's a sort of documentation (with added efficiency bonus; since OnceCallback is move-only, we don't have to reference count its internal state). There are also some other neat things, like if you move something into the BindOnce call, it will be moved out when the callback is called. Basically, use BindOnce/OnceCallback when the callback should only be called once, if that compiles.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % some pretty minor nits, and the Bind vs BindOnce thing (good catch, btw). thanks -fl https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:234: please add "// static". https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.h (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.h:74: static void DoLog(int stream_id, const std::string& msg); please add a comment about why this needs to be static. it's a little subtle. https://codereview.chromium.org/2900043002/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2900043002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_controller.cc:377: return "unknown"; will it compile if one omits a case in the switch? i don't think it will. if not, then one can remove this. https://codereview.chromium.org/2900043002/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/2900043002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_controller.h:78: please delete the extra blank line, here and elsewhere.
The CQ bit was checked by ossu@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...
I figured DoLog doesn't need to be visible outside of OnLog, so I turned it into a lambda. Also addressed most of the comments in the review. https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:234: On 2017/05/30 16:53:34, liberato wrote: > please add "// static". See comment in audio_output_delegate_impl.h. https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.h (right): https://codereview.chromium.org/2900043002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.h:74: static void DoLog(int stream_id, const std::string& msg); On 2017/05/30 16:53:34, liberato wrote: > please add a comment about why this needs to be static. it's a little subtle. I've looked at this again and I think it would be even better if I just folded this into OnLog as a lambda instead. There's no reason this function needs to be in AudioOutputDelegateImpl nor publicly visible. https://codereview.chromium.org/2900043002/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2900043002/diff/60001/media/audio/audio_outpu... media/audio/audio_output_controller.cc:377: return "unknown"; On 2017/05/30 16:53:35, liberato wrote: > will it compile if one omits a case in the switch? i don't think it will. > > if not, then one can remove this. It won't, but my thought is if state ever goes bad (i.e. OnDeviceChange getting called after or during the destruction of an AudioOutputController), it's more informative to have that be logged as "unknown" than to have a crash in StringPrintf. Though, I may be wrong. Perhaps change it to something more obvious, like "bad state".
Still lgtm, but some nits. https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:45: : delegate_(std::move(delegate)), stream_id_(delegate_->stream_id_) {} Since you're dereferencing |delegate_|, maybe add DCHECK_CURRENTLY_ON(BrowserThread::IO) here? (The weak pointer may only be used on one thread). https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:84: content::MediaStreamManager::SendMessageToNativeLog(out_message); Since I'm nit-picking anyways, remove unnecessary content::. DCHECK_CURRENTLY_ON feels unnecessary when you use a lambda, since it's clear that this is posted to the IO thread here and cannot be referenced from anywhere else.
https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:84: content::MediaStreamManager::SendMessageToNativeLog(out_message); On 2017/05/31 11:52:40, Max Morin wrote: > Since I'm nit-picking anyways, remove unnecessary content::. DCHECK_CURRENTLY_ON > feels unnecessary when you use a lambda, since it's clear that this is posted to > the IO thread here and cannot be referenced from anywhere else. Oh, I just noticed SendMessageToNativeLog is safe to call from any thread so posting a task is not needed :) (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...)
https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:84: content::MediaStreamManager::SendMessageToNativeLog(out_message); On 2017/05/31 11:56:23, Max Morin wrote: > On 2017/05/31 11:52:40, Max Morin wrote: > > Since I'm nit-picking anyways, remove unnecessary content::. > DCHECK_CURRENTLY_ON > > feels unnecessary when you use a lambda, since it's clear that this is posted > to > > the IO thread here and cannot be referenced from anywhere else. > > Oh, I just noticed SendMessageToNativeLog is safe to call from any thread so > posting a task is not needed :) > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...) Ah, yes, you are right! The task was only posted to the IO thread so that we'd be able to access delegate_, but since we aren't anymore, this should be fine to just fold into OnLog directly. Unless there's some reason to keep the DVLOG call on the IO thread? I bet there isn't. I'll simplify further and post a new PS.
Simplified version up. https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:45: : delegate_(std::move(delegate)), stream_id_(delegate_->stream_id_) {} On 2017/05/31 11:52:40, Max Morin wrote: > Since you're dereferencing |delegate_|, maybe add > DCHECK_CURRENTLY_ON(BrowserThread::IO) here? (The weak pointer may only be used > on one thread). I've no reasonable place to put it - in the code block would be too late (the dereference would already have happened). Also want stream_id_ to be const, which it wasn't yet here. I've instead made stream_id a separate parameter, so the caller has to ensure it can safely access it (which it already does).
The CQ bit was checked by ossu@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...
Final nit now, promise :) https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_output_delegate_impl.cc:45: : delegate_(std::move(delegate)), stream_id_(delegate_->stream_id_) {} On 2017/05/31 13:47:20, ossu-chromium wrote: > On 2017/05/31 11:52:40, Max Morin wrote: > > Since you're dereferencing |delegate_|, maybe add > > DCHECK_CURRENTLY_ON(BrowserThread::IO) here? (The weak pointer may only be > used > > on one thread). > > I've no reasonable place to put it - in the code block would be too late (the > dereference would already have happened). Also want stream_id_ to be const, > which it wasn't yet here. I've instead made stream_id a separate parameter, so > the caller has to ensure it can safely access it (which it already does). Alright, that's fine by me. https://codereview.chromium.org/2900043002/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_output_delegate_impl.cc (right): https://codereview.chromium.org/2900043002/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_output_delegate_impl.cc:29: explicit ControllerEventHandler( Remove explicit since it has two parameters now.
The CQ bit was checked by ossu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org, maxmorin@chromium.org Link to the patchset: https://codereview.chromium.org/2900043002/#ps140001 (title: "impliciticity")
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": 140001, "attempt_start_ts": 1496307525037280, "parent_rev": "2e699f248dfd86168b3f4c80e8e099891b1fddf9", "commit_rev": "9e1250518374b610d8a23e8b87c44a717a8c919a"}
Message was sent while issue was closed.
Description was changed from ========== Added initial set of logging for AudioOutputController. BUG=chromium:714119 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 ========== Added initial set of logging for AudioOutputController. BUG=chromium:714119 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/2900043002 Cr-Commit-Position: refs/heads/master@{#476257} Committed: https://chromium.googlesource.com/chromium/src/+/9e1250518374b610d8a23e8b87c4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9e1250518374b610d8a23e8b87c4...
Message was sent while issue was closed.
+cc grunell |