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 load pending routine. #2564

Closed
wants to merge 1 commit into from
Closed

Fix load pending routine. #2564

wants to merge 1 commit into from

Conversation

mofneko
Copy link
Contributor

@mofneko mofneko commented Mar 16, 2017

The loadPlaylist method has multiple conditions called.
I do not think that only postDelayed tasks can inhibit it.

@AquilesCanta
Copy link
Contributor

Can you provide more context on what you are trying to achieve here?

Any onLoadCompleted() that does not have a corresponding processLoadedPlaylist() is an unnecessary http request (and parsing), so you should have made the network request in the first place. Can you explain and provide concrete examples?

@mofneko
Copy link
Contributor Author

mofneko commented Mar 16, 2017

The philosophy of this modification is limited to where to suppress execution of processLoadedPlaylist, which corresponds to reload processing based on HLS spec v 20, section 6.3.4.
Before the change, execution of loadPlaylist, which is the start of the reloading process, was suppressed by the condition, but since calling loadPlaylist is performed even other than executing with processLoadedPlaylist as the starting point, it depends on the state of processLoadedPlaylist It is that it should not be.

@AquilesCanta
Copy link
Contributor

Can you please clarify:

but since calling loadPlaylist is performed even other than executing with processLoadedPlaylist as the starting point, it depends on the state of processLoadedPlaylist It is that it should not be.

Again, if possible, a concrete example would be helpful.

@mofneko
Copy link
Contributor Author

mofneko commented Mar 17, 2017

For example, loadPlaylist may be executed also from refreshPlaylist, maybeSelectNewPrimaryUrl, maybeSetPrimaryUrl at present.
refreshPlaylist is specifically invoked by the onPlaylistRefreshRequired event. Perhaps the current implementation will not cause problems. However, the name of the method is important, and in this case, the HLS reload routine takes precedence over the above event.

@AquilesCanta
Copy link
Contributor

In all three cases it makes sense to make the reloading of the playlist take place later: If there is a pending refresh, it means that we have loaded the playlist recently, and that a reloading will most likely not see anything new.

Regarding this particular pull request: Do you agree that we should not discard a playlist that has already been loaded? If so, please close the issue.

Regarding

refreshPlaylist is specifically invoked by the onPlaylistRefreshRequired event.

My original aim with the extra condition was to avoid any kind of reloading when a playlist reload is pending. If there is a pending load, it means that the Playlist tracker has decided it needs to try at that point. Any other component has no better knowledge of playlist lifecycle than the tracker, so it does not matter where the loadPlaylist() comes from. Basically, as you said:

the HLS reload routine takes precedence over the above event

I don't think the method's name (I guess you mean loadPlaylist()) is misleading. The playlist will be loaded. It's up to the playlist tracker to decide when. I hope I am not misunderstanding anything. Again, thanks for the pull request! All improvement suggestions are more than welcome.

@mofneko
Copy link
Contributor Author

mofneko commented Mar 18, 2017

I certainly think that this is reasonable. Thank you for your reply.
One last thing.
I also think loadPlaylist should be loaded accordingly when necessary.
For example, this implementation can not cope when it is necessary to reacquire the playlist immediately within the postLoad period.
If so, I think that it is best to suppress duplication of the next processLoadedPlaylist with onLoadCompleted after performing the acquisition process as requested by loadPlaylist execution event like this PR.
Also, I think that processLoadedPlaylist should exist as an independent process until run. You should not interfere with other parts.
Please let me hear your opinion regarding this.

@AquilesCanta
Copy link
Contributor

should be loaded accordingly when necessary

I agree with that. But, if loaded, it should be processed. This is not the case in the PR.

this implementation can not cope when it is necessary to reacquire the playlist immediately

When would this be the case? Bear in mind that the fact that someone other than the tracker requests a playlist loading does not mean that it is the correct moment to load the playlist. Please clarify why you think it is necessary to reacquire the playlist immediately.

processLoadedPlaylist should exist as an independent process until run.

Why? Do you think there is a heavy computational load in processLoadedPlaylist? Otherwise, it will add a lot of synchronization complexity to the tracker, which now runs completely on the playback thread. This is highly desirable.

@mofneko
Copy link
Contributor Author

mofneko commented Mar 21, 2017

I understood the difference in thinking with you.

In all three cases it makes sense to make the reloading of the playlist take place later: If there is a pending refresh, it means that we have loaded the playlist recently, and that a reloading will most likely not see anything new.

You think that loadPlaylist from other parts is unnecessary while the tracker is functioning normally. Because there is almost no probability that a difference will occur at the time of reloading.
However, I think that as long as there is a possibility that a difference will occur depending on the playlist distribution environment, I should reload it.

Even if multiple trackers are activated by this correction, they are suppressed and converged in the next routine.

Please clarify why you think it is necessary to reacquire the playlist immediately.

My answer on the question,

RefreshPlaylist is called by the onPlaylistRefreshRequired event, in particular.

That was all it was. As long as you named "RefreshRequired" you should have the most priority other than the tracker.

Your answer to this,

My original aim with the extra condition was to avoid any kind of reloading when a playlist reload is pending.

And I have not obtained a specific answer.
Do you consider Tracker's behavior to be the highest priority in any case?

@AquilesCanta
Copy link
Contributor

Do you consider Tracker's behavior to be the highest priority in any case?

Yes. Please let me know if you run into any new issues with the new version. Thanks again for the PR!

@mofneko
Copy link
Contributor Author

mofneko commented Mar 21, 2017

Thanks(`・ω・´)ゞ

@mofneko mofneko deleted the dev-v2-fix-load-pending-routine branch March 21, 2017 10:00
@google google locked and limited conversation to collaborators Aug 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants