-
Notifications
You must be signed in to change notification settings - Fork 254
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_AC4
andFOURCC_AC_4
constants toUtils.h
- Introduce
CAdaptiveAc4Parser
and itsFindFrameHeader
implementation - Extend
ADTSReader
to parse AC4 frames and updateSession
to 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
CAdaptiveAc4Parser
or 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; |
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; |
Copilot uses AI. Check for mistakes.
buffer.SetDataSize(AP4_AC3_HEADER_SIZE); | ||
|
||
if (!AP4_SUCCEEDED(stream->Read(buffer.UseData(), AP4_AC3_HEADER_SIZE))) |
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))) |
Copilot uses AI. Check for mistakes.
AP4_Size sz = buffer.GetDataSize(); | ||
parser.Feed(buffer.GetData(), &sz); | ||
|
||
AP4_Ac3Frame frame; |
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.
Copilot uses AI. Check for mistakes.
|
||
unsigned char* rawframe = new unsigned char[sync_frame_size]; | ||
|
||
// copy the whole frame becasue toc size is unknown |
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 |
Copilot uses AI. Check for mistakes.
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 |
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 |
Copilot uses AI. Check for mistakes.
{ | ||
public: | ||
CAdaptiveAc4Parser() {} | ||
AP4_Result FindFrameHeader(AP4_Ac4Frame& frame); |
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; |
Copilot uses AI. Check for mistakes.
return AP4_ERROR_NOT_ENOUGH_DATA; | ||
} | ||
|
||
unsigned char* rawframe = new unsigned char[sync_frame_size]; |
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.
Copilot uses AI. Check for mistakes.
Description
Implement AC4 audio
TODO: check for possible required changes to
AudioCodecHandler
Motivation 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_name
dont 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: