|
|
Created:
4 years, 10 months ago by Anand Mistry (off Chromium) Modified:
4 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix hotword module constantly restarting on Pixel 2.
|hotwordEnabled| is in the advanced constraints vector becuase it's an
optional constraint.
BUG=583720
TEST=Manually tested "Ok Google" on Pixel 2.
Committed: https://crrev.com/9115bb0067e799534808890b56728e412814d8e3
Cr-Commit-Position: refs/heads/master@{#378177}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comment. #Messages
Total messages: 28 (9 generated)
amistry@chromium.org changed reviewers: + mgiuca@chromium.org, tommi@chromium.org
tommi: Because you reviewed https://codereview.chromium.org/1581103002 mgiuca: As hotword owner
I don't understand this so will defer to Tommi (who is OWNER anyway). But in principle I agree with this and it is fairly urgent (need to merge it into M49). Note that the code has changed a lot since M49 (due to https://crrev.com/370008), so this will not be a trivial merge to M49. But Anand has prepared an equivalent patch to the M49 branch (https://codereview.chromium.org/1731593002/) which we will manually land after this is in trunk.
tommi: Ping. This is urgent (needs merge to M49).
mgiuca@chromium.org changed reviewers: + perkj@chromium.org
Hi Per, I haven't gotten a response from Tommi. Would you be able to look at this? If you get time, we also need the same fix applied in M49 where the code is very different: https://codereview.chromium.org/1731593002/ As I said previously, this is urgent (merge to M49 ASAP).
Description was changed from ========== Fix hotword module constantly restarting on Pixel 2. |hotwordEnabled| is in the advanced constraints vector becuase it's an optional constraint. BUG=583720 TEST=Manually tested "Ok Google" on Pixel 2. ========== to ========== Fix hotword module constantly restarting on Pixel 2. |hotwordEnabled| is in the advanced constraints vector becuase it's an optional constraint. BUG=583720 TEST=Manually tested "Ok Google" on Pixel 2. ==========
perkj@chromium.org changed reviewers: + hta@chromium.org
Not quite right. Right should be simple. Test it! https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl.cc:75: controls->hotword_enabled = true; 1) put a break in this loop. 2) instead use if (audio_basic.hasExact()) { controls->hotword_enabled = audio_advanced.hotwordEnabled.exact(); } else { if(audio_advanced.hotwordEnabled.hasExact()) { controls->hotword_enabled = audio_advanced.hotwordEnabled.exact(); break; } Yes, there's an utility function to do this, but it's in a CL that hasn't been submitted yet :-( 3) write an unit test!
On 2016/02/26 11:35:44, hta - Chromium wrote: > Not quite right. Right should be simple. > > Test it! > > https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user... > File content/renderer/media/user_media_client_impl.cc (right): > > https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user... > content/renderer/media/user_media_client_impl.cc:75: controls->hotword_enabled = > true; > 1) put a break in this loop. > > 2) instead use > if (audio_basic.hasExact()) { > controls->hotword_enabled = > audio_advanced.hotwordEnabled.exact(); > } else { > if(audio_advanced.hotwordEnabled.hasExact()) { > controls->hotword_enabled = > audio_advanced.hotwordEnabled.exact(); > break; > } > > Yes, there's an utility function to do this, but it's in a CL that hasn't been > submitted yet :-( > > 3) write an unit test! Arg, forgot the loop. if (audio_basic.hasExact()) { controls->hotword_enabled = audio_basic.hotwordEnabled.exact(); } else { for (const auto& audio_advanced: constraints) // like the loop you have if(audio_advanced.hotwordEnabled.hasExact()) { controls->hotword_enabled = audio_advanced.hotwordEnabled.exact(); break; } The code you wrote may fail if the hotwords constraint is not the last constraint in the advanced list (I think).
On 2016/02/26 11:35:44, hta - Chromium wrote: > Not quite right. Right should be simple. > > Test it! > > https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user... > File content/renderer/media/user_media_client_impl.cc (right): > > https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user... > content/renderer/media/user_media_client_impl.cc:75: controls->hotword_enabled = > true; > 1) put a break in this loop. > > 2) instead use > if (audio_basic.hasExact()) { > controls->hotword_enabled = > audio_advanced.hotwordEnabled.exact(); > } else { > if(audio_advanced.hotwordEnabled.hasExact()) { > controls->hotword_enabled = > audio_advanced.hotwordEnabled.exact(); > break; > } > > Yes, there's an utility function to do this, but it's in a CL that hasn't been > submitted yet :-( > > 3) write an unit test! Arg, forgot the loop. if (audio_basic.hasExact()) { controls->hotword_enabled = audio_basic.hotwordEnabled.exact(); } else { for (const auto& audio_advanced: constraints) // like the loop you have if(audio_advanced.hotwordEnabled.hasExact()) { controls->hotword_enabled = audio_advanced.hotwordEnabled.exact(); break; } The code you wrote may fail if the hotwords constraint is not the last constraint in the advanced list (I think).
Sorry about the delay. I've been heads down on other things in the past few days. Looks like Harald has suggested an alternate way of doing this, can you give that a go and ping back? feel free to use IM.
On 2016/02/26 12:12:40, tommi-chromium wrote: > Sorry about the delay. I've been heads down on other things in the past few > days. Looks like Harald has suggested an alternate way of doing this, can you > give that a go and ping back? feel free to use IM. We will have a look at doing this on Monday. Probably a good idea to add a unit test, but I don't want it to hold us up (remember, we are in an urgent situation here since we need to merge it back to M49). I also don't want to block on unsubmitted CLs. I'll see what we can do.
On 2016/02/26 12:47:20, Matt Giuca wrote: > On 2016/02/26 12:12:40, tommi-chromium wrote: > > Sorry about the delay. I've been heads down on other things in the past few > > days. Looks like Harald has suggested an alternate way of doing this, can you > > give that a go and ping back? feel free to use IM. > > We will have a look at doing this on Monday. Probably a good idea to add a unit > test, but I don't want it to hold us up (remember, we are in an urgent situation > here since we need to merge it back to M49). I also don't want to block on > unsubmitted CLs. I'll see what we can do. Understood. I'm OK with doing the unit test separately and please file a bug for that. Just ping me when you want me to take a look.
On 2016/02/26 12:49:34, tommi-chromium wrote: > Understood. I'm OK with doing the unit test separately and please file a bug > for that. Just ping me when you want me to take a look. OK. Anand, can you please update the CL as suggested in #9--#11.
ptal https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/1725213002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl.cc:75: controls->hotword_enabled = true; On 2016/02/26 11:35:44, hta - Chromium wrote: > 1) put a break in this loop. Done. > > 2) instead use > if (audio_basic.hasExact()) { > controls->hotword_enabled = > audio_advanced.hotwordEnabled.exact(); > } else { > if(audio_advanced.hotwordEnabled.hasExact()) { > controls->hotword_enabled = > audio_advanced.hotwordEnabled.exact(); > break; > } > > Yes, there's an utility function to do this, but it's in a CL that hasn't been > submitted yet :-( Done. > > 3) write an unit test! And there's the problem. It looks like this code doesn't have any unit tests. In fact, I'm not sure I can write any unit tests given this nugget of comment in UserMediaClientImpl::requestUserMedia(): // |user_media_request| can't be mocked. So in order to test at all we check // if it isNull.
The CQ bit was checked by amistry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1725213002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1725213002/20001
lgtm assume tests are coming in a later CL Note - still need tommi for owner lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rs lgtm
The CQ bit was checked by amistry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1725213002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1725213002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix hotword module constantly restarting on Pixel 2. |hotwordEnabled| is in the advanced constraints vector becuase it's an optional constraint. BUG=583720 TEST=Manually tested "Ok Google" on Pixel 2. ========== to ========== Fix hotword module constantly restarting on Pixel 2. |hotwordEnabled| is in the advanced constraints vector becuase it's an optional constraint. BUG=583720 TEST=Manually tested "Ok Google" on Pixel 2. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix hotword module constantly restarting on Pixel 2. |hotwordEnabled| is in the advanced constraints vector becuase it's an optional constraint. BUG=583720 TEST=Manually tested "Ok Google" on Pixel 2. ========== to ========== Fix hotword module constantly restarting on Pixel 2. |hotwordEnabled| is in the advanced constraints vector becuase it's an optional constraint. BUG=583720 TEST=Manually tested "Ok Google" on Pixel 2. Committed: https://crrev.com/9115bb0067e799534808890b56728e412814d8e3 Cr-Commit-Position: refs/heads/master@{#378177} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9115bb0067e799534808890b56728e412814d8e3 Cr-Commit-Position: refs/heads/master@{#378177} |