darktable-org / rawspeed Goto Github PK

Suggestion: don't clutter the compile log with 'shadow' warnings. Due to increased inlining in my latest PR this has become more of a 'problem'. This log file here is hardly readable: https://ci.appveyor.com/project/LebedevRI/rawspeed/build/111/job/uwbodr6y85ewx0y6.

And 'fixing' things like:

struct A {
  int i;
  A(int i) : i(i) {}
}

by introducing ugly new names like _i is something I really don't want to do. The above code is perfectly valid c++.

klauspost/rawspeed#166

I added that api in klauspost/rawspeed@a45724e and klauspost/rawspeed@e1b5cca.
And now i have just discovered how it backfires.
With raw file from https://redmine.darktable.org/issues/11354 from Canon EOS Rebel T6, which currently has broken alias, it matches to Canon EOS Rebel T6i.
That happens because internally "make model mode" is a string, https://github.com/klauspost/rawspeed/blob/a45724ec3c201b67f5ff732317921c0feca878c6/RawSpeed/CameraMetaData.h#L37

At least some cameras, when the "White balance correction" is enabled, bake white balance into the raw data. So far i'm aware than Canon 6D does that. In such a case, the white balance info is still present.

[MakerNotes]    0x040a ViewfinderWarnings              : Monochrome, WB corrected

(from exiftool)
So we would have to detect that "WB corrected" flag is set, and then either say that wb is (1.0f, 1.0f, 1.0f), or, for consistency, un-apply WB...

Thank you, Canon!

While just teaching rs to load it is as simple as adding to LJpegDecompressor::decodeScan()

  case 1:
    decodeN<1>();
    break;

there is something off about it.

Such file can be acquired by ADC'ing the DSCF0151.RAF, a compressed sample from X100F.
rawspeed loads it garbled:

If i load that DNG in hdrmerge, which uses libraw, and then save as DNG, that new DNG loads correctly,

However, that does not happen for DSCF0150.RAF, an uncompressed sample. It appears to be fine.
This is not a regression, rawspeed did not support such 1-component raws before.

https://www.dropbox.com/s/7ouvwl93yl6494r/DSCF0151.DNG?dl=0

I have 3 issues with the custom types specified here: https://github.com/darktable-org/rawspeed/blob/develop/src/librawspeed/common/Common.h#L35-L41

1. Their naming contains the size info twice, e.g. ushort16 and the names (strings) are not equally long.

I like to suggest two alternative variants:

• Rust names (u32, i16, etc.)
• <cstdint>: http://en.cppreference.com/w/cpp/types/integer

I'd prefer the short Rust names, especially because the std names are longer than what we have right now and the _t postfix looks very 'c-ish' to me. I'd also suggest to keep the word byte and even use that term as a type name for the unsigned char case. That name even almost made it into c++17. The idea is to distinguish between simple data bytes and characters (as in bits of a string).

2. The usage of the uint32 for offets/sizes is 'non-standard'

I'd like to go over the interfaces that I touched and simply use size_t for that, even in the cases where I introduced the size_type name that can be found in the std:: containers. I always found that part a bit 'over engineered'.

3. Their definition is potentially wrong (depending on the data model, see http://en.cppreference.com/w/cpp/language/types). E.g. the tiff code breaks if 'unsigned int' would be 16bits wide. This should be non-controversial and is trivial to fix.

@LebedevRI What is your opinion on 1) and 2)?

• #127
• #128
• #129

@axxel
hi.
0. i'm deeply saddened by the amount of sanity checks / input data validation in your code :(
1.

$ rstest -c "raw.pixls.us/Nikon/1 J5/20151027-10431071-NIKON_1_J5.nef"
20151027-10431071-NIKON_1_J5.nef                       : starting decoding ...                                                                                                                                                                                                 
20151027-10431071-NIKON_1_J5.nef failed: ../src/librawspeed/decompressors/HuffmanTable.h:176: void rawspeed::HuffmanTable::setCodeValues(const rawspeed::Buffer &): Corrupt Huffman. Code value 57 is bigger than 16                                                           
Total decoding time: 0s                                                                                                                                                                                                                                                        
                                                                                                                                                                                                                                                                               
WARNING: the following 1 tests have failed:                                                                                                                                                                                                                                    
20151027-10431071-NIKON_1_J5.nef failed: ../src/librawspeed/decompressors/HuffmanTable.h:176: void rawspeed::HuffmanTable::setCodeValues(const rawspeed::Buffer &): Corrupt Huffman. Code value 57 is bigger than 16                                                           
See rstest.log for details.

Though, until the production camera is actually released, and the full set of freely-licensed samples is contributed to the https://raw.pixls.us/, it won't be merged.

I started introducing c++11 default member initializations a while back. I used the 'old' syntax:

int x = 0;

I noticed @LebedevRI also added some new code but chose the 'new' uniform initialization syntax:

int x{0};

I personally prefer the first because it is easier for my eye to parse it. I can also type it way faster. It's a matter of style. I'd like to use it consistently across the code base, but then ideally not only for default member initialization inside a class but like it was invented: uniformly. This would also mean using it for any other kind of object initialization:

   ByteStream bs{t->getData()};
   TiffIFD* p{parent};
   return new DngDecoder{move(root), mInput};

Disclamer: I dislike those 3 lines so much, that I'd still rather use it only in class definitions and not everywhere. My point being: can we just agree on the type var = value; version. Please? :)

See https://redmine.darktable.org/issues/10078.

Example image: https://raw.pixls.us/getfile.php/683/raw/RAW_CANON_POWERSHOT_SX130IS_CHDK.DNG

I stumbled on this issues while cleaning up the DngOpcode classes. This has almost certainly never worked (I found 2 separate issues).

I have a complete rewrite of the DngOpcode classes, fixing this issue (among others). Just waiting for #75 to be merged.

Affected samples: (are currently not supported (decode to garbage).)

• Olympus/E-M5 Mark II/M0082437.ORF (multishot)
• Olympus/SH-2/PC260009.ORF

jpeg_read_header() returns JPEG_SUSPENDED i believe

Here is what I came up with after playing a bit with the options:

# strongly requested
ColumnLimit: 100 # at least 100
BreakBeforeBraces: Linux # break before braces on function/namespace/class

# nice to have
KeepEmptyLinesAtTheStartOfBlocks: true # at least if BreakBeforeBraces is set to default
MaxEmptyLinesToKeep: 2

# maybe nice to have
#BreakConstructorInitializersBeforeComma: true
#AllowShortIfStatementsOnASingleLine: false
#AllowShortFunctionsOnASingleLine: false
#AllowShortBlocksOnASingleLine: false

Just completely broken. d4be7f4 4ebeddc
Should be rewritten (with tests!) cleanly, to something much like the Spline (1, 2)

Please add support for the new Canon EOS M6. I have one of these so am able to provide any data / sample images as needed. Just let me know.

https://github.com/darktable-org/rawspeed/pull/41/files#r97982370

==================
WARNING: ThreadSanitizer: data race (pid=28023)
  Write of size 4 at 0x7d540006f4bc by thread T44:
    #0 void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:179 (rstest+0x00000048c68a)
    #1 RawSpeed::LJpegPlain::decodeScan() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:115 (rstest+0x0000004884cc)
    #2 RawSpeed::LJpegDecompressor::parseSOS() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegDecompressor.cpp:163 (rstest+0x0000004855ff)
    #3 RawSpeed::LJpegDecompressor::decode(unsigned int, unsigned int, unsigned int, unsigned int) /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegDecompressor.cpp:88 (rstest+0x00000048718c)
    #4 RawSpeed::DngDecoderSlices::decodeSlice(RawSpeed::DngDecoderThread*) /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:390 (rstest+0x00000045daff)
    #5 RawSpeed::DecodeThread(void*) /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:61 (rstest+0x00000045e6d4)
    #6 <null> <null> (libtsan.so.0+0x000000024459)

  Previous write of size 4 at 0x7d540006f4bc by thread T53:
    #0 void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:179 (rstest+0x00000048c68a)
    #1 RawSpeed::LJpegPlain::decodeScan() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:115 (rstest+0x0000004884cc)
    #2 RawSpeed::LJpegDecompressor::parseSOS() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegDecompressor.cpp:163 (rstest+0x0000004855ff)
    #3 RawSpeed::LJpegDecompressor::decode(unsigned int, unsigned int, unsigned int, unsigned int) /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegDecompressor.cpp:88 (rstest+0x00000048718c)
    #4 RawSpeed::DngDecoderSlices::decodeSlice(RawSpeed::DngDecoderThread*) /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:390 (rstest+0x00000045daff)
    #5 RawSpeed::DecodeThread(void*) /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:61 (rstest+0x00000045e6d4)
    #6 <null> <null> (libtsan.so.0+0x000000024459)

  Location is heap block of size 600 at 0x7d540006f400 allocated by thread T7:
    #0 operator new(unsigned long) <null> (libtsan.so.0+0x000000065af3)
    #1 RawSpeed::RawImage::create(RawSpeed::RawImageType) /home/lebedevri/rawspeed/src/librawspeed/common/RawImage.h:244 (rstest+0x000000459cae)
    #2 RawSpeed::DngDecoder::decodeRawInternal() /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoder.cpp:111 (rstest+0x000000459cae)
    #3 RawSpeed::RawDecoder::decodeRaw() /home/lebedevri/rawspeed/src/librawspeed/decoders/RawDecoder.cpp:723 (rstest+0x000000426345)
    #4 process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, RawSpeed::CameraMetaData*, bool, bool) /home/lebedevri/rawspeed/src/utilities/rstest/rstest.cpp:207 (rstest+0x00000041accb)
    #5 main._omp_fn.0 /home/lebedevri/rawspeed/src/utilities/rstest/rstest.cpp:295 (rstest+0x00000041be0e)
    #6 <null> <null> (libgomp.so.1+0x0000000166c5)

  Thread T44 (tid=30247, running) created by thread T7 at:
    #0 pthread_create <null> (libtsan.so.0+0x000000027e9d)
    #1 RawSpeed::DngDecoderSlices::startDecoding() /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:115 (rstest+0x00000045f244)
    #2 RawSpeed::DngDecoder::decodeRawInternal() /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoder.cpp:324 (rstest+0x00000045b046)
    #3 RawSpeed::RawDecoder::decodeRaw() /home/lebedevri/rawspeed/src/librawspeed/decoders/RawDecoder.cpp:723 (rstest+0x000000426345)
    #4 process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, RawSpeed::CameraMetaData*, bool, bool) /home/lebedevri/rawspeed/src/utilities/rstest/rstest.cpp:207 (rstest+0x00000041accb)
    #5 main._omp_fn.0 /home/lebedevri/rawspeed/src/utilities/rstest/rstest.cpp:295 (rstest+0x00000041be0e)
    #6 <null> <null> (libgomp.so.1+0x0000000166c5)

  Thread T53 (tid=30246, running) created by thread T7 at:
    #0 pthread_create <null> (libtsan.so.0+0x000000027e9d)
    #1 RawSpeed::DngDecoderSlices::startDecoding() /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:115 (rstest+0x00000045f244)
    #2 RawSpeed::DngDecoder::decodeRawInternal() /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoder.cpp:324 (rstest+0x00000045b046)
    #3 RawSpeed::RawDecoder::decodeRaw() /home/lebedevri/rawspeed/src/librawspeed/decoders/RawDecoder.cpp:723 (rstest+0x000000426345)
    #4 process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, RawSpeed::CameraMetaData*, bool, bool) /home/lebedevri/rawspeed/src/utilities/rstest/rstest.cpp:207 (rstest+0x00000041accb)
    #5 main._omp_fn.0 /home/lebedevri/rawspeed/src/utilities/rstest/rstest.cpp:295 (rstest+0x00000041be0e)
    #6 <null> <null> (libgomp.so.1+0x0000000166c5)

  Thread T7 (tid=28031, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x000000027e9d)
    #1 <null> <null> (libgomp.so.1+0x000000016c6f)
    #2 __libc_start_main <null> (libc.so.6+0x0000000202b0)

SUMMARY: ThreadSanitizer: data race /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:179 in void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>()
==================

The pthreads based multi-threading code should be improved (IMHO). I see two options:

1. keep the internal 'explicit' interface more or less like it is but switch from pthreads to c++11 threads.
2. remove the 'explicit' interface, which should simplify the code significantly and switch to OpenMP where applicable.

I would strongly prefer the second option as is would reduce the complexity of the code and thereby make it a lot easier or even make it possible for the first time to add it where it currently is not. E.g. CR2 files could potentially be decoded in parallel when they are composed of multiple strips. I have not looked into this properly, just wanted to bring up the idea and get feedback from others.

If I see it correctly darktable is also embracing the OpenMP methodology. With recent Apple/Clang compilers supporting it as well platform support should be sufficient.

I converted a GoPro .gpr image with Adobe DNG Converter 9.8 to a .dng file. now I get this message from darktable:

RawSpeed:Unable to find camera in database: 'GoPro' 'HERO5 Black' 'dng'
Please consider providing samples on https://raw.pixls.us/, thanks!

As raw.pixls.us is not interested in DNG files converted from Adobe DNG Converter, aren't you able to not show this message, if the DNGs are from the Adobe DNG Converter?

@shoffmeister brought up the question of whether the habit of quoting class/function names in error strings adds value (#61 (comment)). There is a (inconsistently used) pattern that looks like this:

  if (frame.cps != soscps)
    ThrowRDE("LJpegDecompressor::parseSOS: Component number mismatch.");

This pattern has two issues:

• manually typed class/function name
• there are different ThrowXXX options available that have questionable value, expecially as they lead to other code patterns like the following:

  try {
    return checkSupportInternal(meta);
  } catch (TiffParserException &e) {
    ThrowRDE("%s", e.what());
  } catch (FileIOException &e) {
    ThrowRDE("%s", e.what());
  } catch (IOException &e) {
    ThrowRDE("%s", e.what());
  }

And that is both verbose and error prone. Both the set of exception classes as well as the way they are used could be improved.

First question: what exception types are useful?

From a library users point of view, I see only two 'types' of problems:

1. unsupported file: he can't do anything about it, except maybe contact the developers and provide a sample
2. corrupted file (probably truncated, maybe otherwise 'randomly' broken): he can try to copy it again from his camera, there is no point in contacting the developers.

Unfortunately, the two cases can not reliably be distinguished as long as the library does not implement each and every spec completely (e.g. only one LJpeg predictor mode is currently implemented, so we bail out if predictorMode != 1. We can not say for sure whether that is a valid file with another predictor or a broken file with the right predictor). Even with only 95% accuracy, I'd say it would still be valuable to make that distinction.

From a developers point of view, we might need more diverse types internally, that would have to be determined by inspecting the different catch() sites and see what they actually make of the type. I'd expect to find quite a few inconsistent uses / bugs there.

Second question: what error messages are useful?

From a users point of view, there really is only the question of unsupported vs. corrupted. Information like Component number mismatch provides no value to him at all.

From a develops point of view, I see the need to identify the exact location in the source code if something goes wrong (the function name is only half way).

Before I continue this with some concrete suggestions about how we can better achieve the above goals, I'll wait for some feedback on this 'analysis'.

Working on a simplification of the TrimSpaces function, it seems I fixed a bug by accident. The two mentioned sony models get a new hash. But interestingly the new values are exactly those from the cameras.xml. So they must have been wrong before?!?

@LebedevRI Question is: which are the correct ones?

--- test/Sony_DSLR-A850_0519.arw.hash	2017-01-29 16:25:17.249804551 +0100
+++ test/Sony_DSLR-A850_0519.arw.hash.failed	2017-01-29 17:47:09.757193209 +0100
@@ -9,2 +9,2 @@
-blackLevel: 125
-whitePoint: 3750
+blackLevel: 500
+whitePoint: 15000
@@ -21 +21 @@
-dimUncropped: 6080x4048
+dimUncropped: 6080x4056
@@ -29 +29 @@
-data md5sum: 1919026cd90ace97aa66743adfd9c55a
+data md5sum: 5810840e9c542b9b51ffe9222e7ff5da
--- test/Sony_DSLR-A900_0520.arw.hash	2017-01-29 16:25:20.481738480 +0100
+++ test/Sony_DSLR-A900_0520.arw.hash.failed	2017-01-29 17:47:09.725193869 +0100
@@ -9,2 +9,2 @@
-blackLevel: 130
-whitePoint: 4095
+blackLevel: 520
+whitePoint: 16383
@@ -21 +21 @@
-dimUncropped: 6080x4048
+dimUncropped: 6080x4056
@@ -29 +29 @@
-data md5sum: 3c2b50754a57bc57388fecc06d38ef20
+data md5sum: 0b3860cd1ad251c38e80a0cf6c1ab811

The filenames are local renames but the make/model are enough to identify them.

These are shots on the color target of X-Rite ColorChecker Passport:
http://www.weldpass.com/upload/Fujifilm-DBP-for-GX680-daylight.RAF
http://www.weldpass.com/upload/Fujifilm-DBP-for-GX680-tungsten.RAF

I wonder if rawspeed will be able to support it so that I can process it with Darktable.

Thanks.

Hi,

I use the commands as below to build rawspeed but getting some errors.
Is that correct rule to build? or something wrong with codes?

thank you... :)

mkdir build; cd build;
cmake .. -DUSE_BUNDLED_PUGIXML=ON -DALLOW_DOWNLOADING_GOOGLETEST=ON
make

[ 20%] Building CXX object src/librawspeed/CMakeFiles/rawspeed.dir/decompressors/FujiDecompressor.cpp.o
/home/jeff/rawspeed/src/librawspeed/decompressors/FujiDecompressor.cpp: In instantiation of ‘rawspeed::FujiDecompressor::xtrans_decode_block(rawspeed::FujiDecompressor::fuji_compressed_block*, rawspeed::BitPumpMSB*, int) const::<lambda(auto:1&&, rawspeed::FujiDecompressor::_xt_lines, rawspeed::FujiDecompressor::_xt_lines, int, rawspeed::FujiDecompressor::xtrans_decode_block(rawspeed::FujiDecompressor::fuji_compressed_block*, rawspeed::BitPumpMSB*, int) const::ColorPos&, rawspeed::FujiDecompressor::xtrans_decode_block(rawspeed::FujiDecompressor::fuji_compressed_block*, rawspeed::BitPumpMSB*, int) const::ColorPos&)> [with auto:1 = rawspeed::FujiDecompressor::xtrans_decode_block(rawspeed::FujiDecompressor::fuji_compressed_block*, rawspeed::BitPumpMSB*, int) const::<lambda(rawspeed::FujiDecompressor::_xt_lines, rawspeed::FujiDecompressor::_xt_lines, int, rawspeed::FujiDecompressor::xtrans_decode_block(rawspeed::FujiDecompressor::fuji_compressed_block*, rawspeed::BitPumpMSB*, int) const::ColorPos&, rawspeed::FujiDecompressor::xtrans_decode_block(rawspeed::FujiDecompressor::fuji_compressed_block*, rawspeed::BitPumpMSB*, int) const::ColorPos&)>]’:
/home/jeff/rawspeed/src/librawspeed/decompressors/FujiDecompressor.cpp:485:24:   required from here
/home/jeff/rawspeed/src/librawspeed/decompressors/FujiDecompressor.cpp:464:41: error: ‘line_width’ is not captured
     while (g.even < line_width || g.odd < line_width) {
                                         ^
/home/jeff/rawspeed/src/librawspeed/decompressors/FujiDecompressor.cpp:464:19: note: ‘const int& line_width’ declared here
     while (g.even < line_width || g.odd < line_width) {

The raw.pixls.us/Canon/EOS 5D Mark IV/B13A0800.CR2 sample.
Image is all right, but the wb provided by rawspeed is wrong.
If i manually select (1,1,1) as wb, image looks right.

CFA2 is not, thankfully.

So e.g. if we have:

	<Camera make="Panasonic" model = "DMC-FZ28">
		<ID make="Panasonic" model="DMC-FZ28">Panasonic DMC-FZ28</ID>
		<CFA width="2" height="2">
			<Color x="0" y="0">BLUE</Color>
			<Color x="1" y="0">GREEN</Color>
			<Color x="0" y="1">GREEN</Color>
			<Color x="1" y="1">RED</Color>
		</CFA>
		<Crop x="0" y="0" width="-56" height="-4"/>
		<Sensor black="15" white="3986"/>
	</Camera>

If we change crop x/y by a multiple of two, everything is fine.
But if we change it by just one, we also need to adjust <CFA>

I'm not sure if this technically counts as a regression because there aren't any Credo 40 RAW files in the test set (I added one to raw.pixls.us a few hours ago, I'll attach one to this issue as well), but I believe that commit 65cc3c5 broke reading of IIQ files from the Leaf Credo 40 digital back, and I'm guessing all other Credo backs as well. I think the problem is that now that MOS and IIQ reading have been split into separate classes, you're using the manufacturer value to decide which decoder to use, "Leaf" gets the MOS decoder and "Phase One A/S" gets the IIQ decoder.

The problem is that Leaf backs used to use MOS files, but the new Leaf Credo backs use IIQ files. So I'm assuming the fix to this would be to look at the model value as well as the camera value when making this determination. Presumably all future Leaf backs will be using IIQ files as well, so at least the set of backs you'll have to divert to the MOS loader shouldn't ever expand. Let me know if y'all have time to address this soon-ish, if not I think this might be something I can write a decent patch for

24-robinson_preserve.zip

https://www.mail-archive.com/[email protected]/msg01806.html

Just so it does not get lost
Copied from https://redmine.darktable.org/issues/11466

The .GPR format is an extension Adobe’s .DNG format, adding enhanced compression (VC5 Standard)

A sample file:
https://raw.pixls.us/getfile.php/936/raw/GOPR0020.GPR

FAQ from GoPro about the file format:
https://gopro.com/help/articles/Question_Answer/What-is-RAW-Format

An interesting blogpost about the "VC5 Standard" 
https://medium.com/@kierank_/reverse-engineering-the-gopro-cineform-codec-7411312bfe1c#.9f7ztpjo8

On my local darktable installation

/usr/share/darktable/rawspeed$ grep "Hint " * | sort

shows that none of the cameras defined in cameras.xml implements the hint "zero_is_not_bad".

In rawspeed implementation, zero_is_not_bad is only used in the context of RW2 processing. It is always used in such a fashion that zero_is_bad = !zero_is_not_bad.

This implies that for all RW2 formats, zero_is_bad == true, always.

Why is there implementation for such an implied metadata hint if there is nothing that exploits that hint?

Given the current implementation, it would appear to be

• safe (and prudent) to remove handling of the unused zero_is_not_bad metadata hint
• necessary (still) to review how exactly to handle zero values in raw data in the V5 format

I'd like to suggest to remove the codecov tests from the set of tests that take part in the final "sucess or failure" conclusion of the ci system. E.g. the failing of #27 makes no sense to me. I see the value those tests have on a code base with full or nearly full test coverage but right now they just hide the really useful information the other two tests provide.

Some raws appear not to have padding at the end, so when we do bs.fill(32);, we fail.
Changing

-bs.fill(32);
+bs.fill(maxCodePlusDiffLength());

seems to help somewhat, but i don't know the full solution yet.

I converted a GoPro .gpr image with Adobe DNG Converter 9.8 to a .dng file. The files are upside down in darktable 2.2.1.

https://filebin.net/7h8d4s8vmw6z7gtn/GOPR0020.dng
As github doesn't support uploading dng files I used filebin. The files will expire automatically 2 months from now. Please ping me if it is expired and you need it again.

Using Apple LLVM version 6.1.0, the compilation of

rawspeed/src/fuzzers/HuffmanTable/main.cpp

stops with an error like

..../rawspeed/src/fuzzers/HuffmanTable/main.cpp:84:3: error: code will never be
  executed [-Werror,-Wunreachable-code]
  __builtin_unreachable();

while I guess it should simply proceed with the compilation... can this line be safely commented for not-so-recent versions of clang? Or is there another solution that does not involve upgrading the compiler?

Thanks!

@axxel

When working towards minimizing hard-require dependencies for msvs ci build, i noticed that there is no lossy jpeg samples. I used ADC to produce all the variants of the compression it can, and the lossless compression is broken in current git develop branch.

the sample is 5G4A9394-compressed-lossless.DNG

=================================================================
==12936==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fee514db780 at pc 0x0000006d1e11 bp 0x7fee4e83d490 sp 0x7fee4e83d488
WRITE of size 2 at 0x7fee514db780 thread T1
    #0 0x6d1e10 in void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>()::{lambda(int)#2}::operator()(int) const /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:208:21
    #1 0x6d1c1f in RawSpeed::unroll_loop_t<void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>()::{lambda(int)#2}, 1ul>::repeat({lambda(int)#2} const&) /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:109:5
    #2 0x6d1be4 in RawSpeed::unroll_loop_t<void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>()::{lambda(int)#2}, 2ul>::repeat({lambda(int)#2} const&) /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:108:5
    #3 0x6d1a04 in void RawSpeed::unroll_loop<2ul, void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>()::{lambda(int)#2}>(void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>()::{lambda(int)#2} const&) /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:120:3
    #4 0x6cde59 in void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:207:11
    #5 0x6c90e1 in RawSpeed::LJpegPlain::decodeScan() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:90:7
    #6 0x6b9f10 in RawSpeed::LJpegDecompressor::parseSOS() /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegDecompressor.cpp:131:3
    #7 0x6b8665 in RawSpeed::LJpegDecompressor::decode(unsigned int, unsigned int) /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegDecompressor.cpp:53:18
    #8 0x5f189a in RawSpeed::DngDecoderSlices::decodeSlice(RawSpeed::DngDecoderThread*) /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:424:11
    #9 0x5f02e7 in RawSpeed::DecodeThread(void*) /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:64:13
    #10 0x7fee5689a423 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x7423)
    #11 0x7fee559239be in clone (/lib/x86_64-linux-gnu/libc.so.6+0xe89be)

0x7fee514db780 is located 0 bytes to the right of 46768000-byte region [0x7fee4e841800,0x7fee514db780)
allocated by thread T0 here:
    #0 0x542eb0 in posix_memalign (/home/lebedevri/rawspeed/build/src/utilities/identify/darktable-rs-identify+0x542eb0)
    #1 0x65bc69 in _aligned_malloc(unsigned long, unsigned long) /home/lebedevri/rawspeed/src/librawspeed/common/Common.cpp:82:12
    #2 0x58623a in RawSpeed::RawImageData::createData() /home/lebedevri/rawspeed/src/librawspeed/common/RawImage.cpp:92:20
    #3 0x5de5df in RawSpeed::DngDecoder::decodeData(RawSpeed::TiffIFD*) /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoder.cpp:149:9
    #4 0x5e0b61 in RawSpeed::DngDecoder::decodeRawInternal() /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoder.cpp:300:7
    #5 0x631094 in RawSpeed::RawDecoder::decodeRaw() /home/lebedevri/rawspeed/src/librawspeed/decoders/RawDecoder.cpp:304:20
    #6 0x580add in main /home/lebedevri/rawspeed/src/utilities/identify/rawspeed-identify.cpp:167:8
    #7 0x7fee5585b2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

Thread T1 created by T0 here:
    #0 0x4a505d in pthread_create (/home/lebedevri/rawspeed/build/src/utilities/identify/darktable-rs-identify+0x4a505d)
    #1 0x5f3bd3 in RawSpeed::DngDecoderSlices::startDecoding() /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoderSlices.cpp:118:5
    #2 0x5df98b in RawSpeed::DngDecoder::decodeData(RawSpeed::TiffIFD*) /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoder.cpp:226:10
    #3 0x5e0b61 in RawSpeed::DngDecoder::decodeRawInternal() /home/lebedevri/rawspeed/src/librawspeed/decoders/DngDecoder.cpp:300:7
    #4 0x631094 in RawSpeed::RawDecoder::decodeRaw() /home/lebedevri/rawspeed/src/librawspeed/decoders/RawDecoder.cpp:304:20
    #5 0x580add in main /home/lebedevri/rawspeed/src/utilities/identify/rawspeed-identify.cpp:167:8
    #6 0x7fee5585b2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/lebedevri/rawspeed/src/librawspeed/decompressors/LJpegPlain.cpp:208:21 in void RawSpeed::LJpegPlain::decodeN_X_Y<2, 1, 1>()::{lambda(int)#2}::operator()(int) const
Shadow bytes around the buggy address:
  0x0ffe4a2936a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffe4a2936b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffe4a2936c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffe4a2936d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffe4a2936e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ffe4a2936f0:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffe4a293700: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffe4a293710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffe4a293720: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffe4a293730: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ffe4a293740: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==12936==ABORTING

$ darktable-rs-identify Pictures/IMG_3030.CR2
Loading file: "Pictures/IMG_3030.CR2"
make: Canon
model: Canon EOS 700D
canonical_make: Canon
canonical_model: EOS 700D
canonical_alias: EOS 700D
ERROR: [rawspeed] RawSpeed::RawImage RawSpeed::RawDecoder::decodeRaw(), line 305: const uchar8* RawSpeed::Buffer::getData(RawSpeed::Buffer::size_type, RawSpeed::Buffer::size_type) const, line 103: Buffer overflow: image file may be truncated

Raw direct from camera, manual M42 lens with chinese "focus assist" chip on ring.
Grab raw here: IMG_3030.CR2

I see that the original Sony RX1R is supported but not the RX1R-II (DSC-RX1RM2) that has been released in 2015. Is there any chance to add it or use former RX1R as a baseline ?

Thanks.

The fact that CFA2 was introduced (probably because someone noticed that this verbose x, y, color interface is inappropriate to specify a 6x6 array) shows that the CFA tag has an issue. But the solution to come up with a CFA2 like it is, is not a good solution to this problem.

My issues with this are:

1. the interface is too generic, i.e. it allows more stuff than is useful and this extra freedom allows plenty of errors, e.g. specifying two colors for position 0x0, or specifying a 2x3 pattern.
2. having two options for one problem is at least confusing
3. it is way too much text to look at just convey the info "RGGB" (almost half of the whole file content could be removed)

Proposal by example:

                <CFA width="2" height="2">
                        <Color x="0" y="0">RED</Color>
                        <Color x="1" y="0">GREEN</Color>
                        <Color x="0" y="1">GREEN</Color>
                        <Color x="1" y="1">BLUE</Color>
                </CFA>

                <CFA2 width="6" height="6">
                        <ColorRow y="0">RBGBRG</ColorRow>
                        <ColorRow y="1">GGRGGB</ColorRow>
                        <ColorRow y="2">GGBGGR</ColorRow>
                        <ColorRow y="3">BRGRBG</ColorRow>
                        <ColorRow y="4">GGBGGR</ColorRow>
                        <ColorRow y="5">GGRGGB</ColorRow>
                </CFA2>

becomes

                <CFA>RGGB</CFA>

                <CFA>RBGBRG GGRGGB GGBGGR BRGRBG GGBGGR GGRGGB</CFA>

Note: one could insert line breaks inside the string if desired to align the different lines below each other

Advantages:

1. way less unnecessary text to look at and edit
2. no way to specify one entry twice or miss one
3. only one way to do it and only one code path to parse it

The implementation would simply remove all whitespace from the string, then the number of characters has to be a square (well, actually only 4 and 36 are interesting right now). Then the string is parsed character by character and the internal array is filled. done.

@LebedevRI getting rid of another couple hundred lines of duplicated code inside the CR2Decoder I noticed something that looks suspicious. It is about the selection of the predictors on a line change. What would help me, is some kind of ground truth data. Do you have a CR2 file and an accompanying 16bit raw tiff or PPM/PGM from some other source than rawspeed?

What may also work is a DNG that got created from the CR2. Thanks.

del

wrong repo :)

@axxel just so that it does not get lost

*** CID 1397763:    (SIZEOF_MISMATCH)
/RawSpeed/LJpegPlain.cpp: 232 in RawSpeed::LJpegPlain::decodeN_X_Y<(int)2, (int)1, (int)1>()()
226               dest[3 + i*pixel_pitch] = p[0] += ht[0]->decodeNext(bitStream);
227             });
228
229             dest[1] = p[1] += ht[1]->decodeNext(bitStream);
230             dest[2] = p[2] += ht[2]->decodeNext(bitStream);
231
>>>     CID 1397763:    (SIZEOF_MISMATCH)
>>>     Adding "6UL /* 3 * sizeof (RawSpeed::ushort16) */" to pointer "dest" of type "RawSpeed::ushort16 *" is suspicious because adding an integral value to this pointer automatically scales that value by the size, 2 bytes, of the pointed-to type, "RawSpeed::ushort16".  Most likely, the multiplication by "sizeof (RawSpeed::ushort16)" in this expression is extraneous and should be eliminated.
232             dest += 3 * sizeof(ushort16);
233           }
234
235           pixInSlicedLine -= X_S_F;
236         }
237
/RawSpeed/LJpegPlain.cpp: 232 in RawSpeed::LJpegPlain::decodeN_X_Y<(int)3, (int)2, (int)2>()()
226               dest[3 + i*pixel_pitch] = p[0] += ht[0]->decodeNext(bitStream);
227             });
228
229             dest[1] = p[1] += ht[1]->decodeNext(bitStream);
230             dest[2] = p[2] += ht[2]->decodeNext(bitStream);
231
>>>     CID 1397763:    (SIZEOF_MISMATCH)
>>>     Adding "6UL /* 3 * sizeof (RawSpeed::ushort16) */" to pointer "dest" of type "RawSpeed::ushort16 *" is suspicious because adding an integral value to this pointer automatically scales that value by the size, 2 bytes, of the pointed-to type, "RawSpeed::ushort16".  Most likely, the multiplication by "sizeof (RawSpeed::ushort16)" in this expression is extraneous and should be eliminated.
232             dest += 3 * sizeof(ushort16);
233           }
234
235           pixInSlicedLine -= X_S_F;
236         }
237
/RawSpeed/LJpegPlain.cpp: 232 in RawSpeed::LJpegPlain::decodeN_X_Y<(int)3, (int)2, (int)1>()()
226               dest[3 + i*pixel_pitch] = p[0] += ht[0]->decodeNext(bitStream);
227             });
228
229             dest[1] = p[1] += ht[1]->decodeNext(bitStream);
230             dest[2] = p[2] += ht[2]->decodeNext(bitStream);
231
>>>     CID 1397763:    (SIZEOF_MISMATCH)
>>>     Adding "6UL /* 3 * sizeof (RawSpeed::ushort16) */" to pointer "dest" of type "RawSpeed::ushort16 *" is suspicious because adding an integral value to this pointer automatically scales that value by the size, 2 bytes, of the pointed-to type, "RawSpeed::ushort16".  Most likely, the multiplication by "sizeof (RawSpeed::ushort16)" in this expression is extraneous and should be eliminated.
232             dest += 3 * sizeof(ushort16);
233           }
234
235           pixInSlicedLine -= X_S_F;
236         }
237
/RawSpeed/LJpegPlain.cpp: 232 in RawSpeed::LJpegPlain::decodeN_X_Y<(int)4, (int)1, (int)1>()()
226               dest[3 + i*pixel_pitch] = p[0] += ht[0]->decodeNext(bitStream);
227             });
228
229             dest[1] = p[1] += ht[1]->decodeNext(bitStream);
230             dest[2] = p[2] += ht[2]->decodeNext(bitStream);
231
>>>     CID 1397763:    (SIZEOF_MISMATCH)
>>>     Adding "6UL /* 3 * sizeof (RawSpeed::ushort16) */" to pointer "dest" of type "RawSpeed::ushort16 *" is suspicious because adding an integral value to this pointer automatically scales that value by the size, 2 bytes, of the pointed-to type, "RawSpeed::ushort16".  Most likely, the multiplication by "sizeof (RawSpeed::ushort16)" in this expression is extraneous and should be eliminated.
232             dest += 3 * sizeof(ushort16);
233           }
234
235           pixInSlicedLine -= X_S_F;
236         }
237

The LUT code assumes that all values are actually lenghts, and are smaller than 16, which isn't the case for nikon lossy-after-split raws.

This has been circulating through my head for some time, so it might as well be the time to actually write it down.

Presently RawSpeed is mostly[1] up-to-date with the C++ standard.
It requires C++14-conformant compiler/standard library (clang-3.5+, gcc-5+)

It also has very few external dependencies:

• C++ STL.
• pugixml, for data/cameras.xml reading
• libjpeg (e.g. libjpeg-turbo), for Lossy JPEG compressed DNG tiles.
• zlib, for Deflate-compressed DNG tiles
• googletest and googlebenchmark for obvious development reasons.

There are a few things that are the matter of fact as of now, and they should remain valid going forward:

1. All the current dependencies (except STL, as expected) are optional, in the sense that RS can be built without (#) them
2. and retain practically all functionality (#).
3. All dependencies except two (C++ STL, and libjpeg (should be doable, too)) can be natively built in-tree (#) [4]
   by the current CMake build system,
4. and they are not too large, heavy (#).

This is super important.

This is great.
However, there are still, and always will be, things that are not present in the used standard.
Some of them are domain-specific, and won't ever[2] be in the standard:

• Huffman table (multiple implementation variants: binary tree, vector with strict ordering, LUT+vector, ...)
• JPEG
• All the various manufacturer-specific decompressors
• TIFF
• CIFF
• ISO Base Media File Format (CR3)
• ...

So those just have to[3] be implemented.

But, for sufficiently good, maintainable code, which i'm trying to write here, there are always some smaller patterns,
data structures that can be noticed, factored-out and re-used, thus adding abstraction and increasing readability.
I can summarize this as a table:

All these things need to be

• Implemented sanely
• Play nicely with ASAN/UBSAN/MSAN/TSAN (!)
• Ideally, use sanitizer annotations (!!)
• Don't have overhead / have minimal overhead

Now, it is of course possible to either

• Don't bother, not use any abstractions, keep taking shit on the codebase.
• Use std::experimental/newer standards.
  But not really, not everything is even in the future C++ standard's drafts/proposals.
  And it will introduce additional restrictions on supported standard libraries versions,
  E.g. libcxx trunk just dropped most of the std::experimental.
• Copy single needed headers from somewhere (LLVM ADT / libcxx trunk / ...) to src/external/
  • Snowball effect, headers are usually not self-contained
  • It's the dirtiest version bundling, they wont ever be updated, and will rot.
  • I'm not a lawyer, i don't know what licensing reprecussions that will have on the LGPLv2-licensed RS.
• Same as the previous step, but trim the headers to a minimum
• Reinvent the wheel, iteratively implement things from scratch, adding functionality when the need arises.
  Basically what is being done currently. Not too horrible. But this is the wheel reinvention.
  I'd much rather actually use the appropriate atomic (in the sense of them being the minimal tool to solve the task)
  data types to solve the actual raw decoding problems at hand.
• Or, bite the bullet, and pick some library which already implements these things, and depend on it.

I'd like to explore the last option.
To recap, see the things at the beginning marked with (#).
It is obvious that points 1. and 2. will NOT be applicable here.
But the points 3. and 4. MUST remain true.

There are several possibilities:

• LLVM Adaptive Data Types.
  Not really a standalone library.
  So would require copying/bundling the selected headers. Not cool.
  But should be pretty small.
• I'm sure everyone is familiar with Boost :)
  • Rather old library, has a lot of mileage.
  • 23 MLOC as per https://www.openhub.net/p/boost.
    So this undoubtedly does NOT fit the 3. and 4.
• folly
  Has a long list of deps, including boost. Skipping.
• Guidelines Support Library
  Super minimalisting,d oes not contain most of the things, only some.
• ???
• Google's abseil.
  • Well, yeah, it's google :/
  • No releases. Copylib ('live at head', as it's now called). Fedora will be delighted!
    • So 3. will be strongly satisfied with this.
  • Rather small, at least right now:

    abseil-cpp$ find -iname '*.h' -or -iname '*.cc' | xargs wc -l | tail -n 1
    66605 total
    abseil-cpp$ find -iname '*.h' -or -iname '*.cc' -not -iname '*_test*' | xargs wc -l | tail -n 1
    40502 total

    • So at least right now, 4. will satisfied.
  • As a bonus, it does have sanitizers annotations!

Is there some other alternative i missed?

Please just don't even suggest RIIR :) Rust does have a lot of potential, and will hopefully murder C, but it is just not ready to fill the shoes it is trying to wear.

I'm in the corner here, and not sure where to push. At least this somewhat summarizes things...

1. mainly, pthreads vs. C++ native threading concepts (thread, lock)
2. see hilarious proposal to standardize cairo-based 2D graphic for C++. :(
3. Of course, there are always some libraries out there which already implement them, but as a general principle, which i feel quite strong about, only the very very basic or very very edge-case/complicated stuff should be used from libraries.
   AND it is highly ideal that all three things marked with (#) remain possible.
4. In-tree building of dependencies. This is crucial, and fatally important.
   And there are reasons for it:
   • Sanitizers/fuzzing. If some code is not built by the project, it won't be instrumented, and several things can happen.
     • In ASAN/UBSAN case, bugs there mostly won't be detected.
     • In MSAN case, all uninstrumented code will generate false-positives, thus making it impossible to use MSAN. This is unacceptable.
     • Fuzzing engine won't get any coverage feedback, resulting in poor results. Not fatal, but bad.
   • Debugging.
     When a problem with some used dependency surfaces, to debug it, the first thing that everyone does it install debug symbols. An ability to actually step through that code, [and instrument it] is the next logical step.

the hashes seem to differ between ASAN and release builds:

WARNING: the following 7 tests have failed:
./Canon/EOS 5D Mark III/5G4A9395.CR2 failed: hash/metadata mismatch
./Canon/EOS 5D Mark IV/B13A0733.CR2 failed: hash/metadata mismatch
./Canon/EOS 60D/IMG_2015.CR2 failed: hash/metadata mismatch
./Canon/EOS 6D/EOS_6D_mRAW.CR2 failed: hash/metadata mismatch
./Canon/EOS 70D/mRAW_CANON_EOS70D_01.CR2 failed: hash/metadata mismatch
./Canon/EOS 7D/RAW_CANON_EOS_7D-mraw.CR2 failed: hash/metadata mismatch
./Canon/EOS 80D/20160623_0001.CR2 failed: hash/metadata mismatch
See rstest.log for details.

Split off from #100
The current HuffmanTable implementation is just completely broken.
Like, i have stopped counting the amount of bugs it has.
In retrospect, #36 should not have been merged.

Since predictor is reset for each row of the slice to the first pixel of previous slice row, i believe it means only the first pixel of the row depends on the value of the first pixel of the previous row. Thus it should be feasible to parallelize it?

darktable 2.4.3 (for Windows) appears to fail reading Panasonic LUMIX DC-G9 high-resolution RW2 raw files.

The error message emitted by darktable is "failed to read camera white balance".

The photos have the following dimensions (according to exiftool):

• high-resolution item ("HR"): 10368x7776
• "simul record normal shot" (simulate recording a single normal shot = "SIM"): 5184x3888

If one was to trust the documentation, then SIM "should be" a totally plain normal RW2 file: "Saves the first picture of pictures taken before the combining process ... [is started]"

Whatever hides inside HR - I have no idea. Theoretically this is the combined result of a total of eight pixel-shifted exposures.