Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
264973012:
Use seq instead of rawmidi for MidiManagerAlsa (Closed)
|
Created:
6 years, 7 months ago by Adam Goode Modified:
6 years, 6 months ago Reviewers:
Takashi Toyoshima CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUse seq instead of rawmidi for MidiManagerAlsa
Using seq is more complicated, but allows other applications
access to the MIDI hardware.
I still have some more verification to do and probably some bugs to fix but I think this is a good start.
Note that Chrome OS might need a new kernel module built.
BUG=344410
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275732
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix C++ initialization #
Total comments: 23
Patch Set 4 : Fix crash due to negative delay in MIDI send #Patch Set 5 : Increase MIDI send buffer size to 256, to avoid splitting sysex messages too much. #
Total comments: 4
Patch Set 6 : Address many issues from code review #
Total comments: 4
Patch Set 7 : Implement manufacturer and version, also some minor cleanups #Patch Set 8 : Fix small mistake in midi_manager_unittest #
Total comments: 4
Patch Set 9 : Constify a few last things #
Messages
Total messages: 22 (0 generated)
Hi Adam, thank you for your contribution. Is this CL ready for a review? By the way, I ran 'cros_x86' in Trybot. It tries to build CrOS including the browser with your patch, then you can confirm if your change needs additional OS side work or not.
On 2014/05/06 13:12:29, Takashi Toyoshima (chromium) wrote: > Hi Adam, thank you for your contribution. > Is this CL ready for a review? > I would appreciate a first look, but I don't feel it's ready to submit until I manually try out sending and a few other things. > By the way, I ran 'cros_x86' in Trybot. It tries to build CrOS including the > browser with your patch, then you can confirm if your change needs additional OS > side work or not. This will likely build without issue, since the changes do not involve different libraries. But kernel support might be missing. (snd-seq.ko is required, I do not know if this is included in CrOS) Once we get this patch in, we can write a much more thorough test on CrOS that involves a software midi client running outside the browser.
> But kernel support might be missing. (snd-seq.ko is required, I do not know if > this is included in CrOS) Ah, I see. I checked it with my Chromebook 11. It seems to have it. Here is a log of ls -R /lib/modules/3.8.11/kernel/sound/ /lib/modules/3.8.11/kernel/sound/: core usb /lib/modules/3.8.11/kernel/sound/core: seq snd-hrtimer.ko snd-hwdep.ko snd-rawmidi.ko /lib/modules/3.8.11/kernel/sound/core/seq: snd-seq-device.ko snd-seq.ko snd-seq-midi.ko snd-seq-dummy.ko snd-seq-midi-event.ko /lib/modules/3.8.11/kernel/sound/usb: snd-usb-audio.ko snd-usbmidi-lib.ko
Thanks for the ls -R :) I think it's ready for review now with this last patch set. I have verified manually that send and receive work and that timestamps in and out are valid. To test this better, I would like to write a midi client that can connect to the sequencer and do an end-to-end test to/from the browser. But for now this seems ok.
I've now manually tested sysex send/receive.
It's crossed. This is a quick review for Patch Set 3. I'll take a look the latest patch set too. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:28: SND_SEQ_PORT_CAP_READ | SND_SEQ_PORT_CAP_SUBS_READ; Having a link to http://www.alsa-project.org/alsa-doc/alsa-lib/seq.html or something as a comment seems to be useful. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:52: void MidiManagerAlsa::StartInitialization() { Now that StartInitialization() can be performed asynchronously, can you run following initialization code on the event thread? It will be like EventReset() running via PostTask at the end of this function. StartInitialization() is called on the I/O thread. Blocking calls and heavy processing like encryption are forbidden on the thread. Actually, my original code is using snd_ctl_open() that may call open(), and it was not recommended. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:69: err = snd_seq_set_client_name(in_client_, "Google Chrome (input)"); Can you remove "Google" from the strings? _mac.cc also used "Google Chrome", but I changed it recently since this code is also used by Chromium, or other derived projects. So, containing "Google" is not correct. I think using "Chrome" instead of "Chromium" is ok since Chromium project uses the word "Chrome" here and there, e.g. binary name for Linux is chrome. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:74: err = snd_seq_set_client_name(out_client_, "Google Chrome (output)"); ditto https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:185: AddOutputPort(MidiPortInfo(id, "", name, "")); Can you try to get a manufacturer string and generate a version string as I did for USB devices? We want to use the same name and manufacturer in all platforms as we can as possible especially for USB devices. Of course, there is no perfect way to get the same name, and difficult heuristics is not needed. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:202: shutdown_mu_.Acquire(); option: It's up to you, but I feel using AutoLock with {} scope is more popular and safe. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:219: if (in_client_) { Usually chromium style omit {} for a one line clause. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:228: snd_midi_event_free(*i); ditto https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:295: // We've lost events: check another way to see if we need to shut down. Why do you check shutdown here? I mean why only for ENOSPC. Anyway, Thread::Stop() post a task to stop event loop gracefully, so we do not need to have special code for stopping the loop. But, I'm afraid that if we do not have any coming events, snd_seq_event_input() may block forever, and until returning from it, the thread can not be stopped. It will make Chrome shutdown hang-up. Does ALSA have a API to post a dummy event to the subscriber to unblock snd_seq_event_input() call quickly? https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.h:62: base::Lock shutdown_mu_; // guards event_thread_shutdown_ Using suffix _mu_ for mutex is not usual in chromium coding style. I think *_lock_ is popular.
additional comments for patch set 5. https://codereview.chromium.org/264973012/diff/80001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/80001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:26: const size_t kSendBufferSize = 256; If larger buffer size improves sysex performance, more aggressive size is acceptable since MidiManager is single instance object. It this enough for large sysex messages, e.g. downloading sampling or something like that? https://codereview.chromium.org/264973012/diff/80001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:271: } Just a question, but why do you want to change this delay calculation?
https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:28: SND_SEQ_PORT_CAP_READ | SND_SEQ_PORT_CAP_SUBS_READ; On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > Having a link to http://www.alsa-project.org/alsa-doc/alsa-lib/seq.html or > something as a comment seems to be useful. Done. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:52: void MidiManagerAlsa::StartInitialization() { On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > Now that StartInitialization() can be performed asynchronously, can you run > following initialization code on the event thread? It will be like EventReset() > running via PostTask at the end of this function. > > StartInitialization() is called on the I/O thread. Blocking calls and heavy > processing like encryption are forbidden on the thread. Actually, my original > code is using snd_ctl_open() that may call open(), and it was not recommended. I don't think I can do this on the event thread, since the destructor runs on the browser thread. Actually this code already looks wrong because Thread::Start() and Thread::Stop() have to be called from the same thread, and we are calling Start() on the I/O thread here and Stop() on the browser thread. What I think I want is the other half to StartInitialization (StartDeinitialization?), called from the I/O thread. Then I can move some of the destructor code there. Or is there a better way? https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:69: err = snd_seq_set_client_name(in_client_, "Google Chrome (input)"); On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > Can you remove "Google" from the strings? > _mac.cc also used "Google Chrome", but I changed it recently since this code is > also used by Chromium, or other derived projects. So, containing "Google" is not > correct. I think using "Chrome" instead of "Chromium" is ok since Chromium > project uses the word "Chrome" here and there, e.g. binary name for Linux is > chrome. Done. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:74: err = snd_seq_set_client_name(out_client_, "Google Chrome (output)"); On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > ditto Done. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:185: AddOutputPort(MidiPortInfo(id, "", name, "")); On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > Can you try to get a manufacturer string and generate a version string as I did > for USB devices? > We want to use the same name and manufacturer in all platforms as we can as > possible especially for USB devices. Of course, there is no perfect way to get > the same name, and difficult heuristics is not needed. I will look into this. It is complex to do. I think I will also look into adding the clean way to do this into the kernel. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:202: shutdown_mu_.Acquire(); On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > option: It's up to you, but I feel using AutoLock with {} scope is more popular > and safe. Done. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:219: if (in_client_) { On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > Usually chromium style omit {} for a one line clause. Done. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:228: snd_midi_event_free(*i); On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > ditto Done. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:295: // We've lost events: check another way to see if we need to shut down. On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > Why do you check shutdown here? I mean why only for ENOSPC. > This is only for the case where we miss the shutdown event (SND_SEQ_EVENT_CLIENT_EXIT) because of a FIFO buffer overrun in the kernel. The typical shutdown code is below. We also want this logic when we support hotplug devices so we can trigger a full rescan if we lose messages. > Anyway, Thread::Stop() post a task to stop event loop gracefully, so we do not > need to have special code for stopping the loop. But, I'm afraid that if we do > not have any coming events, snd_seq_event_input() may block forever, and until > returning from it, the thread can not be stopped. It will make Chrome shutdown > hang-up. Does ALSA have a API to post a dummy event to the subscriber to unblock > snd_seq_event_input() call quickly? Yes, see the "else" clause below. 'Check for disconnection of out client. This means "shut down".' https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.h:62: base::Lock shutdown_mu_; // guards event_thread_shutdown_ On 2014/05/07 04:38:28, Takashi Toyoshima (chromium) wrote: > Using suffix _mu_ for mutex is not usual in chromium coding style. I think > *_lock_ is popular. Done. https://codereview.chromium.org/264973012/diff/80001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/80001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:26: const size_t kSendBufferSize = 256; On 2014/05/07 13:06:13, Takashi Toyoshima (chromium) wrote: > If larger buffer size improves sysex performance, more aggressive size is > acceptable since MidiManager is single instance object. It this enough for large > sysex messages, e.g. downloading sampling or something like that? I put a note here. I don't think a large buffer is necessary, but 256 seems reasonable. It only affects the Alsa event delivery (sysex messages will be split across multiple events). There is no limit imposed on sysex messages by this splitting. https://codereview.chromium.org/264973012/diff/80001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:271: } On 2014/05/07 13:06:13, Takashi Toyoshima (chromium) wrote: > Just a question, but why do you want to change this delay calculation? Thanks for pointing this out. I actually think my change was unnecessary! I will revert this part.
Also, the tests are failing because of permissions issues. I filed https://code.google.com/p/chromium/issues/detail?id=371230.
Your original heuristic for extracting the manufacturer seems good. I just need to do another small heuristic to map seq client -> card. I will probably not be able to get to this for a few days right now.
It looks almost ok, and now is good timing to try this new implementation in terms of branch dates (see http://www.chromium.org/developers/calendar). We still have a infra issue to run a test, MidiManagerTest.CreateMidiManager, but for now I'm ok to modify the test and leave TODO with crbug.com/371230 there. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:52: void MidiManagerAlsa::StartInitialization() { Hum... you are right. And other MidiManager* implementations have the same problem. Bad news is that I/O thread is already stopped when ~MidiManagerAlsa is called. Can you add "crbug.com/374341" to your comment at line 60? https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:295: // We've lost events: check another way to see if we need to shut down. Ah, I see. I understand how shutdown works. Thanks. https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:145: // TODO(agoode): Determine good values for manufacturer and version. If it takes time, filing a bug and adding the bug id as crbug.com/<id> here is better. https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... media/midi/midi_manager_alsa.h:41: snd_seq_t* in_client_; How about using scoped_ptr here. See ScopedMIDIHDR in midi_manager_win.cc.
> but for now I'm ok to modify the test and leave TODO with crbug.com/371230 > there. I meant to change EXPECT_EQ line in the test. It is MIDI_NOT_SUPPORTED for unsupported configurations, and can be MIDI_INITIALIZATION_ERROR for alsa.
On 2014/05/16 19:34:26, Takashi Toyoshima (chromium) wrote: > > but for now I'm ok to modify the test and leave TODO with crbug.com/371230 > > there. > > I meant to change EXPECT_EQ line in the test. It is MIDI_NOT_SUPPORTED for > unsupported configurations, and can be MIDI_INITIALIZATION_ERROR for alsa. Thanks for the review! I just want to let you know I am planning to get back to this in the next day or so.
I am feeling pretty good at this point. The next two things would be hotplug(!) and fixing the kernel(!) to make getting properties much easier. https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... media/midi/midi_manager_alsa.cc:52: void MidiManagerAlsa::StartInitialization() { On 2014/05/16 19:31:52, Takashi Toyoshima (chromium) wrote: > Hum... you are right. And other MidiManager* implementations have the same > problem. > Bad news is that I/O thread is already stopped when ~MidiManagerAlsa is called. > Can you add "crbug.com/374341" to your comment at line 60? Done. https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:145: // TODO(agoode): Determine good values for manufacturer and version. On 2014/05/16 19:31:52, Takashi Toyoshima (chromium) wrote: > If it takes time, filing a bug and adding the bug id as crbug.com/<id> here is > better. I implemented it. And filed a bug to make it better. (Needs kernel work.) https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... File media/midi/midi_manager_alsa.h (right): https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... media/midi/midi_manager_alsa.h:41: snd_seq_t* in_client_; On 2014/05/16 19:31:52, Takashi Toyoshima (chromium) wrote: > How about using scoped_ptr here. > See ScopedMIDIHDR in midi_manager_win.cc. I could do this but I would prefer to add as an enhancement later. This is currently handled by snd_seq_close() in the destructor. Seems like it would add a lot of lines of code to replace 2 lines.
On 2014/05/25 04:57:05, Adam Goode wrote: > I am feeling pretty good at this point. The next two things would be hotplug(!) > and fixing the kernel(!) to make getting properties much easier. > > https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... > File media/midi/midi_manager_alsa.cc (right): > > https://codereview.chromium.org/264973012/diff/40001/media/midi/midi_manager_... > media/midi/midi_manager_alsa.cc:52: void MidiManagerAlsa::StartInitialization() > { > On 2014/05/16 19:31:52, Takashi Toyoshima (chromium) wrote: > > Hum... you are right. And other MidiManager* implementations have the same > > problem. > > Bad news is that I/O thread is already stopped when ~MidiManagerAlsa is > called. > > Can you add "crbug.com/374341" to your comment at line 60? > > Done. > > https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... > File media/midi/midi_manager_alsa.cc (right): > > https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... > media/midi/midi_manager_alsa.cc:145: // TODO(agoode): Determine good values for > manufacturer and version. > On 2014/05/16 19:31:52, Takashi Toyoshima (chromium) wrote: > > If it takes time, filing a bug and adding the bug id as crbug.com/<id> here is > > better. > > I implemented it. And filed a bug to make it better. (Needs kernel work.) > > https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... > File media/midi/midi_manager_alsa.h (right): > > https://codereview.chromium.org/264973012/diff/100001/media/midi/midi_manager... > media/midi/midi_manager_alsa.h:41: snd_seq_t* in_client_; > On 2014/05/16 19:31:52, Takashi Toyoshima (chromium) wrote: > > How about using scoped_ptr here. > > See ScopedMIDIHDR in midi_manager_win.cc. > > I could do this but I would prefer to add as an enhancement later. This is > currently handled by snd_seq_close() in the destructor. Seems like it would add > a lot of lines of code to replace 2 lines. Hi, any update? It would be nice to get this in before the next branch cut.
Sorry for late response. Now, I'm visiting MTV, and can not try this new implementation working as intended since I do not have a local Linux workstation. Did you check if it works? I think you did, but just in case. If you checked it works, LGTM with some nit picks. > I am feeling pretty good at this point. The next two things would be hotplug(!) > and fixing the kernel(!) to make getting properties much easier. Yes, hotplug should be the next focus. Though the API depends on undecided ES6 spec a little, I can make it move by providing compatible interfaces with minimum functionalities as I did for Promise. I'm working on Blink side API update. Your continuous help would be great. https://codereview.chromium.org/264973012/diff/140001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/140001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:45: CardInfo(std::string name, std::string manufacturer, can't we make these arguments const? https://codereview.chromium.org/264973012/diff/140001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:214: CardInfo info = cards[current_card]; const CardInfo
https://codereview.chromium.org/264973012/diff/140001/media/midi/midi_manager... File media/midi/midi_manager_alsa.cc (right): https://codereview.chromium.org/264973012/diff/140001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:45: CardInfo(std::string name, std::string manufacturer, On 2014/06/04 20:28:17, Takashi Toyoshima (chromium) wrote: > can't we make these arguments const? Yes, by changing to ScopedVector below. https://codereview.chromium.org/264973012/diff/140001/media/midi/midi_manager... media/midi/midi_manager_alsa.cc:214: CardInfo info = cards[current_card]; On 2014/06/04 20:28:17, Takashi Toyoshima (chromium) wrote: > const CardInfo Done.
The CQ bit was checked by agoode@chromium.org
On 2014/06/08 02:53:48, Adam Goode wrote: > The CQ bit was checked by mailto:agoode@chromium.org Thanks for the review! I am looking forward to continuing to improve Web MIDI with you.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agoode@chromium.org/264973012/160001
Message was sent while issue was closed.
Change committed as 275732 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
