+sky/fsamuel - I seem to recall that mash/viz doesn't have a "context_factory_private" in the browser process, though I'm not quite sure what that does.

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/power/ml/user_activity_logging_controller.cc?q=useractivityloggingcontroller&sq=package:chromium&dr=CSs&l=41

Hello,

Thanks for letting me know. I'm not familiar with mash, but our code works (and has been tested) under non-mash environment. I'd really appreciate it if you could suggested a workaround under mash. 

Here is a bit of background. We are working on a project that uses ML to improve power management. Video events are important signals for us.  We have been suggested not to use ash's video detector and we should implement our own viz::mojom::VideoDetectorObserver. In order to be notified about the video events, we have to call AddVideoDetectorObserver (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/power/ml/user_activity_logging_controller.cc?dr=CSs&l=40) to register ourselves. I looked at how we could register our impl under mash, but the required components are buried very deep in ash. As mash wouldn't be rolled out any time soon, I put in the non-mash solution. 

Thanks,
Jia

+sky/fsamuel for real

Would ash::VideoActivityNotifier [1] provide enough info? How would you consume the video signal? If you need to send it to a server, then you would need some mojo interface to send the info from ash to the browser.

[1]: https://cs.chromium.org/chromium/src/ash/system/power/video_activity_notifier.cc?rcl=ca530111ae4446bc1492d10f96356b700d7b4978&l=37

Yes ash::VideoActivityNotifier would give us the info we need and it'd be really good if we could use it. We only implemented our own because we were advised not to use it. :(

Re how we're using video signals, we only need to be notified when video starts to play and when it ends. 

Thanks.
Jia 

It looks like you need the video start/stop signal in the browser process where chrome/browser/chromeos/power/ml code runs. ash::VideoActivityNotifier runs in ash so we would need some mojo interface to pass the info back to the browser.

sky/fsamuel, do we already have API/sample to detect video activity in the browser? Can we bind to VideoDetector interface in browser (its comment says it is a privileged interface)?

Thanks a lot for this. Can we register our class as an observer of ash's VideoDetector? (https://cs.chromium.org/chromium/src/ash/wm/video_detector.h?type=cs&l=35)

We have another related project (also under power/ml) that needs to listen to video start/end signals well. What do you think is the best way to receive video signals?

Thanks!
Jia

You cannot use ash::VideoDetector directly since it runs in ash and would be in a separate process other than the browser process under mash. I was thinking of something like
  https://cs.chromium.org/chromium/src/ash/shell_port_mus.cc?rcl=dd85a2211af3a16c4a7330196b2529157c9a0eb0&l=287-289

when saying bind to VideoDetector interface. But I am not sure whether it is allowed from the browser process. Please get an approval from security owners (//ipc/SECURITY_OWNERS) before doing that.

If that is not allowed, we need to have the observer code running in ash and have a mojo interface (in //ash/public/interfaces/) to pass back the info to c/b/chromeos/power/ml code running in the browser process.

Meanwhile, could you put the context_factory_private() access code under 
  if (chromeos::GetAshConfig() != ash::Config::MASH)
to exclude it from running under mash ?

I suspect this breaks auto-test. Please either revert, or add an early out for mash.

I believe you should be able to bind VideoDetector in the browser side too.

Thanks all, I'll put in a cl to exclude mash as a short term solution and will work on binding it in the browser side next.
Jia

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d443126db5827163c6ea50f4c00c3d3d9829633c

commit d443126db5827163c6ea50f4c00c3d3d9829633c
Author: Jia <jiameng@chromium.org>
Date: Wed Mar 14 00:41:01 2018

Disable user activity logging if we're under MASH.

Bug:  821036 
Change-Id: I98e392f3dd60e4148fd04a164e3a9fe5580fe57f
Reviewed-on: https://chromium-review.googlesource.com/961165
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Jia Meng <jiameng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542969}
[modify] https://crrev.com/d443126db5827163c6ea50f4c00c3d3d9829633c/chrome/browser/chromeos/power/ml/user_activity_controller.cc

I've put in a short term solution so you won't see crash any more.

Would you know the percentage of chrome os users that have MASH enabled? 

Also, when would MASH be rolled out completely?

Thanks!
Jia

Thanks for landing the fix!

Mash is a long-term rearchitecture project. It will be several quarters before it rolls out. However, it's so big that we have to make new features work with it, otherwise we'll never finish the project. That's why there are mash_ash_unittests and mash_browser_tests in the CQ.

I'm a little surprised that mash_browser_tests didn't catch this. That implies there are no browser_tests that exercise this code path. Is it all tested with unit_tests? Or is this code path not tested?

Thanks a lot for this info! :)

The crash occurred in our logging controller class, which sets up all the objects and video connection. It doesn't have a unit test as it only sets up the objects, but I would imagine browser test would check it. Or is some other test script/file that I should update to include our controller class?

Thanks,
Jia

You should consider writing a browser test that exercises at least part of your feature end to end. There are lots of examples, look for files ending in _browsertest.cc. If this code talks to an underlying device daemon you might need to fake out that daemon, or simulate the answers it will give when the feature is turned on. I'm not sure why we only saw the crash on device and not linux-chromeos, maybe the code only runs when a flag is passed or a feature is enabled? If that's the case then the browser test will need to flip the flag.

Thanks James! I'll look into adding tests as suggested.

In the meantime, could someone please verify the crash has been fixed so that I'll close this ticket?

Thanks,
Jia

Yep, confirmed that the crash is gone under mash.

Thanks! :) I'm closing the ticket now.
Jia