Consider minimum update period to refresh manifest (Simpler alternative) #347
Conversation
|
This looks pretty nice 👍 . I'm thinking about if we can write code even more concize by changing the place where we catch "needs-manifest-refresh" events, to perform all manifest updates in the same place. |
|
Oh yeah, also run |
grenault73
force-pushed
the
feat/minimum-update-period2
branch
from
ae943e4
to
35a1a0c
Compare
peaBerberian
added
enhancement
src/core/pipelines/manifest/index.ts
Outdated
| return parser({ response: value, url }).pipe( | ||
| map(({ manifest: parsedManifest }) => { | ||
| const manifest = new Manifest(parsedManifest, warning$, transport.options); | ||
| const manifestFetchingDuration = sentTime ? |
There was a problem hiding this comment.
I've two main problems with the fact that we communicate a manifestFetchingDuration:
- it is not a fetching duration but a fetch + parse one
- the time between this operation and the usage of it could be extended in future evolutions without us expecting any problem with that
If I understood it correctly this operation should be the closest possible to performance.now at the time the corresponding timeout is started.
Why not just communicating the requestTime. It might even make more sense as a manifestUpdate event, as it can be used to sort chronologically such updates.
src/core/stream/index.ts
Outdated
| const loadStream = StreamLoader({ // Behold! | ||
| mediaElement, | ||
| manifest, | ||
| manifestFetchingDuration, |
There was a problem hiding this comment.
Why not regrouping these two under a manifestInfos name or something?
(With both the manifest and the requestTime)
src/core/stream/refresh_manifest.ts
Outdated
| }), | ||
| share() // share the previous side effect | ||
| ); | ||
| } |
src/core/stream/stream_loader.ts
Outdated
| // the update period has elapsed. | ||
| const updateManifest$ = refreshMinimumUpdatePeriod$.pipe( | ||
| mergeMap((lastUpdatePeriod) => { | ||
| return observableTimer(lastUpdatePeriod * 1000).pipe( |
There was a problem hiding this comment.
To think of it the performance.now() - requestTime difference might even be calculated here... To see if that makes sense and if it is readable
src/core/stream/stream_loader.ts
Outdated
| } | ||
| return EMPTY; | ||
| case "needs-manifest-refresh": | ||
| log.debug("needs manifest to be refreshed"); |
There was a problem hiding this comment.
I prepend now a Stream: namespace in the StreamLoader's logs, to better help us debugging with logs on
src/core/stream/stream_loader.ts
Outdated
| case "needs-manifest-refresh": | ||
| log.debug("needs manifest to be refreshed"); | ||
| // out-of-index messages require a complete reloading of the | ||
| // manifest to refresh the current index |
There was a problem hiding this comment.
this comment makes no sense here (might me mine, but I just realized that)
src/core/stream/stream_loader.ts
Outdated
| tap((manifestUpdateEvt: IManifestUpdateEvent) => { | ||
| const { minimumUpdatePeriod } = manifestUpdateEvt.value.manifest; | ||
| if (minimumUpdatePeriod && minimumUpdatePeriod > 0) { | ||
| refreshMinimumUpdatePeriod$.next(minimumUpdatePeriod); |
There was a problem hiding this comment.
Wait, didn't you forget to perform the substraction here?
src/core/stream/stream_loader.ts
Outdated
| // Creates an observable which will refresh the manifest after | ||
| // the update period has elapsed. | ||
| const updateManifest$ = refreshMinimumUpdatePeriod$.pipe( | ||
| mergeMap((lastUpdatePeriod) => { |
There was a problem hiding this comment.
Not a fan of this variable's name. It's actually the maximum refresh timeout of the manifest right here.
There was a problem hiding this comment.
Also, shouldn't it be a switchMap?
peaBerberian
force-pushed
the
feat/minimum-update-period2
branch
from
cc4860a
to
8b0853e
Compare
Add a minimumUpdatePeriod field to Manifest, and use it in the StreamLoader to refresh the manifest at the given interval.
… manifest auto update
peaBerberian
force-pushed
the
feat/minimum-update-period2
branch
from
0f9152c
to
58508fe
Compare
No description provided.