Release/v4.3.0 #1689
Merged
Release/v4.3.0 #1689
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some private methods of the `MediaSourceContentInitializer` were difficult to follow due to the heavy usage of inner functions with unclear names (such as `recursivelyLoadOnMediaSource`). I now tried to isolate some of those and rename the different methods called at content load so it makes more sense. Methods in order are: ``` start > _setupInitialMediaSourceAndDecryption > _onInitialMediaSourceReady > _setupContentWithNewMediaSource > _startLoadingContentOnMediaSource ``` Note: `_setupContentWithNewMediaSource` and `_startLoadingContentOnMediaSource` are still similarly named, though the former just has the task of setting up a reloading logic and then calling the latter. Note2: `_startLoadingContentOnMediaSource` is still a huge and complex method that we might improve on in the future.
…ctions The callback called on the `"updateend"` and `"error"` events on a SourceBuffer were previously declared as arrow functions inside the constructor. This allowed to keep a clear JS context for the `this` keyword, but it made the `MainSourceBufferInterface`'s constructor harder to read. I found it more readable to declare both callback as private methods instead, with the drawback of having to bind the `this` explictely.
Make MainSourceBufferInterface code more readable isolating inner functions
This has been a request: to clearly indicate when the current Representation has linked HDR information in our debug element. I don't think it causes issue and it makes sense, so I wrote the proposal commit here.
[Proposal] DEBUG_ELEMENT: Add `hdr` information to video Representation
We changed the name of classes from *SegmentBuffer to *SegmentSink but didn't actually update the file names. This is now done.
…ndDecryption Rename and factorize some methods of the MediaSourceContentInitializer
…t_sink Rename segment_buffer file into the new segment_sink name
While continuing proof-of-concepts related to how much buffer we build, I found that our estimate of the current buffer size in bytes (more exactly the combined size of pushed segments that still have at least a sub-part present in the buffer) was a potentially-interesting metric that we did not expose anywhere: neither in logs nor in our DEBUG_ELEMENT feature. This commit adds it to the `DEBUG_ELEMENT` feature, just after the corresponding buffer content graph. It may be even more useful in logs, but I did not bother to do it for now as I currently mainly relied on the debug element.
[Proposal] Add buffer size estimate to debug buffer content graph
We recently defined a `onmessageerror` global callback on the WebWorker-side of the `MULTI_THREAD` feature, thinking that it was defined, like the `onmessage` callback, as long as WebWorker were accessible. Turns out that was not the case at all, and the multithreading mode was breaking on older device. Oops, sorry about that. As it is not required, we can just access it through our `globalScope` shortcut to the global object. This way, it will just define a callback which will never be called if it does not exist.
MULTI_THREAD: Fix onmessageerror being undefined
We have in the RxPlayer a really reactive discontinuity-detection mechanism which seeks over seen holes in the buffer that won't be filled. A few subset of the smaller of those holes could theoretically be already be seeked over by the browser/device logic, but to have a faster generalization, we generally seek directly and do not wait to observe what the browser or device will do. But with Tizen (samsung TVs) this may cause sometimes problems as seeking is approximate on those devices. We could thus seek over a discontinuity, yet have tizen seeking before it. We already have a complex code detecting this exact scenario to avoid an infinite loop of discontinuity seeks when that happens, and it works. Yet we have again seen scenarios when what we called "the seek-back issue" came to bite us: When the discontinuity is encountered at the exact beginning if the content, and we're unlucky enough for that seeking resulting in a 'freeze' (the playback does not advance despite playback conditions being OK), we encountered an infinite buffering on those Tizen devices. To fix this, @Florent-Bouisset already proposed #1586 which allows to still un-freeze that post-initial-disconinuity-seek freeze by having less conditions to do so, which works, yet which needs to wait for our heuristic to detect a freeze, a rather long multiple-seconds process for now. As a supplementary logic, I propose here to just let tizen encounter discontinuity for 1 second, before trying to do something. This means we risk to have a 1-2 seconds rebuffering time at real discontinuities, but it prevent all those seek-back issues when the discontinuity is automatically seeked over by the device.
Tizen: don't directly seek over discontinuity on tizen
fix(freeze-detector): perform a seek regardless of the wanted position
…anted key system When loading a content with the RxPlayer, you can provide multiple DRM configurations (e.g. Widevine L1, PlayReady SL3000, Widevine L3 etc.) in a certain order of preference. The idea is that the RxPlayer will attempt one after another and finish with the first compatible one. To speed up later video loading operations, the RxPlayer always re-check at each video load operations if the last created MediaKeySystemAccess is compatible with one of the configuration, and if it is, use that one. However turns out that this is not always what application wants. We've seen for example a device with both Widevine and Playready where the application wanted Widevine in priority for some contents and PlayReady in priority for other contents, yet always asking for both (just in a different order of preferences for those various contents). Due to our MediaKeySystemAccess-reusage strategy, even if PlayReady is prioritized for a latter content and the device is compatible with it, we could be still using Widevine because it was already the DRM system used for the last content and because Widevine was still OK for the application, just less prioritized. Instead of this, this modification only reuse the "cached" `MediaKeySystemAccess` if it corresponds to the current most wanted configuration (i.e. it is re-checked with each iterations on the `keySystems` option before the actual `navigator.requestMediaKeySystemAccess` call).
…en-its-turn DRM: Only check cached MediaKeySystemAccess compatibility with most w…
We've seen minor memory leaks in the project due to forgetting to clean-up what's basically event listeners (mainly JS-only ones, not DOM ones). To put a supplementary level of security against memory leaks, I propose here to make the `SharedReference` and any `IPlaybackObserver`'s `clearSignal` parameter as mandatory. Note that this is not always needed to clean up a listener, there's for example the `stopListening` argument provided in the callback, "finishing" the reference etc., so a reference without a CancellationSignal could still not be leaking, but ensuring there's a CancellationSignal also attached looks like a minor sacrifice to limit memory leaks to me.
Signed-off-by: Emmanuel Ferdman <[email protected]>
Signed-off-by: Emmanuel Ferdman <[email protected]>
Update HDR reference
[Proposal] Make SharedReference's clearSignal param mandatory
In our demo, we have specific scripts to start that demo with our WebAssembly MPD parser, the `DASH_WASM` feature. What those do is to set a `__INCLUDE_WASM_PARSER__` boolean to `true` in global scope, which signals to the demo page that it should include the `DASH_WASM` feature and load the corresponding WebAssembly file. However I noticed that when enabling "multithreading" mode in our demo page, it tried to download the WebAssembly MPD parser regardless of the `__INCLUDE_WASM_PARSER__` boolean. This made sense pre-`v4.1.0` when we only relied on the WebAssembly parser to parse a DASH MPD in "multithreading" mode. But now that we also have a JS parser embedded inside our worker code, it seems unnecessary. It also led to some http 404 when testing the demo locally without having built the WebAssembly file. Those don't matter much though because the RxPlayer automatically fallbacks to the JS parser. Also, the demo built on our GitHub pages accessible on https://developers.canal-plus.com/rx-player/ do have the WebAssembly MPD parser file built. This means that when enabling the "multithreading" mode on that demo, we're actually loading and using that WebAssembly file to parse MPDs. Doing the change I'm proposing here of only using the WebAssembly parser if `__INCLUDE_WASM_PARSER__` is set to `true` will thus lead to a small change in the globally-accessible demo page, where we will be using the default JS parser instead in "multithreading" mode. We could also in the future add a checkbox in our demo specifically for the inclusion or not of the WebAssembly parser.
…ntent switching We've recently noticed on some LG TVs only that we triggered a `MEDIA_KEYS_ATTACHMENT_ERROR` if loading different contents very fast (e.g. "zapping" like crazy on the remote ~15-20 times on those). Turns out that the issue was due to a complex combination of optimizations and work-arounds, where we would "reset" the `MediaKeys` instance attached to the `HTMLMediaElement` (that's the work-around part of some LG issues) by calling the `setMediaKeys` API with a `null` `MediaKeys` (as documented in the EME recommendation), yet without awaiting that the original `setMediaKeys` call with the actual `MediaKeys` instance for that same content to be finished. Doing multiple `setMediaKeys` call without waiting for the previous one to finish leading to an error is an actually documented behavior in the EME recommendation so this could be categorized as a bug on our side. To fix this, I tried an easy way first but found out that there could be multiple other issues with `MediaKeys` attachment: if for whatever the still-pending `setMediaKeys` call for a previous content ends up failing when another content, which needs to rely on the same `MediaKeys` instance, is playing, no `MEDIA_KEYS_ATTACHMENT_ERROR` will be triggered and the RxPlayer could instead end up rebuffering indefinitely. So I decided to do a bigger code update: Now the method returning information on the `MediaKeys` attached to the `HTMLMediaElement` is always asynchronous (it returns a Promise) just in case the attachment task is still pending (and to also handle cases where it would fail). I still let a synchronous call for the `getKeySystemConfiguration` API, where it may thus also for now return the "expected" `MediaKeys` information (e.g. it will return the future expected state when attachment is still pending), because I wasn't sure of the behavior we want here - though I still am.
Now that eslint seems OK to be updated to v9 (#1660) I attempt another PR for vitest to its v3... But it seems there's an issue: though vitest list-files do see the corresponding files and running it in a JSDom environment run those tests, running it in a browser env always seem to lead to an exit code 1 before any test had a chance to run. So it may be a configuration issue linked to webdriverio <-> vitest interactions? What's sad is that I did not find any way to make vitest more verbose about what the error is, it just silently directly return `1` without anything being outputed to stdout nor sterr.
…on tests in separate steps in the CI for clarity
Misc/vitest3 fix
At last updating to `eslint` 9 does not lead to an infinite thread of issues and arcane errors, we just had to wait some months :p Though I chose to sacrifice some things, most notably the `no-undef` rule (which checks if variables are defined) for the tests and scripts directories only, because to be able to rely on Node.js globals it seems that we had to add another dependency "`globals`" and to add again more configurations in our already needlesly bloated and hard to maintain (and understand) `eslint` conf. `eslint` also decided to split into much more separate packages for some reasons. Now more than a third of our dependencies (10/29) are dedicated to `eslint` and to `eslint` only - and this after making efforts to remove some other I judged not worth the headaches.
Dependency updates: notably eslint 9
Issue: performance tests results produce too much noise on PR
-------------------------------------------------------------
We recently decided to perform performance tests (which check for
performance regressions) on all Pull Requests each time a push occurs on
it.
Those performance tests post a message detailling the results when
complete. The idea is to better see the impact for each tested scenario:
a monothreaded content loading, a multithreaded one, a seek etc. -
including when tests pass.
However, we generally have very long review time in the RxPlayer team :D.
It's not unheard of to stay in review phases for weeks, months or even
sometimes years (!), time during which we continue developing/maintaining
those branches and pushing, re-triggering the tests!
The issue is not running the tests multiple times, nor it is really the
review times, the problem we now have is that after a while, we end up
with a huge part of a Pull Request summary dedicated to performance tests
results.
Current solution: HTML `<details>` element
------------------------------------------
This commit tries to reduce this issue by relying on the `<details>`
HTML element, which results in a collapsed message.
The idea is that results are still posted each time, but the table
summarizing results is now collapsed inside the comment.
To profit from this, I decided that our performance tests now produce
HTML reports instead of markdown ones. Technically, markdown specs like
CommonMark do support HTML inside, but I that point I though it would be
more compatible to just produce HTML anyway.
The inconvenients are:
- HTML is less readable than markdown. To alleviate this, I tried to
make HTML as readable as possible by "playing" with line breaks and
spaces.
- We now have functions both producing mardown tables (for stdout) and
HTML tables (for the file report).
This doesn't bother me too much though.
[Proposal] Make Performance test report HTML and to be collapsed by default
I was working on a refacto proposal for better debugging capabilities (incoming!) and while working on it, that work actually detected a leak: a `RepresentationStream`'s own `TaskCanceller` is still listened to even if we decide to switch the Representation since. It was only cleared once either the content was stopped or some other situations (a track change, AdaptationStream aborted due to a Period change, a reload etc.). While debugging a content, I saw that the amount of `TaskCanceller` aborted when stopping the content was actually equal to the number of quality switches through ABR - instead of the one per Period-type combination that I would have expected. This should be relatively minor, I don't even think that it is actually visible to end customers even long term (multiple hours, a day etc.), but it is still a leak as we don't really need to keep that `TaskCanceller` alive once a `RepresentationStream` finished its business. --- The fix was not straightforward though, as there was a reason we did not want to cancel the `TaskCanceller` as soon as the Representation changed: if at that time a segment was loaded but not yet pushed, we have no problem (and even prefer) with the fact that the `RepresentationStream` can continue to push that segment to the buffers. I ended up declaring a single `TaskCanceller` for a `RepresentationStream`, yet by being careful as to not let it impact segment pushing.
Fix minor memory leak when switching RepresentationStream through ABR
Fix: microsoft edge some codecs are not supported by MSE in worker
…ROR` We had kind of a dumb mistake where an error arising at license-communication time was falsely categorized as an error at license-fetching time. This is because we inadvertently also catched errors from the `MediaKeySession.prototype.update` EME call where we only intended to catch errors at the higher-up `getLicense` (the RxPlayer API) level. The fix to that issue is relatively straigtforward.
DRM: Fix `KEY_UPDATE_ERROR` by not translating it into a `KEY_LOAD_ERROR`
Fixes #1668 Basically, if an application stopped playback while buffer-removal due to `maxBufferAhead` / `maxBufferBehind` options were set, an error log would be outputed: ``` Could not run BufferGarbageCollector: This task was cancelled. ``` This log in this case is unnecessary (the behavior is normal and wanted) and its verbosity could make people think there was an issue.
Avoid error log when stopping a stream with a pending GC buffer removal
Collaborator
Author
|
Github actions' internal cache seemingly broken at the best of times |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Features
ManagedMediaSourceon iOS devices [feature: add the possibility to rely on managedMediaSource on iOS devices #1562]renderThumbnailandgetAvailableThumbnailTracksAPI [[Proposal] Implement DASH Thumbnail tracks #1496]keySystems[].wantedSessionTypesloadVideooption to also initialize a DRMconfig for future contents, not just the current one [[Proposal] DRM: Add
keySystems[].wantedSessionTypesloadVideooption #1604]experimentalOptions.enableRepresentationAvoidanceoption toloadVideoto enableour new Representation avoidance mechanism [[Proposal] Improve
FREEZINGmechanisms and addRepresentationavoidance mechanism #1523]Bug fixes
onmessageerrorbeing undefined on older devices [MULTI_THREAD: Fix onmessageerror being undefined #1585]codec is not supported specifically in a Worker context (previous behavior led to some
fatal errors on Edge with HEVC support) [Fix: microsoft edge some codecs are not supported by MSE in worker #1664]
to fix a remaining Tizen (Samsung) infinite rebuffering issue [fix(freeze-detector): perform a seek regardless of the wanted position #1586]
MediaKeysreusage on some devices including desktopbrowsers [Fix typo which prevented MediaKeys reusage and fix corresponding eslint rule #1615]
"persistent-license"MediaKeySession(and not also for"temporary"license) when only akeySystems[].persistentLicenseConfigiscommunicated [[Proposal] DRM: Add
keySystems[].wantedSessionTypesloadVideooption #1604]keySystems[]option changing when reusing aMediaKeySystemAccesswith a differentkeySystems[]configuration [Fix keySystems option changing when reusing MediaKeySystemAccess #1616]KEY_UPDATE_ERRORwhich was mistakenly inheriting the codeKEY_LOAD_ERROR[DRM: Fix
KEY_UPDATE_ERRORby not translating it into aKEY_LOAD_ERROR#1670]video segments have a gap at the expected initial position [Fix yet another issue with those Tizen seek-backs #1637]
newAvailablePeriodsevent [TracksStore keeps the last reference in case of several being communicatedr #1643]Other improvements
before seeking ourselves over a discontinuity [Tizen: don't directly seek over discontinuity on tizen #1587]
MediaKeySystemAccessif none is more wanted for the currentcontent [DRM: Only check cached MediaKeySystemAccess compatibility with most w… #1591]
LOADINGandRELOADINGattempts may have taken more time thannecessary due to a wrong "initial Period prediction", this is fixed [MultiThread: Fix initial period prediction in multithread mode #1628]
FREEZINGwork-arounds by reloading if our initial strategies do not give aresult [[Proposal] Improve
FREEZINGmechanisms and addRepresentationavoidance mechanism #1523]hdrinformation to video Representation [[Proposal] DEBUG_ELEMENT: Addhdrinformation to video Representation #1583]fullon RxPlayer's debug mode [[Proposal] set LogFormat tofullon RxPlayer's debug mode #1625]BufferGarbageCollectorbufferremoval [Avoid error log when stopping a stream with a pending GC buffer removal #1684]
protect against performance regressions [Run performance tests on all PR against base branch #1630]