Web MIDI: do not keep virtual synth open

media/midi/midi_manager_win.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/midi/midi_manager_win.h"
 
 #include <windows.h>
 #include <ks.h>
 #include <ksmedia.h>
 #include <mmreg.h>
@@ skipping 810 matching lines @@
     result = midiOutGetDevCaps(
         device_id, reinterpret_cast<LPMIDIOUTCAPSW>(&caps), sizeof(caps));
     if (result != MMSYSERR_NOERROR) {
       DLOG(ERROR) << "midiInGetDevCaps failed: " << GetOutErrorMessage(result);
       return;
     }
     auto state =
         make_scoped_refptr(new MidiOutputDeviceState(MidiDeviceInfo(caps)));
     state->midi_handle = midi_out_handle;
     const auto& state_device_info = state->device_info;
-    if (IsUnsupportedDevice(state_device_info))
-      return;
+    bool unsupported = IsUnsupportedDevice(state_device_info);

[Takashi Toyoshima, 2016/11/10 08:12:26]

Note: I tried to close handle here so to just return. But, closing handle inside
this callback seems to fail always. According to MSDN documents, calling MMAPI
inside midi callbacks are not allowed officially, and it may cause an internal
deadlock. I'm planning to rework legacy win32 backend impl later as a side work
of mojo-fication.

     bool add_new_port = false;
     uint32_t port_number = 0;
     {
       base::AutoLock auto_lock(output_ports_lock_);
       const auto it = unused_output_ports_.find(state_device_info);
       if (it == unused_output_ports_.end()) {
         port_number = static_cast<uint32_t>(output_ports_.size());
         add_new_port = true;
         output_ports_.push_back(nullptr);
         output_ports_ages_.push_back(0);
@@ skipping 10 matching lines @@
       output_ports_[port_number]->port_index = port_number;
       output_ports_[port_number]->port_age = output_ports_ages_[port_number];
     }
     if (add_new_port) {
       const MidiPortInfo port_info(
           // TODO(toyoshim): Use a hash ID insted. crbug.com/467448
           base::IntToString(static_cast<int>(port_number)),
           GetManufacturerName(state_device_info),
           base::WideToUTF8(state_device_info.product_name),
           MmversionToString(state_device_info.driver_version),
-          PortState::OPENED);
+          unsupported ? PortState::DISCONNECTED : PortState::OPENED);
       task_thread_.task_runner()->PostTask(
-          FROM_HERE, base::Bind(&MidiServiceWinImpl::AddOutputPortOnTaskThread,
-                                base::Unretained(this), port_info));
+          FROM_HERE,
+          base::Bind(&MidiServiceWinImpl::AddOutputPortOnTaskThread,
+                     base::Unretained(this), port_number, port_info));
     } else {
       task_thread_.task_runner()->PostTask(
           FROM_HERE,
-          base::Bind(&MidiServiceWinImpl::SetOutputPortStateOnTaskThread,
-                     base::Unretained(this), port_number,
-                     PortState::CONNECTED));
+          base::Bind(
+              &MidiServiceWinImpl::SetOutputPortStateOnTaskThread,
+              base::Unretained(this), port_number,
+              unsupported ? PortState::DISCONNECTED : PortState::CONNECTED));
     }
   }
 
   void OnMidiOutDoneOnMultimediaThread(HMIDIOUT midi_out_handle,
                                        DWORD_PTR param1) {
     auto state = GetOutputDeviceFromHandle(midi_out_handle);
     if (!state)
       return;
     // Take ownership of the MIDIHDR object.
     ScopedMIDIHDR header(reinterpret_cast<MIDIHDR*>(param1));
@@ skipping 184 matching lines @@
                                    base::TimeTicks time) {
     AssertOnTaskThread();
     delegate_->OnReceiveMidiData(port_index, data, time);
   }
 
   void AddInputPortOnTaskThread(MidiPortInfo info) {
     AssertOnTaskThread();
     delegate_->OnAddInputPort(info);
   }
 
-  void AddOutputPortOnTaskThread(MidiPortInfo info) {
+  void AddOutputPortOnTaskThread(uint32_t port_index, MidiPortInfo info) {
     AssertOnTaskThread();
+    if (info.state == PortState::DISCONNECTED) {

[yukawa, 2016/11/10 08:13:13]

Fine as long as this is a short-term fix.  Ideally I think we should have an
explicit PortState UNSUPPORTED rather than reusing DISCONNECTED though.

+      // Close immediately if the device is unsupported one. Device information
+      // should be updated inside MOM_CLOSE callback.
+      HMIDIOUT midi_handle = kInvalidMidiOutHandle;
+      {
+        base::AutoLock auto_lock(output_ports_lock_);
+        scoped_refptr<MidiOutputDeviceState> device_state =
+            output_ports_[port_index];
+        if (device_state)
+          midi_handle = device_state->midi_handle;
+      }
+      if (midi_handle != kInvalidMidiOutHandle)
+        midiOutClose(midi_handle);
+    }
     delegate_->OnAddOutputPort(info);
   }
 
   void SetInputPortStateOnTaskThread(uint32_t port_index, PortState state) {
     AssertOnTaskThread();
     delegate_->OnSetInputPortState(port_index, state);
   }
 
   void SetOutputPortStateOnTaskThread(uint32_t port_index, PortState state) {
     AssertOnTaskThread();
+    if (state == PortState::DISCONNECTED) {

[yukawa, 2016/11/10 08:13:13]

Ditto.

+      // Close immediately if the device is unsupported one and still opened.
+      // Device information should be updated inside MOM_CLOSE callback.
+      HMIDIOUT midi_handle = kInvalidMidiOutHandle;
+      {
+        base::AutoLock auto_lock(output_ports_lock_);
+        scoped_refptr<MidiOutputDeviceState> device_state =
+            output_ports_[port_index];
+        if (device_state && device_state->midi_handle != kInvalidMidiOutHandle)
+          midi_handle = device_state->midi_handle;
+      }
+      if (midi_handle != kInvalidMidiOutHandle)
+        midiOutClose(midi_handle);
+    }
     delegate_->OnSetOutputPortState(port_index, state);
   }
 
   /////////////////////////////////////////////////////////////////////////////
   // Fields:
   /////////////////////////////////////////////////////////////////////////////
 
   // Does not take ownership.
   MidiServiceWinDelegate* delegate_;
 
@@ skipping 95 matching lines @@
 }
 
 MidiManager* MidiManager::Create() {
   if (base::FeatureList::IsEnabled(features::kMidiManagerWinrt) &&
       base::win::GetVersion() >= base::win::VERSION_WIN10)
     return new MidiManagerWinrt();
   return new MidiManagerWin();
 }
 
 }  // namespace midi