Fix some videos having offsetted (incorrect) timestamps#8029
Conversation
Wumpf
added
🪳 bug
Wumpf
changed the title
|
tested on firefox & chrome now to have correct timestamps |
|
just noticed a bug here |
|
Fixed. This also |
| #[inline] | ||
| pub fn from_nanos(v: i64, timescale: Timescale) -> Self { | ||
| Self::from_secs(v as f64 / 1e9, timescale) | ||
| pub fn from_nanos_since_start( |
There was a problem hiding this comment.
Maybe a single fn from_duration_sicne_start(duration: std::time::Duration, …) could replace all these, to produce a more minimal interface
There was a problem hiding this comment.
still have to keep a nanos version one because std::time::Duration has imho waaaay too much overhead for converting to our VideoTimestamp (we expect to converts hundereds of thousands in some cases)
There was a problem hiding this comment.
the other nastiness is that duration panics when using negative values.
Actually that last one compells me strongly to roll back what I did. std::time::duration is not safe when dealing with user data
| /// Convert to a duration | ||
| #[inline] | ||
| pub fn duration(self, timescale: Timescale) -> std::time::Duration { |
There was a problem hiding this comment.
It seems like it is time to split Time into two types; one for time-points and one for durations. Both require timescale, but only one require the start_time offset.
There was a problem hiding this comment.
let's not explode and delay this PR even further with cosmetics like that. The time type was never meant to be all that complex, it's just a typed "this is a video time that uses a video time scale"
|
Not going to follow through on std::time::Duration suggestions: It is too error prone since its constructors panic on negative values, causing us to do lots of extra checking in various places. |
Wumpf
changed the title
2 participants
What
.. and adds a few more "expert" information fields to video blob plus improving the selection panel view overall a bit
Before:
After:
-> note how the timestamp component and the timestamped etched into the video are now the same.
The problem is that in the presence of B-frames it can happen that presentation timestamps don't start at 0. This is because in MP4 (and other formats?) timestamps can't be negative and the first timestamp may have a decode timestamp of zero but a presentation timestamp some point later. Turns out this happens in a bunch of the example video material I'm using - not too surprising given that ffmpeg's default encoding settings on h264 tend to produce this!
From what I found (see links in comments) this is compensated by players. We do so now on the fly in key locations, preserving the internal PTS/DTS values of the samples. I tried to sharpen the docs where appropriate.
(on a personal note: I don't quite fully follow yet how this makes sense practically given that the first frame should be an IDR frame (it is, right?). But that's 100% what's happening and I found strong indication that players compensate for this just like we do here now)
Checklist
mainbuild: rerun.io/viewernightlybuild: rerun.io/viewerCHANGELOG.mdand the migration guideTo run all checks from
main, comment on the PR with@rerun-bot full-check.