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

Colordata Struct #4

Closed
wants to merge 2 commits into from
Closed

Colordata Struct #4

wants to merge 2 commits into from

Conversation

gennyble
Copy link
Contributor

@gennyble gennyble commented Nov 30, 2021

This has been a bit coming it seems! The issue, #3, is from January 7th of this year and now it's almost January again! I was using the re-exported struct for a good while before I decided to finally copy it into Rust. It was an interesting undertaking. I don't think I'm done yet, but it's been sitting in my fork for a bit and there are some things I'd like your advice about. Notes below.

Libraw's cblack: unsigned cblack[4102]

Per-channel black level correction. First 4 values are per-channel correction, next two are black level pattern block size, than cblack[4]*cblack[5] correction values (for indexes [6....6+cblack[4]*cblack[5]).

Going on the documentation, I broke cblack into each separate thing it claimed to be. So there's a field for the channels, width, height, and the block data. Do you think we should provide some kind of helper method to assist in getting each block out of the black_block_data like an iterator?

Further down Colordata there's a dng_levels that has it's own struct. This has a cblack, too, which I have not split into separate fields. If you think the field-splitting is a good idea I'll go along and do that. Maybe there could be a struct ChannelBlack for the both of them?

Libraw's linear_max: unsigned linear_max[4]
I put a comment above this one in the code, too: The documentation says it's unsigned but libraw-sys gives it as an i64? I left it as an [i64; 4] in the struct.

Libraw's model2, UniqueCameraModel, and LocalizedCameraModel: all [i8; 64]
The latter two are DNG tag names and the former is the "Firmware revision (for some cameras)". It seems like these want to be strings. Should we provide a method like OsString has to_string_lossy and into_string?

Libraw's profile: void *profile

Pointer to the retrieved ICC profile (if it is present in the RAW file).

So this one is weird. There is a value after it that gives the length so I create a slice using unsafe code and then clone it with to_vec. I am pretty sure this operation is safe but I am not certain. How would you handle this?

Inconsistent names

Libraw is reasonably consistent with their names, but it does not fit with Rust's conventions and rustfmt gets mad. It seems that they use the exif tag name if that field is directly from the exif data and snake_case otherwise. Should we snake-caseify these names or is the #[allow(non_snake_case)] annotation fine?

Also there's a model2 but no model?

Misc. Question

In trying to document the colordata struct, I realized I'm not very certain at all what some of the fields indicate.

In our Colordata there's a black_per_channel which is the extracted values from cblack. It says per-channel but it has four values. So it's probably not RGB, right? And the values are... not what I'd expect. I tried to figure it out here but have not come to any solid conclusions yet.

This has been a long PR message, so I might as well sign it like an email :3
I took a year to get this PR here so don't feel the need to rush to reply to it if that's something you might feel. I'm in no rush if you aren't.

~ Genevive

@gennyble gennyble closed this by deleting the head repository Dec 20, 2022
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.

1 participant