Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243183002:
Web MIDI backend for Windows 10 (Closed)
|
Created:
4 years, 4 months ago by Shao-Chuan Lee Modified:
4 years, 3 months ago Reviewers:
Takashi Toyoshima 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 backend for Windows 10
Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy
WinMM APIs on Windows 10.
Note: the new implementation is currently enabled by setting a compile-time flag
(midi_winrt); since we distribute same binaries for all Windows versions, we
should include both implementations in the build and use a runtime flag for
enabling the new one.
BUG=512433
R=toyoshim@chromium.org
Committed: https://crrev.com/e7237592868a179ba66e6a44c19f89abb9d4483e
Cr-Commit-Position: refs/heads/master@{#415566}
Patch Set 1 #
Total comments: 27
Patch Set 2 #
Total comments: 67
Patch Set 3 #
Total comments: 29
Patch Set 4 : rebase, revise thread-safety #
Total comments: 2
Patch Set 5 : rebase, fixup #
Total comments: 27
Patch Set 6 : use base::win::ScopedComPtr, error handling #
Total comments: 58
Patch Set 7 : fixup, remove event handlers properly #
Total comments: 9
Patch Set 8 : StopWatcher(), invalid token value constant, fixup #
Total comments: 5
Patch Set 9 : RemovePortEventHandlers() in StopWatcher(), fixup #
Total comments: 5
Patch Set 10 : fix receiving timestamp #Patch Set 11 : crbug IDs #
Messages
Total messages: 46 (15 generated)
Here are my first thoughts. This review does not take a look details inside MidiService, but points general topics like styles, and design overview. https://codereview.chromium.org/2243183002/diff/1/build/config/win/midi_winrt... File build/config/win/midi_winrt.gni (right): https://codereview.chromium.org/2243183002/diff/1/build/config/win/midi_winrt... build/config/win/midi_winrt.gni:6: midi_winrt = false Can you have this declaration in media/midi? See media/media_options.gni for examples for media. Adding new file as media/midi/midi_options.gni, or just writing directly into media/midi/BUILD.gn looks fine. https://codereview.chromium.org/2243183002/diff/1/media/midi/BUILD.gn File media/midi/BUILD.gn (right): https://codereview.chromium.org/2243183002/diff/1/media/midi/BUILD.gn#newcode140 media/midi/BUILD.gn:140: deps += [ "//device/usb" ] Hum... this line 140 might be an existing bug. I may miss something, but current Windows backend does not depend on //device/usb. This is not your fault, but could you check if we can compile without this line? https://codereview.chromium.org/2243183002/diff/1/media/midi/BUILD.gn#newcode141 media/midi/BUILD.gn:141: sources += [ "midi_manager_win.h" ] I think we should have a dedicated header, _winrt.h, for for your new implementation. https://codereview.chromium.org/2243183002/diff/1/media/midi/BUILD.gn#newcode145 media/midi/BUILD.gn:145: libs += [ "runtimeobject.lib" ] Can you add TODO(crbug.com/512433) with comments for the line 145? This would be a simplified version of your note in the CL description. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:29: using Microsoft::WRL::Callback; To shorten this as Callback is a little confusing because Chromium developers will expect just Callback means base::Callback. How about 'using namespace Microsfot::WRL;' to use it as WRL::Callback? Or we may want to consider not to use using for Microsoft::*. I grep-ed chromium repository, and find some users of Micfosoft::WRL::*, but no one use 'using'. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:37: class WrlStatics { Please use base::LazyInstance to implement a singleton. This is because chromium disallow having static initializer as possible to startup as fast as possible. Or probably, we do not need this class at all. We can just have following functions and members in anonymous namespace in this file. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:69: static T* GetStatics(ComPtr<T>& com_ptr, wchar_t const* class_id) { chromium style suggests to use pointers* or const references&. So, if there is no technical reason, ComPtr<T>* is the right type here. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:69: static T* GetStatics(ComPtr<T>& com_ptr, wchar_t const* class_id) { Also wchar_t is disallowed in chromium even though it is for interacting with Windows API. Can you use bae::string16 instead. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > Don't use std::wstring. Use base::string16 or base::FilePath instead. (Windows-specific code interfacing with system APIs using wstring and wchar_t can still use string16 and char16; it is safe to assume that these are equivalent to the “wide” types.) https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:87: class DeviceWatcherWrapper { This looks over-abstracted to me. The midi_manager_win.cc is also a little over-abstracted. Could you reduce abstraction layers as possible unless you have enough reasons? https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:122: std::bind(&CallbackHandler::OnAdded, handler, _1, _2)) std::bind is classified to a C++11 features to be discussed. https://chromium-cpp.appspot.com/ But, this is required by the API. So, I think it's ok. But probably it's better to add some comments here because someone would grep code base and misunderstood that it's allowed to use. Also, it would be nice if you post a mail to a relevant group to excuse this. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:482: class MidiServiceWinrtImpl : public MidiServiceWin { Can you remove MidiService* layers and implement directly in MidiManagerWinrt? https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:599: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN10); Is this check enough? I mean is 10.0.10240.0 the first minor version of Windows 10? https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:621: midi_service_->SendMidiDataAsync(port_index, data, time_to_send); Can you use MidiScheduler here? It would also make it possible to remove the TODO below.
https://codereview.chromium.org/2243183002/diff/1/build/config/win/midi_winrt... File build/config/win/midi_winrt.gni (right): https://codereview.chromium.org/2243183002/diff/1/build/config/win/midi_winrt... build/config/win/midi_winrt.gni:6: midi_winrt = false On 2016/08/15 08:46:23, toyoshim wrote: > Can you have this declaration in media/midi? > See media/media_options.gni for examples for media. > > Adding new file as media/midi/midi_options.gni, or just writing directly into > media/midi/BUILD.gn looks fine. It looks like putting this in media/media_options.gni is a good choice. Anyway this is temporary and should be removed upon merge. https://codereview.chromium.org/2243183002/diff/1/media/midi/BUILD.gn File media/midi/BUILD.gn (right): https://codereview.chromium.org/2243183002/diff/1/media/midi/BUILD.gn#newcode140 media/midi/BUILD.gn:140: deps += [ "//device/usb" ] On 2016/08/15 08:46:23, toyoshim wrote: > Hum... this line 140 might be an existing bug. I may miss something, but current > Windows backend does not depend on //device/usb. > > This is not your fault, but could you check if we can compile without this line? Will have it removed in another CL. https://codereview.chromium.org/2243183002/diff/1/media/midi/BUILD.gn#newcode141 media/midi/BUILD.gn:141: sources += [ "midi_manager_win.h" ] On 2016/08/15 08:46:23, toyoshim wrote: > I think we should have a dedicated header, _winrt.h, for for your new > implementation. Done. https://codereview.chromium.org/2243183002/diff/1/media/midi/BUILD.gn#newcode145 media/midi/BUILD.gn:145: libs += [ "runtimeobject.lib" ] On 2016/08/15 08:46:23, toyoshim wrote: > Can you add TODO(crbug.com/512433) with comments for the line 145? > This would be a simplified version of your note in the CL description. Done. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:29: using Microsoft::WRL::Callback; On 2016/08/15 08:46:23, toyoshim wrote: > To shorten this as Callback is a little confusing because Chromium developers > will expect just Callback means base::Callback. > > How about 'using namespace Microsfot::WRL;' to use it as WRL::Callback? > Or we may want to consider not to use using for Microsoft::*. > I grep-ed chromium repository, and find some users of Micfosoft::WRL::*, but no > one use 'using'. > Now using `namespace WRL = Microsoft::WRL`. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:37: class WrlStatics { On 2016/08/15 08:46:23, toyoshim wrote: > Please use base::LazyInstance to implement a singleton. This is because chromium > disallow having static initializer as possible to startup as fast as possible. > > Or probably, we do not need this class at all. We can just have following > functions and members in anonymous namespace in this file. The singleton is now replaced with functions. Although GetActivationFactory() will be invoked on each call, it should not incur too much overhead. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:69: static T* GetStatics(ComPtr<T>& com_ptr, wchar_t const* class_id) { On 2016/08/15 08:46:23, toyoshim wrote: > Also wchar_t is disallowed in chromium even though it is for interacting with > Windows API. Can you use bae::string16 instead. > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > Don't use std::wstring. Use base::string16 or base::FilePath instead. > (Windows-specific code interfacing with system APIs using wstring and wchar_t > can still use string16 and char16; it is safe to assume that these are > equivalent to the “wide” types.) Done. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:69: static T* GetStatics(ComPtr<T>& com_ptr, wchar_t const* class_id) { On 2016/08/15 08:46:23, toyoshim wrote: > chromium style suggests to use pointers* or const references&. So, if there is > no technical reason, ComPtr<T>* is the right type here. Done. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:87: class DeviceWatcherWrapper { On 2016/08/15 08:46:24, toyoshim wrote: > This looks over-abstracted to me. > The midi_manager_win.cc is also a little over-abstracted. Could you reduce > abstraction layers as possible unless you have enough reasons? Merged into MidiPortManager. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:122: std::bind(&CallbackHandler::OnAdded, handler, _1, _2)) On 2016/08/15 08:46:23, toyoshim wrote: > std::bind is classified to a C++11 features to be discussed. > https://chromium-cpp.appspot.com/ > > But, this is required by the API. So, I think it's ok. But probably it's better > to add some comments here because someone would grep code base and misunderstood > that it's allowed to use. Also, it would be nice if you post a mail to a > relevant group to excuse this. Acknowledged, will look for advice on this issue (also on other WRL related stuff such as using ComPtr instead of base::win::ScopedComPtr). https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:482: class MidiServiceWinrtImpl : public MidiServiceWin { On 2016/08/15 08:46:24, toyoshim wrote: > Can you remove MidiService* layers and implement directly in MidiManagerWinrt? Done. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:599: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN10); On 2016/08/15 08:46:23, toyoshim wrote: > Is this check enough? > I mean is 10.0.10240.0 the first minor version of Windows 10? 10.0.10240.0 seems to be the first release of Windows 10 SDK. base::win::IsWindows10TabletMode also uses APIs since 10.0.10240.0 and checked the same. https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:621: midi_service_->SendMidiDataAsync(port_index, data, time_to_send); On 2016/08/15 08:46:24, toyoshim wrote: > Can you use MidiScheduler here? It would also make it possible to remove the > TODO below. Using MidiScheduler now, but an extra wrapper is required to have the scheduler run on COM thread.
https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/1/media/midi/midi_manager_win... media/midi/midi_manager_winrt.cc:122: std::bind(&CallbackHandler::OnAdded, handler, _1, _2)) On 2016/08/16 07:42:25, Shao-Chuan Lee wrote: > On 2016/08/15 08:46:23, toyoshim wrote: > > std::bind is classified to a C++11 features to be discussed. > > https://chromium-cpp.appspot.com/ > > > > But, this is required by the API. So, I think it's ok. But probably it's > better > > to add some comments here because someone would grep code base and > misunderstood > > that it's allowed to use. Also, it would be nice if you post a mail to a > > relevant group to excuse this. > > Acknowledged, will look for advice on this issue (also on other WRL related > stuff such as using ComPtr instead of base::win::ScopedComPtr). Just realized that WRL::Callback supports calling instance methods, will get rid of std::bind in next CL.
Great. Overall design looks almost fine. Let's go looking details. Sorry for many comments, but major comments are for styles. Others are comments for me to understand thread related code easily for the next review. https://codereview.chromium.org/2243183002/diff/20001/media/media_options.gni File media/media_options.gni (right): https://codereview.chromium.org/2243183002/diff/20001/media/media_options.gni... media/media_options.gni:51: midi_use_winrt = false I think it's fine to submit current CL with some TODOs left, and this could be one of them. In such case, this isn't a technical comment, but if you have this flag under media/midi, you do not need to ask an extra owner's review for media/ :) https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:37: InterfaceType* ptr; Probably com_ptr or something is better. Usually we avoid abbreviation in chromium code base as much as possible. Only exceptional cases are using it for a well-known terms like HTTP or API providing names like this case. |ptr| sounds too generic even here. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:46: auto GetBufferFactory() { Generally speaking, trailing return types are not encouraged to declare as "auto" because it makes code less readable. In this case, explicit declaration makes it easy to understand object ownership, like this returns ComPtrRef and we do not need to care for it. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:67: inline std::string GetIdString(T obj) { Shall we make this "inline" compiler's decision so that compiler can optimize for size or speed on demand. I think this code paths are not performance sensitive. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:68: HSTRING hs; "HRESULT hr" is very traditional abbreviation, but I'm not sure about HSTRING. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:70: DCHECK(!FAILED(hr)); Just a question: can we assume this call should success always unless OS panics? In some platforms, OS returns null or empty string for this kind of information, and we made some crash bugs before. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:74: template <typename T> same comments on the following methods. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:95: base::TimeTicks start_time; This looks working differently from the spec. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:104: MidiPortManager() {} FYI, we are allowed to use "= default" if you want. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:172: // TODO(shaochuan): Disable Microsoft GS Wavetable Synth due to security Do you know if we can identify software virtual ports here? Or plan is to have an explicit flag for the first Web MIDI call, requestMIDIAccess(), and if software is allowed, we popup a permission bubble to ask permission to users. So, it's ok to disable all software synths including MS one here at this point if it's easy to implement. Also probably we want to try running attack code against MS synth with this flag enabled later in order to understand what happens if it's exploited. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:175: HSTRING dev_id_hs; Can you use GetIdString you defined? https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:207: // TODO(shaochuan) What's missing here? At least, we should complete the initialization after the first enumeration finishes. It's ok to implement it in the next CL, but please write a clear TODO. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:223: // TODO(shaochuan) ditto https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:228: // TODO(shaochuan) ditto https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:250: // TODO(shaochuan) Clear comments for this TODO? https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:277: // Implemented by child classes to call corresponding input/output methods. "to return a corresponding device selector for input and output" sounds clear for me. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:280: virtual void RegisterGetPortFromIdAsync( Also can you add some comments for other virtual methods declared in this class? https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:294: base::Lock ports_lock_; Can you explain (== add comments) why locks are needed for them? Or rephrasing it, can you have a comment on threading in this file. Also can you have base/threading/thread_checker? It's useful to avoid methods running on unexpected threads, and also useful to provide thread expectation for other developers reading your code. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:344: midi_manager_->SetInputPortState(port_index, state); Probably, this call should be called on the I/O thread, but is this on com thread? https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:362: WRL::ComPtr<IInspectable> insp( Shall we have a clear name? https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:364: WRL::ComPtr<Windows::Storage::Streams::IBufferByteAccess> bba; ditto https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:368: uint8_t* data_arr = nullptr; ditto https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:372: uint32_t len; probably we may also want to avoid the name like len here. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:386: port->start_time + Can we check if how much start_time+time_span can be different from TimeTicks::Now() here. OnMessageReceived must be delivered on time as we can perceive, but I have a little concern about port opening, that means start_time might be inaccurate. Just in case. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:475: DCHECK_EQ(com_thread_.GetThreadId(), base::PlatformThread::CurrentId()); Ah, you may want to replace this with the thread checker I commented above. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:513: // Use BufferByteAccess object to access buffer memory directly. Clear names. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... File media/midi/midi_manager_winrt.h (right): https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:31: ///////////////////////////////////////////////////////////////////////////// This style of comments were used in _win.cc, but it isn't common in chromium. Sometime people use it for critical places that need developers' paying attentions. But in this case, normal style looks enough. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:43: ///////////////////////////////////////////////////////////////////////////// ditto https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:50: ///////////////////////////////////////////////////////////////////////////// ditto
Major changes in patch 3:
- WrlStaticsFactory (was GetStatics): now returns ComPtr objects, class_id moved
to template argument
- Check nullptr conditions during conversion from HSTRING to std::string,
templates for code reuse
- Adding template arguments for Midi{In,Out}PortStatics in MidiPortManager,
removing GetDeviceSelector and RegisterGetPortFromIdAsync for overriding
- Getting rid of std::bind
- Replacing !FAILED(hr) with SUCCEEDED(hr)
TODOs:
- Investigate WinRT threading model
- Identify software synths
- Check MidiPort start_time accuracy
https://codereview.chromium.org/2243183002/diff/20001/media/media_options.gni
File media/media_options.gni (right):
https://codereview.chromium.org/2243183002/diff/20001/media/media_options.gni...
media/media_options.gni:51: midi_use_winrt = false
On 2016/08/16 09:30:28, toyoshim wrote:
> I think it's fine to submit current CL with some TODOs left, and this could be
> one of them.
>
> In such case, this isn't a technical comment, but if you have this flag under
> media/midi, you do not need to ask an extra owner's review for media/ :)
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
File media/midi/midi_manager_winrt.cc (right):
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:33: using std::placeholders::_2;
Unused, removed
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:37: InterfaceType* ptr;
On 2016/08/16 09:30:29, toyoshim wrote:
> Probably com_ptr or something is better.
> Usually we avoid abbreviation in chromium code base as much as possible. Only
> exceptional cases are using it for a well-known terms like HTTP or API
providing
> names like this case. |ptr| sounds too generic even here.
Done. Also changed to using ComPtr to have ref-counting work correctly. "In the
general case any COM pointer you get back from a function should have its
reference count incremented so that you're the owner of that object."
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:46: auto GetBufferFactory() {
On 2016/08/16 09:30:29, toyoshim wrote:
> Generally speaking, trailing return types are not encouraged to declare as
> "auto" because it makes code less readable.
>
> In this case, explicit declaration makes it easy to understand object
ownership,
> like this returns ComPtrRef and we do not need to care for it.
Done. Now these factory functions return ComPtr which is owned by the caller.
Added comment on this.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:67: inline std::string GetIdString(T obj) {
On 2016/08/16 09:30:29, toyoshim wrote:
> Shall we make this "inline" compiler's decision so that compiler can optimize
> for size or speed on demand. I think this code paths are not performance
> sensitive.
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:68: HSTRING hs;
On 2016/08/16 09:30:28, toyoshim wrote:
> "HRESULT hr" is very traditional abbreviation, but I'm not sure about HSTRING.
Using `HSTRING result' now.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:70: DCHECK(!FAILED(hr));
On 2016/08/16 09:30:29, toyoshim wrote:
> Just a question: can we assume this call should success always unless OS
panics?
> In some platforms, OS returns null or empty string for this kind of
information,
> and we made some crash bugs before.
HSTRING uses nullptr to represent empty strings, in such cases I guess it should
still return S_OK, however I'll check if the users of HSTRING accepts nullptr.
It seems that WRL suggests checking every returning HRESULT. Simple calls like
these may return errors in cases such as running out of memory. Checks may be
skipped for methods that are documented to always return S_OK.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:71: return
base::WideToUTF8(WindowsGetStringRawBuffer(hs, nullptr));
This is calling base::WideToUTF8(const std::wstring&), and constructing
std::wstring from nullptr is undefined behavior. Added additional checks.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:74: template <typename T>
On 2016/08/16 09:30:29, toyoshim wrote:
> same comments on the following methods.
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:104: MidiPortManager() {}
On 2016/08/16 09:30:29, toyoshim wrote:
> FYI, we are allowed to use "= default" if you want.
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:172: // TODO(shaochuan): Disable Microsoft GS
Wavetable Synth due to security
On 2016/08/16 09:30:29, toyoshim wrote:
> Do you know if we can identify software virtual ports here?
>
> Or plan is to have an explicit flag for the first Web MIDI call,
> requestMIDIAccess(), and if software is allowed, we popup a permission bubble
to
> ask permission to users.
>
> So, it's ok to disable all software synths including MS one here at this point
> if it's easy to implement.
>
> Also probably we want to try running attack code against MS synth with this
flag
> enabled later in order to understand what happens if it's exploited.
The MS synth can be identified using WinRT MidiSynthesizer class
(https://msdn.microsoft.com/en-us/library/windows/apps/windows.devices.midi.mi...).
Will look for ways to identify other software synths.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:175: HSTRING dev_id_hs;
On 2016/08/16 09:30:29, toyoshim wrote:
> Can you use GetIdString you defined?
This HSTRING is required for RegisterGetPortFromIdAsync afterwards. Renamed to
dev_id_hstring.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:180:
base::WideToUTF8(WindowsGetStringRawBuffer(dev_id_hs, nullptr));
Now using GetIdString to reuse nullptr checking code.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:207: // TODO(shaochuan)
On 2016/08/16 09:30:28, toyoshim wrote:
> What's missing here?
> At least, we should complete the initialization after the first enumeration
> finishes.
> It's ok to implement it in the next CL, but please write a clear TODO.
Should CompleteInitialization() be moved here? I wonder if this would slow down
Chrome startup.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:223: // TODO(shaochuan)
On 2016/08/16 09:30:29, toyoshim wrote:
> ditto
Does nothing for now, leaving as a placeholder for future usage.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:228: // TODO(shaochuan)
On 2016/08/16 09:30:29, toyoshim wrote:
> ditto
Should update device information here, will check which fields are updated
through this callback.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:250: // TODO(shaochuan)
On 2016/08/16 09:30:28, toyoshim wrote:
> Clear comments for this TODO?
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:277: // Implemented by child classes to call
corresponding input/output methods.
On 2016/08/16 09:30:28, toyoshim wrote:
> "to return a corresponding device selector for input and output" sounds clear
> for me.
This comment was meant to describe all the following virtual functions... Now
GetDeviceSelector and RegisterGetPortFromIdAsync is removed, and comments added
for the rest 3 methods.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:280: virtual void RegisterGetPortFromIdAsync(
On 2016/08/16 09:30:29, toyoshim wrote:
> Also can you add some comments for other virtual methods declared in this
class?
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:294: base::Lock ports_lock_;
On 2016/08/16 09:30:29, toyoshim wrote:
> Can you explain (== add comments) why locks are needed for them?
> Or rephrasing it, can you have a comment on threading in this file.
Locks are required since fields are manipulated from WinRT callback threads,
where fields may be accessed concurrently. Added comments.
>
> Also can you have base/threading/thread_checker?
> It's useful to avoid methods running on unexpected threads, and also useful to
> provide thread expectation for other developers reading your code.
For now only WinRT methods should be either call on the COM thread or on the OS
callback thread. Will further investigate the WinRT threading model and revise
this.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:344:
midi_manager_->SetInputPortState(port_index, state);
On 2016/08/16 09:30:29, toyoshim wrote:
> Probably, this call should be called on the I/O thread, but is this on com
> thread?
This is currently called on the OS callback thread. Are there documents on which
methods should be called on which threads?
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:362: WRL::ComPtr<IInspectable> insp(
On 2016/08/16 09:30:29, toyoshim wrote:
> Shall we have a clear name?
Moved to a separate function, using `buffer_byte_access' and `inspectable'.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:364:
WRL::ComPtr<Windows::Storage::Streams::IBufferByteAccess> bba;
On 2016/08/16 09:30:29, toyoshim wrote:
> ditto
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:368: uint8_t* data_arr = nullptr;
On 2016/08/16 09:30:29, toyoshim wrote:
> ditto
Using `p_buffer_data'.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:369: hr =
bba->Buffer(reinterpret_cast<byte**>(&data_arr));
Removing reinterpret_cast here since byte** is compatible with uint8_t**, and to
be able to detect errors during compilation if any mistake is made in the
future.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:372: uint32_t len;
On 2016/08/16 09:30:29, toyoshim wrote:
> probably we may also want to avoid the name like len here.
Using `data_length'.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:386: port->start_time +
On 2016/08/16 09:30:29, toyoshim wrote:
> Can we check if how much start_time+time_span can be different from
> TimeTicks::Now() here. OnMessageReceived must be delivered on time as we can
> perceive, but I have a little concern about port opening, that means
start_time
> might be inaccurate. Just in case.
message->get_Timestamp() returns "the duration from when the MidiInPort was
created to the time the message was received," according to
https://msdn.microsoft.com/en-us/library/windows/apps/windows.devices.midi.im....
Now retrieving the "create time" in the beginning of
OnCompletedGetPortFromIdAsync, will check accuracy.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:513: // Use BufferByteAccess object to access
buffer memory directly.
On 2016/08/16 09:30:29, toyoshim wrote:
> Clear names.
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.cc:520: hr =
bba->Buffer(reinterpret_cast<byte**>(&data_arr));
ditto
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
File media/midi/midi_manager_winrt.h (right):
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.h:31:
/////////////////////////////////////////////////////////////////////////////
On 2016/08/16 09:30:29, toyoshim wrote:
> This style of comments were used in _win.cc, but it isn't common in chromium.
> Sometime people use it for critical places that need developers' paying
> attentions. But in this case, normal style looks enough.
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.h:43:
/////////////////////////////////////////////////////////////////////////////
On 2016/08/16 09:30:30, toyoshim wrote:
> ditto
Done.
https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager_winrt.h:50:
/////////////////////////////////////////////////////////////////////////////
On 2016/08/16 09:30:30, toyoshim wrote:
> ditto
Done.
https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:33: WRL::ComPtr<InterfaceType> WrlStaticsFactory() { Missing TODO: try replacing WRL::ComPtr with base::win::ScopedComPtr. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:124: hr = WrlStaticsFactory<StaticsInterfaceType, runtime_class_id>() Retrieve object from WrlStaticsFactory at constructor for reuse?
OK, let's discuss thread related topics. I didn't take a look carefully around port managers yet. Probably, it's better to have a consensus on thread related designs first. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:70: DCHECK(!FAILED(hr)); On 2016/08/18 08:48:56, Shao-Chuan Lee wrote: > On 2016/08/16 09:30:29, toyoshim wrote: > > Just a question: can we assume this call should success always unless OS > panics? > > In some platforms, OS returns null or empty string for this kind of > information, > > and we made some crash bugs before. > > HSTRING uses nullptr to represent empty strings, in such cases I guess it should > still return S_OK, however I'll check if the users of HSTRING accepts nullptr. > > It seems that WRL suggests checking every returning HRESULT. Simple calls like > these may return errors in cases such as running out of memory. Checks may be > skipped for methods that are documented to always return S_OK. Acknowledged. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:172: // TODO(shaochuan): Disable Microsoft GS Wavetable Synth due to security On 2016/08/18 08:48:57, Shao-Chuan Lee wrote: > On 2016/08/16 09:30:29, toyoshim wrote: > > Do you know if we can identify software virtual ports here? > > > > Or plan is to have an explicit flag for the first Web MIDI call, > > requestMIDIAccess(), and if software is allowed, we popup a permission bubble > to > > ask permission to users. > > > > So, it's ok to disable all software synths including MS one here at this point > > if it's easy to implement. > > > > Also probably we want to try running attack code against MS synth with this > flag > > enabled later in order to understand what happens if it's exploited. > > The MS synth can be identified using WinRT MidiSynthesizer class > (https://msdn.microsoft.com/en-us/library/windows/apps/windows.devices.midi.mi...). > Will look for ways to identify other software synths. Acknowledged. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:175: HSTRING dev_id_hs; Ah, I see. I misunderstood these lines. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:207: // TODO(shaochuan) This isn't a problem for two reasons. - MidiManager's initialization was designed to be asynchronous. Thus, slow initialization does not block anything. - The initialization happens on the first JavaScript call to use Web MIDI API. So this does not run until Web MIDI is used. Here is a scenario how the initialization happens for Web MIDI. 1. JavaScript calls navigator.requestMIDIAccess() that returns a Promise object immediately 2. renderer ask browser to initialize the MidiManager asynchronously 3. MidiManager.StartSession is called for each Web MIDI request on the I/O thread 4. MidiManagerWinrt.StartInitialization runs on the I/O thead if this is the first StartSession call 5. Existing devices should be enumerated here 6. CompleteInitialization() is called when asynchronous platform dependent initialization finishes 7. the renderer was notified to resolve the promise object. 8. For JavaScript the Promise object was resolved, and success or error callback will be invoked to obtain the first set of MIDIPort objects. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:344: midi_manager_->SetInputPortState(port_index, state); Basically, if you can not find a clear comment, it isn't thread safe. But as you requested, we should have some comments on MidiManagerClient because MidiManager runs on multiple threads, and it needs to interact with the class. Another question: Can we ensure such OS running callbacks always run before the MidiManager instance is destructed? Otherwise, these callbacks will touch the pointer for the destructed instance, and result in read-after-free issues. Some approaches I see often are: - All callbacks are aborted when a kind of interface pointer is released, or interface is closed. - Use static method with a weak pointer to call a member method indirectly only when the pointer is valid. https://codereview.chromium.org/2243183002/diff/20001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:475: DCHECK_EQ(com_thread_.GetThreadId(), base::PlatformThread::CurrentId()); not fixed yet? https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:63: if (buffer) { You don't need "{}" for one line. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:66: return ""; returning std::string() is recommended to return an empty string in chromium. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:423: void MidiManagerWinrt::StartInitialization() { You need a lock object to access class members to ensure modifications are synchronized between the main thread and the I/O thread. MidiManagerWinrt is constructed and destructed on the main thread, and StartInitialization() and Finalize() run on the I/O thread. Otherwise, double-free-ish crashes may happen randomly inside the destructor. Note: If you are not so familiar with multi-threading specific problems, please let me know. I can have an offline chat. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:426: scheduler_.reset(new MidiScheduler(this)); Now scheduler_ is constructed on the I/O thread, and used on the com thread too. This causes race issues. As I discussed offline, can you modify MidiScheduler to take a task runner that runs the posted task on requested time? https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:431: com_thread_.message_loop()->PostTask( Instead of message_loop(), task_runner() is recommended to be used for new code. See crbug.com/616447 for details. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:435: CompleteInitialization(Result::OK); As I commented for the previous patch set, it's fine to move this call to the place there code runs after the all initialization finishes. TODO is fine for this CL. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:439: // TODO(shaochuan): Check if everything is destructed gracefully. As I commented for StartInitialization(), we need a lock here. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:440: port_manager_in_.reset(); Since these are initialized on the com thread, these should be destructed on the com thread, too. To solve this, you can post a finalize task to the com thread, then call Stop(). It runs all posted tasks on the target thread, and the caller blocks until tasks finish. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:452: com_thread_.message_loop()->PostTask( task_runner https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:459: DCHECK_EQ(com_thread_.GetThreadId(), base::PlatformThread::CurrentId()); Use base/threading/therad_checker instead of having this method? https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:493: Is it fine to continue following steps when HRESULT contains an error result? DCHECK runs only on debug build, and production will continue to run even on the condition the check fails. I'm afraid that buffer related operation's failure could result in a security issue.
Some updates. Changes to MidiScheduler now available for review at https://codereview.chromium.org/2262043002/. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:40: return std::move(com_ptr); std::move not required, NRVO applies here. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:44: return std::move( ditto https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:50: return std::move( ditto https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:63: if (buffer) { On 2016/08/18 11:09:22, toyoshim wrote: > You don't need "{}" for one line. Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:66: return ""; On 2016/08/18 11:09:22, toyoshim wrote: > returning std::string() is recommended to return an empty string in chromium. Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:426: scheduler_.reset(new MidiScheduler(this)); On 2016/08/18 11:09:21, toyoshim wrote: > Now scheduler_ is constructed on the I/O thread, and used on the com thread too. > This causes race issues. > > As I discussed offline, can you modify MidiScheduler to take a task runner that > runs the posted task on requested time? Changes in separate CL https://codereview.chromium.org/2262043002/. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:431: com_thread_.message_loop()->PostTask( On 2016/08/18 11:09:22, toyoshim wrote: > Instead of message_loop(), task_runner() is recommended to be used for new code. > See crbug.com/616447 for details. Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:452: com_thread_.message_loop()->PostTask( On 2016/08/18 11:09:21, toyoshim wrote: > task_runner Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:459: DCHECK_EQ(com_thread_.GetThreadId(), base::PlatformThread::CurrentId()); On 2016/08/18 11:09:22, toyoshim wrote: > Use base/threading/therad_checker instead of having this method? Done.
Thread-safety revised: - Now MidiPortManager is constructed/destructed on the COM thread, and all callbacks from WinRT post tasks back to COM thread to avoid racing issues. Callbacks are lambda functions that capture WeakPtrs by value during registration for future use in PostTask, guaranteeing the posted tasks are run iff MidiPortManager is still alive. Unnecessary locks may also be removed. - In MidiManagerWinrt, all manipulations to MidiPortManager are done on COM thread now. An additional lock for |scheduler_| is added since it's being destructed on COM thread but may be used in DispatchSendMidiData() from Chrome_IOThread. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:40: return std::move(com_ptr); On 2016/08/22 06:53:17, Shao-Chuan Lee wrote: > std::move not required, NRVO applies here. Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:44: return std::move( On 2016/08/22 06:53:17, Shao-Chuan Lee wrote: > ditto Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:50: return std::move( On 2016/08/22 06:53:17, Shao-Chuan Lee wrote: > ditto Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:124: hr = WrlStaticsFactory<StaticsInterfaceType, runtime_class_id>() On 2016/08/18 08:57:50, Shao-Chuan Lee wrote: > Retrieve object from WrlStaticsFactory at constructor for reuse? Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:435: CompleteInitialization(Result::OK); On 2016/08/18 11:09:22, toyoshim wrote: > As I commented for the previous patch set, it's fine to move this call to the > place there code runs after the all initialization finishes. > > TODO is fine for this CL. Now called when both MidiPortManagers are ready (OnEnumerationComplete). https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:440: port_manager_in_.reset(); On 2016/08/18 11:09:22, toyoshim wrote: > Since these are initialized on the com thread, these should be destructed on the > com thread, too. > > To solve this, you can post a finalize task to the com thread, then call Stop(). > It runs all posted tasks on the target thread, and the caller blocks until tasks > finish. Done. https://codereview.chromium.org/2243183002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:493: On 2016/08/18 11:09:22, toyoshim wrote: > Is it fine to continue following steps when HRESULT contains an error result? > DCHECK runs only on debug build, and production will continue to run even on the > condition the check fails. > > I'm afraid that buffer related operation's failure could result in a security > issue. I guess buffer allocation may fail on cases such as OOM. We should confirm which DCHECKs should be replaced with CHECKs.
https://codereview.chromium.org/2243183002/diff/60001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/60001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:120: void StartWatcher() { Missing |thread_checker_| check. https://codereview.chromium.org/2243183002/diff/60001/media/midi/midi_manager... File media/midi/midi_manager_winrt.h (right): https://codereview.chromium.org/2243183002/diff/60001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:32: // Sets |is_initialized_| and calls CompleteInitialization() when both |is_initialized_| is already removed, fix comments.
Now design looks almost fine. Let's solve minor nits. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:131: DCHECK(SUCCEEDED(hr)); Probably, TODO is needed so that CompleteInitialization() is called with an error code for failed cases? https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:249: // The owning MidiManagerWinrt. "owning" sounds confusing here. Probably, you need to say taking a raw pointer to the instance, and the instance is expected to outlive always. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:266: port_names_[dev_id] = dev_name; question: is |dev_id| unique and persistent ID for each device? Can we just expect the same |dev_id| is reassigned when you disconnect and connect the same device again? Web MIDI expects reusing the same port_index for the same device. So, if the dev_id is unique and persistent, and exists in |port_ids_|, we should re-enable the existing port. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:332: DCHECK(thread_checker_.CalledOnValidThread()); Probably this method may run after OnEnumerationCompleted() is invoked? If so, we need to mange this async operations' completion to run OnPortManagerReady() at the right place. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:477: DCHECK(port != nullptr); DCHECK_NE or just DCHECK(port)? https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:545: port_manager_ready_count_ = 0; Probably, this isn't needed unless you have a special reason. It's easy to say that |port_manager_ready_count_| is accessed only from the com thread. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:563: if (!com_thread_checker_) Can we just simply assume InitializeOnComThread is called only once, and com_thread_checker should be null here? If so, can we just use CHECK? https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:579: void MidiManagerWinrt::FinalizeOnComThread() { We need to have a lock to reflect pointer resets to other threads. Otherwise, MidiManagerWinrt's destructor might release them again even though they are destructed on another thread. Also, we should reset all objects constructed on the com thread. So, com_thread_checker_ should be also reset at the end of this method. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:610: hr = port->handle->SendBuffer(buffer.Get()); This is just a question because of my lack of C++/CX knowledge. buffer.Get() returns IBuffer* here, but SendBuffer() requires IBuffer^. My understanding is ComPtr is unique_ptr like smart pointer, and I'm not sure how it works with "^" that manages ref counted objects. => asked offine, but let me leave this here for record. SendBuffer() takes a raw IBuffer pointer if we use vanilla C++ without CX extension. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... File media/midi/midi_manager_winrt.h (right): https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:13: #include "media/midi/midi_scheduler.h" Now we do not need to include midi_scheduler.h, but forward declaration is enough (and recommended). https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:32: // Calls CompleteInitialization() when both MidiPortManagers are ready. If we do not have any negative impact, can we make MidiPortManager subclass too so that we can make OnPortManagerReady private method? https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:41: // Subclasses that access protected members of MidiManager. note: private? https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:46: base::Thread com_thread_; It would be better to make this unique_ptr and construct and destruct on the I/O thread.
Major updates: - MidiScheduler CL merged (https://codereview.chromium.org/2262043002/). - Replaced all Microsoft::WRL::ComPtr with base::win::ScopedComPtr. Note that now we're using raw pointers for IAsyncOperation<> and should maintain references by hand, since ScopedComPtr<IAsyncOperation<>> cannot be declared as class member due to some compiler bug. - Revised error handling during API calls. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:131: DCHECK(SUCCEEDED(hr)); On 2016/08/23 08:09:37, toyoshim wrote: > Probably, TODO is needed so that CompleteInitialization() is called with an > error code for failed cases? Now returning Result::INITIALIZATION_ERROR if any API calls in StartWatcher() fails. Failures in callbacks are ignored with log emitted. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:249: // The owning MidiManagerWinrt. On 2016/08/23 08:09:38, toyoshim wrote: > "owning" sounds confusing here. > Probably, you need to say taking a raw pointer to the instance, and the instance > is expected to outlive always. Done. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:266: port_names_[dev_id] = dev_name; On 2016/08/23 08:09:38, toyoshim wrote: > question: is |dev_id| unique and persistent ID for each device? > Can we just expect the same |dev_id| is reassigned when you disconnect and > connect the same device again? > > Web MIDI expects reusing the same port_index for the same device. So, if the > dev_id is unique and persistent, and exists in |port_ids_|, we should re-enable > the existing port. I'm using device IDs provided by WinRT, like this: \\?\SWD#MMDEVAPI#MIDII_E959792C.P_0001#{504be32c-ccf6-4d2c-b73f-6f8b3747e22b} However the device ID seems associated with the connecting I/O bus, e.g. for USB devices the ID changes when plugged into another USB port. Now the existing MIDI port is re-enabled if device ID matches, i.e. if the same device is plugged back to the same port. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:332: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/08/23 08:09:38, toyoshim wrote: > Probably this method may run after OnEnumerationCompleted() is invoked? > If so, we need to mange this async operations' completion to run > OnPortManagerReady() at the right place. Done, now OnPortManagerReady() is called after all ports are ready. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:477: DCHECK(port != nullptr); On 2016/08/23 08:09:38, toyoshim wrote: > DCHECK_NE or just DCHECK(port)? Done. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:545: port_manager_ready_count_ = 0; On 2016/08/23 08:09:38, toyoshim wrote: > Probably, this isn't needed unless you have a special reason. It's easy to say > that |port_manager_ready_count_| is accessed only from the com thread. Done. I was assuming that StartInitialization() and Finalize() may be called more that once, but this is not the case. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:563: if (!com_thread_checker_) On 2016/08/23 08:09:37, toyoshim wrote: > Can we just simply assume InitializeOnComThread is called only once, and > com_thread_checker should be null here? > If so, can we just use CHECK? Done, I think CHECK is not required here. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:579: void MidiManagerWinrt::FinalizeOnComThread() { On 2016/08/23 08:09:38, toyoshim wrote: > We need to have a lock to reflect pointer resets to other threads. Otherwise, > MidiManagerWinrt's destructor might release them again even though they are > destructed on another thread. > > Also, we should reset all objects constructed on the com thread. > > So, com_thread_checker_ should be also reset at the end of this method. Now using a single lock for all lazy-initialized members, in InitializeOnComThread(), FinalizeOnComThread() and the dtor. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... File media/midi/midi_manager_winrt.h (right): https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:32: // Calls CompleteInitialization() when both MidiPortManagers are ready. On 2016/08/23 08:09:38, toyoshim wrote: > If we do not have any negative impact, can we make MidiPortManager subclass too > so that we can make OnPortManagerReady private method? Done. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:41: // Subclasses that access protected members of MidiManager. On 2016/08/23 08:09:38, toyoshim wrote: > note: private? Done. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:46: base::Thread com_thread_; On 2016/08/23 08:09:38, toyoshim wrote: > It would be better to make this unique_ptr and construct and destruct on the I/O > thread. Now I'm using a lock to synchronize between FinalizeOnComThread() and the destructor, to do this I'll need additional locks. In ALSA and Mac implementations the threads are also declared as class member, maybe this is OK?
https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... File media/midi/midi_manager_winrt.h (right): https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:13: #include "media/midi/midi_scheduler.h" On 2016/08/23 08:09:38, toyoshim wrote: > Now we do not need to include midi_scheduler.h, but forward declaration is > enough (and recommended). Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:15: #include "base/containers/hash_tables.h" Deprecated (https://crbug.com/576864). base::hash_map is just an alias of std::unordered_map now, will replace all usages. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:500: std::set<IAsyncOperation<RuntimeType*>*> async_ops_; Now using raw pointers and is hashable, may use std::unordered_set now. Is memory usage a concern? If so, std::list should be a good alternative as |async_op|s seems likely, though not guaranteed, to complete in the order they were issued.
Minor issues, will fix in next patch set. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:532: handle->add_MessageReceived( Add error handling here, this MidiInPort should not be available if event registration fails. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:709: port_manager_out_->StartWatcher())) { Should initialize MidiOutPortManager first in case the initialization failed but MidiInPortManager begins to receive data. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:758: return; Move this to the beginning of function to return early if port is invalid. |port->handle| should also be checked in case the port is disconnected.
Description was changed from ========== WIP: Windows 10 MIDI API backend Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy WinMM APIs on Windows 10. Note: the new implementation is currently enabled by setting a compile-time flag (midi_winrt); since we distribute same binaries for all Windows versions, we should include both implementations in the build and use a runtime flag for enabling the new one. BUG=512433 R=toyoshim@chromium.org ========== to ========== Windows 10 MIDI API backend Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy WinMM APIs on Windows 10. Note: the new implementation is currently enabled by setting a compile-time flag (midi_winrt); since we distribute same binaries for all Windows versions, we should include both implementations in the build and use a runtime flag for enabling the new one. BUG=512433 R=toyoshim@chromium.org ==========
Description was changed from ========== Windows 10 MIDI API backend Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy WinMM APIs on Windows 10. Note: the new implementation is currently enabled by setting a compile-time flag (midi_winrt); since we distribute same binaries for all Windows versions, we should include both implementations in the build and use a runtime flag for enabling the new one. BUG=512433 R=toyoshim@chromium.org ========== to ========== Web MIDI backend for Windows 10 Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy WinMM APIs on Windows 10. Note: the new implementation is currently enabled by setting a compile-time flag (midi_winrt); since we distribute same binaries for all Windows versions, we should include both implementations in the build and use a runtime flag for enabling the new one. BUG=512433 R=toyoshim@chromium.org ==========
First comments for Patch Set 6. This comments do not contain port manager related classes. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.cc:266: port_names_[dev_id] = dev_name; Is this a combination of bus address like thing and device/product ID? If so, this ID is an ideal one for us. https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... File media/midi/midi_manager_winrt.h (right): https://codereview.chromium.org/2243183002/diff/80001/media/midi/midi_manager... media/midi/midi_manager_winrt.h:46: base::Thread com_thread_; Agreed. I read thread.cc again. So, it seems to reset the thread information on returning from the ctor. So, it's allowed to run Start() on another thread from the one ran ctor. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:15: #include "base/containers/hash_tables.h" Can you add TODO for record? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:37: // Helpers for printing HRESULTs. Oh, we do not have this kind of utilities under base/? That's bad. In this simple code, we just dump the HRESULT as a hex code, right? If it's enough, we can just cast to integer to redirect it to LOG(). Otherwise, we may want to have more graceful messages by using HRESULT_CODE() and FormatMessage. Or, base::SystemErrorCodeToString() in base/logging is the one we want to use here? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:68: ScopedComPtr<IBufferFactory> GetBufferFactory() { Since now we have only one caller for GetBufferFactory() and GetDeviceInformationStatics(), I'm not sure it's useful to have these method. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:111: HRESULT GetPointerToBufferData(IBuffer* buffer, uint8_t** out) { Why do you change this interface? HRESULT isn't used effectively outside this function. So just returning nullptr for error cases looks reasonable as you did before. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:689: if (!scheduler_) Could this happen? If this happens only because of program errors, this could be CHECK(). Probably, we need to reset scheduler after resetting port_manager_* to make this CHECK()? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:709: port_manager_out_->StartWatcher())) { MidiManager<Platform> would ask MidiManager to deliver received messages, and MidiManager deliver it to clients that are registered to the manager. The first client is registered when the initialization finishes successfully, or CompleteInitialization() is called with OK. So, even if this class start receiving, received data will be just ignored in MidiManager. (I didn't try this scenario actually. So, I may overlook something) https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:732: if (!buffer_factory) If this happens only when out of memory case, or rare situation OS facing a critical error, CHECK would be fine, or how about using VLOG? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:751: if (FAILED(hr)) Similar to the buffer_factory, why no VLOG here? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:757: if (!port) Probably, this is the case we should log something, but this also could happen when our code has a bug. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:758: return; Yeah, can you add TODO with comments? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... File media/midi/midi_manager_winrt.h (right): https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.h:65: com_thread_checker_; // GUARDED_BY(lazy_init_member_lock_) Just a comment: GUARDED_BY() is not an enforced style by chromium coding guideline. So using it or not is up to you. Originally this style comes from Android since Windows port author moves to Android team, and I'm also familiar with this style because I worked in ARC team. Probably this is handy than verbal comments.
Remaining other comments for Patch Set 6. There are no big concern in this CL now. We can submit this soon after fixing small nits or having TODOs. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:287: if (!is_initialized_) When initialization fails, some callbacks could be registered, and the others couldn't. Can you use tokens to decide if we need to remove a callback for each? Probably, we can use is_initialized_ for Stop(). https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:301: if (!is_initialized_) When does this happen? If only on bugs, CHECK() is fine. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:312: if (!is_initialized_) ditto https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:336: if (!is_initialized_) ditto https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:362: WindowsDeleteString(dev_id_hstring); // Always returns S_OK. Oh, should we manage HSTRING with ScopedHandle-ish (base/win/scoped_handle) class? Removing manual handle management is highly recommended. In this case, you actually leak the object on returning for errors above. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:403: if (!is_initialized_) ditto https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:414: if (!is_initialized_) ditto https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:419: VLOG(1) << "Removing non-existent port " << dev_id; Shall we return here? Otherwise, following dereferences will crash. Or CHECK() as you do for OnMessageReceived? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:431: if (!is_initialized_) ditto
https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:15: #include "base/containers/hash_tables.h" On 2016/08/25 06:09:45, toyoshim wrote: > Can you add TODO for record? Will replace in next patch set. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:37: // Helpers for printing HRESULTs. On 2016/08/25 06:09:44, toyoshim wrote: > Oh, we do not have this kind of utilities under base/? That's bad. > > In this simple code, we just dump the HRESULT as a hex code, right? > If it's enough, we can just cast to integer to redirect it to LOG(). > > Otherwise, we may want to have more graceful messages by using HRESULT_CODE() > and FormatMessage. > > Or, base::SystemErrorCodeToString() in base/logging is the one we want to use > here? base::SystemErrorCodeToString() is using error codes from Win32 APIs, here we are using COM-style HRESULTs with WinRT. I'll look into using FormatMessage() or _com_error (https://msdn.microsoft.com/en-us/library/0ye3k36s.aspx) for retrieving error messages. Printing error codes in 32-bit hex since it's common in documents. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:68: ScopedComPtr<IBufferFactory> GetBufferFactory() { On 2016/08/25 06:09:44, toyoshim wrote: > Since now we have only one caller for GetBufferFactory() and > GetDeviceInformationStatics(), I'm not sure it's useful to have these method. Will replace usages with WrlStaticsFactory(). https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:111: HRESULT GetPointerToBufferData(IBuffer* buffer, uint8_t** out) { On 2016/08/25 06:09:45, toyoshim wrote: > Why do you change this interface? > HRESULT isn't used effectively outside this function. So just returning nullptr > for error cases looks reasonable as you did before. Since this is used in WRL callback which should return HRESULTs, I'd like to return the HRESULT here if it fails (line 556). Actually I tried returning errors in WRL callbacks but there seems no difference, so maybe this is unnecessary. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:287: if (!is_initialized_) On 2016/08/25 06:43:08, toyoshim wrote: > When initialization fails, some callbacks could be registered, and the others > couldn't. > > Can you use tokens to decide if we need to remove a callback for each? > > Probably, we can use is_initialized_ for Stop(). I'm aware of this but I couldn't find definitions for an invalid EventRegistrationToken (a INT64 wrapper). I wonder if it's safe to call all remove_*'s and just ignore errors, otherwise we need another field to keep track of which callbacks are registered successfully. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:336: if (!is_initialized_) On 2016/08/25 06:43:08, toyoshim wrote: > ditto Callbacks may be successfully registered and initialization fails later. Otherwise we should unsubscribe registered events immediately after failure in StartWatcher(). https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:362: WindowsDeleteString(dev_id_hstring); // Always returns S_OK. On 2016/08/25 06:43:08, toyoshim wrote: > Oh, should we manage HSTRING with ScopedHandle-ish (base/win/scoped_handle) > class? > > Removing manual handle management is highly recommended. In this case, you > actually leak the object on returning for errors above. I can write a wrapper class for this. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:419: VLOG(1) << "Removing non-existent port " << dev_id; On 2016/08/25 06:43:08, toyoshim wrote: > Shall we return here? Otherwise, following dereferences will crash. > Or CHECK() as you do for OnMessageReceived? Return is missing, will fix. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:689: if (!scheduler_) On 2016/08/25 06:09:45, toyoshim wrote: > Could this happen? > If this happens only because of program errors, this could be CHECK(). Probably, > we need to reset scheduler after resetting port_manager_* to make this CHECK()? I see, this may be replaced with CHECK() since DispatchSendMidiData() shouldn't be called after Finalize() which blocks until FinalizeOnComThread finishes. I guess lock is not required here either. At least |port_manager_out_| should outlive the scheduler since SendOnComThread() uses it. If the scheduler is destroyed SendOnComThread() won't be called. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:709: port_manager_out_->StartWatcher())) { On 2016/08/25 06:09:45, toyoshim wrote: > MidiManager<Platform> would ask MidiManager to deliver received messages, and > MidiManager deliver it to clients that are registered to the manager. The first > client is registered when the initialization finishes successfully, or > CompleteInitialization() is called with OK. So, even if this class start > receiving, received data will be just ignored in MidiManager. (I didn't try this > scenario actually. So, I may overlook something) It will eventually be ignored but I guess it's better to stop here early? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:732: if (!buffer_factory) On 2016/08/25 06:09:44, toyoshim wrote: > If this happens only when out of memory case, or rare situation OS facing a > critical error, CHECK would be fine, or how about using VLOG? Error logs will emit in GetBufferFactory(), do we need additional logs here? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:751: if (FAILED(hr)) On 2016/08/25 06:09:44, toyoshim wrote: > Similar to the buffer_factory, why no VLOG here? ditto https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:758: return; On 2016/08/25 06:09:45, toyoshim wrote: > Yeah, can you add TODO with comments? Will fix in next patch set.
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:15: #include "base/containers/hash_tables.h" On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:09:45, toyoshim wrote: > > Can you add TODO for record? > > Will replace in next patch set. Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:37: // Helpers for printing HRESULTs. On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:09:44, toyoshim wrote: > > Oh, we do not have this kind of utilities under base/? That's bad. > > > > In this simple code, we just dump the HRESULT as a hex code, right? > > If it's enough, we can just cast to integer to redirect it to LOG(). > > > > Otherwise, we may want to have more graceful messages by using HRESULT_CODE() > > and FormatMessage. > > > > Or, base::SystemErrorCodeToString() in base/logging is the one we want to use > > here? > > base::SystemErrorCodeToString() is using error codes from Win32 APIs, here we > are using COM-style HRESULTs with WinRT. I'll look into using FormatMessage() or > _com_error (https://msdn.microsoft.com/en-us/library/0ye3k36s.aspx) for > retrieving error messages. Printing error codes in 32-bit hex since it's common > in documents. Now using _com_error(hr).ErrorMessage(), printing something like: "CoInitialize has not been called. (0x800401F0)" https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:68: ScopedComPtr<IBufferFactory> GetBufferFactory() { On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:09:44, toyoshim wrote: > > Since now we have only one caller for GetBufferFactory() and > > GetDeviceInformationStatics(), I'm not sure it's useful to have these method. > > Will replace usages with WrlStaticsFactory(). Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:287: if (!is_initialized_) On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:43:08, toyoshim wrote: > > When initialization fails, some callbacks could be registered, and the others > > couldn't. > > > > Can you use tokens to decide if we need to remove a callback for each? > > > > Probably, we can use is_initialized_ for Stop(). > > I'm aware of this but I couldn't find definitions for an invalid > EventRegistrationToken (a INT64 wrapper). I wonder if it's safe to call all > remove_*'s and just ignore errors, otherwise we need another field to keep track > of which callbacks are registered successfully. Found that tokens with value = 0 is considered invalid in <wrl/event.h>. Now initializing tokens to 0 and add checks before removing. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:301: if (!is_initialized_) On 2016/08/25 06:43:08, toyoshim wrote: > When does this happen? If only on bugs, CHECK() is fine. Using CHECK() now. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:312: if (!is_initialized_) On 2016/08/25 06:43:08, toyoshim wrote: > ditto Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:336: if (!is_initialized_) On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:43:08, toyoshim wrote: > > ditto > > Callbacks may be successfully registered and initialization fails later. > Otherwise we should unsubscribe registered events immediately after failure in > StartWatcher(). Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:362: WindowsDeleteString(dev_id_hstring); // Always returns S_OK. On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:43:08, toyoshim wrote: > > Oh, should we manage HSTRING with ScopedHandle-ish (base/win/scoped_handle) > > class? > > > > Removing manual handle management is highly recommended. In this case, you > > actually leak the object on returning for errors above. > > I can write a wrapper class for this. Now using Microsoft::WRL::Wrappers::HString which provides this functionality. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:403: if (!is_initialized_) On 2016/08/25 06:43:08, toyoshim wrote: > ditto Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:414: if (!is_initialized_) On 2016/08/25 06:43:08, toyoshim wrote: > ditto Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:419: VLOG(1) << "Removing non-existent port " << dev_id; On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:43:08, toyoshim wrote: > > Shall we return here? Otherwise, following dereferences will crash. > > Or CHECK() as you do for OnMessageReceived? > > Return is missing, will fix. Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:431: if (!is_initialized_) On 2016/08/25 06:43:08, toyoshim wrote: > ditto Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:532: handle->add_MessageReceived( On 2016/08/25 02:50:16, Shao-Chuan Lee wrote: > Add error handling here, this MidiInPort should not be available if event > registration fails. Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:689: if (!scheduler_) On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:09:45, toyoshim wrote: > > Could this happen? > > If this happens only because of program errors, this could be CHECK(). > Probably, > > we need to reset scheduler after resetting port_manager_* to make this > CHECK()? > > I see, this may be replaced with CHECK() since DispatchSendMidiData() shouldn't > be called after Finalize() which blocks until FinalizeOnComThread finishes. I > guess lock is not required here either. > > At least |port_manager_out_| should outlive the scheduler since > SendOnComThread() uses it. If the scheduler is destroyed SendOnComThread() won't > be called. Done. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:757: if (!port) On 2016/08/25 06:09:45, toyoshim wrote: > Probably, this is the case we should log something, but this also could happen > when our code has a bug. Now emitting logs. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:758: return; On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > On 2016/08/25 06:09:45, toyoshim wrote: > > Yeah, can you add TODO with comments? > > Will fix in next patch set. Done.
https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:111: HRESULT GetPointerToBufferData(IBuffer* buffer, uint8_t** out) { Aha, I understand the reason of this change. Since you already handle errors properly, it's probably ok to return S_OK from WRL callbacks. Otherwise, callers may do something unpredictable to recover? https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:362: WindowsDeleteString(dev_id_hstring); // Always returns S_OK. On 2016/08/29 08:27:51, Shao-Chuan Lee wrote: > On 2016/08/25 07:06:42, Shao-Chuan Lee wrote: > > On 2016/08/25 06:43:08, toyoshim wrote: > > > Oh, should we manage HSTRING with ScopedHandle-ish (base/win/scoped_handle) > > > class? > > > > > > Removing manual handle management is highly recommended. In this case, you > > > actually leak the object on returning for errors above. > > > > I can write a wrapper class for this. > > Now using Microsoft::WRL::Wrappers::HString which provides this functionality. Acknowledged. https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:709: port_manager_out_->StartWatcher())) { Yeah, not having any assumption for other modules is a good strategy to make it work correctly :) https://codereview.chromium.org/2243183002/diff/100001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:732: if (!buffer_factory) If you think this won't happen in general, CHECK is enough. Otherwise right recovery code is needed. There VLOG is nice to have. https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:40: struct PrintHr { Ah, I just thought something like following is enough to show HRESULT. LOG(ERROR) << base::SystemErrorCodeToString(HRESULT_CODE(hr)) But, it's ok to have PrintHr if error string with hex code as you do here is useful though this is not a kind of code each module should have, but base/ should provide. https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:219: RemoveWatcherEventHandlers(); As I commented at line 754, it would be safe and simple to call RemoveWatcherEventHandlers() in caller side. That would make your code bug-proof against other developers' changes in the future. Ditto on following error handling. https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:290: watcher_->Stop(); Any reason not to move watcher_->Stop() inside the RemoveWatcherEventHandlers()? After that, you can rename the method to StopWatcher that is correspond with StartWatcher. https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:707: CompleteInitialization(Result::NOT_SUPPORTED); We use NOT_SUPPORTED error to indicate build configuration errors now. This error means No MidiManager to support this platform wasn't build with this binary. This is used so that unit tests can detect someone mistakenly change build rules for Android, ChromeOS, ChromeCast and so on in a wrong way that makes API not work. At this point, this module is built behind a build time flag. So this error code could be ok, but INITIALIZATION_ERROR would be better to be used even after we make this behind a runtime flag. https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:754: port_manager_in_->StartWatcher())) { Probably, we want to call RemoveWatcherEventHandlers() for both managers?
https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:219: RemoveWatcherEventHandlers(); On 2016/08/29 09:48:12, toyoshim wrote: > As I commented at line 754, it would be safe and simple to call > RemoveWatcherEventHandlers() in caller side. That would make your code bug-proof > against other developers' changes in the future. > > Ditto on following error handling. Done. https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:290: watcher_->Stop(); On 2016/08/29 09:48:12, toyoshim wrote: > Any reason not to move watcher_->Stop() inside the RemoveWatcherEventHandlers()? > > After that, you can rename the method to StopWatcher that is correspond with > StartWatcher. Done. https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:707: CompleteInitialization(Result::NOT_SUPPORTED); On 2016/08/29 09:48:12, toyoshim wrote: > We use NOT_SUPPORTED error to indicate build configuration errors now. This > error means No MidiManager to support this platform wasn't build with this > binary. This is used so that unit tests can detect someone mistakenly change > build rules for Android, ChromeOS, ChromeCast and so on in a wrong way that > makes API not work. > > At this point, this module is built behind a build time flag. So this error code > could be ok, but INITIALIZATION_ERROR would be better to be used even after we > make this behind a runtime flag. Done. https://codereview.chromium.org/2243183002/diff/140001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:754: port_manager_in_->StartWatcher())) { On 2016/08/29 09:48:12, toyoshim wrote: > Probably, we want to call RemoveWatcherEventHandlers() for both managers? Now calling StopWatcher() for both if either fails.
It looks almost gtm. Thank you for making great effort to polish this CL. Your updates not related to my comments looks great too. https://codereview.chromium.org/2243183002/diff/160001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/160001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:42: HRESULT hr_; I overlooked this style nit, but we do not use underbar suffix for struct members. https://codereview.chromium.org/2243183002/diff/160001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:760: CompleteInitialization(Result::INITIALIZATION_ERROR); It's natural and probably safe to call CompleteInitialization after calling StopWatcher()s.
https://codereview.chromium.org/2243183002/diff/160001/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/160001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:42: HRESULT hr_; On 2016/08/29 16:20:17, toyoshim wrote: > I overlooked this style nit, but we do not use underbar suffix for struct > members. Done. https://codereview.chromium.org/2243183002/diff/160001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:121: // Tokens with value = 0 are considered as invalid (as in <wrl/event.h>). grammar https://codereview.chromium.org/2243183002/diff/160001/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:760: CompleteInitialization(Result::INITIALIZATION_ERROR); On 2016/08/29 16:20:17, toyoshim wrote: > It's natural and probably safe to call CompleteInitialization after calling > StopWatcher()s. Done. This was actually a copy/paste mistake.. https://codereview.chromium.org/2243183002/diff/140002/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/140002/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:150: virtual ~MidiPortManager() { DCHECK(thread_checker_.CalledOnValidThread()); } Although we are not destructing Midi{In,Out}PortManagers through MidiPortManager*, this should be best practice. https://codereview.chromium.org/2243183002/diff/140002/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:520: ports_; Moved back to private since we are not using these fields in ~MidiInPortManager() now. https://codereview.chromium.org/2243183002/diff/140002/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:537: bool is_initialized_ = false; ditto https://codereview.chromium.org/2243183002/diff/140002/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:764: port_manager_in_->StopWatcher(); RemovePortEventHandlers() moved from dtor to StopWatcher().
Thank you for polishing this up. I may still miss something, but it looks good enough for the first submit. LGTM :) Once you submit this CL, you may want to file sub-bugs that block the meta bug to implement Windows 10 backend. It would help us to track remaining TODOs.
https://codereview.chromium.org/2243183002/diff/140002/media/midi/midi_manage... File media/midi/midi_manager_winrt.cc (right): https://codereview.chromium.org/2243183002/diff/140002/media/midi/midi_manage... media/midi/midi_manager_winrt.cc:596: hr = message->get_Timestamp(&time_span); According to docs this is time since "the MidiInPort was created," where "created" means when the device is plugged into the PC.
The CQ bit was checked by shaochuan@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shaochuan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from toyoshim@chromium.org Link to the patchset: https://codereview.chromium.org/2243183002/#ps210001 (title: "crbug IDs")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shaochuan@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by shaochuan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Web MIDI backend for Windows 10 Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy WinMM APIs on Windows 10. Note: the new implementation is currently enabled by setting a compile-time flag (midi_winrt); since we distribute same binaries for all Windows versions, we should include both implementations in the build and use a runtime flag for enabling the new one. BUG=512433 R=toyoshim@chromium.org ========== to ========== Web MIDI backend for Windows 10 Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy WinMM APIs on Windows 10. Note: the new implementation is currently enabled by setting a compile-time flag (midi_winrt); since we distribute same binaries for all Windows versions, we should include both implementations in the build and use a runtime flag for enabling the new one. BUG=512433 R=toyoshim@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #11 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== Web MIDI backend for Windows 10 Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy WinMM APIs on Windows 10. Note: the new implementation is currently enabled by setting a compile-time flag (midi_winrt); since we distribute same binaries for all Windows versions, we should include both implementations in the build and use a runtime flag for enabling the new one. BUG=512433 R=toyoshim@chromium.org ========== to ========== Web MIDI backend for Windows 10 Using new WinRT APIs introduced since SDK version 10.0.10240.0 to replace legacy WinMM APIs on Windows 10. Note: the new implementation is currently enabled by setting a compile-time flag (midi_winrt); since we distribute same binaries for all Windows versions, we should include both implementations in the build and use a runtime flag for enabling the new one. BUG=512433 R=toyoshim@chromium.org Committed: https://crrev.com/e7237592868a179ba66e6a44c19f89abb9d4483e Cr-Commit-Position: refs/heads/master@{#415566} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e7237592868a179ba66e6a44c19f89abb9d4483e Cr-Commit-Position: refs/heads/master@{#415566} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
