Skip to content

Conversation

paulomorgado
Copy link
Contributor

This might look a bit extreme, but the intention is to assure to the caller that the data provided won't be modified by using ReadOnlySequence<byte> instead of List<byte[]> and reduce memory allocated by ITrack implementations.

Changes:

  • Refactored audio and video track handling to utilize ReadOnlySequence<byte> for better performance and memory efficiency.
  • Introduced AdjustedSizeMemoryOwner for effective memory allocation.
  • Updated method signatures for RTP packet creation and sample feeding to accept ReadOnlySequence<byte>, ensuring consistency and optimizing memory usage.
  • Modified RTPPacketUtil to read timestamps from ReadOnlySpan<byte>, enhancing performance.
  • Overall code structure refined for improved readability and memory management.

@paulomorgado paulomorgado marked this pull request as draft January 17, 2025 17:04
@paulomorgado paulomorgado marked this pull request as ready for review January 17, 2025 18:34
@paulomorgado paulomorgado marked this pull request as draft January 18, 2025 19:55
@jimm98y
Copy link
Owner

jimm98y commented Jan 19, 2025

When it comes to SharpOnvifServer library changes, the only concern I have regarding these new APIs is the netstandard2.0 (in)compatibility. It might require using a polyfill such as https://github.com/[meziantou/Meziantou.Polyfill](https://github.com/meziantou/Meziantou.Polyfill) - this should be the same one SharpRTSP is already using, so it will not introduce any additional dependency if possible.

@paulomorgado
Copy link
Contributor Author

Making these changes helps me understand the library. 😄

@paulomorgado
Copy link
Contributor Author

Hy @jimm98y, can you have a look at this?

There's some extra copying, but it's all pooled and preserves memory ownership.

@paulomorgado paulomorgado force-pushed the performance branch 3 times, most recently from 7bdd55b to 5645c18 Compare June 2, 2025 15:43
@jimm98y
Copy link
Owner

jimm98y commented Jun 2, 2025

Awesome work, thank you very much! I'll have to try it out and understand how it works before merging it, I hope to get to later this week, or early next week.

Just one minor thing I noticed by briefly looking at the changes: using System.Drawing; in ProxyTrack.cs was probably not intentional :)

@paulomorgado
Copy link
Contributor Author

Awesome work, thank you very much! I'll have to try it out and understand how it works before merging it, I hope to get to later this week, or early next week.

I've submitted ngraziano/SharpRTSP#136. This will make it easier for this library.

Just one minor thing I noticed by briefly looking at the changes: using System.Drawing; in ProxyTrack.cs was probably not intentional :)

Definitely not intentional.

- Replaced `List<byte[]>` with `ReadOnlySequence<byte>` for RTP packets and raw samples, enhancing memory management and performance.
- Introduced `PooledByteBuffer` class implementing `IBufferWriter<byte>` for efficient byte data writing using an array pool.
- Updated `CreateRtpPackets` method signatures to return `IByteBuffer`, streamlining the API and reducing memory overhead.
- Modified `FeedInRawSamples` to accept `ReadOnlySequence<byte>`, improving flexibility in sample feeding.
- Added `MemoryExtensions` class for utility methods related to `ReadOnlySequence<byte>`.
- Improved error handling and logging consistency.
- Refactored code for better readability, maintainability, and alignment with modern C# practices, including standardized `using` statements and the use of `Span<byte>`.
@paulomorgado
Copy link
Contributor Author

Just one minor thing I noticed by briefly looking at the changes: using System.Drawing; in ProxyTrack.cs was probably not intentional :)

Definitely not intentional.

Fixed!

@jimm98y
Copy link
Owner

jimm98y commented Jun 11, 2025

Thank you very much! I finally got to try it yesterday evening, but there seems to be some issue when you just compile and run the RTSPServerApp:
image
Could you please take a look and try it?

@paulomorgado
Copy link
Contributor Author

I have a pending PR on SharpRTSP. I'll ping you when it's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants