midi: Refactor Mac MIDI manager to use platform-neutral UMP utility [chromium/src : main]
Takashi Toyoshima (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 4 comments![]( "Open in Gerrit")
I am not super familiar with these changes, but I also understand we don't have other experts who can review this.
It'll be slow, but we (the WebAudio) will try to complete the review. Thanks for your patience!
size_t chunk_size = std::min(words_to_send, static_cast<size_t>(256));I am not familiar with the UMP packets in general. Why 256 is used here?
// Returns the number of bytes from `data` successfully translated.This method seems to return the size of `data`. (not "successfully translated bytes")
while (idx < data.size() && data[idx] != 0xF7 && data[idx] < 0x80) {
idx++;
}From Gemini:
Legacy MIDI 1.0 permits 1-byte System Real-Time messages (status bytes 0xF8 to 0xFF) to be interleaved at any point in the stream, including inside a SysEx message.
However, the SysEx parsing loop here terminates immediately upon encountering an interleaved status byte (since data[idx] >= 0x80). This causes the SysEx message to be truncated (treated as complete or end prematurely). Furthermore, the remaining data bytes of the SysEx in the subsequent loop iterations are skipped because they are < 0x80 but do not start with a status byte.
---
Does this look reasonable?
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 4 comments![]( "Open in Gerrit")
I am not super familiar with these changes, but I also understand we don't have other experts who can review this.
It'll be slow, but we (the WebAudio) will try to complete the review. Thanks for your patience!
Thank you!
I'm also sorry about slow updates. I originally thought this was a simple API migration to drop unexpected dependencies to the legacy API, but the reality is that this is the exact macOS level MIDI 2.0 support, and migration requires much more complicated changes than I assumed.
size_t chunk_size = std::min(words_to_send, static_cast<size_t>(256));I am not familiar with the UMP packets in general. Why 256 is used here?
Oh, you are right. This is an unnecessary internal limit.
Now the logic was rewritten, and the legacy code was also kept just in case behind a kill-switch.
This is because I noticed that the interleaved realtime messages were pre-existing bug, and I`m not sure if any existing service relies on it.
// Returns the number of bytes from `data` successfully translated.This method seems to return the size of `data`. (not "successfully translated bytes")
Let me drop the returning value as we didn't need it, or even it might be misleading.
while (idx < data.size() && data[idx] != 0xF7 && data[idx] < 0x80) {
idx++;
}From Gemini:
Legacy MIDI 1.0 permits 1-byte System Real-Time messages (status bytes 0xF8 to 0xFF) to be interleaved at any point in the stream, including inside a SysEx message.
However, the SysEx parsing loop here terminates immediately upon encountering an interleaved status byte (since data[idx] >= 0x80). This causes the SysEx message to be truncated (treated as complete or end prematurely). Furthermore, the remaining data bytes of the SysEx in the subsequent loop iterations are skipped because they are < 0x80 but do not start with a status byte.
---
Does this look reasonable?
This is called only for a message that is delivered from Web MIDI to OS.
Web MIDI defines that the output.send accepts only one or multiple *complete* messages, and explicitly disallows running status, and if we read the spec straight and forward, it's reasonable to interpret that we disallow such interleaved realtime messages.
Actually, IIRC, they are a kind of legacy transport layer's spec, and USB and Bluetooth don't allow them, and on some platforms, even for the legacy transport, OS reorder them so that we do not have to handle such complicated sequences in the application layer.
On the other hand, it seems our Blink side validator overlooked this exceptional case unfortunately. So, can we keep this logic as-is with a fallback switch to the legacy API for a few milestones?
I will also send another patch for the Blink side, to validate it correctly, but behind yet another feature flag.
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 5 comments![]( "Open in Gerrit")
Takashi ToyoshimaI am not super familiar with these changes, but I also understand we don't have other experts who can review this.
It'll be slow, but we (the WebAudio) will try to complete the review. Thanks for your patience!
Thank you!
I'm also sorry about slow updates. I originally thought this was a simple API migration to drop unexpected dependencies to the legacy API, but the reality is that this is the exact macOS level MIDI 2.0 support, and migration requires much more complicated changes than I assumed.
Acknowledged
I assume you'll be addressing some comments from the previous PS; turning off my attention bit.
size_t chunk_size = std::min(words_to_send, static_cast<size_t>(256));Takashi ToyoshimaI am not familiar with the UMP packets in general. Why 256 is used here?
Oh, you are right. This is an unnecessary internal limit.
Now the logic was rewritten, and the legacy code was also kept just in case behind a kill-switch.
This is because I noticed that the interleaved realtime messages were pre-existing bug, and I`m not sure if any existing service relies on it.
Acknowledged
// Returns the number of bytes from `data` successfully translated.Takashi ToyoshimaThis method seems to return the size of `data`. (not "successfully translated bytes")
Let me drop the returning value as we didn't need it, or even it might be misleading.
Acknowledged
while (idx < data.size() && data[idx] != 0xF7 && data[idx] < 0x80) {
idx++;
}Takashi ToyoshimaFrom Gemini:
Legacy MIDI 1.0 permits 1-byte System Real-Time messages (status bytes 0xF8 to 0xFF) to be interleaved at any point in the stream, including inside a SysEx message.
However, the SysEx parsing loop here terminates immediately upon encountering an interleaved status byte (since data[idx] >= 0x80). This causes the SysEx message to be truncated (treated as complete or end prematurely). Furthermore, the remaining data bytes of the SysEx in the subsequent loop iterations are skipped because they are < 0x80 but do not start with a status byte.
---
Does this look reasonable?
This is called only for a message that is delivered from Web MIDI to OS.
Web MIDI defines that the output.send accepts only one or multiple *complete* messages, and explicitly disallows running status, and if we read the spec straight and forward, it's reasonable to interpret that we disallow such interleaved realtime messages.
Actually, IIRC, they are a kind of legacy transport layer's spec, and USB and Bluetooth don't allow them, and on some platforms, even for the legacy transport, OS reorder them so that we do not have to handle such complicated sequences in the application layer.
On the other hand, it seems our Blink side validator overlooked this exceptional case unfortunately. So, can we keep this logic as-is with a fallback switch to the legacy API for a few milestones?
I will also send another patch for the Blink side, to validate it correctly, but behind yet another feature flag.
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
I assume you'll be addressing some comments from the previous PS; turning off my attention bit.
Ah, my changes from ps8 were for fixing build issue that was caused by the last change, midi_switches.h's renaming to midi_features.h.
So, the current patchset is ready for the final review on my side.
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 2 comments![]( "Open in Gerrit")
I will make one more change for the following reason.
I will set attention bit again once the CL gets ready again.
while (idx < data.size() && data[idx] != 0xF7 && data[idx] < 0x80) {
idx++;
}Takashi ToyoshimaFrom Gemini:
Legacy MIDI 1.0 permits 1-byte System Real-Time messages (status bytes 0xF8 to 0xFF) to be interleaved at any point in the stream, including inside a SysEx message.
However, the SysEx parsing loop here terminates immediately upon encountering an interleaved status byte (since data[idx] >= 0x80). This causes the SysEx message to be truncated (treated as complete or end prematurely). Furthermore, the remaining data bytes of the SysEx in the subsequent loop iterations are skipped because they are < 0x80 but do not start with a status byte.
---
Does this look reasonable?
Hongchan ChoiThis is called only for a message that is delivered from Web MIDI to OS.
Web MIDI defines that the output.send accepts only one or multiple *complete* messages, and explicitly disallows running status, and if we read the spec straight and forward, it's reasonable to interpret that we disallow such interleaved realtime messages.
Actually, IIRC, they are a kind of legacy transport layer's spec, and USB and Bluetooth don't allow them, and on some platforms, even for the legacy transport, OS reorder them so that we do not have to handle such complicated sequences in the application layer.
On the other hand, it seems our Blink side validator overlooked this exceptional case unfortunately. So, can we keep this logic as-is with a fallback switch to the legacy API for a few milestones?
I will also send another patch for the Blink side, to validate it correctly, but behind yet another feature flag.
Acknowledged
Well... it seems Firefox also accepts the realtime message interruption within the sysex and also in usual channel messages. So, I will change this backend to handle it correctly here.
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Commit-Queue | +1 |
1 comment
while (idx < data.size() && data[idx] != 0xF7 && data[idx] < 0x80) {
idx++;
}Takashi ToyoshimaFrom Gemini:
Legacy MIDI 1.0 permits 1-byte System Real-Time messages (status bytes 0xF8 to 0xFF) to be interleaved at any point in the stream, including inside a SysEx message.
However, the SysEx parsing loop here terminates immediately upon encountering an interleaved status byte (since data[idx] >= 0x80). This causes the SysEx message to be truncated (treated as complete or end prematurely). Furthermore, the remaining data bytes of the SysEx in the subsequent loop iterations are skipped because they are < 0x80 but do not start with a status byte.
---
Does this look reasonable?
Hongchan ChoiThis is called only for a message that is delivered from Web MIDI to OS.
Web MIDI defines that the output.send accepts only one or multiple *complete* messages, and explicitly disallows running status, and if we read the spec straight and forward, it's reasonable to interpret that we disallow such interleaved realtime messages.
Actually, IIRC, they are a kind of legacy transport layer's spec, and USB and Bluetooth don't allow them, and on some platforms, even for the legacy transport, OS reorder them so that we do not have to handle such complicated sequences in the application layer.
On the other hand, it seems our Blink side validator overlooked this exceptional case unfortunately. So, can we keep this logic as-is with a fallback switch to the legacy API for a few milestones?
I will also send another patch for the Blink side, to validate it correctly, but behind yet another feature flag.
Takashi ToyoshimaAcknowledged
Well... it seems Firefox also accepts the realtime message interruption within the sysex and also in usual channel messages. So, I will change this backend to handle it correctly here.
Firefox and Blink accepts interleaved realtime messages, and backends parse it again to reconstruct non-interleaved complete messages. This was done in message_util and midi_message_queue in Chrome. Unfortunately, macOS port didn't use it as it has own scheduler inside CoreMIDI. So, only macOS port had a potential issue that will drop interleaved messages as CoreMIDI didn't permit it.
Now, the ump_message_util supports it. So, once this is enabled, interleaved messages will work correctly among all the platforms.
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hongchan Choi
| Code-Review | +1 |
| Commit-Queue | +1 |
2 comments
while (idx < data.size() && data[idx] != 0xF7 && data[idx] < 0x80) {
idx++;
}Takashi ToyoshimaFrom Gemini:
Legacy MIDI 1.0 permits 1-byte System Real-Time messages (status bytes 0xF8 to 0xFF) to be interleaved at any point in the stream, including inside a SysEx message.
However, the SysEx parsing loop here terminates immediately upon encountering an interleaved status byte (since data[idx] >= 0x80). This causes the SysEx message to be truncated (treated as complete or end prematurely). Furthermore, the remaining data bytes of the SysEx in the subsequent loop iterations are skipped because they are < 0x80 but do not start with a status byte.
---
Does this look reasonable?
Hongchan ChoiThis is called only for a message that is delivered from Web MIDI to OS.
Web MIDI defines that the output.send accepts only one or multiple *complete* messages, and explicitly disallows running status, and if we read the spec straight and forward, it's reasonable to interpret that we disallow such interleaved realtime messages.
Actually, IIRC, they are a kind of legacy transport layer's spec, and USB and Bluetooth don't allow them, and on some platforms, even for the legacy transport, OS reorder them so that we do not have to handle such complicated sequences in the application layer.
On the other hand, it seems our Blink side validator overlooked this exceptional case unfortunately. So, can we keep this logic as-is with a fallback switch to the legacy API for a few milestones?
I will also send another patch for the Blink side, to validate it correctly, but behind yet another feature flag.
Takashi ToyoshimaAcknowledged
Takashi ToyoshimaWell... it seems Firefox also accepts the realtime message interruption within the sysex and also in usual channel messages. So, I will change this backend to handle it correctly here.
Firefox and Blink accepts interleaved realtime messages, and backends parse it again to reconstruct non-interleaved complete messages. This was done in message_util and midi_message_queue in Chrome. Unfortunately, macOS port didn't use it as it has own scheduler inside CoreMIDI. So, only macOS port had a potential issue that will drop interleaved messages as CoreMIDI didn't permit it.
Now, the ump_message_util supports it. So, once this is enabled, interleaved messages will work correctly among all the platforms.
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi voted and added 3 comments![]( "Open in Gerrit")
Votes added by Hongchan Choi
| Code-Review | +0 |
Takashi ToyoshimaI assume you'll be addressing some comments from the previous PS; turning off my attention bit.
Ah, my changes from ps8 were for fixing build issue that was caused by the last change, midi_switches.h's renaming to midi_features.h.
So, the current patchset is ready for the final review on my side.
Acknowledged
I will make one more change for the following reason.
I will set attention bit again once the CL gets ready again.
Acknowledged
To resolve outstanding comment threads.
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
DEFINE_LLVM_FUZZER_TEST_ONE_INPUT_SPAN(const base::span<const uint8_t> data) {LLVM-style fuzzers are deprecated and will need migrating. Please write a FUZZ_TEST instead. You can add it to ump_message_util_unittest.cc directly: https://chromium.googlesource.com/chromium/src/+/HEAD/testing/libfuzzer/README.md#libFuzzer-deprecated
Same for the other fuzzer.
Related details
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
DEFINE_LLVM_FUZZER_TEST_ONE_INPUT_SPAN(const base::span<const uint8_t> data) {LLVM-style fuzzers are deprecated and will need migrating. Please write a FUZZ_TEST instead. You can add it to ump_message_util_unittest.cc directly: https://chromium.googlesource.com/chromium/src/+/HEAD/testing/libfuzzer/README.md#libFuzzer-deprecated
Same for the other fuzzer.
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |