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

DPX read_native_scanline() doesn't always read a native scanline #1770

Closed
gregcotten opened this issue Sep 22, 2017 · 12 comments
Closed

DPX read_native_scanline() doesn't always read a native scanline #1770

gregcotten opened this issue Sep 22, 2017 · 12 comments

Comments

@gregcotten
Copy link

gregcotten commented Sep 22, 2017

Problem

When I open a 10 or 12-bit DPX and call read_native_scanline(...), I don't receive properly formatted 10-bit (packed or unpacked) or 12-bit (packed or unpacked) scanlines. From a deep dive in libdpx, it seems it will spit out "upscaled" 16-bit per channel data instead.

Expected behavior: read_native_scanline(...) for 10-bit or 12-bit DPX returns native image scanline

Actual behavior: read_native_scanline(...) for 10-bit or 12-bit DPX returns non-native image scanline

Versions

  • OIIO branch/version: all
  • OS: all
  • C++ compiler: any
@lgritz
Copy link
Collaborator

lgritz commented Sep 22, 2017

I believe that this is the correct behavior. (Or at least, the intended/designed behavior.)

In order to prevent apps from becoming unmanageably complex by having to support the full cross product of all possible data types and bit depths that might be supported by any current or future image format, we enforce a number of simplifying assumptions. Among those is that the ONLY pixel channel data formats supported on the "app side" of the OIIO APIs are 1-, 2-, and 4-byte signed and unsigned integers, which map 1.0 to their maximum value, also 2-, 4-, and 8-byte IEEE 754 floats.

The reader will set metadata (in the ImageSpec) named "oiio:BitsPerSample" in cases where the data in the file was actually a different bit depth than the data passed through the OIIO API. So, for example in this case, you'd find "oiio:BitsPerSample" might have a value of 10 or 12. With this data you can:

  • Convert back to original bit depth if you want, by bit shifting, with no loss of precision.
  • If you are writing images, pass the metadata to signal to the file output that you want the file encoded to that bit depth, even though the pixel data are in full-range uint16, and the file output plugin will handle the conversion for you (potentially losing precision, if you've passed a buffer that has important data in the least significant bits).

Put another way, OIIO's API only supports full-range uint16, but it will read and write files 10 or 12 bits as a file implementation detail, much in the same way that it handles compression in the files, but does not pass compressed data back and forth to the app.

@gregcotten
Copy link
Author

gregcotten commented Sep 22, 2017

Totally makes sense - for whatever reason I assumed "native" reading made no sort of conversions to the image. I think for most plugins this is the case. I honestly think the DPX plugin (and I can only assume Cineon is the same way) should actually support native reading of non-standard integer formats. Obviously I don't think this sort of functionality should be exposed above read_native_scanline(...) in these plugins as having OIIO support those formats could be a disaster.

There's also a "dpx:RawData" flag for the client config ImageSpec that sets an internal bool called m_wantRaw that tells the reader to not perform RGB/YUV conversions on the bitmap. Perhaps if someone (or me) ever gets to implementing correct non-standard (but NATIVE!) integer format scanline reading in libdpx, we should have another "dpx" namespaced flag to make this happen?

@lgritz
Copy link
Collaborator

lgritz commented Sep 23, 2017

As you noted, sometimes we do this with non-RGB color spaces. As well, there's a way to ask for the un-associated colors (not "premultiplied" with alpha) for file formats that store that natively (rather than following the stated OIIO convention of returning only associated alpha & color values).

So, yes, a (non-default) "give me the file's native bit depth" configuration option is a totally reasonable request. It could be for DPX specifically, or we could make it something we try to support (like "oiio:UnassociatedAlpha" or "oiio:RawColor") across all the format readers for any image file formats that support non-whole-range encodings.

@lgritz
Copy link
Collaborator

lgritz commented Sep 23, 2017

I can take a stab at this if you want, at least try to do a quick sanity check to see if it's easy.

Though if somebody else wanted to implement it and make a PR, I would be very happy to not have to do it myself.

@gregcotten
Copy link
Author

gregcotten commented Sep 23, 2017

Would be at least interesting to see how hard of a task it would be. I'm just now getting into non-standard bit depths, and I might need an Advil after reading the brilliant stuff to make packed/unpacked 10 and 12 bit DPX image reading work.

@lgritz
Copy link
Collaborator

lgritz commented Nov 21, 2018

Sorry, this one festered longer than I intended. Is this still something that's important? Would it be helpful to have a "configuration hint" that preserves the origin bit depth rather than the usual automatic scaling up to full bit depth? (Akin to how you can ask to not automatically convert to associated alpha.) Or is was this a passing urge and now you are ok with the usual rescaling?

@gregcotten
Copy link
Author

gregcotten commented Nov 21, 2018 via email

@lgritz
Copy link
Collaborator

lgritz commented Nov 21, 2018

You can use OIIO to read the file, fill your buffer with 16 bit data, and then convert back to 10 with:

tenbit = OIIO::bit_range_convert<16,10>(sixteenbit);   // in <OpenImageIO/fmath.h>

for each value, to get back to 10 bits. It should be a lossless conversion, exactly the opposite of how it scales the 10 bit up to 16.

So it's easy to convert back to the original bit depth if you want to. In light of that, does anybody believe the shift and then un-shift of this round trip is so expensive that it's helpful to have a hint that would cause it to not happen at all? I can certainly do it.

@gregcotten
Copy link
Author

gregcotten commented Nov 21, 2018 via email

@lgritz
Copy link
Collaborator

lgritz commented Nov 21, 2018

OK, I took a look at this and it's pretty straightforward. But I have a question.

There already is an (undocumented, sorry) input configuration option called "dpx:RawData", which if nonzero will leave the color channels in their original color space (e.g., leaving CbCrY in their original form rather than auto-converting to RGB as would be the default). A similar option exists for TIFF and PSD (called "oiio:RawColor", I should probably uniformize it to just one name across all the readers).

I could just capitalize on this attribute and interpret it to mean that it should also not expand the bit depth to the full range. Or, I could add a separate control just for bit depth.

Thoughts?

@lgritz
Copy link
Collaborator

lgritz commented Nov 21, 2018

Ouch, I see now that the underlying dpx library seems to do the bit conversion automatically, it's not really on "our side" after all.

I thought it would be a 15 minute thing, but it looks a lot more intrusive. I think I will put this aside for now if there's not a pressing need.

I may still have a very minor patch just to uniformize the RawData/RawColor nomenclature across the different plugins, though.

@lgritz
Copy link
Collaborator

lgritz commented Nov 21, 2018

Proposed fix to docs and naming conventions in #2075.

I'm going to skip implementing the bit depth preservation for now. Closing. Reopen if it becomes important to somebody.

@lgritz lgritz closed this as completed Nov 21, 2018
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

No branches or pull requests

2 participants