Add 12-bit profile (Main_4xx_12_IP2)#5112
Conversation
Flip the default for the 12-bit test-only profile from 0 to 1 so that 12-bit encode/decode is built in by default. Builds that explicitly opt out can still pass -DCONFIG_TESTONLY_12BIT_SUPPORT=0.
wantehchang
left a comment
wantehchang
left a comment
There was a problem hiding this comment.
Xin: Thank you for writing this PR. I suggest some simple changes.
| * | ||
| * Notes: | ||
| * - seq_profile_idc: Allowed profile values | ||
| * - bit_depth_idc: 0=8-bit, 1=10-bit |
There was a problem hiding this comment.
Optional: It would be good to create a separate pull request to correct the comment error. The comment fix could then be cherry-picked to the normative branch.
There was a problem hiding this comment.
@urvangjoshi Any preference on this? I am good either way (include this change in a separate PR or keep it in this PR).
There was a problem hiding this comment.
Fine either way as well. Slight preference for a separate PR since this is a separate issue from this PR's intent.
| if (profile == MAIN_4xx_12_IP2 && bit_depth == AVM_BITS_12) { | ||
| is_valid_bit_depth = 1; | ||
| } | ||
| #endif // CONFIG_12BIT_PROFILE |
There was a problem hiding this comment.
Nit: I think it would be better to change check_bit_depth_8_10() to take the profile parameter as follows:
static int check_bit_depth_8_10(int bit_depth, BITSTREAM_PROFILE profile) {
(void)profile;
// All profiles support 8-bit and 10-bit. Profile MAIN_4xx_12_IP2
// additionally supports 12-bit.
if (bit_depth == AVM_BITS_8 || bit_depth == AVM_BITS_10) {
return 1;
}
#if CONFIG_12BIT_PROFILE
if (profile == MAIN_4xx_12_IP2 && bit_depth == AVM_BITS_12) {
return 1;
}
#endif // CONFIG_12BIT_PROFILE
return 0;
}
Then here we can just say:
int is_valid_bit_depth = check_bit_depth(bit_depth, profile);
There was a problem hiding this comment.
Thanks for the suggestion. Agreed this is better, updated this PR.
| case MAIN_4xx_12_IP2: | ||
| // Supports all chroma formats (4:0:0/4:2:0/4:2:2/4:4:4) at 8/10/12-bit | ||
| break; | ||
| #endif // CONFIG_12BIT_PROFILE |
There was a problem hiding this comment.
Nit: I suggest adding case MAIN_4xx_12_IP2 before case CONFIGURABLE
There was a problem hiding this comment.
Agreed and updated.
411bbea to
9527f14
Compare
- Rename CONFIG_TESTONLY_12BIT_SUPPORT to CONFIG_12BIT_PROFILE and the profile enum to MAIN_4xx_12_IP2 (spec Main_4xx_12_IP2) - Accept 8/10/12-bit and all chroma formats for profile 5 - Set max mlayer count for profile 5 to 3 (IOP2 limit) - Give profile 5 its own PicSize/Bitrate factor row (36 / 3.0) per Table A.2 - Update Annex A profile/config tables and comments
9527f14 to
7493fd0
Compare
4 participants
This PR enables the 12-bit profile by default and brings its implementation into conformance with AV2 Spec Draft with 12-bit support: https://github.com/AOMediaCodec/av2-spec-internal/pull/776.