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

base audio demuxer: track PTS of last sample for ID3 frames #5014

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

jprjr
Copy link
Contributor

@jprjr jprjr commented Nov 7, 2022

This PR will...

  • Add a new field to the base audio demuxer (lastPTS), which tracks the PTS of the most recently added audio frame.
  • Use lastPTS as the PTS time when adding ID3 frames

Why is this Pull Request needed?

Changes in #4847 (to fix #4835) caused the basePTS value to only update on discontinuity events, which causes ID3 frames to all have the same PTS.

This tracks the PTS of added frames in a new field, which is then used when adding ID3 frames. This field is reset anytime on discontinuities and defaults to the basePTS value when no other value is available (first sample, etc).

Are there any points in the code the reviewer needs to double check?

Not that I'm aware of

Resolves issues:

#4963

Additional note, I also tested with the test video in #4835 and confirmed it still plays correctly as well.

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@robwalch robwalch added this to the 1.2.6 milestone Nov 7, 2022
@jprjr jprjr changed the title base audio demuxer: reset frameIndex after flush (fixes #4963) (WIP) base audio demuxer: reset frameIndex after flush (fixes #4963) Nov 7, 2022
@jprjr jprjr marked this pull request as draft November 7, 2022 23:01
@jprjr jprjr changed the title (WIP) base audio demuxer: reset frameIndex after flush (fixes #4963) base audio demuxer: recalculate basePTS anytime a timestamp is available Nov 7, 2022
@jprjr jprjr marked this pull request as ready for review November 7, 2022 23:12
@jprjr
Copy link
Contributor Author

jprjr commented Nov 7, 2022

@robwalch sorry for the initial noise - my first attempt with resetting the frameindex allowed the test video with broken timestamp values to play - but my volume was low, and I didn't notice the audio got out of sync. I've force-pushed a new fix.

In this version, I changed the logic for recalculating the basePTS value to trigger whenever a timestamp is available.

Copy link
Collaborator

@robwalch robwalch left a comment

Choose a reason for hiding this comment

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

I'm guessing this works because each of your segments contains an ID3 timestamp?

Comment on lines 78 to 81
if (
this.basePTS === null ||
(this.frameIndex === 0 && Number.isFinite(timestamp))
) {
Copy link
Collaborator

@robwalch robwalch Nov 7, 2022

Choose a reason for hiding this comment

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

This probably works because each of your audio segments has timestamp data (Number.isFinite(timestamp) is true). If they did not, basePTS would be the first PTS of the first segment we started demuxing. And that's what it should be as segments without ID3 timestamps are added based on frameIndex * sampleRate (ADTS.appendFrame).

So to resolve this issue we either need another property like lastPTS which is the timestamp of the last frame or a method that will read the sample rate and return a current pts track time like we have at the top of appendFrame in each audio demuxer. This way the latest PTS can be assigned to ID3 samples, and not only audio sample.

Big thank you for root causing this!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also have a feeling that frameIndex === 0 was important, but I don't recall why off the top of my head - maybe for MP3 and/or one of the audio-only demo page streams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robwalch yeah - all my segments have ID3 timestamps.

No problem finding the root cause! Happy to help.

I just force-pushed a new attempt that adds a lastPTS field and uses that, this seems to work with both the test stream from #4835 as well as others.

@jprjr jprjr changed the title base audio demuxer: recalculate basePTS anytime a timestamp is available base audio demuxer: track PTS of last sample for ID3 frames Nov 8, 2022
@robwalch robwalch merged commit 25a3906 into video-dev:master Nov 8, 2022
@robwalch robwalch modified the milestones: 1.2.6, 1.2.7 Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HLS.js play failed with the AAC audio stream for v1.2
2 participants