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

Fix ivf timestamps #2987

Merged
merged 7 commits into from
Jan 2, 2025
Merged

Fix ivf timestamps #2987

merged 7 commits into from
Jan 2, 2025

Conversation

xdrudis
Copy link
Member

@xdrudis xdrudis commented Jan 2, 2025

Description

IVFwriter produces incorrect timestamps leading to unplayable video with the changes introduced in #2853.

The confusion lies in the different timestamps.
The RTP timestamp refects the sampling clock for the media stream. This timestamp increments based on the sampling rate.
The PTS of a media stream represents the presentation time in the time base units.

Example

v3 first captured packets from examples/save-to-disk (ffprobe -select_streams v -show_packets -show_entries packet=pts,pts_time,dts,dts_time,duration,duration_time,size,pos -of default output.ivf)

[PACKET]
pts=0
pts_time=0.000000
dts=0
dts_time=0.000000
duration=1
duration_time=0.033333
size=2749
pos=32
[/PACKET]
[PACKET]
pts=1
pts_time=0.033333
dts=1
dts_time=0.033333
duration=1
duration_time=0.033333
size=61
pos=2793
[/PACKET]
[PACKET]
pts=2
pts_time=0.066667
dts=2
dts_time=0.066667
duration=1
duration_time=0.033333
size=46
pos=2866
[/PACKET]
[PACKET]
pts=3
pts_time=0.100000
dts=3
dts_time=0.100000
duration=1
duration_time=0.033333
size=222
pos=2924
[/PACKET]

v4 first captured packets from examples/save-to-disk (prior to this PR)

[PACKET]
pts=556350009
pts_time=18545000.300000
dts=556350009
dts_time=18545000.300000
duration=1
duration_time=0.033333
size=2712
pos=32
[/PACKET]
[PACKET]
pts=556353159
pts_time=18545105.300000
dts=556353159
dts_time=18545105.300000
duration=1
duration_time=0.033333
size=64
pos=2756
[/PACKET]
[PACKET]
pts=556356129
pts_time=18545204.300000
dts=556356129
dts_time=18545204.300000
duration=1
duration_time=0.033333
size=229
pos=2832
[/PACKET]

v4 first captured packets with this PR

pts=0
pts_time=0.000000
dts=0
dts_time=0.000000
duration=1
duration_time=0.033333
size=2851
pos=32
[/PACKET]
[PACKET]
pts=1
pts_time=0.033333
dts=1
dts_time=0.033333
duration=1
duration_time=0.033333
size=61
pos=2895
[/PACKET]
[PACKET]
pts=2
pts_time=0.066667
dts=2
dts_time=0.066667
duration=1
duration_time=0.033333
size=232
pos=2968
[/PACKET]

@xdrudis
Copy link
Member Author

xdrudis commented Jan 2, 2025

cc @radekg, @edaniels

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

Attention: Patch coverage is 76.47059% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.38%. Comparing base (92fce5f) to head (803fd08).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
pkg/media/ivfwriter/ivfwriter.go 80.00% 2 Missing and 3 partials ⚠️
pkg/media/ivfreader/ivfreader.go 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2987      +/-   ##
==========================================
+ Coverage   77.84%   79.38%   +1.54%     
==========================================
  Files          89       78      -11     
  Lines       10524     9649     -875     
==========================================
- Hits         8192     7660     -532     
+ Misses       1842     1541     -301     
+ Partials      490      448      -42     
Flag Coverage Δ
go 79.38% <76.47%> (-0.03%) ⬇️
wasm ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -5,7 +5,6 @@ package ivfwriter

import (
"bytes"
"encoding/binary"
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes in this test just undo https://github.com/pion/webrtc/pull/2853/files

Copy link
Member

@edaniels edaniels left a comment

Choose a reason for hiding this comment

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

LGTM

@edaniels
Copy link
Member

edaniels commented Jan 2, 2025

Just need to fix the lint and we're good to go

@xdrudis
Copy link
Member Author

xdrudis commented Jan 2, 2025

Just need to fix the lint and we're good to go

The problem lies in the two Revert commits , which take the message just a bit over 50 characters. Do you have the power to overwrite this check? Ideally the linter would be able to account for this particular case.

@edaniels
Copy link
Member

edaniels commented Jan 2, 2025

Just need to fix the lint and we're good to go

The problem lies in the two Revert commits , which take the message just a bit over 50 characters. Do you have the power to overwrite this check? Ideally the linter would be able to account for this particular case.

I do :)

@edaniels edaniels merged commit 1ee0299 into pion:master Jan 2, 2025
16 of 17 checks passed
@edaniels
Copy link
Member

edaniels commented Jan 2, 2025

Done!

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

Successfully merging this pull request may close these issues.

2 participants