Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1056333002:
Web MIDI: Improve device change monitornig on Windows (Closed)
|
Created:
5 years, 8 months ago by Takashi Toyoshima Modified:
5 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, toyoshim+midi_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb MIDI: Improve device change monitornig on Windows
Current implementation monitors DBT_DEVICEARRIVAL mesage, but that
is not a right message to monitor device changes since actual device
list that OS holds will be updated a little later. Instead, monitor
DBT_DEVNODES_CHANGES that is delivered after the list being updated.
In Chrome, SystemMessageWindowWin invokes base::SystemMonitor's observer
with DEVTYPE_UNKNOWN for DBT_DEVNODES_CHANGED, and the SystemMonitor
provides a way to observe the event on the registered thread.
Using the monitor must be the right implementation.
BUG=472980
Committed: https://crrev.com/3e47c6dbf899d98f38db45f4cc766bd6a2eca5cd
Cr-Commit-Position: refs/heads/master@{#324031}
Patch Set 1 #Patch Set 2 : update #
Total comments: 25
Patch Set 3 : review #
Total comments: 4
Patch Set 4 : remove LOG(ERROR) for debugging #Patch Set 5 : for submit #Patch Set 6 : SystemMonitor in test #Messages
Total messages: 19 (6 generated)
toyoshim@chromium.org changed reviewers: + yhirano@chromium.org, yukawa@chromium.org
PTAL. This is a base change to fix the issue 472980. But this change itself fix a race on DBT_DEVICEARRIVAL.
My change note for the review. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:288: std::string GetManufacturerName(const MidiDeviceInfo& info) { This fix is split to another CL. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:459: sender_thread_.Start(); TYPE_IO has a misleading name, but it isn't for blocking call, but for async non-blocking calls. Let's use default type. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:547: void UpdateDeviceList() { Name is changed because we do not want to open the device here in the future. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:556: // - MIM_OPEN: This event is sent the input device is opened. Note that Note is added. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:593: DLOG_IF(ERROR, result != MMSYSERR_NOERROR && result != MMSYSERR_ALLOCATED) Minor cleanup to use DLOG_IF as the first loop for in devices does. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:604: OnMidiInEventOnMainlyMultimediaThread(HMIDIIN midi_in_handle, Name is changed because MIM_OPEN is not called on multimedia thread, but on the caller thread.
Thank you for deep investigation and improvement. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:288: std::string GetManufacturerName(const MidiDeviceInfo& info) { On 2015/04/06 18:02:38, Takashi Toyoshima (chromium) wrote: > This fix is split to another CL. Acknowledged. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:369: // delivered to the thread through the thread-safe SystemMonitor service. I'm afraid that "thread-safe" here is ambiguous. Although |base::SystemMonitor::AddDevicesChangedObserver| can be called from any thread except or the notification callback, the document comment on |base::SystemMonitor::RemoveDevicesChangedObserver| is extremely misleading. You MUST call |base::SystemMonitor::RemoveDevicesChangedObserver| from the same thread which called |base::SystemMonitor::AddDevicesChangedObserver|. See comment on |ObserverListThreadSafe::RemoveObserver|. Otherwise, you might see wired use-after-free crashes. http://crbug.com/404767 is an unfortunate example. The same comment to "thread-safe" in the CL description. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:408: base::SystemMonitor::Get()->RemoveDevicesChangedObserver(this); We might want to make sure that |RemoveDevicesChangedObserver| is called from the same thread which called AddObserver. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:459: sender_thread_.Start(); On 2015/04/06 18:02:38, Takashi Toyoshima (chromium) wrote: > TYPE_IO has a misleading name, but it isn't for blocking call, but for async > non-blocking calls. Let's use default type. Acknowledged. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:462: // Start monitoring device changes. This should start before the Can you elaborate a bit more on why we can avoid race. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:510: void OnDevicesChanged(base::SystemMonitor::DeviceType device_type) final { Can we predict (and have an assertion) that which thread calls this method? https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:518: UpdateDeviceList(); |UpdateDeviceList()| is supposed to be a blocking call. What will happen in other observers of SystemMonitor if |OnDevicesChanged| doesn't return immediately? https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:547: void UpdateDeviceList() { On 2015/04/06 18:02:38, Takashi Toyoshima (chromium) wrote: > Name is changed because we do not want to open the device here in the future. Acknowledged. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:556: // - MIM_OPEN: This event is sent the input device is opened. Note that On 2015/04/06 18:02:38, Takashi Toyoshima (chromium) wrote: > Note is added. Acknowledged. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:593: DLOG_IF(ERROR, result != MMSYSERR_NOERROR && result != MMSYSERR_ALLOCATED) On 2015/04/06 18:02:38, Takashi Toyoshima (chromium) wrote: > Minor cleanup to use DLOG_IF as the first loop for in devices does. Acknowledged. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:604: OnMidiInEventOnMainlyMultimediaThread(HMIDIIN midi_in_handle, On 2015/04/06 18:02:38, Takashi Toyoshima (chromium) wrote: > Name is changed because MIM_OPEN is not called on multimedia thread, but on the > caller thread. Acknowledged. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:972: Is this blank line intentional?
https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:369: // delivered to the thread through the thread-safe SystemMonitor service. Aha. "thread-safe" is always a vague word. I just mean that the callback can be registered from any thread and the callback is always invoked on the same thread here. I'd remove "thread-safe" word here. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:408: base::SystemMonitor::Get()->RemoveDevicesChangedObserver(this); ok, ThreadChecker is our friend. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:462: // Start monitoring device changes. This should start before the On 2015/04/06 18:41:40, yukawa wrote: > Can you elaborate a bit more on why we can avoid race. Done. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:510: void OnDevicesChanged(base::SystemMonitor::DeviceType device_type) final { On 2015/04/06 18:41:40, yukawa wrote: > Can we predict (and have an assertion) that which thread calls this method? Done. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:518: UpdateDeviceList(); OnDeviceChanged is invoked via PostTask on the registered thread. In our case, it's on the Chrome IO Thread. So, it isn't critical, but also isn't good. As far as I tried, all operations are not black listed on the thread. Anyway, let's post it to task thread. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:972: oops, that's a mistake.
PTAL. https://codereview.chromium.org/1056333002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/1056333002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_win.cc:589: LOG(ERROR) << "MIM_CLOSE"; oops, this is mistakenly merged from local debugging changes. removed in the next patch set.
LGTM with few minor/optional comments. https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/1056333002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_win.cc:518: UpdateDeviceList(); On 2015/04/06 19:51:47, Takashi Toyoshima (chromium) wrote: > OnDeviceChanged is invoked via PostTask on the registered thread. In our case, > it's on the Chrome IO Thread. So, it isn't critical, but also isn't good. As far > as I tried, all operations are not black listed on the thread. > Anyway, let's post it to task thread. The question which thread should be used for opening MIDI deviecs is difficult in practice because we also rely on the task thread to dispatch the MIDI Receive event. So in theory overloading tasks to the task thread might increase the risk of jitter in MIDI receive events. It depends on the event rate of DBT_DEVNODES_CHANGES and worst case blocking time of enumerating all the MIDI devices and opening all of them in real world though. I'm OK to start with the design of patch set #3, but we need to understand stats in real world to choose the best design. https://codereview.chromium.org/1056333002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/1056333002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_win.cc:412: base::SystemMonitor::Get()->RemoveDevicesChangedObserver(this); optional: We could update |destructor_started| first and immediately return from |OnDevicesChanged| when |destructor_started| is true. Note that all pending events will be fired from |RemoveDevicesChangedObserver(this)|. https://codereview.chromium.org/1056333002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_win.cc:589: LOG(ERROR) << "MIM_CLOSE"; Is this a debug code?
https://codereview.chromium.org/1056333002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/1056333002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_win.cc:412: base::SystemMonitor::Get()->RemoveDevicesChangedObserver(this); Oh, good catch. That's a good thing to be fixed. In general, volatile does not ensure load / store ordering between threads. But in our case, PostTask internally uses a membar to ensure the ordering for bound arguments, IIRC?
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukawa@chromium.org Link to the patchset: https://codereview.chromium.org/1056333002/#ps80001 (title: "for submit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1056333002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Oops, SystemMonitor should be instantiated explicitly in the unit test.
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukawa@chromium.org Link to the patchset: https://codereview.chromium.org/1056333002/#ps100001 (title: "SystemMonitor in test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1056333002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3e47c6dbf899d98f38db45f4cc766bd6a2eca5cd Cr-Commit-Position: refs/heads/master@{#324031} |
