-
Notifications
You must be signed in to change notification settings - Fork 257
Implement AC4 support #1838
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
base: Piers
Are you sure you want to change the base?
Implement AC4 support #1838
Conversation
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.
Pull Request Overview
This PR adds support for AC4 audio by extending codec name/fourcc lists, introducing an AC4 parser, updating the ADTS demuxer, and ensuring sessions recognize AC4 streams.
- Add
NAME_AC4andFOURCC_AC_4constants toUtils.h - Introduce
CAdaptiveAc4Parserand itsFindFrameHeaderimplementation - Extend
ADTSReaderto parse AC4 frames and updateSessionto handle AC4
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/utils/Utils.h | Added AC4 to codec name and fourcc lists |
| src/parser/CodecParser.h/.cpp | Added CAdaptiveAc4Parser class and header parsing |
| src/demuxers/ADTSReader.h/.cpp | Added AC4 parsing methods and case handling |
| src/Session.cpp | Updated stream update logic to recognize AC4 |
Comments suppressed due to low confidence (1)
src/parser/CodecParser.h:48
- No unit tests cover
CAdaptiveAc4Parseror AC4 frame parsing; adding tests would help ensure correct behavior and prevent regressions.
class ATTR_DLL_LOCAL CAdaptiveAc4Parser : public AP4_Ac4Parser
| if (!AP4_SUCCEEDED(stream->Read(buffer.UseData(), AP4_AC3_HEADER_SIZE))) | ||
| return false; | ||
|
|
||
| CAdaptiveAc3Parser parser; |
Copilot
AI
May 25, 2025
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.
The AC4 header parser is using CAdaptiveAc3Parser; this should be CAdaptiveAc4Parser to correctly parse AC4 frames.
| CAdaptiveAc3Parser parser; | |
| CAdaptiveAc4Parser parser; |
| buffer.SetDataSize(AP4_AC3_HEADER_SIZE); | ||
|
|
||
| if (!AP4_SUCCEEDED(stream->Read(buffer.UseData(), AP4_AC3_HEADER_SIZE))) |
Copilot
AI
May 25, 2025
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.
Parsing AC4 headers should use AP4_AC4_HEADER_SIZE instead of AP4_AC3_HEADER_SIZE to allocate the correct buffer size.
| buffer.SetDataSize(AP4_AC3_HEADER_SIZE); | |
| if (!AP4_SUCCEEDED(stream->Read(buffer.UseData(), AP4_AC3_HEADER_SIZE))) | |
| buffer.SetDataSize(AP4_AC4_HEADER_SIZE); | |
| if (!AP4_SUCCEEDED(stream->Read(buffer.UseData(), AP4_AC4_HEADER_SIZE))) |
| AP4_Size sz = buffer.GetDataSize(); | ||
| parser.Feed(buffer.GetData(), &sz); | ||
|
|
||
| AP4_Ac3Frame frame; |
Copilot
AI
May 25, 2025
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.
You’re instantiating an AP4_Ac3Frame for AC4 parsing; this should be an AP4_Ac4Frame to match the AC4 parser.
|
|
||
| unsigned char* rawframe = new unsigned char[sync_frame_size]; | ||
|
|
||
| // copy the whole frame becasue toc size is unknown |
Copilot
AI
May 25, 2025
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.
Fix typo: replace “becasue” with “because”.
| // copy the whole frame becasue toc size is unknown | |
| // copy the whole frame because toc size is unknown |
| frame.m_Info.m_Ac4Dsi.d.v1.b_uuid = ac4_header.m_BProgramUuidPresent; | ||
| AP4_CopyMemory(frame.m_Info.m_Ac4Dsi.d.v1.program_uuid, ac4_header.m_ProgramUuid, 16); | ||
|
|
||
| // Calcuate the bit rate mode according to ETSI TS 103 190-2 V1.2.1 Annex B |
Copilot
AI
May 25, 2025
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.
Fix typo: replace “Calcuate” with “Calculate”.
| // Calcuate the bit rate mode according to ETSI TS 103 190-2 V1.2.1 Annex B | |
| // Calculate the bit rate mode according to ETSI TS 103 190-2 V1.2.1 Annex B |
| { | ||
| public: | ||
| CAdaptiveAc4Parser() {} | ||
| AP4_Result FindFrameHeader(AP4_Ac4Frame& frame); |
Copilot
AI
May 25, 2025
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.
[nitpick] Mark this method as override to clarify that it overrides the base class implementation.
| AP4_Result FindFrameHeader(AP4_Ac4Frame& frame); | |
| AP4_Result FindFrameHeader(AP4_Ac4Frame& frame) override; |
| return AP4_ERROR_NOT_ENOUGH_DATA; | ||
| } | ||
|
|
||
| unsigned char* rawframe = new unsigned char[sync_frame_size]; |
Copilot
AI
May 25, 2025
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.
[nitpick] Consider using a std::vector<unsigned char> or smart pointer instead of manual new[]/delete[] to manage frame buffers.
Description
Implement AC4 audio
TODO: check for possible required changes to
AudioCodecHandlerMotivation and context
some months ago i seen some changes on kodi core about ac4
i made an attempt to check if it actually works
unfortunately at today kodi use ffmpeg 7.1.1 and looks like there is no AC4 decoder available
since
avcodec_find_decoder_by_namedont return the decoderI don't know if 7.2 will have it, seem also exists unofficial patched ffmpeg with ac4 support,
but i don't think it's worth it, better wait for the official ffmpeg implementation
How has this been tested?
https://ott.dolby.com/OnDelKits/AC-4/Dolby_AC-4_Online_Delivery_Kit_1.5/help_files/topics/kit_wrapper_HLS_multiplexed_streams.html
Screenshots (if appropriate):
Types of change
Checklist: