Repository navigation
Conversation
The Multiple Essence Location Style is used for Long GOP and Intra VBR. The Single Essence Location Style is used for Intra CBG (RP 2027 Constrained Class 50/100/200). A new --avc-cbg option has been added to specify Class 300 and 480, which cannot be detected automatically. Assisted-By: Claude Sonnet 5
XAVC Long GOP HD, XAVC Long GOP UHD (3840×2160), RP 2027 XAVC Intra Class 100, XAVC 4K Intra Class 300 CBG Authored-By: Claude Sonnet 5
scenarnick
left a comment
scenarnick
left a comment
There was a problem hiding this comment.
Initial comment after quick glance (not full review). I'm not sure how much I like the redefinition of the "flavour". I foresee it causing problems in maintainability of the code in the long run. Deferring to other reviewers.
| #define MXFDESC_ARD_ZDF_XDF_PROFILE_FLAVOUR 0x0080 | ||
|
|
||
| #define MXFDESC_RDD32_FLAVOUR 0x0080 | ||
| #define MXFDESC_ARD_ZDF_XDF_PROFILE_FLAVOUR 0x0080 // The ARD-ZDF-XDF profile is a restricted subset of this RDD32 implementation and is linked to it here. |
There was a problem hiding this comment.
I have a feeling this will create confusion in the future. For now, redefinition seems elegant but your comment already hints at a future TODO. Can we find another way?
There was a problem hiding this comment.
The XDF flavor was linked to the HDF flavor, which didn't really make sense. I tried to untangle and simplify that. In this MXF descriptor implementation, RDD 32 and XDF are identical and will remain so for the foreseeable future. I brought this up at the MXF Plugfest.
To avoid accidental problems caused by future changes, I would insert an ARD-ZDF-XDF test before this RDD-32 patch.
| public: | ||
| OP1AIndexTableSegment(uint32_t index_sid, uint32_t body_sid, mxfRational edit_rate, int64_t start_position, | ||
| uint32_t index_entry_size, uint32_t slice_count, bool force_write_slice_count, bool force_write_cbe_index_duration_0, | ||
| mxfOptBool single_index_location, mxfOptBool single_essence_location, |
There was a problem hiding this comment.
To Check in in-depth review: Is single_index_location not required anywhere else?
| cdci_descriptor->setDisplayXOffset(essence_parser->GetDisplayXOffset()); | ||
| cdci_descriptor->setDisplayYOffset(essence_parser->GetDisplayYOffset()); | ||
| if ((mFlavour & MXFDESC_ARD_ZDF_XDF_PROFILE_FLAVOUR)) { | ||
| if ((mFlavour & MXFDESC_RDD32_FLAVOUR)) { |
There was a problem hiding this comment.
This will break if the redefinition is ever lifted. Suggest this one:
| if ((mFlavour & MXFDESC_RDD32_FLAVOUR)) { | |
| if ((mFlavour & MXFDESC_RDD32_FLAVOUR) || (mFlavour & MXFDESC_ARD_ZDF_XDF_PROFILE_FLAVOUR)) { |
| cdci_descriptor->setPaddingBits(0); | ||
|
|
||
| if ((mFlavour & MXFDESC_ARD_ZDF_XDF_PROFILE_FLAVOUR)) { | ||
| if ((mFlavour & MXFDESC_RDD32_FLAVOUR)) { |
| case 2: // 4:2:2 | ||
| cdci_descriptor->setHorizontalSubsampling(2); | ||
| cdci_descriptor->setVerticalSubsampling(1); | ||
| if (!cdci_descriptor->haveColorSiting() || (mFlavour & MXFDESC_ARD_ZDF_XDF_PROFILE_FLAVOUR)) |
| mAVCSubDescriptor->setAVCConstantBPictureFlag(0); | ||
| mAVCSubDescriptor->setAVCSequenceParameterSetFlag(0x30); | ||
| mAVCSubDescriptor->setAVCPictureParameterSetFlag(0x30); | ||
| if ((mFlavour & MXFDESC_RDD32_FLAVOUR)) { |
Hi,
the first patch enables the creation of RDD32-compatible XAVC files. The files generated with it have been successfully verified using the Baton QC tool. The second patch adds the corresponding tests.
To avoid code duplication, for the Long GOP variants I used code from the implemented ARD-ZDF-XDF profile, which complies with RDD32. The ARD-ZDF-XDF profile is now based on the RDD32 implementation. I also retained the Multiple Essence Location Style for the Long GOP variants because it is the most suitable option for growing files. For the Intra variants, I am using the Single Essence Location Style, based on previous RP2027 implementations.
Thomas