darktable-org / darktable

darktable is an open source photography workflow application and raw developer
https://www.darktable.org
GNU General Public License v3.0
9.25k stars 1.11k forks source link

GoPro HERO10 Black Raw file has no white balance #10072

Closed supertobi closed 1 year ago

supertobi commented 2 years ago

To Reproduce

  1. open the attached file (I've added the file to raw.pixls.us too) GOPR0119.zip
  2. unzipp it
  3. open it in dt
  4. see error

Expected behavior less pink

Screenshots grafik

A bisect is much appreciated and can significantly simplify the developer's job. HowTo: https://github.com/darktable-org/darktable/wiki#finding-bug-causes and https://www.youtube.com/watch?v=D7JJnLFOn4A

Platform

kmilos commented 2 years ago

There might be something fishy going on with the HERO10 CFAPattern, BlackLevel, WhiteLevel, and AsShotNeutral tags

Exif.Image.CFAPattern                         1 2 0 1
Exif.Image.BlackLevel                         51200/256 51200/256 51200/256 51200/256
Exif.Image.WhiteLevel                         4095
Exif.Image.AsShotNeutral                      0/1000000 0/1000000 0/1000000

when compared to e.g. a HERO9 sample

Exif.Image.CFAPattern                         0 1 1 2
Exif.Image.BlackLevel                         0/256 0/256 0/256 0/256
Exif.Image.WhiteLevel                         16383
Exif.Image.AsShotNeutral                      487619/1000000 1000000/1000000 543524/1000000

Sensor is supposedly the same going by the reviews and press releases so I would expect the same CFAPattern, BlackLevel, and WhiteLevel....

WhiteLevel is furthermore used as a parameter for rawspeed's VC5 decompressor so that one seems extra important...

0,0,0 white balance seems just plain wrong...

Does it open ok in any other apps?

supertobi commented 2 years ago

Does it open ok in any other apps?

I've just tested RawTherapee, but RawTherapee doesn't support GPR files.

Adobe DNG Converter is able to convert the file and creates this DNG: GOPR0119.dng.zip

grafik

I haven't tested the free GPR converter from GoPro: https://github.com/gopro/gpr

kmilos commented 2 years ago

Interesting, same CFAPattern, BlackLevel, and WhiteLevel are preserved in the DNG and it indeed works... Both files complain about missing white balance information and seem to fall back to some D65 default, so don't think that one was crucial...

It is possible then that there is indeed something wrong going in in the rawspeed decoder/decompressor... There's been some changes around VC5 as I've noticed recently, do you mind trying out the current development build?

kmilos commented 2 years ago

I tested 3.7.0+1122_g362a952f6, no change...

At this point I think it's best to close this issue, and open a formal camera support one (link to this one in there though).

supertobi commented 2 years ago

I think we don't need a new issue. Adding the tag "scope: camera support" and assigning it to @LebedevRI does the trick.

LebedevRI commented 2 years ago

Can you contribute a sample to RPU that has been directly copied from camera and not modified in any way?

LebedevRI commented 2 years ago

I guess let's wait on https://github.com/gopro/gpr/issues/33 first?

supertobi commented 2 years ago

@LebedevRI

  1. The file I added here is directly copied from the camera and not modified in any way and I already uploaded it to RPU.
  2. To be honest, I don't expect any reaction on gopro/gpr#33
LebedevRI commented 2 years ago
  • The file I added here is directly copied from the camera and not modified in any way and I already uploaded it to RPU.

I see. It looked like it had some windows filepaths, but then the other files also have that. Honestly this bug is rather sad, because i'm not quite sure which particular wording in DNG spec says that we should detect that WB (all-zeros) as invalid and use camera-neutral one instead.

kmilos commented 2 years ago

I don't think it's a white balance issue - in both the GPR and converted DNG case the field is invalid and dt gives up and defaults to something sane (D65). It looks more like a decoding issue to me: no matter how you change the black & white levels in dt and white balance, the image looks pink/purple...

Here's the histogram of the GPR once I set the black level to 0 and white level to 16384 (white balance is at 6504K):

image

Here's the histogram of the Adobe's VC5 decode -> JPEG reencode from the DNG (same black/white and WB settings):

image

It's as if like the color planes are swapped almost, but the decoded values are also somehow strange (esp. for the green plane)...

For the HERO9 the histograms of the GPR and the converted DNG look identical w/ equivalent dt settings.

supertobi commented 2 years ago

Perhaps its a different pattern of the sensor, have a look at the exif information:

HERO10

Exif.Image.CFAPattern 1 2 0 1

when compared to a HERO9 sample

Exif.Image.CFAPattern 0 1 1 2

LebedevRI commented 2 years ago

Yeah, it's pretty obvious wrong, just need to unbreak the compiler first..

kmilos commented 2 years ago

It seems more than that to me, the decoded values are really strange even if you swap the channels around - lots of them come out at lower than the supposed black level of 200 in that "green" channel... But I don't know it that's just the side effect of the wrong table order being used @LebedevRI just pointed to, or this has to be accounted for at all wavelet levels and not just the base one...

Going by another gpr_tools issue comment, it looks like rggb12, rggb12p, [rggb14], gbrg12, gbrg12p inputs are to be expected, and a simple workaround of a top/bottom flip (gbrg->rggb) of the input and output after decode is an option...

Edit: I take it back, it looks like it might be as simple as outputting channels in the correct CFA order as input has been already been reassigned to correct wavelet planes during encoding... The strange reconstructed R/G/B histogram values above are a side effect of the demosaic method - there are no values below the 200 black level in the "passthrough" demosaic mode.

kmilos commented 2 years ago

Confirmed that indeed

        out(2 * row + 0, 2 * col + 0) = static_cast<uint16_t>(mVC5LogTable[g1]);
        out(2 * row + 0, 2 * col + 1) = static_cast<uint16_t>(mVC5LogTable[b]);
        out(2 * row + 1, 2 * col + 0) = static_cast<uint16_t>(mVC5LogTable[r]);
        out(2 * row + 1, 2 * col + 1) = static_cast<uint16_t>(mVC5LogTable[g2]);

resolves the issue. I used something like bool rggb = mRaw->cfa.getDcrawFilter() == 0x94949494; as the condition...

LebedevRI commented 2 years ago

Heh.

github-actions[bot] commented 2 years ago

This issue did not get any activity in the past 60 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

supertobi commented 2 years ago

Yeah, it's pretty obvious wrong, just need to unbreak the compiler first..

How is your compiler doing?

arbasel commented 2 years ago

Hi, I see that this issue is still happening. Is there some way for me to fix it?

Thanks

supertobi commented 2 years ago

@arbasel the easiest you can do is to use @kmilos patch. It's not elegant but works. Or provide a better patch, that satisfies @LebedevRI .

github-actions[bot] commented 2 years ago

This issue did not get any activity in the past 60 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

ghost commented 2 years ago

@arbasel the easiest you can do is to use @kmilos patch. It's not elegant but works. Or provide a better patch, that satisfies @LebedevRI .

How do I use @kmilos patch? Is that something I can plug-in to the current version of the app or is that something that requires recompiling?

supertobi commented 2 years ago

It requires recompiling. And I think it should be merged, even if it is ugly. It works and is better then the something that doesn't wok.

LebedevRI commented 1 year ago

FWIW, the other problem (at least on the HERO10 samples available on RPU), is that they simply lack white balance in EXIF.

kmilos commented 1 year ago

I think this can be closed now as Roman merged a proper fix for 4.2? @dterrahe @LebedevRI

LebedevRI commented 1 year ago

Technically, no. HERO10 indeed do not have camera white balance, at least the samples on RPU.