Skip to content
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

Set chroma_sample_position only for YUV 4:2:0 #41

Merged

Conversation

wantehchang
Copy link
Contributor

The chroma_sample_position syntax element is only used for YUV 4:2:0 in
the AV1 bitstream. So set chroma_sample_position to
ChromaSamplePosition::Unknown for all other YUV formats, including
monochrome (YUV 4:0:0).

Note: For still images, the most common chroma sample position in
practice is the "center" position. Unfortunately the AV1 specification
does not have a value for the "center" chroma sample position. See
AOMediaCodec/av1-avif#88 and
AOMediaCodec/av1-spec#308. This pull request
preserves the ChromaSamplePosition::Colocated value that is currently
used, but it is likely to be incorrect when YUV 4:2:0 is supported.

The chroma_sample_position syntax element is only used for YUV 4:2:0 in
the AV1 bitstream. So set chroma_sample_position to
ChromaSamplePosition::Unknown for all other YUV formats, including
monochrome (YUV 4:0:0).

Note: For still images, the most common chroma sample position in
practice is the "center" position. Unfortunately the AV1 specification
does not have a value for the "center" chroma sample position. See
AOMediaCodec/av1-avif#88 and
AOMediaCodec/av1-spec#308. This pull request
preserves the ChromaSamplePosition::Colocated value that is currently
used, but it is likely to be incorrect when YUV 4:2:0 is supported.
@wantehchang
Copy link
Contributor Author

@kornelski Please review. Thanks!

@kornelski kornelski merged commit 1bd087c into kornelski:main Sep 10, 2021
@kornelski
Copy link
Owner

kornelski commented Sep 10, 2021

Thanks.

I'm not planning to support chroma subsampling of color channels ever. If I remember correctly, this chroma setting was only a workaround for lack of monochrome encoding.

@wantehchang
Copy link
Contributor Author

I see. I guess older versions of rav1e could not encode monochrome (YUV 4:0:0) images. The current version of rav1e encodes alpha images as monochrome correctly.

The current cavif-rs code uses only ChromaSampling::Cs444 and ChromaSampling::Cs400. If it will stay this way, we can simply set chroma_sample_position to ChromaSamplePosition::Unknown unconditionally. Would you like me to do that?

@wantehchang wantehchang deleted the correct-chroma-sample-position-value branch September 10, 2021 16:34
@kornelski
Copy link
Owner

Yes, please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants