Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701783003:
Web MIDI: implement receiving for dynamic manager instantiation on Windows (Closed)
|
Created:
3 years, 10 months ago by Takashi Toyoshima Modified:
3 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, toyoshim+midi_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb MIDI: implement receiving for dynamic manager instantiation on Windows
This patch makes input ports work perfectly.
BUG=497573, 617086, 672793
Review-Url: https://codereview.chromium.org/2701783003
Cr-Commit-Position: refs/heads/master@{#453850}
Committed: https://chromium.googlesource.com/chromium/src/+/6d4bb28c8ba6e8bf8f53879aa02f0c823a0aa98d
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase #Patch Set 3 : make map non-global/statically initialized object #
Total comments: 8
Patch Set 4 : move methods into private #
Total comments: 6
Patch Set 5 : review #7 #Patch Set 6 : remove unused function that clang found #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 23 (9 generated)
Description was changed from ========== Web MIDI: implement receiving for dynamic manager instantiation on Windows This patch makes input ports work perfectly. BUG=497573, 617086, 672793 ========== to ========== Web MIDI: implement receiving for dynamic manager instantiation on Windows This patch makes input ports work perfectly. BUG=497573, 617086, 672793 ==========
toyoshim@chromium.org changed reviewers: + yhirano@chromium.org
just in case https://codereview.chromium.org/2701783003/diff/1/media/midi/dynamically_init... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/1/media/midi/dynamically_init... media/midi/dynamically_initialized_midi_manager_win.cc:314: in_handle_ = kInvalidInHandle; Actually this was a real reason why Open() wasn't symmetric :) I just drop this a little complicated error handling part in the previous patch.
maybe we can start a review for the next patch in parallel? https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:144: // Resets the device. This stops receiving messages, and allows to release Actually, our legacy code of midi_manager_win.cc does not have this, and can not shutdown gracefully now. https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:186: const uint8_t status_byte = static_cast<uint8_t>(param1 & 0xff); Following code through PostReplyTask is same with midi_manager_win.cc https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:520: GetTaskLock()->AssertAcquired(); task lock should be obtained outside this method, and following code can access members safely. https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.h (right): https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.h:42: // Posts a task to TaskRunner, and ensures that the instance keeps alive while Move from private to public so to use this from PortManager. Another choice is to make PortManager friend class.
https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:198: } else { // msg == MIM_LONGDATA Is this true? In the old code, the corresponding switch has four clauses. If you are sure that msg == MIN_LONGDATA, please place a DCHECK instead of the comment. https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.h (right): https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.h:42: // Posts a task to TaskRunner, and ensures that the instance keeps alive while On 2017/02/24 08:53:34, Takashi Toyoshima wrote: > Move from private to public so to use this from PortManager. > Another choice is to make PortManager friend class. PortManager is an inner class so it can access to private members.
PTAL Patch Set 4. https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:198: } else { // msg == MIM_LONGDATA See line 166. But, it would be nicer to have DCHECK at the line 185. https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.h (right): https://codereview.chromium.org/2701783003/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.h:42: // Posts a task to TaskRunner, and ensures that the instance keeps alive while Oh, thanks. My understanding around access specifiers looks not clear. And, my previous comment wasn't also accurate. This is used with PortManager's method, but called outside PortManager, but in a static callback function. So, this still needs to be public. But... let me try converting the function to PortManager's static method. That would help me to avoid making these methods public.
https://codereview.chromium.org/2701783003/diff/60001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/60001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:302: base::Bind(&FinalizeInPort, in_handle_, hdr_.release())); Why don't you make FinalizeInPort take ScopedMidiHDR instead of LPMIDIHDR? You can use base::Passed. https://codereview.chromium.org/2701783003/diff/60001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:377: int instance_id_; const https://codereview.chromium.org/2701783003/diff/60001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:542: std::vector<uint8_t> data; DCHECK_LE(len, arraysize(kData));
Thanks. PTAL. https://codereview.chromium.org/2701783003/diff/60001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/60001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:302: base::Bind(&FinalizeInPort, in_handle_, hdr_.release())); On 2017/02/28 11:02:02, yhirano (slow) wrote: > Why don't you make FinalizeInPort take ScopedMidiHDR instead of LPMIDIHDR? You > can use base::Passed. Done. https://codereview.chromium.org/2701783003/diff/60001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:377: int instance_id_; On 2017/02/28 11:02:02, yhirano (slow) wrote: > const Done. https://codereview.chromium.org/2701783003/diff/60001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:542: std::vector<uint8_t> data; Done. I reordered lines around here a little to clarify what we want to check by the DCHECK.
lgtm
The CQ bit was checked by toyoshim@chromium.org
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
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2701783003/#ps100001 (title: "remove unused function that clang found")
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": 100001, "attempt_start_ts": 1488338847156970,
"parent_rev": "77e7b6ab3cce4efe1c1114920de1c7a951fe2552", "commit_rev":
"6d4bb28c8ba6e8bf8f53879aa02f0c823a0aa98d"}
Message was sent while issue was closed.
Description was changed from ========== Web MIDI: implement receiving for dynamic manager instantiation on Windows This patch makes input ports work perfectly. BUG=497573, 617086, 672793 ========== to ========== Web MIDI: implement receiving for dynamic manager instantiation on Windows This patch makes input ports work perfectly. BUG=497573, 617086, 672793 Review-Url: https://codereview.chromium.org/2701783003 Cr-Commit-Position: refs/heads/master@{#453850} Committed: https://chromium.googlesource.com/chromium/src/+/6d4bb28c8ba6e8bf8f53879aa02f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/6d4bb28c8ba6e8bf8f53879aa02f...
Message was sent while issue was closed.
https://codereview.chromium.org/2701783003/diff/100001/media/midi/dynamically... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/100001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:353: manager_->port_manager()->RegisterInHandle(in_handle_, index_); If |HandleMidiInCallback| receives |MIM_LONGDATA| before we reach here then |HandleMidiInCallback| just ignores |MIM_LONGDATA|, which means that we fail to call |::midiInAddBuffer()| and we no longer able to receive subsequent sysex message, right?
Message was sent while issue was closed.
https://codereview.chromium.org/2701783003/diff/100001/media/midi/dynamically... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/100001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:353: manager_->port_manager()->RegisterInHandle(in_handle_, index_); TaskLock is used to avoid such race situations. Open() runs while TaskLock being locked. (Note that we call GetTaskLock->AssertAcquired() inside RegisterInHandle to ensure that) HandleMidiInCallback should wait until Open() runs away at the very first step of obtaining the TaskLock. New implementation uses multi-threading only to be aligned with Chrome browser process and Win32 API global designs, but basically, we never want to run our actual code in parallel. TaskLock is used to serialize all class methods running on different threads.
Message was sent while issue was closed.
yukawa@chromium.org changed reviewers: + yukawa@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2701783003/diff/100001/media/midi/dynamically... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701783003/diff/100001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:353: manager_->port_manager()->RegisterInHandle(in_handle_, index_); Thank you for the explanation. On 2017/04/26 05:35:15, Takashi Toyoshima wrote: > Open() runs while TaskLock being locked. (Note that we call > GetTaskLock->AssertAcquired() inside RegisterInHandle to ensure that) > > HandleMidiInCallback should wait until Open() runs away at the very first step > of obtaining the TaskLock. If doing that does not cause any dead lock, then I'm OK. A bit more background: I had an impression that |midiInOpen()| is a synchronous API, which waits until |MIM_OPEN| is handled in |HandleMidiInCallback|. I suppose we are not hitting dead lock right now just because currently |HandleMidiInCallback| does not try to acquire the lock unless the event is either |MIM_DATA| or |MIM_LONGDATA|. Perhaps |midiInUnprepareHeader()| might have behaved in a similar way: it could be a blocking API that waits until some pending task is handled in |HandleMidiInCallback|. Either way, again, if we are not hitting any dead lock, then I'm OK. and thank you for cleaning up the code and design. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
