pedrocr / rawloader

rust library to extract the raw data and some metadata from digital camera images
GNU Lesser General Public License v2.1
304 stars 53 forks source link

Exposed masked black areas on RawImage #17

Closed powturns closed 5 years ago

powturns commented 5 years ago

Fixes #16

If you get a chance please verify I understood the DNG spec and the raf decoder.

pedrocr commented 5 years ago

Thanks for the PR. I think you're doing two things which are inconsistent:

So I suggest two things:

powturns commented 5 years ago

For DNGs you're grabbing the masked areas from the metadata. That may not be safe if the masked areas are not all actually black areas

From the spec:

MaskedAreas ... contains a list of non-overlapping rectangle coordinates of fully masked pixels, which can be optionally used by DNG readers to measure the black encoding level.

Is the concern that the metadata contains bad data, or that the specified masked areas actually receive a tiny amount of light? How would one go about validating that the MaskedAreas are actually usable? My only idea is to perform stat analysis on the image and ensure the specified MaskedAreas are indeed the darkest part of the image. Presumably, couldn't a DNG be a crop of the image that has the MaskedAreas removed, meaning it isn't safe to use the blackarea from the make/model metadata?

If the raw image data has already had its black encoding level subtracted, then this tag should not be used, since the masked pixels are no longer useful. Note that DNG writers are still required to include estimate and store the black encoding level using the black level DNG tags

I read this as the BlackLevel field being the authority that should be used normal situations if present. Is that a correct interpretiation?

... for other formats (CRW really) you are not exposing the black areas that are there and being used for blackpoint

Aren't the blackpoints for CRW being calculated in RawImage::new which uses the same camera.blackarea* values that are used to store RawImage.blackarea?

Thanks for your patience, this is all new to me!

pedrocr commented 5 years ago

Is the concern that the metadata contains bad data, or that the specified masked areas actually receive a tiny amount of light?

Bad data is the usual concern because even when there is a spec the actual implementations are all over the place. But DNG is usually fairly sanely implemented and this is a minor enough tag that those that do fill it probably did it properly. If it works in the actual files you have then that's fine.

Presumably, couldn't a DNG be a crop of the image that has the MaskedAreas removed, meaning it isn't safe to use the blackarea from the make/model metadata?

Definitely, which is why when doing quirks for DNG what's actually done is to set values for "files from Make Y Model X, when in DNG do Z".

I read this as the BlackLevel field being the authority that should be used normal situations if present. Is that a correct interpretiation?

I don't read it like that. What is says is if the DNG has had the actual pixel data altered with the blacklevel calculated from the black areas then adding the black areas doesn't make sense. But if that wasn't done maybe the black level tag is best or the calculation from the black areas. Again I'd use whatever makes sense for the specific files that actually use this. If others come along that break quirks or more complex behaviors will be needed.

Aren't the blackpoints for CRW being calculated in RawImage::new which uses the same camera.blackarea* values that are used to store RawImage.blackarea?

I read that code wrong, you're actually exposing everything.

Thanks for your patience, this is all new to me!

No problem whatsoever, let me know what you need. You may want to look at imagepipe for code to actually process the files:

https://github.com/pedrocr/imagepipe/

I'd be more than happy to that a PR there to automatically process these files early in the pipeline so then everything else is the same.

pedrocr commented 5 years ago

Looks good to me, thanks!