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 (2) #864

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

dsparano
Copy link
Contributor

@dsparano dsparano commented Dec 5, 2023

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.

@dsparano
Copy link
Contributor Author

dsparano commented Dec 5, 2023

About the added unit test.
I've created sample_h264.ts by converting the existing sample_h265 end enforcing key frames at frame indexes 0, 10 and 20:

ffmpeg -i sample_h265.ts -c:v libx264 -x264-params keyint=10:min-keyint=10 sample_h264.ts

flags = 1 (i.e. key frame) show at the correct sample numbers in this branch and at the wrong sample numbers if commit c3fcac9 is applied (cherry picked) to the main branch.

@tonihei tonihei self-assigned this Dec 11, 2023
@tonihei
Copy link
Collaborator

tonihei commented Dec 11, 2023

Thanks for the fix! I will work on merging this.

@tonihei
Copy link
Collaborator

tonihei commented Dec 11, 2023

I just realized that we don't have this push access to the branch for further changes. Would it be possible to open a new PR from an individual-owned fork? We can't push changes to organization-owned forks like this one. If that's not possible then we can still merge this PR but it will result in an 'evil' merge. See more info here: https://github.com/androidx/media/blob/release/CONTRIBUTING.md#push-access-to-pr-branches

@dsparano
Copy link
Contributor Author

Hi @tonihei, no prob we've had the same issue for previous PRs, I just gave you write access to our repo, so you should be ok, please let me know.

@tonihei tonihei force-pushed the dsparano-exo129_2 branch 2 times, most recently from 6603f5d to 4faed59 Compare December 11, 2023 14:55
@copybara-service copybara-service bot merged commit f465efe into androidx:main Dec 12, 2023
1 check passed
microkatz pushed a commit that referenced this pull request Jan 11, 2024
PiperOrigin-RevId: 590234505
(cherry picked from commit f465efe)
@androidx androidx locked and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants