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.

Sign up for GitHub

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

Jump to bottom

fix: ignore buffered packet data with no pes header #378

Merged
merged 3 commits into from
Mar 5, 2021
Merged

fix: ignore buffered packet data with no pes header #378

merged 3 commits into from
Mar 5, 2021

Conversation

realeyes-ben
Copy link
Contributor

@realeyes-ben realeyes-ben commented Feb 24, 2021 •
edited
Loading

Description
This is a fix for ts files that carry PES packets of framed data from the previous ts fragment. This addresses the ERROR: (CODE:3 MEDIA_ERR_DECODE) for the cause

The media playback was aborted due to a corruption…media used features your browser did not support

or

[video|audio] append of [x]b failed for segment #[n]

Content
The stream I was able to reproduce the bug with is https://e3e2p5m2.ssl.hwcdn.net/hls/main.m3u8. I was able to record when the bug occurred and uploaded the recorded content to https://drive.google.com/drive/folders/174ef0f6DOdMaXI6f3CLYSw_9H5emxzM0?usp=sharing. There is a generated VOD manifest for the recorded content.

Steps to reproduce
I was able to produce this bug using Video.js (v7.11.4).
If you try to playback the content, you can trigger the bug when the player changes bitrates from segment segment51788.ts @5-1280x720 to segment segment51788.ts @0-1920x1080. Another way to quickly see this is by commenting out all the playlists besides 0-1920x1080 and comment out segments segment51786.ts and segment51787.ts. This is will generate an incorrect init segment. The corrupted generated init segment has an sps that is incorrect. The width and height are 96 X 96 when the content is really 1920 X 1080. The avcC box has incorrect properties that looks like this

{
  "AVCProfileIndication": 252,
  "profile_compatibility": 233,
  "AVCLevelIndication": 163
}

when the codec profile and level values should be

{
  "AVCProfileIndication": 77,
  "profile_compatibility": 64,
  "AVCLevelIndication": 40
}

Issue
This is happening because in fragment segment51788.ts @0-1920x1080, its first 3 ts packets are video pes packets that are continue frame data from the previous fragment. So those 3 PES packets have been buffered and when the next payload_unit_start_indicator comes by, it flushes those packets and assumes there is a PES header and when it's pushed to the NalByteStream it incorrectly sees an sps and creates the incorrect config from there.


| pes[0] (v) | pes[1] (v) | pes[2] (v) | ... | pes[9] (payload_start_indicator) (v)| pes[10] (v) | ...

Solution
The solution I came up with is when the buffered PES packets are flushed to the be parsed, it will check for the first three bytes of the data payload for the start prefix (0x000001) and it will ignore the payload the return an empty Uint8Array.

gkatsev
gkatsev reviewed Feb 24, 2021
View reviewed changes
lib/m2ts/m2ts.js Outdated Show resolved Hide resolved
@gkatsev
Copy link
Member

gkatsev commented Feb 25, 2021

I can confirm that this fixes the issue. Though, having a bit of trouble wrapping my head around the issue.
In the new segment we're trying to play, the first 4 packets are loaded, when we get that 4th packet that has the payload unit start indicator flag set we flush the other 3 packets and we try to play append them things fail.
These three packets aren't PES packets but we're trying to parse them as such?

gkatsev
gkatsev reviewed Feb 25, 2021
View reviewed changes
lib/m2ts/m2ts.js Outdated Show resolved Hide resolved
@realeyes-ben
Copy link
Contributor Author

realeyes-ben commented Feb 25, 2021 •
edited
Loading

The first three packets are PES packets, they are just continued frame data from the last frame in segment51787.ts. That's why in the 9th ts packet of segment51788.ts, it starts a new frame that contains the pps and sps.

@gkatsev
Copy link
Member

gkatsev commented Feb 25, 2021

Thanks, that makes sense. Wouldn't we want to keep those if we are progressing properly through the segments? Or would we pick those up because we have the data from the previous segment?

@realeyes-ben
Copy link
Contributor Author

realeyes-ben commented Feb 25, 2021 •
edited
Loading

I'm not sure honestly. This was the dilemma I was trying to figure out. Let's say we are at segment segment51787.t @ 0-1920x1080 and continue to segment segment51788.t @ 0-1920x1080. This should be no issue and keeping those pes packets should be fine. The buffer should just append those to the currently buffered media data in the source buffer. The situation I'm not sure about is if it's buffered the segment segment51787.ts @ 5-1280x720 and then changes the bitrate to segment51788.ts @ 0-1920x1080, including those three initial video pes packets are continuous frame data from segment51787.ts @ 0-1920x1080 instead of segment51787.ts @ 5-1280x720. I'm not sure how the decoders handles this. It could work fine. It also looks like it works fine leaving them out. I'm not sure which solution is best.

@gkatsev
Copy link
Member

gkatsev commented Feb 25, 2021

Thanks, I'll try it out today. I also asked @gesinger to take a look because he knows a lot more about the internals of things than I do.

brandonocasey
brandonocasey approved these changes Mar 4, 2021
View reviewed changes
Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

Going to approve this as pes is required to start with 0x00, 0x00, 0x01. The reason this happens is that we don't account for SDT packets in our code so an SDT packet makes it all the way to parsePes, where it would obviously mess everything up. Long term we should fix the root cause.

@realeyes-ben
Copy link
Contributor Author

I agree. I think the correct solution would be including those extra PES packets of video data instead of throwing it away but that would be more complicated.

@gkatsev gkatsev merged commit bb984db into videojs:main Mar 5, 2021
@dependabot dependabot bot mentioned this pull request Mar 8, 2021
Closed
@gkatsev gkatsev mentioned this pull request Apr 2, 2021
Closed
@gkatsev gkatsev mentioned this pull request Apr 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkatsev gkatsev gkatsev left review comments

@brandonocasey brandonocasey brandonocasey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@realeyes-ben @gkatsev @brandonocasey