fix(aac): missing timestamp rollover

videojs / mux.js

Lightweight utilities for inspecting and manipulating video container formats.

Apache License 2.0

1.11k stars 210 forks source link

fix(aac): missing timestamp rollover #430

Open amtins opened 1 year ago

amtins commented 1 year ago

Description

This PR fixes the missing timestamp rollover in AAC stream packets.

Refer https://github.com/videojs/http-streaming/issues/1371#issue-1582588141

Specific Changes proposed

handle PTS and DTS timestamp rollover

Requirements Checklist

[X] Feature implemented / Bug fixed
[ ] If necessary, more likely in a feature request than a bug fix
- [ ] Change has been verified in an actual browser (Chrome, Firefox, IE)
- [ ] Unit Tests updated or fixed
- [ ] Docs/guides updated
- [ ] Example created (starter template on JSBin)
[ ] Reviewed by Two Core Contributors

amtins commented 1 year ago

@adrums86 thank you for taking the time to review this PR.

Unfortunately, I'm not sure I'm much help with this PR, it was only by luck that I managed to find a fix. So it is quite possible that there is a better way to fix this behavior.

I would have liked to add unit tests, but unfortunately I can't figure out how it all works so I can't produce a test case.

However, if you need a media to test I have a sample that I can share via slack.

adrums86 commented 1 year ago

@amtins, no worries, your contribution is very much appreciated! I'll spend some time in the next week or so and see if I can help at all with a test or possible segment extraction of the dts.

adrums86 commented 1 year ago

I think this is a great improvement in general and we should roll forward with this change if possible. I'd like to get another set of eyes on this, however. @dzianis-dashkevich can you take a look at this when you have a chance and possibly weigh in on my original question, regarding extracting the AAC dts value in a different way or does this seem the best way to go about it? TIA.

amtins commented 1 year ago

@adrums86 I tried to add test coverage as best I could.

adrums86 commented 1 year ago

@amtins Thank you for getting back to this and adding a test! This LGTM.

dzianis-dashkevich commented 1 year ago

@adrums86 , I believe that AAC has only raw encoded audio data without timing or synchronization information. I think pts/dts info is only available on the container level (ts/mp4).

I am still not sure it is a good idea to have pts/dts info on codecs level streams (I mean forward it), but I would rather not touch it and keep it as is. I would hope that we all will move from TS to CMAF, and this repo will become history :

adrums86 commented 1 year ago

@dzianis-dashkevich makes sense. Big +1 to moving to CMAF 😄 . Lets merge this thing. Thanks again for doing the work @amtins!

Ruud-Gerrits commented 1 year ago

Hi, any idea on when this will be merged?