fundamental / rtosc

Realtime Safe OSC packet serialization and dispatch
https://fundamental-code.com/wiki/rtosc/
MIT License
85 stars 16 forks source link

Read lookahead #62

Closed polluxsynth closed 2 years ago

polluxsynth commented 2 years ago

This PR adds a read lookahead functionality to ThreadLink, so that messages can be read from the read side without actually removing them from the buffer (until a subsequent read does this as usual). This is intended to support a cleaner way to implement the fix in https://github.com/zynaddsubfx/zynaddsubfx/pull/195 so that it's not necessary to pull out the messages up until /state_frozen and then reinsert them into the ring buffer later.

This PR in itself does not change any behaviour in ThreadLink unless the new lookahead read functionality is actually used by the client; my plan is to submit a new PR for zynaddsubfx which uses the new read lookahead functionality once this PR has been approved.

The PR includes a test for the lookahead read operation to validate that it doesn't affect the normal read operation.

fundamental commented 2 years ago

The refactoring looks good, I'm just not sure about the design choice of having a full lookahead functionality. Lookahead for message_count=1 is very clean to implement and doesn't even touch most of the API, though it doesn't address the original problem.

Ideally the system just doesn't send any messages over to the RT thread while the state is pending-frozen, frozen, or pending-thawed, which would sidestep the requirement and additional complexity. I'll have to look back at the other issue to see if it's resolvable in that sense and if that's the case, at least cherry pick the code cleanups as you're right that having unique message arg values makes for a better unit test.

polluxsynth commented 2 years ago

I can understand your hesitation to the design choice. I agree that it is rather a large change for a special use case, and frankly, the current solution using a separate little temporary queue in Middleware works fine, and as you say there may be other solutions that completely sidestep the need for the lookahead feature. At the same time I feel the actual implementation of the lookahead feature turned out to be rather clean, with no real change to the core functionality, the only additional code in ordinary (non-lookahead) use is the synchronization of the lookahead pointer when non-lookahead read is called.

EDIT: Without having looked closely at the code, I'm wondering if the current situation of having various messages in the message queue prior to the /state_frozen message is simply a result of those messages having been generated prior to receiving /freeze_state, but not yet picked up by middleware. So they are actually not generated while the backend is the frozen state, they were generated before the backend saw the /freeze_state message.

fundamental commented 2 years ago

Gave the code another pass in case there was anything out of place, but the feature appears to be sound.

Thanks for the enhancement :+1:

polluxsynth commented 2 years ago

Thanks! I'll get onto pushing the corresponding change in Zyn to utiilize this feature as a PR shortly.