-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
decoupling Streaming and Buffering logic #225
Conversation
this.state = State.IDLE; | ||
} else { | ||
this.state = State.PARSED; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem with adding the above here is that it brings us back to where we were. namely the MSE objects and segment queue being dealt with in the download scheduler.
we should actually rather evaluate this in a callback from the buffer events.
in fact that is what onBufferAppended
is for. when you get that event, you know that something has been appended and that mp4segments
should be "emptier" than before. or the subsequent FRAG_BUFFERED
event that is triggered by the BUFFER_APPENDED
handler.
but i think what you really want is to know wether segments have been appended to put your state machine back in idle. that could be achieved definitely via the above events and an appropriate counter on the state machine side of how often BUFFER_APPENDING
event was triggered.
in fact maybe i don't really understand what this is about though ... can you explain why you need to make this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I know it will be problematic while splitting into two classes:
my first first idea was to do it right straight from the beginning by counting the BUFFER_APPENDING
/BUFFER_APPENDED
, and ensuring that everything is appended...
but the issue is that onBufferCodecs is implicitely pushing segments instead of triggering BUFFER_APPENDING
. but still after init segment moov are appended, a BUFFER_APPENDED
is triggered ... so counting cannot work until onBufferCodecs uses BUFFER_APPENDING
I will try to sort this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the meantime, I will merge similar logic to master 'as is', as this case (receiving FRAGMENT_PARSED although all segments have already been appended) could also happen there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK just wanted to make sure you're aware of that :)
I am happy to find a solution to this in the next step!
I think its good that such architecture will also force us to become more clear in the semantics.
To describe and differentiate what is media segment and what is codec data.
But I still didn't understand I think why do you need this logic exactly? Can't the streaming logic operate independently of the fact wether all segments were appended? Doesn't it simply need to know how much data we have buffered (which it can check upon the FRAG_BUFFERED event for example)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can try this way. this means that when receiving FRAG_PARSED, we could move back to IDLE state straight away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. is there a reason why we have to check if the SourceBuffer is still appending? in fact the state machine doesn't need to care right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, in fact not really ...
current logic is that stream loader is using media buffered ranges to determine which fragment to be loaded next. by looking for the next buffer hole upfront of current media position.
if we start looking for the next fragment to be loaded although buffer appending is not completed, media buffered ranges might still be the same ... so we will end up selecting the same fragment (or a fragment at another quality level matching same position ...)
this is why we need to wait for previous fragment to be completely appended to start loading next one.
if we want to get rid of this, we would have to rely on buffer range extracted from demuxer/remuxer, and not from the one from the underlying media engine.
this would mean keeping a virtual buffer layer in stream loader.
but the issue is that the media engine can evict data from the underlying range, without noticing.
this means that there needs to be a kind of consistency check to keep virtual buffer and real media buffer in sync...
maybe syncing on BUFFER_APPENDED ? this might be feasible, but let's do it in a second step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUFFER_APPENDED is triggered when SourceBuffer is done with appending (updateend). That's exactly what you want. That will allow you to know when buffered ranges have been updated.
That's when you have to schedule more segments to be downloaded I'd say.
If you have to constantly check on the MSE object to make sure you don't download things twice, that'd rather indicate another problem in your downloader.
In fact you simply need to keep state in your downloader what you have downloaded last, and continue with the next segment as long as you didn't seek I think.
The buffered range indicated by the media object should simply help you to limit how far ahead you want to download, or in case of seek if a download is necessary or not.
let me know if i can help with anything or if there are other things to fix! :) |
@tchakabam i did a first split. i checked basic functionalities, instant and smooth switching against FF/Chrome/Edge, and everything is working as expected. |
great! will check this out asap! |
…hing state useless
restore codec selection logic don't flush buffer range less than 500ms large (fix smooth switching not working on FF)
wait for SB update end to flush another buffer range if sourcebuffer is busy updating
when FRAG_PARSED event is received after successful appending of all segments
on QuotaExceededError while appending a sourceBuffer, trigger a smooth level switch this will free past and future buffer range seamlessly, without interrupting the playback. also reduce maxMaxBufferLength to avoid loop of QuotaExceededError, and loop of flushing ... related to https://github.com/dailymotion/hls.js/issues/100 related to https://github.com/dailymotion/hls.js/issues/165
pass container info from remuxer related to https://github.com/dailymotion/hls.js/issues/233
get rid of moof/mdat fields related to https://github.com/dailymotion/hls.js/issues/233
remuxer will depend on input container type and MediaSource capabilities => remuxer choice should be done after input container type probing related to https://github.com/dailymotion/hls.js/issues/233
decoupling Streaming and Buffering logic
👏 🍻 great! :D |
when this PR was not merged, my issue on problem when audio and video has 2 separate urls was linked to this PR, but when this got merged, the issue is still there. |
@whatvn I think it needs further work to fix that specific issue, I think what I meant is that this will help to fix it maybe. |
* Improve alternate audio fragment loading, and general fragment finding behavior for live streams JW8-9842
refer to https://github.com/dailymotion/hls.js/pull/215