Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return status code instead of bool in IComposer #766

Closed
gavv opened this issue Jul 26, 2024 · 4 comments
Closed

Return status code instead of bool in IComposer #766

gavv opened this issue Jul 26, 2024 · 4 comments
Labels
easy hacks The solution is expected to be straightforward even if you are new to the project help wanted An important and awaited task but we have no human resources for it yet refactoring
Milestone

Comments

@gavv
Copy link
Member

gavv commented Jul 26, 2024

Summary

packet::IComposer is interface for protocol-specific serializators. See documentation.

Currently its methods return bool (true on success or false on error). We need to replace bool with status::StatusCode and return code that described why the operation failed.

Implementation

  • Update IComposer interface and implementations:
    • rtp::Composer
    • fec::Composer
    • rtcp::Composer
  • Report appropriate statuses:
    • when allocation failed, return StatusNoMem
    • when buffer is too small, return StatusBadBuffer
    • on success, return StatusOK
  • Update users of composers. They should forward status to upper level:
    • fec::BlockWriter
    • packet::Shipper
    • audio::Packetizer
    • rtcp::Communicator

Testing

  • Add unit tests for rtp::Composer and fec::Parser that check returned statuses in case of errors.

  • Add unit tests for components which use composers, and check that they forward statuses from composer to the upper level (e.g. if Composer fails with StatusNoMem, Shipper::write() returns StatusNoMem). One test per component should be enough.

    See Unit tests for parser & composer status codes #792

@gavv gavv added refactoring help wanted An important and awaited task but we have no human resources for it yet easy hacks The solution is expected to be straightforward even if you are new to the project labels Jul 26, 2024
@gavv gavv added this to Roc Toolkit Jul 28, 2024
@github-project-automation github-project-automation bot moved this to Frontlog in Roc Toolkit Jul 28, 2024
@gavv gavv moved this from Frontlog to Help wanted in Roc Toolkit Jul 28, 2024
@ronazf
Copy link

ronazf commented Oct 2, 2024

Hi @gavv, I'd be happy to help with this issue.

@gavv
Copy link
Member Author

gavv commented Oct 2, 2024

@ronazf Thanks, welcome!

@gavv
Copy link
Member Author

gavv commented Jan 16, 2025

Re-assigning to @sayedMurtadha

@gavv gavv added this to the next milestone Jan 21, 2025
gavv pushed a commit to sayedMurtadha/roc-toolkit that referenced this issue Jan 21, 2025
@gavv
Copy link
Member Author

gavv commented Jan 21, 2025

Landed

@gavv gavv closed this as completed Jan 21, 2025
@github-project-automation github-project-automation bot moved this from Help wanted to Done in Roc Toolkit Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy hacks The solution is expected to be straightforward even if you are new to the project help wanted An important and awaited task but we have no human resources for it yet refactoring
Projects
Status: Done
Development

No branches or pull requests

2 participants