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

[PROPOSAL] DASH: Remove "native" JS parser #1482

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Jul 11, 2024

From the v4.1.0, we had 3 MPD parsers:

  • The "native" one, relying on the web's DOMParser API.

    This is historically the first one we used and still the one we rely by default on main thread.

  • The "WebAssembly" one, added in 2021.

    This one was initially written when we had to manage multi-megabytes MPD. Those took forever to parse on low-end devices but worse even on a PC, parsing it often (e.g. for a live content) could lead to a crash after several hours due to GC pressure.

    The idea was to parse the MPD in another thread (the initial MULTI_THREAD attempts date back from here :p) and to rely on WebAssembly to better control memory usage and performance (also the "native" one wasn't usable anyway on a WebWorker due to browser limitations).

    It turned out that the WebAssembly version was so light (note: we also rely on XML StAX parsing instead of DOM parsing which may have helped for that part) and fast that we didn't yet need the complexity of bringing another thread here.

  • The "fast js" one, added at the last v4.1.0 release.

    This one follows attempts to make the MULTI_THREAD feature usable on non-WebAssembly devices. We noticed that other developers had recently made attempts for fast JS parsing without even needing the use of the DOMParser. They reported even faster performance due to much fewer XML integrity checks (which is OK for us, as MPD parsing performance is one of the most important aspect for us).

This PR proposes that we remove the "native" one to just replace it by the "fast js" one.

The "fast js" one has already been used in production for more than 6 months without issue, is equal-to-faster than the native one and it would lead to much less code.

@peaBerberian peaBerberian added DASH Relative to the DASH streaming protocol proposal This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it Priority: 3 (Low) This issue or PR has a low priority. labels Jul 11, 2024
@peaBerberian peaBerberian force-pushed the misc/remove-dash-native branch 3 times, most recently from f5cf0c6 to 4413c98 Compare July 11, 2024 17:00
@peaBerberian peaBerberian force-pushed the misc/remove-dash-native branch from 4413c98 to bb4a256 Compare August 5, 2024 10:15
@peaBerberian peaBerberian force-pushed the misc/remove-dash-native branch 3 times, most recently from 44c2921 to 13bf0e5 Compare September 4, 2024 17:47
@peaBerberian peaBerberian force-pushed the misc/remove-dash-native branch from 13bf0e5 to 046a368 Compare October 8, 2024 15:01
From the v4.1.0, we had 3 MPD parsers:

  - The "native" one, relying on the web's DOMParser API.

    This is historically the first one we used and still the one we rely
    by default on main thread.

  - The "WebAssembly" one, added in 2021.

    This one was initially written when we had to manage multi-megabytes
    MPD. Those took forever to parse on low-end devices but worse even
    on a PC, parsing it often (e.g. for a live content) could lead to a
    crash after several hours due to GC pressure.

    The idea was to parse the MPD in another thread (the initial
    MULTI_THREAD attempts date back from here :p) and to rely on
    WebAssembly to better control memory usage and performance (also the
    "native" one wasn't usable anyway on a WebWorker due to browser
    limitations).

    It turned out that the WebAssembly version was so light (note: we
    also rely on XML StAX parsing instead of DOM parsing which may have
    helped for that part) and fast that we didn't yet need the
    complexity of bringing another thread here.

  - The "fast js" one, added at the last `v4.1.0` release.

    This one follows attempts to make the `MULTI_THREAD` feature usable
    on non-WebAssembly devices. We noticed that other developers had
    recently made attempts for fast JS parsing without even needing the
    use of the DOMParser. They reported even faster performance due to
    much fewer XML integrity checks (which is OK for us, as MPD parsing
    performance is one of the most important aspect for us).

This commit proposes that we remove the "native" one to just replace it
by the "fast js" one.

The "fast js" one has already been used in production for more than 6
months without issue, is equal-to-faster than the native one and it
would lead to much less code.
@peaBerberian peaBerberian force-pushed the misc/remove-dash-native branch from 046a368 to b39f2dd Compare November 15, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DASH Relative to the DASH streaming protocol Priority: 3 (Low) This issue or PR has a low priority. proposal This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant