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

Refax: smaller fixes and added utilities #2504

Merged
merged 16 commits into from
Sep 19, 2023

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Oct 28, 2022

Changes (LARGELY edited - lots of changes from this list were pushed through separate PRs):

  1. Added FormatLossArray function for debug purposes.
  2. Added some more logs and improved some existing logs.
  3. FormatDuration now uses fixed floating point format.
  4. Moved traceState to the interface and made generic so that it can be also used with other streams.
  5. The handler for SRTO_RCVBUF moved to a separate function for the sake of future reusage.
  6. Added some utilities to be used in the new code and some support the already added things
  7. Made the group name dispatching in the apps more generic
  8. Renamed raise_expection in FixedArray

@codecov-commenter

This comment was marked as off-topic.

@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Nov 1, 2022
@maxsharabayko maxsharabayko added this to the v1.6.0 milestone Nov 1, 2022
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No mercy to the reviewer 😢

srtcore/list.cpp Outdated Show resolved Hide resolved
@maxsharabayko
Copy link
Collaborator

Removed "syncOnMsgNo" flag in all occurrences. This was part of the future implementation of the balancing groups to use the synchronization on message number. That kind of synchronization will not be restored and the earlier implementation of the balancing groups that was using this kind of synchronization will not be restored.

This flag has landed in the Internet Draft and in previous versions of SRT, thus it has to be handled somehow.

As I see, you removed only the part where it was used to form the handshake.
I see it stays in the handshake processing - that is good, it has to be there.

Please move these changes to a separate PR.

You actually listed 12 changes unrelated to each other and put all of them in a single commit of a single PR with ~500 lines of changes. No mercy to the reviewer.

@ethouris
Copy link
Collaborator Author

ethouris commented Nov 1, 2022

Removed "syncOnMsgNo" flag in all occurrences....

This flag has landed in the Internet Draft and in previous versions of SRT, thus it has to be handled somehow.

That's why I did not remove the flag itself in the handshake data. Simply this time there's no way how it would be set.

As I see, you removed only the part where it was used to form the handshake. I see it stays in the handshake processing - that is good, it has to be there.

Intended. As it is now already in the protocol draft, I didn't want that it causes any changes there - still, there is no use of this feature anymore.

Please move these changes to a separate PR.

Will be done.

You actually listed 12 changes unrelated to each other and put all of them in a single commit of a single PR with ~500 lines of changes. No mercy to the reviewer.

I understand, but then note that - except for those related to the sync-on-msg flag (still, to be pused into a separate PR) - there are no dependencies between them. Every change entry is kinda on its own, unrelated to the others. The alternative would be to make a separate PR for every single change, or group them into multiple PRs by some weird criteria.

srtcore/handshake.h Outdated Show resolved Hide resolved
srtcore/list.cpp Outdated Show resolved Hide resolved
@ethouris ethouris marked this pull request as draft January 17, 2023 17:24
@ethouris ethouris marked this pull request as ready for review February 10, 2023 09:17
@maxsharabayko maxsharabayko modified the milestones: v1.5.2, v1.6.0 Feb 17, 2023
@maxsharabayko maxsharabayko merged commit 39a800c into Haivision:master Sep 19, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants