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 crashes due to missing TrueHD overrun checks #16813

Merged
merged 1 commit into from Dec 21, 2019

Conversation

anssih
Copy link
Member

@anssih anssih commented Oct 20, 2019

CAEBitstreamPacker and CDVDAudioCodecPassthrough copy TrueHD frames into
buffers without checking for overruns.

Fix that.

This should fix crashes of issue #16704.

However, audio dropouts still remain. To fix those, the TrueHD packet
offsets will need to be dynamically scaled based on the input_timing
field (TrueHD bytes 2-3) which allows determining the amount of time
required between the previous frame and the current frame.

CAEBitstreamPacker and CDVDAudioCodecPassthrough copy TrueHD frames into
buffers without checking for overruns.

Fix that.

This should fix crashes of issue xbmc#16704.

However, audio dropouts still remain. To fix those, the TrueHD packet
offsets will need to be dynamically scaled based on the input_timing
field (TrueHD bytes 2-3) which allows determining the amount of time
required between the previous frame and the current frame.
@samnazarko
Copy link
Contributor

CC @graham8

if (m_trueHDPos == 0)
maxSize -= sizeof(mat_start_code) + BURST_HEADER_SIZE;
else if (m_trueHDPos == 11)
maxSize -= -MAT_MIDDLE_CODE_OFFSET;
Copy link

Choose a reason for hiding this comment

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

I don't know the code that well, nor do i know the truehd bitstream protocol. But you are adding to the max size here instead of subtracting. Is that on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

MAT_MIDDLE_CODE_OFFSET is -4, so maxSize ends up being reduced.

I agree it is a bit confusing, though.

@samnazarko
Copy link
Contributor

samnazarko commented Nov 1, 2019

I've pushed this to OSMC tonight to get some feedback, despite it still being in PR stage. Early testing before pushing it to general release was positive.

As we discussed in the audioimprove chat, there's refinement needed to fix the dropouts, but crashes are resolved.

This was discussed in OSMC chat. If anyone wishes to join the chat and wants an invite, let me know. We are keen to get it solved.

@anssih
Copy link
Member Author

anssih commented Dec 21, 2019

jenkins build this please

@anssih
Copy link
Member Author

anssih commented Dec 21, 2019

Android-ARM failure seems unrelated ("Gradle build daemon disappeared unexpectedly"), WIN-UWP-64 failure seems unrelated (python37.lib issue).

@anssih anssih merged commit d29c84c into xbmc:master Dec 21, 2019
@Rechi
Copy link
Member

Rechi commented Dec 23, 2019

@anssih please always set the milestone before or right after merging a PR.

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jan 21, 2020
Fix crashes due to missing TrueHD overrun checks
@DaveTBlake DaveTBlake added this to the Matrix 19.0-alpha 1 milestone Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Audio Type: Fix non-breaking change which fixes an issue v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants