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/issue 1331 error on dai discont #1409

Merged

Conversation

tchakabam
Copy link
Collaborator

Description of the Changes

Fixes #1331

  1. TS->MP4 remuxing: We are making sure to keep consistent track IDs for each type of track.

Before we were following the TS PIDs. These can change on discontinuities. Switching track IDs, following PID, doesn't have any advantage for (we only have one track of each anyway). It is allowed by ISOFF when we only have one track of a type. It turns out that Safari has issues and was dropping all frames when it was set up to demux from a specific track ID at the start, and then the video track having a different one (Hls.js ISSUE-1331).
We add here constant track IDs per remuxed track type. We also make sure that we keep carrying the PID in our track state, and also handle missing PIDs, but the PID never drives the track ID at any point. We also add some more spec/docs and centralize the creation of TS demuxer internal track model.

  1. Adds progress display to webpack builds

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • no commits have been done in dist folder (we will take care of updating it)
  • new unit / functional tests have been added (whenever applicable)
  • Travis tests are passing (or test results are not worse than on master branch :))
  • API or design changes are documented in API.md

… each type of track.

Before we were following the TS PIDs. These can change on discontinuities. Switching track IDs, following PID, doesn't have any advantage for (we only have one track of each anyway). It is allowed by ISOFF when we only have one track of a type. It turns out that Safari has issues and was dropping all frames when it was set up to demux from a specific track ID at the start, and then the video track having a different one (Hls.js ISSUE-1331).
We add here constant track IDs per remuxed track type. We also make sure that we keep carrying the PID in our track state, and also handle missing PIDs, but the PID never drives the track ID at any point. We also add some more spec/docs and centralize the creation of TS demuxer internal track model.
@dailos2coders
Copy link

Confirmed working in our environment. 👍

Thanks!

@mangui
Copy link
Member

mangui commented Oct 27, 2017

thanks @tchakabam !
the code change looks good.
could you remove dist.zip ?
also it would be good to add the #1331 test stream in streams.json so that it will be tested accross all browsers.

@tchakabam
Copy link
Collaborator Author

oh, no idea how dist.zip appeared there :)

@tchakabam
Copy link
Collaborator Author

Will add test stream.

Need to figure out however why the CI job is failing at:

"before each" hook for "should "smooth switch" to highest level and still play(readyState === 4) after 12s for AES encrypted,ABR"

I re-ran the job before to see if it is something occuring rarely, but it happened again, so consistent. Trying again restarting now.

Any ideas?

Would it be related to our change?

Isn't it very weird that 4965d0e was still OK and 5f14df2 not? Only diff was we removed a file that was committed by accident, so cant be the reason.

I wonder how the build is not completely reproducible. Are the browser test-platforms changing, could we have switched versions between these two jobs?

@tchakabam
Copy link
Collaborator Author

tchakabam commented Oct 30, 2017

-> Will try to reproduce that locally later!

-> Probably a bug related to our change but error happening only at ~30% probability ?

EDIT: Build job has normalized now after 4th re-run attempt.

@mangui mangui merged commit ce19344 into video-dev:master Oct 31, 2017
@tchakabam tchakabam deleted the fix/issue-1331-error-on-dai-discont branch October 31, 2017 14:22
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.

None yet

3 participants