-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 PPS array growth in HEVC TS parser #6724
base: master
Are you sure you want to change the base?
Conversation
@@ -180,7 +181,7 @@ class HevcVideoParser extends BaseVideoParser { | |||
track.params[prop] = config[prop]; | |||
} | |||
} | |||
if (this.initVPS !== null || track.pps.length === 0) { | |||
if (track.vps !== undefined && track.vps[0] === this.initVPS) { | |||
track.pps.push(unit.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not fix the issue as track.pps
is never reset. If we play through "HLS M2TS by Bitmoving (HEVC MAIN..." by the end there are almost 100 Uint8Arrays in track.pps
, almost all of them identical, and when we seek back or loop playback the track.pps
array continues to grow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robwalch Are you sure that hls.js was built from 04d2df4 and browser cache had been cleared before test?
I added the following console.log
:
if (track.vps !== undefined && track.vps[0] === this.initVPS) {
console.log('track.pps.push(unit.data)');
track.pps.push(unit.data);
}
During/after playback there's only one logged push into pps array. If nothing is logged it means cache is used (simple check). If you do the same will track.pps.push(unit.data)
occur many times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. If every segment has the same vps and pps, then that case is true for every segment and a pps is pushed as each segment is parsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If every segment has the same vps...
It's not the same in terms of objects. track.vps[0]
is always pointed to the last VPS. So the condition track.vps[0] === this.initVPS
is fulfilled only for the first VPS. On next segment track.vps[0]
will contain VPS for that segment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative way: keep only last SPS and PPS as done for AVC. Video encoded with more than one PPS or SPS will not be supported too, but it seems to be very rare case. If someone opens an issue regarding such video, we can get back to the original solution. TS-files referenced in #6683 will be played, as well as another test streams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the leak.
There's another issue i notice but it is unrelated to these changes:
When seeking forward, all media is appended at 0 (or with these changes, at the position of the last appended segment.)
If the issue is related to the sample media, please replace it with one that has contiguous timestamps.
Why is this Pull Request needed?
This is a fix of track's PPS array growth and related memory leak in HEVC TS parser.
Resolves issues:
#6683
Checklist