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

raw input: fix wrong resolution when not demosaicing #3125

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Oct 10, 2021

When using demosaicing mode "none", we need to use the real sensor
resolution, not the "output resolution" of the processed, demosaiced
image.

Fixes #3115

When using demosaicing mode "none", we need to use the real sensor
resolution, not the "output resolution" of the processed, demosaiced
image.

Fixes 3115
@lgritz
Copy link
Collaborator Author

lgritz commented Oct 10, 2021

@Shootfast Does this look reasonable to you? I believe you were the last person in the raw code.

@Shootfast
Copy link
Contributor

Hey Larry,

I actually have some more patches around this issue coming in the near future, but if this fixes an immediate problem then I don't have an issue with it being merged.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 13, 2021

@Shootfast As I look at it a bit more, I believe also that the recent code you added for the non-demosaic case does not behave well for rotated images. Now I'm wondering if I should commit this small PR now as-is (which at least fixes the case for non-rotated images) and then throw the rest into your lap, since (a) you know this code better than I do and can probably fix it more expediently, (b) you may well have already fixed it on your end if you have in-progress changes, and (c) I don't want to stomp on the same lines and make whatever you're working on be a difficult merge for you.

What do you think? Is this slated in the near enough future for you that I should turn the rest over to you?

@Shootfast
Copy link
Contributor

Looking into this a bit further, I'm not sure this is correct.
The raw sensor data often contains "junk" pixels, that aren't intended to be a part of the final image.
I was taking the effort to trim these off here.
I had tested with a CR2 image in all orientations, but perhaps the behaviour is different for other formats?

I wonder if keeping the junk pixels should be exposed as another read option?

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 14, 2021

If you're not confident about this, would it be better for me to abandon this PR and you can just be sure to tackle the correct unmosaiced+rotated combinations in the next set of changes you were working on?

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 23, 2021

@Shootfast Ping -- Would you like me to merge this partial fix, or will that just make life harder for you and I should let you fold the more correct fixes into your next PR in the raw code?

@Shootfast
Copy link
Contributor

If you don't mind waiting a little bit, I'd prefer if you could hold off on the partial fix. I've worked a bit more at this over the weekend, but still not quite ready to push anything your way.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 24, 2021

No, I don't mind the delay, I'd rather get a more complete fix and I lack the time and libraw experience to do it efficiently.

@AdamMainsTL
Copy link
Contributor

Looking into this a bit further, I'm not sure this is correct. The raw sensor data often contains "junk" pixels, that aren't intended to be a part of the final image.

What needs to change with this patch to make it more correct? I'm somewhat frequently running into CR2/CR3 issues in particular and would rather just apply a quick patch if the larger fix will take longer, but of course I don't want to break anything else silently.

@lgritz
Copy link
Collaborator Author

lgritz commented Mar 7, 2022

@AdamMainsTL Good question.

@Shootfast What's your opinion here? If this modest patch a step forward (even though not a complete solution), or does it introduce as many new problems as it fixes? Back in October you made it seem like you were imminently going to submit a more comprehensive set of changes. Is that ready to go?

@Shootfast
Copy link
Contributor

Ah sorry, yes, it's pretty close to done. Have just been distracted with other things. Let me take another look and upload my work in progress somewhere for you to take a look at!

@Shootfast
Copy link
Contributor

I've pushed my commits to here and rebased them off the latest master:
https://github.com/Shootfast/oiio/tree/raw_features

Would you be able to try it out? The "junk" pixels are now maintained in the overscan areas of the image. This is so they are maintained when they are re-written out via the new raw DNG writer.

The DNG writer is still WIP, but the raw reader changes are probably mergeable if you don't see any errors in your files.

@AdamMainsTL
Copy link
Contributor

Thanks for the update @Shootfast! I will be able to test these changes probably earliest on Monday or so, but I'll let you know if it fixes our problem cases.

@lgritz
Copy link
Collaborator Author

lgritz commented Mar 18, 2022

@Shootfast -- Not directly related to this issue ticket, but in glancing at your dng output, I had a couple of thoughts for you:

  1. I get the feeling that you may have cribbed material from the TIFF output some time ago and it has diverged. In particular, it doesn't contain any provision for IOProxy support. You may wish to re-synchronize and/or otherwise make sure to get the proxy support in place, as is coming to be expected from all plugins these days.

  2. It looks like you're just writing a compliant TIFF file, using an altered version of tiffoutput.cpp. So I can't help but wonder if, in the grand scheme of things, it would simply be better to implement DNG in the actual TIFF output (just checking the filename extension, and in the case of dng, doing, or not doing, the various things that distinguish a true DNG from a generic TIFF. I'm just thinking that maybe there would be less code duplication this way, and easier to simultaneously make improvements to both tif and dng output at the same time, since they will mostly be literally the same code. (And conversely, if we ever support output of other "camera raw" formats, they will be wholly unrelated to the TIFF/DNG code here.) I mean, you be the judge of the best way to do all this, perhaps I'm wrong and this has so little overlap (despite using libtiff) with the generic TIFF that it's more trouble than it's worth to combine them.

@Shootfast
Copy link
Contributor

  1. Yeah I can totally revisit and add IOProxy support. This code isn't quite ready just yet anyways. Still a fair bit to do.
  2. It has been rather difficult to trick libtiff into writing a compliant DNG, and I think it would be even harder to follow the logic if it were entwined with the regular TIFF writer. That said there is certainly a lot of code that could probably be shared, so I was hoping I could refactor them both and share functions at some point. I don't know how likely another raw format writer would be, and if there was I assume it would probably be better off under another plugin name.

@lgritz
Copy link
Collaborator Author

lgritz commented Mar 18, 2022

It has been rather difficult to trick libtiff into writing a compliant DNG, and I think it would be even harder to follow the logic if it were entwined with the regular TIFF writer.

That's a very compelling rationale. Mostly I just wanted you to know that the option was open to you if it seemed simpler to combine them.

@AdamMainsTL
Copy link
Contributor

AdamMainsTL commented Mar 21, 2022

I finally got around to trying to test this, but unfortunately I'm unable to build it with my build process due to boost errors. I'm not fully sure whether this is due to how I have things setup (Conan with some CMake patches), or something with this branch. Here are build logs for reference: https://gist.github.com/AdamMainsTL/14204073a761a512170a173ed33958e0

What's strange to me is that I can build 2.3.13.0 with the same process just fine which has the same Boost include syntax, so unsure what's causing it to "find" it, but then not find it when linking. I've also been using this same process since around 2.2.9.0, so this break is new.

Edit: I've been able to resolve this now. I thought that current master branch was what 2.3.13 was based on, but it seems like that's not the case. I took a look at the 2.3.13 source and patched the CMake and it is building now. Will be able to test it later today.

Edit again: Turns out that the build process doesn't like the master branch. Might actually not be able to test it today.

@AdamMainsTL
Copy link
Contributor

So I've finally gotten it working after changing Boost and OpenEXR linking stuff in the CMake files. I also had to patch rawoutput.cpp because clang doesn't like narrowing int to uint32_t in initializer lists (error log)

So far it seems like the current code does not address #3115 as all the example files in that post still crash for me with this build. The NEF and PEF might be fine and crash in other code, but the 4 other ones for sure are crashing inside of read_image.

@Shootfast
Copy link
Contributor

Ah sorry, I've dived into this further and found the root cause for these other failures. Short version is that the libraw API does a terrible job of presenting the data inside a RAW file :(

Thus far I'd relied on the rawdata.raw_image pointer being set to point me at the bayer patterned footage. Unfortunately, if the RAW file uses a different underlying data type (ie not single channel bayer), then libraw actually jumps between a number of different pointers (with only one of them being set at a time).

I need to take a closer look, as some of these pointers are also completely different data types. The shortlist is:

/* alias to single_channel variant */
ushort *raw_image;
/* alias to 4-channel variant */
ushort (*color4_image)[4];
/* alias to 3-color variand decoded by RawSpeed */
ushort (*color3_image)[3];
/* float bayer */
float *float_image;
 /* float 3-component */
float (*float3_image)[3];
/* float 4-component */
float (*float4_image)[4];

Your example images actually go through a number of these, so they are a good test list. Apologies that I forgot you had already sent them when I uploaded my last version of the code.

@AdamMainsTL
Copy link
Contributor

I'm glad the examples are helpful. I hope it's not too hard of an issue to work around. If you get another update let me know and I can test it out with some other files that crash on my end.

Thanks for continuing to work on this!

@AdamMainsTL
Copy link
Contributor

@Shootfast Hey, just wanted to know if there's any update for this? Not sure if I can help, but does this change need some?

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.

[BUG] Invalid write crash when opening certain RAW images
3 participants