Skip to content
This repository has been archived by the owner on Jul 22, 2018. It is now read-only.

Fix a buffer overflow on double width fuji images #51

Merged
merged 1 commit into from
Sep 18, 2014

Conversation

pedrocr
Copy link
Contributor

@pedrocr pedrocr commented Sep 18, 2014

When I did the F700 support I messed up and created a buffer overflow as I allocated a widthXheight buffer and then decoded a 2XwidthXheight image into it.

While fixing this I realized that the double width image is actually a second raw image with a darker version of the same image. This is from the fuji SuperCCD sensors that had extra pixels to expand dynamic range. There are a couple of these old fuji cameras with that support so in the future it may make sense to expose both raw files to allow dual-iso processing like in the Magic Lantern Canon 5D hack.

@klauspost
Copy link
Owner

Nice find - LGTM!

klauspost added a commit that referenced this pull request Sep 18, 2014
Fix a buffer overflow on double width fuji images
@klauspost klauspost merged commit dc68e5b into klauspost:develop Sep 18, 2014
@LibRaw
Copy link

LibRaw commented Sep 18, 2014

BTW, something should be made for DNGs created from F700 (and similar cameras).
Unfortunately, these DNGs uses different approach (SamplesPerPixel=2): #45

@pedrocr
Copy link
Contributor Author

pedrocr commented Sep 18, 2014

@LibRaw, that's interesting. Ideally we'd have a single way to expose these dual-iso cases either having two images or a single image with 32bit values.

@LibRaw
Copy link

LibRaw commented Sep 19, 2014

In many real-world cases, the high-iso image may be partially overexposed (clipping), or low-ISO one is underexposed (noise).
Dealing with these images is not as easy as it looks on first sight.

@LebedevRI
Copy link
Contributor

As a developer of darktable, that uses rawspeed to load images, i see the only sane solution is to "expose these dual-iso cases as having two images".
Merging two images from dual-iso set is not as simple as just appending bits, so there is not much point in exposing them as a single image with 32bit values, since to actually merge them, each 32-bit pixel would have to be split into 2x16-bit pixels first.

@pedrocr
Copy link
Contributor Author

pedrocr commented Sep 19, 2014

@LebedevRI that was my assumption as well but I wondered if for some things in the pipeline 32bit pixels wouldn't be easier to handle. Something that does for(width){for(height){process_pixel_somehow(x,y)}} before demosaic could potentially work without modification with 32bit pixels whereas it would need more changes to work with two images. Once dual-iso lands in darktable I'll take a closer look at this and propose an approach.

@LibRaw
Copy link

LibRaw commented Sep 20, 2014

Dual-ISO pixels (regardless of source, MagicLantern hack or Fuji dual-ISO matrix) have different coordinates on camera sensor.
It is not good idea to mix it with neighbors, this will result to lose coordinate info

@pedrocr
Copy link
Contributor Author

pedrocr commented Sep 20, 2014

@LibRaw in the fuji case that's true on sensor but apparently not on-file. The fuji file seems to include two images with the same dimensions and bayer pattern. Since SuperCCD sensors didn't actually have that layout (they had an extra luminance pixel) I assume the cameras do some preprocessing. As for the MagicLantern hack that one should indeed be the same coordinates. If I understand their method correctly they're doing two readouts of the exact same sensor exposure with two different ADC gains.

@LibRaw
Copy link

LibRaw commented Sep 20, 2014

Just compare images from two (sub-sensors) with very known subject (such as resolution target). On fuji S2-S5Pro files, two images are shifted by 'sub-pixel' and this is clearly visible. Have not inspected F700 and other compacts.

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

Successfully merging this pull request may close these issues.

4 participants