Hmm, https://chromium-review.googlesource.com/c/chromium/src/+/513562/ may not be the root cause.

I added log in TimeZone::detectHostTimeZone in third_party/icu/source/i18n/timezone.cc.
uprv_tzname(0) returns CST

Hi Frank/Evan,
Could you help check this issue?

We are trying to fix anti-banding issue in M72.
But it depends on the timezone issue.

Thank you.

I'm not familiar with icu code but why are you using detectHostTimeZone? I don't see where in Chrome we use that. Are you adding it somewhere, is it a part of yavta, or what? I would have expected you to be using createDefault, which should pick up on the time zone the user has set via settings (step 1 above), which goes through TimeZoneMonitorClient::OnTimeZoneChange (you can see the adoptDefault there).

I don't see a bug here because when I set the timezone to Asia/Shanghai, icu correctly gives me a country code of CN for TimeZone::createDefault

I agree with Evan. Why do you call detectHostTimeZone?  That's a very low level API and does not work inside a sandbox.  I don't know where you're calling it, though. 

I don't call detectHostTimeZone.
I call CountryCodeForCurrentTimezone in base/i18n/timezone.cc.
I just traced the code from icu::TimeZone::createDefault() in CountryCodeForCurrentTimezone to detectHostTimeZone()

When I set timezone to Asia/Shanghai, CountryCodeForCurrentTimezone returns US

The regression starts from R71-11140.0.0.
I tried R71-11139.0.0 and there is no issue.

regression CL: https://chromium-review.googlesource.com/c/chromium/src/+/1258986

Seems like after this feature enabled, new process doesn't call TimezoneSettingsImpl.
The constructor of TimezoneSettingsImpl will call icu::TimeZone::setDefault()

Hi Christian,
If we want to have TimezoneSettings instance in Video Capture Service, could you point the source which we can add it?
Thank you.

Hi Christian,

Video capture service needs timezone information to setup anti-banding.
But timezone information is in browser process.

We cannot just add TimezoneSettings instance in video capture service.
I tried to call chromeos::system::TimezoneSettings::GetInstance() in video_cpature_device.cc.
But there are two issues:
1. video capture service is not aware timezone setting change.
2. It breaks dependency

I think we need a way to query timezone from browser process.
Could you help fix it?
Thanks.

If we need to add a dependency to the service on something that lives in the Browser process, the way I recommend we do it is to inject the dependency into the service when it is started from the Browser process. 

We already do this for dependencies on GPU related things, see 
https://cs.chromium.org/chromium/src/services/video_capture/public/mojom/device_factory_provider.mojom?q=device_factory_provider.mojom&dr&l=27.

Would you happen to have time to work on a CL for that?

I'm not familiar with video capture service and I'm also working on other bugs to meet M72 stable.

We are really appreciate If you can help this.

I will do what I can to try to get a CL with a solution ready for this ASAP.

A couple of questions regarding the design of a solution.

Are we trying to support a scenario, where video capture session is in progress, and then the user switches the timezone and we expect the video capture service to react? Or is it that when starting a new session we want to pass a suitable power line frequency to the camera device?

In either case, to me it seems unnecessary to require the camera capture code to depend on and reason about time zones. We already have the ability to pass in a power line frequency when opening a device, see [1]. Would it be possible to solve the issue by making use of this, or is there an issue I am overlooking?

[1] https://cs.chromium.org/chromium/src/media/capture/video_capture_types.h?q=VideoCaptureParams&dr=CSs&l=294

I think we don't need to react timezone setting when video capture session is in progress.

I just have a CL and it works.
But I'm not sure whether it is ok to make VideoCaptureDevice::GetPowerLineFrequency to be static function.
https://chromium-review.googlesource.com/c/chromium/src/+/1420459

I took a look at the CL, and have the following design considerations. I'll post them here, since they might imply some changes that we may have to postpone to a later time, in order to be able to get a fix into M72.

First of all, making VideoCaptureDevice::GetPowerLineFrequency() is okay, since it does not depend on any member variable, indicating that this method is actually more general anyways and does not really belong into class VideoCaptureDevice.

I don't think class ServiceVideoCaptureDeviceLauncher is the right place for overwriting the power_line_frequency requested by the client. Note that for cases where the service is not being used, there is a class InProcessVideoCaptureDeviceLauncher. There should be a single and clear contract for who is responsible for setting this settings and how it is consumed, and this contract should be the same regardless of whether or not video capture runs in the video capture service or not.

Ideally, setting the setting should be the responsibility of the client who initially creates the settings struct media::VideoCaptureParams. This currently seems to happens in the Renderer at [1]. If the contract is supposed to be that Renderers can ask the Browser to fill in an explicit PowerLineFrequency for them while processing the request, there should be an enum value for that. I guess PowerLineFrequency.FREQUENCY_DEFAULT can serve as that value, even though it would be more clear to name this something like TO_BE_RESOLVED_BASED_ON_LOCATION. We could place the responsibility of resolving that somewhere early in the Browser-side video capture stack, e.g. at [2] or [3].

But when we do, this should be the only location where this resolving happens, and the video capture stack below that point should be able to rely on this already being resolved. To express this in code, this would mean using a different struct for the settings that does no longer include a TO_BE_RESOLVED_BASED_ON_LOCATION option below this point.

This means we must remove the calls to GetPowerLineFrequency() everywhere in the lower-level stack, e.g. at [4][5][6] and instead use the setting that has been passed in. And this absolutely needs to be done, because, as per this bug repost, the call to base::CountryCodeForCurrentTimezone() only works correctly in the Browser process.

[1] https://cs.chromium.org/chromium/src/content/renderer/media/stream/media_stream_constraints_util_video_device.cc?g=0&l=672
[2] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_host.cc?dr&g=0&l=204
[3] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?dr&g=0&l=367
[4] https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_device_linux.cc?q=GetPowerLineFrequency&l=77&ct=rc&cd=4&dr=C
[5] https://cs.chromium.org/chromium/src/media/capture/video/win/video_capture_device_win.cc?type=cs&q=GetPowerLineFrequency&g=0&l=924
[6] https://cs.chromium.org/chromium/src/media/capture/video/mac/video_capture_device_mac.mm?q=GetPowerLineFrequency&l=357&ct=rc&cd=5&dr=C

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

commit 2c76706cc87d1e46e7067cffaf91b01859b2d56d
Author: Heng-Ruey Hsu <henryhsu@chromium.org>
Date: Fri Jan 18 22:23:23 2019

Set power line frequency in video capture service

After video capture service is enabled by default, timezone settings
is not applied to video capture service. It causes camera anti-banding
setting is wrong.

BUG=920094,b:121302062
TEST=open camera to see anti-banding setting is correct

Change-Id: I21f8e50c641ac46e75bde7d4cb514091aa03a9e1
Reviewed-on: https://chromium-review.googlesource.com/c/1420459
Reviewed-by: Ricky Liang <jcliang@chromium.org>
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624320}
[modify] https://crrev.com/2c76706cc87d1e46e7067cffaf91b01859b2d56d/content/browser/renderer_host/media/service_video_capture_device_launcher.cc
[modify] https://crrev.com/2c76706cc87d1e46e7067cffaf91b01859b2d56d/media/capture/video/video_capture_device.cc
[modify] https://crrev.com/2c76706cc87d1e46e7067cffaf91b01859b2d56d/media/capture/video/video_capture_device.h

henryhsu@: Once a canary build including the fix is out, please verify on there that the fix is effective. After that, let us request merge into M72.

I'm waiting the chrome version uprev in latest image.
I think we can request merge first.
I'll cherry-pick it after verifying latest image.

This bug requires manual review: We are only 7 days from stable.
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot