Abort management update

[nicosang]

Hi,

this PR has to solve the issue #2186 by notifying the scheduleController when a request has been aborted.

Nico

[davemevans]

xhr.abort is called on any active XHRs when XHRLoader.reset is called (for example during player teardown).

This will now trigger an event when the player is reset, which ultimately calls replaceRequest. Is this the intended behaviour during player teardown, and will it definitely not cause any problems?

[nicosang]

Hi @bbcrddave ,

I think you speak about the abort function of xhrLoader, isn't it? In this specific case, we have to do the same as for the other callbacks : I've just added a last commit for this topic.

[epiclabsDASH]

@nicosang, we should also remove the line in AbrController that triggers the event FRAGMENT_LOADING_ABANDONED, isn't it? Otherwise the abort will be signaled two times.

We lose some level of granularity (in the previous implementation event was raised for a specific StreamProcessor and now for a mediaType) although we are not currently supporting activating multiple tracks of the same type so it should work fine (#1972 asks for multiple tracks of same type support but changing how abort works for implementing that will be the smaller of our problems).

[nicosang]

In AbrController, it's quite weird to trigger the FRAGMENT_LOADING_ABANDONED event without even waiting for the request to be actually aborted. It's done like this because the call to fragmentModel.abortRequests(); will execute the xhrLoader.abort();. As explained previously, that means all xhrLoader's callbacks are unregistered. So, we can leave the FRAGMENT_LOADING_ABANDONED event in AbrController, it will not be triggered twice.
I agree with the lost of granularity. In the previous implementation the mediatype was however already set....

Nico

[epiclabsDASH]

But if you are setting onabort handler to null before calling abort method then you are making it useless. I mean, onabort is never going to be called (at least the abort happen for a reason out of our logic, that is good to cover, but not the problem behind the issue we are trying to fix).

In fact, after the latest commit issue appears again. After a few text track switches, subtitles stop working.

[nicosang]

@epiclabsDASH you're right....forget my last commit, I have to found a better idea. ;-)

[nicosang]

So, I have questions about onFragmentLoadProgress function of AbrController. What is the need to get request from FragmentModel while request is a parameter of the function?
If we take a look at the functions registered on FRAGMENT_LOADING_ABANDONED event, they don't use streamProcessor....
With the answer to my first question, we may perhaps delete FRAGMENT_LOADING_ABANDONED from AbrController....

[epiclabsDASH]

The problem I see with that is BolaRule is using qualityIndex (a parameter that was part of the previous FRAGMENT_LOADING_ABANDONED event to calculate the optimal min buffer for the selected quality. It could be an optional parameter of the event that is passed just when abort is coming from AbrController (some kind of reason behind aborting?). We are very close to get this ready, but still need some modifications.

[nicosang]

I agree with you the newQualityIndex is a parameter used by BolaRule class. Nevertheless, the event is FRAGMENT_LOADING_ABANDONED which has to inform that a fragment request has been aborted, not to give informations on the new quality of fragment. As in the onFragmentLoadingAbandoned callback of BolaRule, only mediaType and newQualityIndex are used, we can add a new event triggered by AbrController to send thoses parameters.

[nicosang]

and for me, this event already exists. it's QUALITY_CHANGE_REQUESTED which is trigerred by ABRController....The only thing to do is to register BolaRule on QUALITY_CHANGE_REQUESTED event.
@epiclabsDASH , do you agree?

[epiclabsDASH]

Yes, agree with you, it makes sense.

@spiterikevin, I could be missing something but after @nicosang suggestion and looking at code seems more reasonably that logic within FRAGMENT_LOADING_ABANDONED event handler is moved to a QUALITY_CHANGE_REQUESTED handler. What do you think?

[nicosang]

I have only one question, @epiclabsDASH and @spiterikevin. The job done currently in onFragmentLoadingAbandoned of BolaRule could it be executed each time quality change occurs not only when an aborted segment triggers a quality change.

[epiclabsDASH]

Apparently seems to be something that should be executed after each quality change but I will let @spiterikevin confirm.

[spiterikevin]

In BolaRule, checkNewSegment() already controls the size of the placeholderBuffer in normal conditions. However, when an abort happens the rule should be more conservative to avoid switching back up too soon.

That said, I agree that the newQuality field is out of place in the FRAGMENT_LOADING_ABANDONED event. I'm submitting a PR to get the newQuality from the QUALITY_CHANGE_REQUESTED event.