-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add colorimetry information API in Picture #94
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
src/lib.rs
Outdated
DAV1D_COLOR_PRI_SMPTE431 => pixel::ColorPrimaries::P3DCI, | ||
DAV1D_COLOR_PRI_SMPTE432 => pixel::ColorPrimaries::P3Display, | ||
DAV1D_COLOR_PRI_EBU3213 => pixel::ColorPrimaries::Tech3213, | ||
DAV1D_COLOR_PRI_RESERVED => unreachable!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that indeed unreachable? Also below in the same places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With current dav1d git master yes, I've checked these appear only in the header file. Maybe we could make these methods return an Option<>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lu-zero What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hacky way is to return pixel::ColorPrimaries::Unspecified
as well, since the user would have no mean to deal with it anyway and add a mean to get the raw bitstream value in case the user knows how to use it since there are few "for future use" values. that assuming PRI_RESERVED maps to the bitstream value.
Anybody willing to read ISO/IEC 23091-4/ITU-T H.273
and doublecheck?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one? https://www.itu.int/rec/T-REC-H.273-202309-P/en
I'm not registered in TIES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.itu.int/rec/T-REC-H.273-201612-S/en is freely accessible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in the spec the reserved value is specified as a range, for instance, for ColorPrimaries, it is 23-255
, while DAV1D_COLOR_PRI_RESERVED
is 255. Same goes for TransferCharacteristics (19-255
) and MatrixCoefficients (15-255
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that mapping it to Unspecified
wouldn't feel too bad. Maybe I could future-proof av-data and add a Reserved(u8) but that's unrelated to this patch.
2ad25b7
to
2b29154
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mapping to unspecified makes most sense to me too
I'd land it |
Thanks for the reviews :) Is a new release planned soon? |
If dav1d-1.4.1 didn't bring anything more we can make one even today (or tomorrow). |
The release notes since 1.3.0 mention mostly optimizations. I've checked 1.4.1 works fine here (on x86_64) with current master branch of dav1d-rs and my WIP colorimetry branch of gst-plugin-dav1d ;) |
There are no changes in the headers since 1.3.0 so yes that seems fine. |
Fixes #93