media: Parse IAMF config in VideoDmpReader#9116
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates VideoDmpReader to parse IAMF configuration from dmp files, which is then used to construct a more accurate MIME type. While the changes introduce useful helpers for parsing LEB128 values and extracting IAMF profiles, a medium-severity integer overflow vulnerability was identified in the parsing of IAMF configuration OBU data. This could be exploited by a malicious media file to cause a denial of service or information leak. Additionally, there is a bug in the ReadLeb128 implementation that prevents it from parsing valid 32-bit integers encoded with 5 bytes, and a portability issue where sizeof(int) is used for a fixed 32-bit field. Detailed comments and suggestions, including a code suggestion for the vulnerability, are provided in the line comments.
There was a problem hiding this comment.
Code Review
This pull request adds IAMF configuration parsing to VideoDmpReader, which is a great enhancement for generating specific MIME types. My review focuses on improving the robustness and portability of the new parsing logic. I've identified two main areas for improvement:
- The
ReadLeb128function has a bug related to the number of bytes it reads and lacks overflow protection. I've suggested a more robust implementation. - The code uses
sizeof(int)to skip a field from the IAMF header, which is not portable. I've recommended using a fixed size based on the specification.
These changes will make the parser more reliable across different platforms.
e9d3d7e to
2c2b389
Compare
This change modifies VideoDmpReader to parse the configuration of IAMF dmp files, allowing it to craft an accurate MIME type string. Bug: 485254424
2c2b389 to
e52f876
Compare
kjyoun
left a comment
There was a problem hiding this comment.
Sorry for the multiple requests, but I believe we can make this logic significantly cleaner and more robust.
This time I wrote a sample code snippet to show the pattern I'm looking for:
https://paste.googleplex.com/5521076055572480
- Return-by-value, instead of modifying parameters or using out-params.
- Eliminate manual pointer arithmetic: Leverage std::basic_string_view<uint8_t> (or a ByteView alias) and its built-in methods like .remove_prefix() and .substr() to handle the buffer bounds safely
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds functionality to parse IAMF sequence header OBUs to extract profile information. This is then used to construct a more specific MIME type for IAMF audio streams. The changes include a new BufferReader utility, a parsing function in IamfMimeUtil, and updates to VideoDmpReader to use this new functionality. New tests are also added to cover the parsing logic.
My review focuses on correctness and efficiency. I've identified a correctness issue in the MIME type formatting where an incorrect width is used for profile numbers. I've also suggested an improvement to make the parsing logic in VideoDmpReader more efficient by avoiding an unnecessary data copy. Finally, I've pointed out a minor typo in the new test cases.
kjyoun
left a comment
There was a problem hiding this comment.
LGTM
Thanks for bearing with me
media: Parse IAMF config in VideoDmpReader
VideoDmpReader now extracts IAMF primary and additional profiles from the
stream's first access unit. This is achieved by parsing the IAMF
Sequence Header OBU.
This change enables the generation of an accurate and specific MIME type
string for IAMF audio, moving from a generic "iamf" to a detailed
"iamf.XXX.YYY.Opus" format. A precise MIME type is essential for
correct media pipeline configuration and playback compatibility.
Bug: 485254424