Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2485903002:
Web MIDI: do not keep virtual synth open (Closed)
|
Created:
4 years, 1 month ago by Takashi Toyoshima Modified:
4 years, 1 month ago Reviewers:
yukawa CC:
chromium-reviews, feature-media-reviews_chromium.org, toyoshim+midi_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb MIDI: do not keep virtual synth open
To release an internally allocated audio stream, close unsupported
device handles as soon as possible.
This is a short-term fix, and will have an essential fix later.
TEST=powercfg/requests shows no audio stream after Web MIDI is called
BUG=617086
Committed: https://crrev.com/491b24ffbe72a50d3af7846bf9207020fb09049e
Cr-Commit-Position: refs/heads/master@{#431297}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix deadlock #
Total comments: 3
Patch Set 4 : plan b #Messages
Total messages: 32 (23 generated)
The CQ bit was checked by toyoshim@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 toyoshim@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...
Description was changed from ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. BUG=617086 ========== to ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. BUG=617086 ==========
toyoshim@chromium.org changed reviewers: + yukawa@chromium.org
Description was changed from ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. BUG=617086 ========== to ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. This is a short-term fix, and will have an essential fix later. BUG=617086 ==========
Description was changed from ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. This is a short-term fix, and will have an essential fix later. BUG=617086 ========== to ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. This is a short-term fix, and will have an essential fix later. TEST=powercfg/requests shows no audio stream after Web MIDI is called BUG=617086 ==========
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 toyoshim@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...
Can you take a look? This fix is not ideal, but handy to be merged into release branches.
https://codereview.chromium.org/2485903002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/2485903002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_win.cc:831: bool unsupported = IsUnsupportedDevice(state_device_info); Note: I tried to close handle here so to just return. But, closing handle inside this callback seems to fail always. According to MSDN documents, calling MMAPI inside midi callbacks are not allowed officially, and it may cause an internal deadlock. I'm planning to rework legacy win32 backend impl later as a side work of mojo-fication.
lgtm https://codereview.chromium.org/2485903002/diff/40001/media/midi/midi_manager... File media/midi/midi_manager_win.cc (right): https://codereview.chromium.org/2485903002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_win.cc:1080: if (info.state == PortState::DISCONNECTED) { Fine as long as this is a short-term fix. Ideally I think we should have an explicit PortState UNSUPPORTED rather than reusing DISCONNECTED though. https://codereview.chromium.org/2485903002/diff/40001/media/midi/midi_manager... media/midi/midi_manager_win.cc:1104: if (state == PortState::DISCONNECTED) { Ditto.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Now PortState is exposed to blink as a mojo enum, it is a little tough to have another state without largely distributed changes. So I would keep this, but write a TODO instead.
sorry after the approval, but I noticed that there is a much simpler fix for this, just post a task to close the handle. please take another look at ps4.
The CQ bit was checked by toyoshim@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.
lgtm
The CQ bit was checked by toyoshim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. This is a short-term fix, and will have an essential fix later. TEST=powercfg/requests shows no audio stream after Web MIDI is called BUG=617086 ========== to ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. This is a short-term fix, and will have an essential fix later. TEST=powercfg/requests shows no audio stream after Web MIDI is called BUG=617086 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. This is a short-term fix, and will have an essential fix later. TEST=powercfg/requests shows no audio stream after Web MIDI is called BUG=617086 ========== to ========== Web MIDI: do not keep virtual synth open To release an internally allocated audio stream, close unsupported device handles as soon as possible. This is a short-term fix, and will have an essential fix later. TEST=powercfg/requests shows no audio stream after Web MIDI is called BUG=617086 Committed: https://crrev.com/491b24ffbe72a50d3af7846bf9207020fb09049e Cr-Commit-Position: refs/heads/master@{#431297} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/491b24ffbe72a50d3af7846bf9207020fb09049e Cr-Commit-Position: refs/heads/master@{#431297} |
