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 TS H264 key frame detection #849

Closed
wants to merge 0 commits into from

Conversation

dsparano
Copy link
Contributor

When playing H264 video from a TS container, it seems the BUFFER_FLAG_KEY_FRAME in addition to being set on the actual IDR frames, is also wrongly set on each frame immediately preceding the IDR one.

In more detail the root cause seems to be the following. In streams with AUD, the consume at the beginning of the PES, on the AUD NAL, calls endNAlUnit() on the previous NAL (NAL_UNIT_TYPE_NON_IDR) and since randomAccessIndicator is set, this causes treatIFrameAsKeyframe to be true and so sampleIsKeyframe to be set while still on the previous sample (outputSample has not been called yet). The following NAL unit (SPS) causes endNAlUnit() to be called on the SampleReader for the AUD NAL which calls outputSample on the previous sample with sampleIsKeyframe wrongly set.

The proposed solution, with this PR, is to pass the randomAccessIndicator parameter in SampleReader in the startNalUnit() instead of the endNalUnit() and have it as a class variable in the SampleReader. With this change the randomAccessIndicator would only depend on the PES the current NAL belongs to and the issue described above would be resolved.

@microkatz microkatz self-requested a review December 1, 2023 13:41
@microkatz
Copy link
Contributor

@dsparano. Thanks for reporting this issue and proposing a fix! Do you have a small ts sample that you can add in a unit test like those in the TsExtractorTests that exposes this issue and provides testing support for your change?

@FongMi
Copy link

FongMi commented Dec 2, 2023

This pr has bug
The url can't play
http://altitcon2.3cx.hk:9991/stream/channel/b7992c06aba8ebfc87e21cda43d287d1
MediaItem need set MimeType to MimeTypes.APPLICATION_M3U8

@microkatz microkatz self-assigned this Dec 4, 2023
@dsparano dsparano closed this Dec 5, 2023
@dsparano
Copy link
Contributor Author

dsparano commented Dec 5, 2023

@microkatz I just committed the added unit test, and not sure how this PR ended up closed (!) could you re-open it?

@FongMi the url above doesn't seem to point to anything useful, if you can fix the url I'm happy to debug

@microkatz
Copy link
Contributor

@dsparano. Unfortunately I cannot reopen the pull-request. The button is not available to click. Perhaps create a new pull request?

@dsparano
Copy link
Contributor Author

dsparano commented Dec 5, 2023

Looks like github doesn't like a force push, even a non history rewriting one :( which is what sourcetree did for me, anyway opening another one...

@dsparano
Copy link
Contributor Author

dsparano commented Dec 5, 2023

@FongMi http://altitcon2.3cx.hk:9991/stream/channel/b7992c06aba8ebfc87e21cda43d287d1 seems to be playing fine for me on this branch

@FongMi
Copy link

FongMi commented Dec 6, 2023

@dsparano Sorry, I will test again with pull/864

@androidx androidx locked and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants