Fix TS H264 key frame detection (2) #864
Conversation
|
About the added unit test. 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. |
|
Thanks for the fix! I will work on merging this. |
|
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 |
|
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. |
6603f5d
to
4faed59
Compare
4faed59
to
eb2092b
Compare
PiperOrigin-RevId: 590234505 (cherry picked from commit f465efe)
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.