Custom shouldRetry in retryConfig

[PavelFomin90]

This PR will...

Adds the ability to set a custom function shouldRetry via retryConfig, which overrides default

Why is this Pull Request needed?

Was recommended to add this improvement in #5647 in cases when user need to have possibility to override default shouldRetry

Resolves issues:

#5647

Checklist

• changes have been done against master branch, and PR does not conflict
• new unit / functional tests have been added (whenever applicable)
• API or design changes are documented in API.md

[robwalch]

retryConfig.shouldRetry should take all the arguments of this shouldRetry helper, plus the result of this helper:

export function shouldRetry(
  retryConfig: RetryConfig | null | undefined,
  retryCount: number,
  isTimeout: boolean,
  httpStatus?: number | undefined
): retryConfig is RetryConfig & boolean {
  if (!retryConfig) {
    return false;
  }
  const retry = retryCount < retryConfig.maxNumRetry && (retryForHttpStatus(httpStatus) || !!isTimeout));
  return (
    (retryConfig.shouldRetry 
      ? retryConfig.shouldRetry(retryConfig, retryCount, isTimeout, httpStatus, retry)
      : retry
     );
}

And then the signature for retryConfig.shouldRetry should be modified to expect these arguments (see comment inline).

[paxelpixel]

This and #5647 are interesting because even if shouldRetry is false In the case of 404 it still retries the same partial segment number instead of moving on to the next one. and it does follow the maxNumRetry, it just doesn't follow the retryDelayMs

[PavelFomin90]

This and #5647 are interesting because even if shouldRetry is false In the case of 404 it still retries the same partial segment number instead of moving on to the next one. and it does follow the maxNumRetry, it just doesn't follow the retryDelayMs

In #5647 it’s happening with the last segment in levels in live streams, because levels doesn’t have #EXT-ENDLIST. In VOD it works as expected

[PavelFomin90]

This and #5647 are interesting because even if shouldRetry is false In the case of 404 it still retries the same partial segment number instead of moving on to the next one. and it does follow the maxNumRetry, it just doesn't follow the retryDelayMs

In #5647 it’s happening with the last segment in levels in live streams, because levels doesn’t have #EXT-ENDLIST. In VOD it works as expected

I was wrong, it's also try to refetch last ts-fragment with 404 in level in VOD

[robwalch]

I was wrong, it's also try to refetch last ts-fragment with 404 in level in VOD

With #5647, the stream controller is loop-loading the failed segment after the retry logic has deemed that hls.js should not retry on 4xx errors. With no alternatives, the failed segment should probably be treated as a gap, at least for a short period (the expectation seems to have been that retryDelay would be that period). This change would allow an application to force the retry.

If you expect a different behavior for handling the segment at the live edge when there are no alternative options (single variant/level playback), please comment in #5647.

[robwalch]

I'll let others chime in, but thinking about this some more; we might want to handle this in the loaders so that the LoaderContext and maybe stats are also passed in. That way an app can performa specific actions based on individual request errors and runtime performance.

[robwalch]

@PavelFomin90, This could be handled in a follow up PR if you don't want to take it on. It just needs to be finalized before 1.5.0.

[PavelFomin90]

Yeah, of couse.
I'm not sure, that I have enough time and skill to dive deeper in context, so I'll prefer not to take this responsibility