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

simpleton live alt-audio support inspired by the stream-controller. #727

Closed
wants to merge 3 commits into from

Conversation

fredvb
Copy link
Collaborator

@fredvb fredvb commented Oct 6, 2016

For issue #718
This is a very barebone attempt at making Live streams work with alt-audio.
Any advice or suggestions or full rewrites are welcome.
I'm not sure why the travis-ci failed though.

@fredvb
Copy link
Collaborator Author

fredvb commented Oct 6, 2016

The code seems to work with a single video level stream. On a multi-level video stream, the audio seems to stop working as soon as the level is changed, and seems to resume if it goes back to the original video level. I believe this might be another issue by itself.

Feel free to reject this pull request and just get inspired if you think it's better off being re-written another way.

@mangui
Copy link
Member

mangui commented Oct 6, 2016

Hi @fredvb it is a good start.
I still have an issue (but I am not blaming you as I originally wrote audio-stream-controller) with duplicated/shared code between stream-controller and audio-stream-controller.
this might deserve some thinkings. but we could do it step by step. first support live alt audio by duplicating a little bit. once working we try to factorize the commonalities.
regarding live alt-audio and level switch not working with your changes, could you share a playlist ?

@mangui
Copy link
Member

mangui commented Oct 7, 2016

There is another important constraint for live alt audio: timestamp needs to be synced between main/alt audio. It is not the case currently... it was not an issue for VoD as main and audio share the same timeline. But this is needed as the two playlists are not sliding in sync. this might require providing a reference PTS to audio demuxer, this reference PTS being extracted from main demuxer. This ref PTS could be provided in FRAG_PARSING_DATA.
This means that a new state would have to be added in audio stream controller: WAITING_MAIN_PTS.

@fredvb
Copy link
Collaborator Author

fredvb commented Oct 7, 2016

Oh my, my personal repo's merge is attempting to commit to this.. I'm lost, should I not be merging my repos until I have a more final change ? Or is this OK ? I'm not sure if you'd want a merge to appear further down the road if you approve upcoming changes.

Lemme know what is your preference. We can always redo a fresh pull request that doesn't require a merge later on.
PS: I'll be looking into your previous feedback today.

@mangui
Copy link
Member

mangui commented Oct 7, 2016

you should better rebase your PR instead of merging upstream.
but anyway that's not a big deal, you could rewrite the history later on user
git rebase -i hash_id
so don't worry, just continue your work here, we will handle that later while merging the PR

@fredvb
Copy link
Collaborator Author

fredvb commented Oct 7, 2016

@mangui "regarding live alt-audio and level switch not working with your changes, could you share a playlist ?" I couldn't reproduce it at the moment, but if I encounter it again, I'll try to send you what I can.

@sergiojm
Copy link
Contributor

sergiojm commented Oct 7, 2016

Confirmed lip sync problems with alt audio

@fredvb
Copy link
Collaborator Author

fredvb commented Oct 9, 2016

@mangui I must say, I've look around in the demuxer, stream controllers and remuxers and I'm not too sure how you'd want to do this. Any more clues might help.
From what I'm imagining, we'd need to ask the audio demuxer to start creating a buffer from the 'main's PTS so that they start in sync, but maybe I'm not totally understanding how hls.js works.

@fredvb
Copy link
Collaborator Author

fredvb commented Oct 10, 2016

Going to re-investigate my options after looking at your commit dailymotion@dcf67a1

@fredvb
Copy link
Collaborator Author

fredvb commented Oct 11, 2016

@mangui I think I'm going to have to withdraw my attempt at fixing the sync issue.
Did you have any chance at looking into it yourself ?

@mangui
Copy link
Member

mangui commented Oct 12, 2016

the idea would be to propagate initPTS/initDTS info in FRAG_PARSING_INIT_SEGMENT event
audio stream controller would then wait for initPTS/initDTS from main demuxer to instantiate audio demuxer (these initPTS/initDTS should be new optional params to demuxer)

this will then ease to keep main and audio demuxer in sync.

I can look into it providing that I have live alt-audio test streams available.

@sergiojm
Copy link
Contributor

@mangui sent u by mail a live m3u8 for alternative audio

@nitrat7
Copy link

nitrat7 commented Oct 13, 2016

here's another live m3u8 with alternate audio:
http://haiv-nginx-test.westeurope.cloudapp.azure.com/playlist.m3u8
audio "ping" on video scene-change.

@kanongil
Copy link
Contributor

kanongil commented Oct 13, 2016

The initPTS relaying is already handled using a INIT_PTS_FOUND event in https://github.com/dailymotion/hls.js/pull/700.

@mangui
Copy link
Member

mangui commented Oct 13, 2016

Hi @kanongil thanks for #700. I will try to review it soon ...
regarding INIT_PTS_FOUND, good idea, but the event should also carry the demuxer id, and the fragment continuity counter.

@fredvb
Copy link
Collaborator Author

fredvb commented Oct 13, 2016

And there is this playlist http://haiv-nginx-test.westeurope.cloudapp.azure.com/master.m3u8 which is the same as @nitrat7 's stream but with the CODECS included. (makes the video work)

@NicolasWeil
Copy link

NicolasWeil commented Nov 16, 2016

Got an Elemental Live demuxed low latency test steam for you, folks :
http://msl32europe-i.akamaihd.net/hls/live/262233/a2shlslowlatnodvr/index.m3u8
It's 1080p+720p+Alt AAC audio, using 1 second segments.
Will try to keep it alive as long as possible.

sergiojm pushed a commit to sergiojm/hls.js that referenced this pull request Nov 17, 2016
Merge branch 'pull-727' into new-live-alternative-audio
@mangui
Copy link
Member

mangui commented Dec 6, 2016

closing in favor of #860 that also contains these changes

@mangui mangui closed this Dec 6, 2016
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

6 participants