feat: Add IAMF support (#1415) #1416
Conversation
|
Please add an end-to-end test in src/packager/app/test/packager_test.py to create golden files for verifying the correctness of the generated codec strings in DASH and HLS manifest files. |
| } else if (audio_info->codec() == kCodecIAMF) { | ||
| // IAMF sets channelcount to 0 | ||
| // https://aomediacodec.github.io/iamf/#iasampleentry-section | ||
| audio.channelcount = 0; |
There was a problem hiding this comment.
I am not very familiar with IAMF, but after reviewing the spec, I have a few questions:
- Based on this section, it seems that the iamf compatible brand is not present in ftyp box. Should this be added after line 265 above?
- I noticed that not all points mentioned in this section are included in the PR. Is there a specific reason for this?
- The documentation on common encryption mentions details about encrypting the IAMF stream. Are only AudioFrameOBUs encrypted?
There was a problem hiding this comment.
Thank you for reviewing!
- Good catch, done.
- I'm probably just misunderstanding what needs to be implemented here - I thought that these are expected already be in the input mp4 files and would be copied over when encrypting, or is this something that needs to be added separately here?
- Parameter Block OBUs would also be encrypted along with AudioFrameOBUs - both of them are part of a Temporal Unit, which is in turn packaged as one temporal unit per IA Sample.
There was a problem hiding this comment.
2.Please ignore. Sorry about that.
Do you think it would be beneficial to add checks for the constraints mentioned in the section to ensure the provided input is correct?
3.Thanks for the explanation.
I have a few questions:
a. For each sample indicated by trun, does it contain one TU (comprising OBU header type 5, Audio Frame OBU, OBU header type 3, and OBU parameter block)?
b. In the encryption handler, during media sample processing at EncryptionHandler::ProcessMediaSample, media subsamples are generated in SubsampleGenerator::GenerateSubsamples.
Is the entire sample encrypted, meaning no subsamples are expected as per here?
Or are the OBU headers skipped, requiring the exact addresses and sizes of the Audio Frame OBU and Parameter Block OBU to be extracted as subsamples, similar to how AV1 tiles are extracted?
c. Suggestion: It might be helpful to include a diagram similar to AV1 in the IAMF documentation
There was a problem hiding this comment.
Sorry I missed this before.
- Agreed checking could be useful, and if these constraints are not met, we could try to fix or at least warn the user. To implement all checks we'd want to make the OBU parsing more detailed to extract relevant info. Do you think this could be a separate PR?
3a. Right, one trun sample is one TU with those OBUs. In one TU, there could be more than one of each Audio Frame OBU and Parameter Block OBU (https://aomediacodec.github.io/iamf/#temporal-unit). E.g., the number of Audio Frame OBUs depend on the number of Audio Substreams for each Audio Element, and the total number of Audio Elements.
3b,c. IAMF uses full sample or whole-block full sample encryption (https://aomediacodec.github.io/iamf/#commonencryption), including the OBU headers, so I believe the code path you indicated is right, we don't expect subsamples. Given that, we weren't sure that a diagram similar to AV1 would be as useful, but it sounds like we could look at some clarification in 6.3. Common Encryption.
There was a problem hiding this comment.
@felicialim
2. Sounds good.
3. Thanks for the explanation.
Thanks for the pointer, done. |
|
@sr1990 does this seem good enough to merge for now and potentially refine later or do we have anything outstanding that must be addressed first? |
@cosmin lgtm. Thanks @felicialim. |
🤖 I have created a release *beep* *boop* --- ## [3.3.0](v3.2.1...v3.3.0) (2024-10-25) ### Features * Add IAMF support ([#1416](#1416)) ([dc6196d](dc6196d)), closes [#1415](#1415) * EXT-X-SESSION-KEY support ([#1427](#1427)) ([d88ed27](d88ed27)), closes [#36](#36) * **http:** Add DELETE method support ([#1442](#1442)) ([ddeacb2](ddeacb2)) ### Bug Fixes * **http:** Fix "Failed sending data to the peer" errors ([#1443](#1443)) ([2c9d100](2c9d100)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
BEGIN_COMMIT_OVERRIDE
feat: Add IAMF support (#1416)
Closes #1415
END_COMMIT_OVERRIDE