fix(aac): missing timestamp rollover

[amtins]

Description

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

Refer videojs/http-streaming#1371 (comment)

Specific Changes proposed

• handle PTS and DTS timestamp rollover

Requirements Checklist

• 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

[adrums86]

A unit test or 2 would be a great addition. Also, I have a question regarding AAC, is it possible to extract the dts from the bytes instead of setting it from the timeStamp, it seems that this would allow TimestampRolloverStream to handle the rollover instead?

[amtins]

@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]

@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]

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]

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

[adrums86]

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

[dzianis-dashkevich]

@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]

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

[Ruud-Gerrits]

Hi, any idea on when this will be merged?