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.

Sign up for GitHub

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

Jump to bottom

Add captureTime and senderCaptureTimeOffset to frame metadata #240

Merged
merged 1 commit into from
Feb 27, 2025
Merged

Add captureTime and senderCaptureTimeOffset to frame metadata #240

merged 1 commit into from
Feb 27, 2025

Conversation

guidou
Copy link
Collaborator

@guidou guidou commented Feb 20, 2025 •
edited by pr-preview bot
Loading

@guidou guidou requested review from youennf and jan-ivar February 20, 2025 14:05
@guidou
Copy link
Collaborator Author

guidou commented Feb 20, 2025

Fixes #225

@guidou
Copy link
Collaborator Author

guidou commented Feb 20, 2025

This is the same as #228, but with the extension URL updated to the IETF version.
Also incorporates a comment to rename captureTimestamp to captureTime for consistency with other specs.

alvestrand
alvestrand reviewed Feb 20, 2025
View reviewed changes
@jan-ivar
Copy link
Member

This is the same as #228, but with the extension URL updated to the IETF version. Also incorporates a comment to rename captureTimestamp to captureTime for consistency with other specs.

SGTM

jan-ivar
jan-ivar approved these changes Feb 20, 2025
View reviewed changes
Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

Next steps would be for each source's spec to specify whether they are to populate this metadata or not, as suggested by @guidou today.

With that this LGTM.

index.bs Outdated
Comment on lines 728 to 729
The {{RTCEncodedAudioFrameMetadata/captureTime}} is set by the frame source, and for frames that come
from the {{RTCRtpReceiver}}, it is extracted by the [[#stream-processing]] algorithm. Its reference clock
Copy link
Member

Choose a reason for hiding this comment

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

Is this trying to say the stream processing algorithm is a consumer of captureTime? It sounds like it's saying it's the source of it instead, which seems confusing. Can we rephrase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaced the confusing text with a simpler definition and what to return. Updated it so that the field is expressed relative to Performance.timeOrigin instead of the NTP epoch, which is an uncommon epoch to use in Web APIs.

@guidou guidou force-pushed the guidou/capture-timestamp branch from 91ab07d to 7510510 Compare February 24, 2025 14:17
jan-ivar
jan-ivar approved these changes Feb 27, 2025
View reviewed changes
@guidou guidou force-pushed the guidou/capture-timestamp branch from eb7bae8 to 7bd2167 Compare February 27, 2025 16:09
@guidou guidou force-pushed the guidou/capture-timestamp branch from 7bd2167 to f259a4d Compare February 27, 2025 16:11
@guidou guidou merged commit c45d125 into main Feb 27, 2025
2 checks passed
github-actions bot added a commit that referenced this pull request Feb 27, 2025
@guidou @github-actions
Merge pull request #240 from w3c/guidou/capture-timestamp
b25db2c 
SHA: c45d125
Reason: push, by guidou

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvestrand alvestrand alvestrand left review comments

@jan-ivar jan-ivar jan-ivar approved these changes

@youennf youennf Awaiting requested review from youennf

Assignees
No one assigned
Labels
Editors can integrate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guidou @jan-ivar @alvestrand