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

fix(DASH): Fix EventStream Elements creation #7194

Merged
merged 3 commits into from
Aug 23, 2024

Conversation

tykus160
Copy link
Member

@tykus160 tykus160 commented Aug 22, 2024

Related to #7148
Does not create parent elements anymore to reduce memory consumption.
Starts to use createElementNS() so created elements will keep proper cases for tag names and they won't be anymore of type HTMLElement, but Element, which should be more lightweight and suitable for our needs.

@avelad
Copy link
Member

avelad commented Aug 22, 2024

@willdharris can you review it? Thanks!

@avelad avelad added type: bug Something isn't working correctly type: performance A performance issue priority: P1 Big impact or workaround impractical; resolve before feature release component: DASH The issue involves the MPEG DASH manifest format labels Aug 22, 2024
@avelad avelad added this to the v4.11 milestone Aug 22, 2024
@shaka-bot
Copy link
Collaborator

shaka-bot commented Aug 22, 2024

Incremental code coverage: 87.50%

@willdharris
Copy link
Contributor

  1. I do see improvement in terms of the number of Detached Elements created and retained.

v4.10.9
unknown-element-4 10 9

fix
element-fix

  1. However, that's not translating to an overall improvement in memory usage. Memory looks about the same between 4.10 and this fix.
Chrome Desktop - DASH LTS - JS Memory

Time | v4.7.15 | v4.10.9 | EventStream Fix
30min | 21MB | 123MB | 122MB
60min | 31MB | 223MB | 224MB
90min | 41MB | 337MB | 341MB

This PR looks beneficial but doesn't resolve the memory issues reported. I will add further detail in #7148.

lib/util/tXml.js Outdated Show resolved Hide resolved
lib/util/tXml.js Show resolved Hide resolved
@avelad avelad requested a review from theodab August 23, 2024 09:54
Copy link
Contributor

@theodab theodab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like this doesn't completely fix the problem, but it at least looks like a good start.

@avelad avelad merged commit bd06fe7 into shaka-project:main Aug 23, 2024
14 of 16 checks passed
@tykus160 tykus160 deleted the wt-txml-element branch August 25, 2024 06:46
avelad pushed a commit that referenced this pull request Aug 26, 2024
Related to #7148 
Does not create parent elements anymore to reduce memory consumption.
Starts to use `createElementNS()` so created elements will keep proper
cases for tag names and they won't be anymore of type `HTMLElement`, but
`Element`, which should be more lightweight and suitable for our needs.
avelad pushed a commit that referenced this pull request Aug 26, 2024
Related to #7148 
Does not create parent elements anymore to reduce memory consumption.
Starts to use `createElementNS()` so created elements will keep proper
cases for tag names and they won't be anymore of type `HTMLElement`, but
`Element`, which should be more lightweight and suitable for our needs.
@zelalami
Copy link

Hello @tykus160
After updating to the latest changes from this PR, I'm encountering an issue related to the createElementNS function. Specifically, I'm getting the following error:

[Failed to execute 'createElementNS' on 'Document': The namespace URI provided ('') is not valid for the qualified name provided ('scte35:Signal').]

This error occurs when parsing the following EventStream section of my DASH manifest:

<?xml version="1.0" ?>
<MPD xmlns="urn:mpeg:dash:schema:mpd:2011" xmlns:scte35="urn:scte:scte35:2014:xml+bin" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" profiles="urn:mpeg:dash:profile:isoff-live:2011,urn:hbbtv:dash:profile:isoff-live:2012" xsi:schemaLocation="urn:mpeg:dash:schema:mpd:2011 DASH-MPD.xsd" publishTime="2024-08-28T09:27:58Z" minBufferTime="PT6S" type="dynamic" availabilityStartTime="1970-01-01T00:00:00Z" timeShiftBufferDepth="PT1M1.88S" minimumUpdatePeriod="PT6S" suggestedPresentationDelay="PT12S">
	<Period id="1" start="P19963DT9H26M52.407S">
		...
	</Period>
	<Period id="2" start="P19963DT9H27M50.807S">
		...
	</Period>
	<Period id="3" start="P19963DT9H27M54.287S">
		<EventStream schemeIdUri="urn:scte:scte35:2014:xml+bin" timescale="90000">
			<Event presentationTime="0" duration="2700000" id="1">
				<scte35:Signal>
					<scte35:Binary>.....</scte35:Binary>
				</scte35:Signal>
			</Event>
		</EventStream>
	</Period>
</MPD>

@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Oct 22, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly type: performance A performance issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants