Enable WebMIDI for Windows behind the flag

Enable WebMIDI for Windows behind the flag

This CL implements media::MIDIManagerWin on top of Windows multimedia API to provide native MIDI support for web applications.

BUG=303599
TEST=manually done on Windows 8.1

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238861

[Takashi Toyoshima, 2013-11-27 14:18:52]

looks almost good to me.
I'll add scherkus@ for media owner review.

https://codereview.chromium.org/29793006/diff/780001/media/midi/midi_manager_...
File media/midi/midi_manager_win.cc (right):

https://codereview.chromium.org/29793006/diff/780001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:464: MIDIManagerWin::MIDIManagerWin() {
I don't think I understand this comment correctly, and why reset() is needed
here.
Can you elaborate this?
AFAIK, MIDIManager is created at the very first step in the browser main loop.
On the other hand, sender thread is created when the first client is going to
send data.

https://codereview.chromium.org/29793006/diff/780001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:526: send_thread_.reset(NULL);
Hum... The same issue seems to happen in mac potentially. Can you add the same
logic in _mac.cc?

[yukawa, 2013-11-27 15:23:37]

https://codereview.chromium.org/29793006/diff/780001/media/midi/midi_manager_...
File media/midi/midi_manager_win.cc (right):

https://codereview.chromium.org/29793006/diff/780001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:464: MIDIManagerWin::MIDIManagerWin() {
On 2013/11/27 14:18:52, Takashi Toyoshima (chromium) wrote:
> I don't think I understand this comment correctly, and why reset() is needed
> here.
> Can you elaborate this?
> AFAIK, MIDIManager is created at the very first step in the browser main loop.
> On the other hand, sender thread is created when the first client is going to
> send data.

Oops, this is just a garbage. Removed.

https://codereview.chromium.org/29793006/diff/780001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:526: send_thread_.reset(NULL);
On 2013/11/27 14:18:52, Takashi Toyoshima (chromium) wrote:
> Hum... The same issue seems to happen in mac potentially. Can you add the same
> logic in _mac.cc?

Probably yes. I'm making a different CL to do it.

[scherkus (not reviewing), 2013-11-27 22:07:29]

it looks like not all of your CL was able to upload :(

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
File media/midi/midi_manager_win.cc (right):

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:7: #include <Windows.h>
Windows -> windows

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:97: // 1) Call midiOutLongMsg API and wait for
its completion within this
nit: can you annotate function calls in comments with ()?

e.g., midiOutLongMsg()

it's a bit of a commenting convention we have to make it easier to distinguish
method names in comments from classes and types

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:100: // 2) Allocate a buffer and copy |message|
to it, then call midiOutLongMsg
ditto here + on line 102

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:107: // synchronously. Thus option 1) still might
be a good option.
how long are these synchronous operations?

as you note, we can wedge the UI thread on shutdown while waiting for this task
to complete + send_thread_ to stop

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:183: obj.reset(NULL);
nit: no need for null

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:387: obj.reset(NULL);
nit: no need for NULL

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:399: // - MOM_DONE: SendLongMIDIMessageInternal
relies on this event to clean up
ditto for function names in comments

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:419: if (midi_handle_ == NULL)
nit: use !midi_handle_

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:436: // in the UI thread.
s/in/on/

also what do you mean by "not always"?

do we know when it's called on UI vs. some other thread? if it's not the UI
thread ... which thread is it?

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:468: {
do we need the extra scope blocks?

I'm not seeing any variables that are declared that would benefit from the extra
level of scope

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:520: // MIDIManagerWin::SendMIDIData will not be
called any longer. A drawback
ditto for function names in comments

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:521: // of this approach is if |send_thread_| is
somehow blocked forever,
do you know how long does it take to run SendMIDIDataInternal()?

is it possible for the OS to block for a long time?

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:523: send_thread_.reset(NULL);
nit: NULL not needed

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:535: // termination of this background thread.
technically this comment would apply to MIDIManagerMac as well, but I don't see
it resetting send_thread_

are you worried about the PostDelayedTask()?

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:559: base::Bind(&SendMIDIDataInternal, handle,
data),
since handle and data get copied, is it possible for handle to refer to invalid
midi device by time this task runs?

what will happen if SendMIDIDataInternal() runs w/ an invalid handle?

[yukawa, 2013-12-04 17:51:22]

scherkus@:
Sorry for big diff but handle lifetime issues and UI thread blocking issue in
cleanup time should be addressed in the patch set #4.
Could you take a look?

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
File media/midi/midi_manager_win.cc (right):

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:7: #include <Windows.h>
On 2013/11/27 22:07:30, scherkus wrote:
> Windows -> windows

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:97: // 1) Call midiOutLongMsg API and wait for
its completion within this
On 2013/11/27 22:07:30, scherkus wrote:
> nit: can you annotate function calls in comments with ()?
> 
> e.g., midiOutLongMsg()
> 
> it's a bit of a commenting convention we have to make it easier to distinguish
> method names in comments from classes and types

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:100: // 2) Allocate a buffer and copy |message|
to it, then call midiOutLongMsg
On 2013/11/27 22:07:30, scherkus wrote:
> ditto here + on line 102

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:107: // synchronously. Thus option 1) still might
be a good option.
On 2013/11/27 22:07:30, scherkus wrote:
> how long are these synchronous operations?
> 
> as you note, we can wedge the UI thread on shutdown while waiting for this
task
> to complete + send_thread_ to stop

On my Windows environment with a certain USB-MIDI keyboard, the transfer rate is
roughly 75 KByte/sec at plateau region. Thus it can take very long time unless
we put some size limit.

I tentatively put 60 KB size limit so that the API is blocked at most 1 sec or
so. I also introduced a |quitting_| flag to prevent pending messages from being
passed to APIs. Now my guts feeling is that |send_thread_.Stop()| takes roughly
1 sec in the worst case, which sounds to be an acceptable duration to me.
What do you think?

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:183: obj.reset(NULL);
On 2013/11/27 22:07:30, scherkus wrote:
> nit: no need for null

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:387: obj.reset(NULL);
On 2013/11/27 22:07:30, scherkus wrote:
> nit: no need for NULL

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:399: // - MOM_DONE: SendLongMIDIMessageInternal
relies on this event to clean up
On 2013/11/27 22:07:30, scherkus wrote:
> ditto for function names in comments

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:419: if (midi_handle_ == NULL)
On 2013/11/27 22:07:30, scherkus wrote:
> nit: use !midi_handle_

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:436: // in the UI thread.
On 2013/11/27 22:07:30, scherkus wrote:
> s/in/on/
> 
> also what do you mean by "not always"?
> 
> do we know when it's called on UI vs. some other thread? if it's not the UI
> thread ... which thread is it?

Done and added comment.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:468: {
On 2013/11/27 22:07:30, scherkus wrote:
> do we need the extra scope blocks?
> 
> I'm not seeing any variables that are declared that would benefit from the
extra
> level of scope

OK. Removed these blocks.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:520: // MIDIManagerWin::SendMIDIData will not be
called any longer. A drawback
On 2013/11/27 22:07:30, scherkus wrote:
> ditto for function names in comments

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:521: // of this approach is if |send_thread_| is
somehow blocked forever,
On 2013/11/27 22:07:30, scherkus wrote:
> do you know how long does it take to run SendMIDIDataInternal()?
> 
> is it possible for the OS to block for a long time?

It is supposed to be fast operation as long as there is no pending tasks. In the
worst case scenario, I estimate the upper limit as roughly 1 sec. See above
comment for details.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:523: send_thread_.reset(NULL);
On 2013/11/27 22:07:30, scherkus wrote:
> nit: NULL not needed

Done.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:535: // termination of this background thread.
On 2013/11/27 22:07:30, scherkus wrote:
> technically this comment would apply to MIDIManagerMac as well, but I don't
see
> it resetting send_thread_
> 
> are you worried about the PostDelayedTask()?

Mas issue has been resolved. PostDelayedTask() is also fine in the latest patch.

https://codereview.chromium.org/29793006/diff/840001/media/midi/midi_manager_...
media/midi/midi_manager_win.cc:559: base::Bind(&SendMIDIDataInternal, handle,
data),
On 2013/11/27 22:07:30, scherkus wrote:
> since handle and data get copied, is it possible for handle to refer to
invalid
> midi device by time this task runs?
> 
> what will happen if SendMIDIDataInternal() runs w/ an invalid handle?

Yeah, your concern was real, although Windows strictly verifies MIDI handles and
most of cases are supposed to cause nothing harmful.

I fixed that problem in the patch set 4.  In short, all we need to do is that to
make sure |send_thread_| is stopped before |out_devices_| is cleared.

Detailed background:
It turned out that MIDI handles will never be invalidated until
midiInClose()/midiOutClose() are called, even when a user has physically
unplugged the MIDI cable.  We actually receives MIM_CLOSE/MOM_CLOSE messages
when the cable is disconnected, but MIDI handles are still valid until we call
midiInClose()/midiOutClose().

[scherkus (not reviewing), 2013-12-04 18:01:04]

lgtm w/ comment

https://codereview.chromium.org/29793006/diff/1210006/media/midi/midi_manager...
File media/midi/midi_manager_win.cc (right):

https://codereview.chromium.org/29793006/diff/1210006/media/midi/midi_manager...
media/midi/midi_manager_win.cc:580:
client->AccumulateMIDIBytesSent(data.size());
hmm.. it seems like on the mac we also call this in ::SendMIDIData()

perhaps you may consider changing this to have the caller (MIDIHost?) calculate
this so each OS implementation doesn't need to concern itself with this API

feel free to file a bug / do a follow up CL -- I'm not intimately familiar w/
the requirements of the AccumulateMIDIBytesSent() api

[yukawa, 2013-12-04 18:40:21]

https://codereview.chromium.org/29793006/diff/1210006/media/midi/midi_manager...
File media/midi/midi_manager_win.cc (right):

https://codereview.chromium.org/29793006/diff/1210006/media/midi/midi_manager...
media/midi/midi_manager_win.cc:580:
client->AccumulateMIDIBytesSent(data.size());
On 2013/12/04 18:01:05, scherkus wrote:
> hmm.. it seems like on the mac we also call this in ::SendMIDIData()
> 
> perhaps you may consider changing this to have the caller (MIDIHost?)
calculate
> this so each OS implementation doesn't need to concern itself with this API
> 
> feel free to file a bug / do a follow up CL -- I'm not intimately familiar w/
> the requirements of the AccumulateMIDIBytesSent() api

Thanks. Left a TODO crbug.com/325810.

[commit-bot: I haz the power, 2013-12-05 00:34:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukawa@chromium.org/29793006/1290001

[commit-bot: I haz the power, 2013-12-05 02:19:04]

Change committed as 238861