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
[Audio] TrueHD rework - totally new MAT packer implementation #24984
Conversation
c851a7d
to
c9415dc
Compare
978f6b7
to
a00cd50
Compare
@anssih as the original author for both ffmpeg and kodi MAT packer, this PR might be interesting to you, also towards ffmpeg upstream. |
8b0792c
to
49d91e3
Compare
2ecff9b
to
f214fb3
Compare
Applied all code review suggestions except vector resize, for performance reasons. |
0b384f4
to
0d157bf
Compare
Rebased only, no changes. |
This has been tested for almost two weeks on the forum (https://forum.kodi.tv/showthread.php?tid=377117) by several users and they have not reported a single issue. They are familiar/intensive TrueHD users and some were still reporting rare/sporadic audio dropouts/cuts in the current implementation. It is time to merge it to obtain a broader test base? LibreELEC / CoreELEC nightlies users... @fritsch ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better code we understand, works and is maintained.
Description
TrueHD rework - totally new MAT packer implementation
Based on LAVFilters: https://github.com/Nevcairiel/LAVFilters/blob/master/decoder/LAVAudio/BitstreamMAT.cpp
with some optimizations.
Motivation and context
Initially I was trying to solve/debug the issues mentioned in #24944 with a specific test stream. While it may not fix all problems on rare (or even broken) streams, I have seen things wrong and limitations in the current TrueHD MAT packer implementation (which is the same as FFMpeg).
The new implementation is logically different but there are also some important conceptual differences:
1) The 8 bytes of the IEC header are now counted as padding, that is, when padding is written 16 more bytes are deducted than before: MAT frames have 61424 bytes but for padding they count as 61440. The other 8 bytes are obtained from
mat_end_code
now is 24 bytes length (16 before).Previously, these bytes were also set to zero until complete 61440 bytes, but they were not used for the padding calculation.
This happens here:
https://github.com/xbmc/xbmc/blob/4d70847511fc1461a12218020b57c77b861c1a67/xbmc/cores/AudioEngine/Utils/AEPackIEC61937.cpp#L111C3-L111C104
the structure is:
| IEC_HEADER (8 bytes) | MAT_FRAME (61424 bytes) | unused 8 bytes |
8 + 61424 + 8 = 61440
these "unused" bytes before was filled with zeros but not counted as padding. Now the size when call
CAEPackIEC61937::PackTrueHD
is 61432 and these 8 bytes are part ofmat_end_code
(last memset call does nothing because all packet bytes already set).2) Before it was not possible to generate padding with a length bigger than one MAT frame (61424 bytes). Now there is virtually no limit e.g. 300,000 bytes of padding can be written and are generated all necessary MAT frames (even if only contains padding) and queued.
In fact, a workaround is currently used to minimize the problem and not consider invalid stream or a seek, but the necessary amount of padding is still not generated:
xbmc/xbmc/cores/AudioEngine/Utils/AEBitstreamPacker.cpp
Lines 225 to 230 in 4d70847
This code is originally in FFmpeg:
if (padding_remaining < 0 || padding_remaining >= MAT_FRAME_SIZE / 2)
This can happen, for example, if a stream is badly cut and is missing audio frames: as each TrueHD unit has timestamps, if eg. 40 frames are removed, the MAT packer needs to fill in the equivalent time of the missing frames with padding bytes.
e.g. 20 ms of stream missing ---> 61440 bytes of padding generated
This is directly related to seamless branching Blu-Ray's since some demuxers removes audio units that overlap at the union of .m2ts segments (in itself you have to remove some data because there are repeated/overlapped TrueHD frames) https://github.com/domyd/mlp?tab=readme-ov-file#faq
This happens simply when converting from BDMV structure to MKV with any video converter tool...
3) Directly related to the previous point, there are specific instants when the MAT packer may generate more frames than are consumed and they need to be queued so that they are not overwritten/lost. This was not possible before.
How has this been tested?
Runtime Windows x64 and Shield
What is the effect on users?
More robust TrueHD passtrough audio. Even if still not fixes ALL issues on ALL streams (even the broken ones).
Many more cases are taken into account and it should work better. In the tests I have not found a single stream where this implementation works worse than before, but there are still some specific streams that have issues at very specific times (small cut/dropout) coinciding with unusually large padding blocks or stream discontinuities events.
Now these "unusual" events are properly registered in logs as warning/debug.
EDIT:
I'm not sure but this could also solve a/v sync problems that only some users have (and only with TrueHD). The reasoning/ guesswork is: almost all TrueHD streams usually start with padding bytes. Since padding > 61424 bytes is not supported now, the beginning of the audio is lost so it starts ahead. In some setups this is compensated by HDMI CEC/lipsync (if enabled)... in others it is not. This could improve if all streams now start with the necessary amount of padding.
Screenshots (if appropriate):
Types of change
Checklist: