#220
- Created on2013-04-07 08:04
- Reported by
- StatusNew
- PriorityNormal (3)
- Assigned to-
- Milestone-
- Plan Level-
- Due Date-
Review OpenBSD sndio Host APl implementation
A patch to add a new Host API for OpenBSD's native "sndio" infrastructure is now in a branch pending review.
https://subversion.assembla.com/svn/portaudio/portaudio/branches/OpenBSD_sndio
If you are logged in to Assembla you should be able to review it here:
https://www.assembla.com/code/portaudio/subversion/commit/1887
If you are logged in you can click on the litte green + signs when you hover over lines in the diff to add comments.
Please add any additional comments to this ticket.
https://subversion.assembla.com/svn/portaudio/portaudio/branches/OpenBSD_sndio
If you are logged in to Assembla you should be able to review it here:
https://www.assembla.com/code/portaudio/subversion/commit/1887
If you are logged in you can click on the litte green + signs when you hover over lines in the diff to add comments.
Please add any additional comments to this ticket.
Leave a comment
I've reviewed the code and have annotated the commit here:
https://www.assembla.com/code/portaudio/subversion/commit/1887
Phil has added some comments too.
Some main points:
The code looks clean but doesn't conform to our usual naming standards. There are many unintelligible variable names that should be fixed. I have not offered suggestions for all of them but am happy to offer advice if needed.
Stream timestamps and GetStreamTime seem to be (1) not implemented, as sndioOnMove is never registered with the OS, and (2) possibly not implemented correctly, since the timestamp should be monotonic while the stream is open. It is also normal for it to be high resolution. ie have microsecond accuracy and be usable for things like scheduling MIDI events. A sample counter updated every buffer update doesn't have this property.
There are some issues with the handling of the user's data buffer parameter for ReadStream and WriteStream in the non-interleaved case.
The debug prints should be cleaned up -- the non-real-time safe ones should use PA_DEBUG(). Others could stay as-is, ideally with a comment indicating that enabling them will break real-time
There are other significant comments that I've added to the commit review. Phil has also added some comments there.
We can discuss this further on the mailing list.
https://www.assembla.com/code/portaudio/subversion/commit/1887
Phil has added some comments too.
Some main points:
The code looks clean but doesn't conform to our usual naming standards. There are many unintelligible variable names that should be fixed. I have not offered suggestions for all of them but am happy to offer advice if needed.
Stream timestamps and GetStreamTime seem to be (1) not implemented, as sndioOnMove is never registered with the OS, and (2) possibly not implemented correctly, since the timestamp should be monotonic while the stream is open. It is also normal for it to be high resolution. ie have microsecond accuracy and be usable for things like scheduling MIDI events. A sample counter updated every buffer update doesn't have this property.
There are some issues with the handling of the user's data buffer parameter for ReadStream and WriteStream in the non-interleaved case.
The debug prints should be cleaned up -- the non-real-time safe ones should use PA_DEBUG(). Others could stay as-is, ideally with a comment indicating that enabling them will break real-time
There are other significant comments that I've added to the commit review. Phil has also added some comments there.
We can discuss this further on the mailing list.