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

Issue 688: UDP packet has timestamp field #719

Merged
merged 1 commit into from
May 8, 2024

Conversation

baranovmv
Copy link
Member

@baranovmv baranovmv commented Apr 28, 2024

UDP::enqueue_ts is filled in
ReceiverSession::route_packet() right before
the packet gets into jitter buffer queue.

#688

@baranovmv baranovmv requested a review from gavv April 28, 2024 18:13
@github-actions github-actions bot added the ready for review Pull request can be reviewed label Apr 28, 2024
Copy link

🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.

@baranovmv baranovmv changed the title UDP packet has timestamp field Issue 688: UDP packet has timestamp field May 6, 2024
Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks, a few comments.

Comment on lines 248 to 249

packet->udp()->enqueue_ts = core::timestamp(core::ClockUnix);
return packet_router_->write(packet);
Copy link
Member

Choose a reason for hiding this comment

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

  1. Maybe move this into packet::Router::write? Packet router is supposed to be entry point to the packets pipeline, so it looks like a good place to be responsible for filling timestamps. On the other hand, it would be nice to keep pipeline more high-level.

  2. We need a guard for the case when packet->udp() returns NULL. (I think we can just skip timestamp filling in this case).

@gavv gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels May 7, 2024
@gavv gavv added this to the next milestone May 8, 2024
@gavv gavv merged commit 693c796 into roc-streaming:develop May 8, 2024
3 checks passed
@github-actions github-actions bot removed the needs revision Pull request should be revised by its author label May 8, 2024
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.

None yet

2 participants