Rawspeed: DngDecoder: add support for deflate decompression #17

anarsoul · 2017-01-06T03:19:23Z

Tested with files generated by hdrmerge, with this change darktable is able to import them.

houz · 2017-01-06T09:08:49Z

I wonder what the future policy wrt. external dependencies is? So far adding zlib was a no-go which is why we never added deflate support. Has that changed with us taking over? Or should we rather look for one of those minimalistic implementations that fit into a single file and can be bundled?

LebedevRI

Looks, promising, i'm not against zlib as a dependency.

LebedevRI · 2017-01-06T11:06:23Z

RawSpeed/DngDecoder.cpp

+  if (sample_format == 3 && bps != 32 && compression != 8)
+    ThrowRDE("DNG Decoder: Float point must be 32 bits per sample.");
+
+  // FIXME: We're creating USHORT16 image for all deflate images


That's not rawspeed's problem. Just provide non-normalized float image.

I tried, darktable can't handle it - it displays it all white.

(i'll handle the darktable-side patch)

LebedevRI · 2017-01-06T11:07:57Z

RawSpeed/DngDecoder.cpp

@@ -81,9 +81,16 @@ RawImage DngDecoder::decodeRawInternal() {
  if (raw->hasEntry(SAMPLEFORMAT))
    sample_format = raw->getEntry(SAMPLEFORMAT)->getInt();

-  if (sample_format == 1)
+  int compression = -1;
+  compression = raw->getEntry(COMPRESSION)->getShort();


And if there is no such tag?

LebedevRI · 2017-01-06T11:09:11Z

RawSpeed/DngDecoderSlices.cpp

+    }
+}
+
+// From DNG SDK dng_utils.h


And what is the legal status of such codedrop?

From DNG SDK license agreement:

LICENSE GRANT
Software License. Subject to the restrictions below and other terms of this Agreement, Adobe hereby grants you a non-exclusive, worldwide, royalty free license to use, reproduce, prepare derivative works from, publicly display, publicly perform, distribute and sublicense the Software for any purpose.

RESTRICTIONS AND OWNERSHIP
You will not remove any copyright or other notice included in the Software or Documentation and you will include such notices in any copies of the Software that you distribute in human-readable format.

I'll move it to another file and will add Adobe copyright note. Looks like RawTherapee violates Adobe license.

Can such code even be used in LGPL library?

Both RawTherapee and hdrmerge use parts of DNG SDK, but of course it's not an excuse for darktable. As far as I can tell, DNG SDK license doesn't contradict LGPL, but I'm not a lawyer, so I can't tell for sure.

I'd personally try really hard to find a better-licensed version of this function.
After all, it looks like it simply converts half-float to full float.
Probably OpenEXR has such a function.

I agree. Will do.

LebedevRI · 2017-01-06T11:11:23Z

RawSpeed/DngDecoderSlices.cpp

+    while (!t->slices.empty()) {
+      DngSliceElement e = t->slices.front();
+      if (!uBuffer)
+        uBuffer = new unsigned char [e.width * e.height * sizeof(float)];


Let's not take any chances and reorder it like: sizeof(float) * e.width * e.height

Multiplication is commutative, what's the difference?

At least in C, typeof(sizeof()) is size_t, but are width and height of size_t type too?
If not, the multiplication can somewhat easily overflow, i believe.

Fair enough

Fixed and pushed.

LebedevRI · 2017-01-06T11:24:28Z

RawSpeed/CMakeLists.txt

@@ -26,6 +26,9 @@ include_directories(SYSTEM ${Pugixml_INCLUDE_DIRS})
 find_package(JPEG REQUIRED)
 include_directories(SYSTEM ${JPEG_INCLUDE_DIRS})

+find_package(ZLIB REQUIRED)


@houz is right. But now, it is up to me i guess.
I consider zlib to be ok as a dep. I may need to revisit this once i finally start to look at fuzzing, but for now it is ok.

What i do however think, is that it should be an option, i.e. it should be able to pass -DWITH_ZLIB=OFF to cmake, and that code will not be compiled.

LebedevRI · 2017-01-06T11:25:49Z

RawSpeed/DngDecoderSlices.cpp

@@ -143,6 +143,159 @@ my_error_throw (j_common_ptr cinfo)
  ThrowRDE("JPEG decoder error!");
 } 

+static void decodeFPDeltaRow(unsigned char * src, unsigned char * dst, size_t tileWidth, size_t realTileWidth, unsigned int bytesps, int factor) {


Where this is from?

From RawTherapee, dcraw.cc, it's license says:

No license is required to download and use dcraw.c. However,
to lawfully redistribute dcraw, you must either (a) offer, at
no extra charge, full source code* for all executable files
containing RESTRICTED functions, (b) distribute this code under
the GPL Version 2 or later, (c) remove all RESTRICTED functions,
re-implement them, or copy them from an earlier, unrestricted
Revision of dcraw.c, or (d) purchase a license from the author.

I chose option (c), since these functions are not restricted.

TBN, it is not from dcraw.c
https://github.com/Beep6581/RawTherapee/blob/880d0305486d8d9f6a4bb71c9f42a45921bfe48e/rtengine/dcraw.patch#L3173

It's from dcraw.cc, not from dcraw.c

jcelaya/hdrmerge#103 (comment)
So MIT License, copyright 2014 Javier Celaya <jcelaya@gmail.com>
Now, can it be used in with LGPL?

Yes, it can be relicensed under LGPL, explanation is here:
http://softwareengineering.stackexchange.com/questions/105912/can-you-change-code-distributed-under-the-mit-license-and-re-distribute-it-unde

The GPL and LGPL licenses are not synonyms, they are 2 different things, and i see nothing about LGPL on that page.
But yes, i do think that this MIT code can be used.
Do put MIT License, copyright 2014 Javier Celaya <jcelaya@gmail.com> as comment before the function.

Of course they are not. From the link above:

Note that the MIT license explicitly includes the right to sublicense. Quoting: "the rights to use, copy, modify, merge, publish,distribute, sublicense, and/or sell"

When I sublicense code, I cannot grant rights that I didn't originally have. In the case of the GPL, I am explicitly forbidden to sublicense only some rights. But neither in law nor in the MIT license do I have an obligation to sublicense all rights as a whole.

LGPL is more permissive than GPL, i.e. you can sublicense more rights that you already have. So I see no problem in sublicensing the code under LGPL.

LebedevRI · 2017-01-06T11:27:37Z

RawSpeed/DngDecoderSlices.cpp

+      decodeFPDeltaRow(src, (unsigned char *)floatBuffer, thisTileWidth, e.width, bytesps, predFactor);
+    expandFloats((unsigned char *)floatBuffer, thisTileWidth, bytesps);
+    unsigned short *dst = (unsigned short *)((unsigned char *)mRaw->getData() + ((e.offY + row) * mRaw->pitch + e.offX * sizeof(ushort16) * mRaw->getCpp()));
+    // FIXME: convert float to ushort16, because darktable doesn't support non-normalized float32 images


codecov-io · 2017-01-07T02:50:45Z

Current coverage is 5.61% (diff: 0.00%)

Merging #17 into develop will decrease coverage by 0.06%

@@           develop       #17   diff @@
========================================
  Files          111       111          
  Lines         9285      9400   +115   
  Methods       1276      1288    +12   
  Messages         0         0          
  Branches      1725      1744    +19   
========================================
  Hits           528       528          
- Misses        8757      8872   +115   
  Partials         0         0

Powered by Codecov. Last update 5f94996...c06aa70

anarsoul · 2017-01-07T02:56:49Z

I rewrote fp-conversion functions from scratch and addressed all other comments, please review it again.

anarsoul · 2017-01-07T03:01:14Z

Please note that I tested it only with float to ushort16 conversion patch.

LebedevRI

Getting there :)

LebedevRI · 2017-01-07T08:17:31Z

RawSpeed/DngDecoder.cpp

@@ -60,7 +60,7 @@ RawImage DngDecoder::decodeRawInternal() {
    try {
      isSubsampled = (*i)->getEntry(NEWSUBFILETYPE)->getInt() & 1; // bit 0 is on if image is subsampled
    } catch (TiffParserException) {}
-    if ((compression != 7 && compression != 1 && compression != 0x884c) || isSubsampled) {  // Erase if subsampled, or not JPEG or uncompressed
+    if ((compression != 7 && compression != 8 && compression != 1 && compression != 0x884c) || isSubsampled) {  // Erase if subsampled, or not deflated, JPEG or uncompressed


If without ZLIB, deflated should be erased.

LebedevRI · 2017-01-07T08:19:32Z

RawSpeed/DngDecoder.cpp

@@ -230,17 +234,23 @@ RawImage DngDecoder::decodeRawInternal() {
      } catch (TiffParserException) {
        ThrowRDE("DNG Decoder: Unsupported format, uncompressed with no strips.");
      }
-    } else if (compression == 7 || compression == 0x884c) {
+    } else if (compression == 7 || compression == 8 || compression == 0x884c) {


If without ZLIB, deflated should be ignored.

In my opinion that's meaningless, since it decoding will fail later, and all extra ifdefs make code readability worse, but OK.

Hmm, now that i think the second time, for now just do https://github.com/darktable-org/rawspeed/pull/17/files/4f2bdbbf76cb8c23be314d0b76effafd397f206e#r95054866

LebedevRI · 2017-01-07T08:21:53Z

RawSpeed/DngDecoderSlices.cpp

@@ -143,6 +143,204 @@ my_error_throw (j_common_ptr cinfo)
  ThrowRDE("JPEG decoder error!");
 } 

+#ifdef HAVE_ZLIB
+static void decodeFPDeltaRow(unsigned char * src, unsigned char * dst, size_t tileWidth, size_t realTileWidth, unsigned int bytesps, int factor)


no i mean, put the exact quote from author right before this function
it should be something like

// decodeFPDeltaRow(): MIT License, copyright 2014 Javier Celaya <jcelaya@gmail.com> static void decodeFPDeltaRow(unsigned char * src, unsigned char * dst, size_t tileWidth, size_t realTileWidth, unsigned int bytesps, int factor)

LebedevRI · 2017-01-07T08:22:49Z

RawSpeed/DngDecoderSlices.cpp

+    }
+}
+
+static uint32_t fp16ToFloat(uint16_t fp16)


Awesome, no more adobe code with questionable license!

Note that i did not actually check that the function is implemented correctly. One day this will get to have an unit-test...

I checked that it returns the same value as Adobe's implementation for all possible fp16 and fp24 values. The only difference is handling +-inf and NaN, Adobe converts them to +-max and 0 respectively, my implementation leaves them as is.

Anyway, unit-testing this code is pretty useless: for normalized values it's just a matter of testing how well compiler translates shifts to machine code, and for non-normalized it's essentially means reimplementing part of these functions in a test. The question is who will watch the watchman^Wtest ;)

Potentially having NAN's in input data will seriously hurt all the further algorithms that e.g. blur

Handle it later in darktable? IIRC DNG 1.4 spec says nothing about clamping infinity or treating NaN as zero.

Currently we simply assume that the [float] input is always ok, and only special-case the code inside the pipe that produces NAN's to not produce them.

I don't see anything that handles NaN's for uncompressed data (compression == 1) in rawspeed.

Good point :)
I don't remember any reports about raws with NAN's, let's hope it stays that way :)

LebedevRI · 2017-01-07T08:23:49Z

RawSpeed/StdAfx.h

@@ -107,6 +107,9 @@ extern "C" {
 #include <list>
 using namespace std;

+// ZLIB
+#include <zlib.h>


#ifdef HAVE_ZLIB

LebedevRI · 2017-01-07T21:38:38Z

CMakeLists.txt

@@ -25,6 +25,7 @@ set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 option(WITH_PTHREADS "Enable threading support through pthread. Highly recommended" ON)
 option(USE_XMLLINT "Run xmllint to test if data/cameras.xml is valid" ON)
 option(BUILD_TESTING "Build the testing tree." ON)
+option(WITH_ZLIB "Enable ZLIB support for DNG deflate support" ON)


While there, please move this new line to after first two option(WITH_
Just OCD :)

* upstream/develop: (78 commits) rstest: -c: do not recreate hashes if already exists rstest: unbreak: RawParser::getDecoder() takes CameraMetaData* CMake: ok, bad idea, don't link librawspeed_static into librawspeed_test Enable OpenMP for tools. Add rstest tool, from Axel Waggershauser. For regression testing. CMake: build identify tool only if BUILD_TOOLS is on Add zlib into dockerfile and msys2 deps. For #17 close another leak, this time in X3fDecoder, which does not decode any of the samples on r.p.u at the moment anyway. CMake: compiler-flags: ASAN: use -O1. Else it is too slow ammend c29c11c (again, overlooked one case, sigh...) fix 'harmless' off-by-one buffer read overflow in PanaBitpump that has always been there but could only be discovered with ASAN after switching from stack to heap allocated memory amend 92e2d9d fixing a off-by-one buffer overflow regression fixed regression introduced by commit 0894ef9 (simple reversion of older commit) make the `curve` lookup table in `NikonDecompressor` just as big as required Revert "Cr2Decoder::decodeRawInternal(): don't stack-allocate LJpegPlain" Revert "Rw2Decoder::decodeThreaded(): don't stack-allocate PanaBitpump()" Revert "NefDecoder::decodeRawInternal(): don't stack-allocate NikonDecompressor." Allocate 32k buffer inside PanaBitpump on heap instead of stack. Move buffer for lookup table creation from NikonDecompressor member to local variable (now similar to all other, equivalent use cases). Switching from stack to heap allocation to reduce stack size from 16k to 32 bytes. Allocate the (one) huge array of struct HuffmanTable on the heap instead of the stack. Stack size of LJpegDecompressor goes down from about 9k to 240. ...

anarsoul · 2017-01-10T04:26:23Z

All comments are addressed now.

LebedevRI · 2017-01-10T07:27:20Z

RawSpeed/DngDecoder.cpp

-           ThrowRDE("DNG Decoder: Only 16 bit unsigned data supported for compressed data.");
+        if (compression == 8 && sample_format != 3)
+           ThrowRDE("DNG Decoder: Only float format is supported for deflate-compressed data.");
+        else if (compression != 8 && sample_format != 1)


But compression != 8 only says that it is not deflate compression, not that it is jpeg compression?

We're in a branch for jpeg and deflate. What else it could be?

LebedevRI · 2017-01-10T07:28:21Z

RawSpeed/DngDecoderSlices.cpp

@@ -143,6 +143,204 @@ my_error_throw (j_common_ptr cinfo)
  ThrowRDE("JPEG decoder error!");
 } 

+#ifdef HAVE_ZLIB
+// decodeFPDeltaRow(): MIT License, copyright 2014 Javier Celaya <jcelaya@gmail.com>


axxel · 2017-01-10T14:21:03Z

RawSpeed/DngDecoderSlices.cpp

+    }
+  } else {
+    union X { unsigned int x; unsigned char c; };
+    if (((union X){1}).c) {


I suggest to use the existing getHostEndianness() from Common.h instead of introducing a new way to determine the endianness.

While i do understand that this is basically a vendordrop from rt/jcelaya, i have to agree, please use getHostEndianness() here.

axxel · 2017-01-10T14:29:42Z

RawSpeed/DngDecoderSlices.cpp

+  return (sign << 31) | (fp32_exponent << 23) | fp32_fraction;
+}
+
+static uint32_t fp24ToFloat(uint32_t fp24)


Question: have you tried if hinting your compiler to inline this function helps him to actually to it? Removing a function call in the innermost loop might be worth it.

axxel · 2017-01-10T14:40:12Z

As a general comment regarding the used types (uint32_t, etc.): I would suggest to stick to the custom types defined in Common.h:101ff just to be consistent with the rest of the code base.

Note: In my opinion, the chosen names suck and I will definitely suggest to rename them later. Maybe exactly to the C99 standard types used here, but maybe to something more concise (I like the type names in rust a lot). Anyway, having it all consistent is a value in itself.

LebedevRI · 2017-01-10T15:14:36Z

RawSpeed/DngDecoderSlices.cpp

+    for (size_t col = 0; col < tileWidth; ++col) {
+      for (size_t byte = 0; byte < bytesps; ++byte)
+        dst[col*bytesps + byte] = src[col + realTileWidth*(bytesps-byte-1)];  // Little endian
+    }


Mis-indented }

it's actually the whole block of 4 lines that is misindented. same for the else block. but: a general clang-format whitespace cleanup will follow later anyway, right? (but only after my infrastructure rework! :))

And since this is just one commit, also please try to format your new functions properly.
Only these few new functions, not the whole file/etc.

LebedevRI

I think after this iteration i will look at the needed dt changes.

LebedevRI · 2017-01-10T20:11:58Z

RawSpeed/DngDecoderSlices.cpp

+    }
+  } else {
+    union X { unsigned int x; unsigned char c; };
+    if (((union X){1}).c) {


While i do understand that this is basically a vendordrop from rt/jcelaya, i have to agree, please use getHostEndianness() here.

LebedevRI · 2017-01-10T20:13:52Z

RawSpeed/DngDecoderSlices.cpp

+    for (size_t col = 0; col < tileWidth; ++col) {
+      for (size_t byte = 0; byte < bytesps; ++byte)
+        dst[col*bytesps + byte] = src[col + realTileWidth*(bytesps-byte-1)];  // Little endian
+    }


And since this is just one commit, also please try to format your new functions properly.
Only these few new functions, not the whole file/etc.

LebedevRI · 2017-01-10T20:19:31Z

RawSpeed/DngDecoderSlices.cpp

+    unsigned char * src = uBuffer + row * e.width * bytesps;
+    unsigned char *dst = (unsigned char *)mRaw->getData() + ((e.offY + row) * mRaw->pitch + e.offX * sizeof(float) * mRaw->getCpp());
+    if (predFactor)
+      decodeFPDeltaRow(src, dst, thisTileWidth, e.width, bytesps, predFactor);


As @axxel points, regardless of whether it actually helps or not, it would be best if these decodeFPDeltaRow(), expandFP16(), expandFP24(), fp16ToFloat() and fp24ToFloat() and be marked with inline keyword.

LebedevRI · 2017-01-10T20:30:51Z

As a general comment regarding the used types (uint32_t, etc.): I would suggest to stick to the custom types defined in Common.h:101ff just to be consistent with the rest of the code base.

Actually, i'm afraid i have to agree here.
So far only the uint32/uint16/etc are used throughout all codebase (not counting 2 special files).
I think i don't like that, but until globally decided otherwise, for consistency reasons it should be kept.

Note: In my opinion, the chosen names suck and I will definitely suggest to rename them later.

Please be a little more specific :)
Which names? uint32/uint16/etc? I think i don't like them either.
The names of new functions in this pr? They seem fine to me.

axxel · 2017-01-10T20:43:22Z

Note: In my opinion, the chosen names suck and I will definitely suggest to rename them later.

Please be a little more specific :)
Which names? uint32/uint16/etc? I think i don't like them either.
The names of new functions in this pr? They seem fine to me.

Oh, potential big misunderstanding! :-). The names in this PR seem totally fine to me. I was referring to the type names in Common.h, which I just suggested to use, even though I think they are 'sub-optimal' (to be more politically correct (sorry for the language before)). uint32, ushort16, char8, etc. I don't like the redundancy in their naming and their verbosity. And what makes it worse still is the inconsistency of member functions names in the TiffEntry/ByteStream API that relate to these types. But please let us discuss this issue of 'aesthetics' after I've got my increasing backlog merged.

anarsoul · 2017-01-12T05:17:12Z

Another round.

shoffmeister · 2017-01-13T21:47:55Z

RawSpeed/CMakeLists.txt

+if (WITH_ZLIB)
+  find_package(ZLIB REQUIRED)
+  if(NOT ZLIB_FOUND)
+    message(SEND_ERROR "Did not found ZLIB! Either make it find ZLIB, or pass -DWITH_ZLIB=OFF to disable ZLIB.")


Spelling fix suggestion:
"found" -> "find"

Oops, copy-pasted from missing pthread error message. Fixed now.

LebedevRI

@anarsoul thanks!
Okay, i took a look.
With https://raw.pixls.us/getfile.php/771/raw/5G4A9394.CR2 converted to 32bit, 24bit and 16bit HDR dng, i get:

[rawspeed] (16b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (16b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (16b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (16b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (16b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (16b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (16b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (16b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (24b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (24b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (24b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (24b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (24b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (24b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (24b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (24b.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (32b-fp-w-predictor.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (32b-fp-w-predictor.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (32b-fp-w-predictor.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (32b-fp-w-predictor.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (32b-fp-w-predictor.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (32b-fp-w-predictor.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (32b-fp-w-predictor.dng) DNGDEcodeThread: Caught exception.
[rawspeed] (32b-fp-w-predictor.dng) DNGDEcodeThread: Caught exception.

LebedevRI · 2017-01-14T15:53:27Z

RawSpeed/DngDecoder.cpp

@@ -60,7 +60,11 @@ RawImage DngDecoder::decodeRawInternal() {
    try {
      isSubsampled = (*i)->getEntry(NEWSUBFILETYPE)->getInt() & 1; // bit 0 is on if image is subsampled
    } catch (TiffParserException) {}
+#ifdef WITH_ZLIB


2 issues here:

it's HAVE_ZLIB

unreadable.

This should be

if (!(compression == 7 || #ifdef HAVE_ZLIB compression == 8 || #endif compression == 1 || compression != 0x884c) || isSubsampled) { // Erase if subsampled, or not deflated, JPEG or // uncompressed

LebedevRI · 2017-01-14T15:54:51Z

RawSpeed/DngDecoder.cpp

@@ -81,6 +85,12 @@ RawImage DngDecoder::decodeRawInternal() {
  if (raw->hasEntry(SAMPLEFORMAT))
    sample_format = raw->getEntry(SAMPLEFORMAT)->getInt();

+  int compression = -1;


Now that i actually look, we have already did int compression = (*i)->getEntry(COMPRESSION)->getShort(); before. So just do

const int compression = raw->getEntry(COMPRESSION)->getShort();;

Sorry for asking about this earlier.

LebedevRI · 2017-01-14T16:00:47Z

Sorry, should have commented already:
I use:

$ pwd
~/rawspeed/
$ mkdir build && cd build
$ cmake -DCMAKE_BUILD_TYPE=ASAN ../ && make
$ find ~/raw-camera-samples/_HDRMERGE/ -not -iname \*.xmp | xargs -L1 ./RawSpeed/darktable-rs-identify

That way you can actually test just the rawspeed changes, without darktable. Kinda sorta.

anarsoul · 2017-01-14T17:14:24Z

Works fine for me with your file coverted either to 16, 24 or 32 bit:

WARNING: Couldn't find cameras.xml in '/usr/local/share/darktable/rawspeed/cameras.xml'
WARNING: Couldn't find cameras.xml in './RawSpeed/../share/darktable/rawspeed/cameras.xml'
Loading file: "/tmp/24bit-5G4A9394.dng"
make: Canon
model: Canon EOS 5D Mark III
canonical_make: Canon
canonical_model: EOS 5D Mark III
canonical_alias: EOS 5D Mark III
RawSpeed:Unable to find camera in database: 'Canon' 'Canon EOS 5D Mark III' 'dng'
Please consider providing samples on https://raw.pixls.us/, thanks!
blackLevel: -1
whitePoint: 15488
blackLevelSeparate: 2047 2047 2048 2047
wbCoeffs: 1.649414 1.000000 2.165041 nan
isCFA: 1
filters: -1802201964 (0x94949494)
bpp: 4
cpp: 1
dataType: 1
dimUncropped: 5920x3950
dimCropped: 5796x3870
cropOffset: 122x80
fuji_rotation_pos: 0
pixel_aspect_ratio: 1.000000
Image byte sum: 7482765221.000000
Image byte avg: 79.998773
Image float sum: 68836756946.437500
Image float avg: 2943.754573

anarsoul · 2017-01-14T17:17:14Z

Rest is addressed.

LebedevRI · 2017-01-14T17:19:40Z

Works fine for me with your file coverted either to 16, 24 or 32 bit:

Hmm, indeed, the tool does not cause it, but if i update darktable's rawspeed submodule with these changes, and when darktable loads these dng, then these exceptions are in output.

anarsoul · 2017-01-14T17:26:41Z

I have no exceptions with darktable either, but the whole image is white. I bet if I patch rawspeed to convert floats to uint16 it'll work.

LebedevRI · 2017-01-14T17:28:30Z

I have no exceptions with darktable either

With that specific 5G4A9394.cr2 coverted to HDR?

, but the whole image is white. I bet if I patch rawspeed to convert floats to uint16 it'll work.

I'm sure it will, but that is a separate problem.

anarsoul · 2017-01-14T17:30:23Z

Yes, with this specific 5G4A9394.cr2 converted to DNG using hdrmerge.

LebedevRI · 2017-01-14T17:35:30Z

@anarsoul hmm.
Let's try it the other way around, do all of these dng load for you, with no exceptions? (not counting the "*-hdr.dng"-one) https://www.dropbox.com/sh/zx1lbelwu6id989/AAAdULn8Lf-DvjC7UBnqNy35a?dl=0

anarsoul · 2017-01-14T17:39:00Z

Yes, they load fine. Do you need a screencast as a proof? :) What compiler do you use to compile darktable?

LebedevRI · 2017-01-14T17:40:08Z

Debian sid, gcc6.

$ cd ~/darktable/build/ && rm -rf * && LDFLAGS="-fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-common -U_FORTIFY_SOURCE -fsanitize=address -fstack-protector-strong" CFLAGS="${LDFLAGS}" CXXFLAGS="${LDFLAGS}" CPPFLAGS="${LDFLAGS}" CC=gcc-6 CXX=g++-6 cmake -DCMAKE_INCLUDE_PATH:PATH=/opt/darktable/include -DCMAKE_LIBRARY_PATH:PATH=/opt/darktable/lib -DCMAKE_PREFIX_PATH:PATH=/opt/darktable -DCMAKE_INSTALL_PREFIX:PATH=/opt/darktable  ../ && make -j9
-- The CXX compiler identification is GNU 6.3.0
-- The C compiler identification is GNU 6.3.0
-- Check for working CXX compiler: /usr/bin/g++-6
-- Check for working CXX compiler: /usr/bin/g++-6 -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Check for working C compiler: /usr/bin/gcc-6
-- Check for working C compiler: /usr/bin/gcc-6 -- works
...

anarsoul · 2017-01-14T17:42:14Z

I'm using gcc-6.2.1 on Archlinux, let me try to recompile darktable with your cflags...

anarsoul · 2017-01-14T17:49:44Z

Still works fine, here's console output:

darktable
RawSpeed:Unable to find camera in database: 'Canon' 'Canon EOS 5D Mark III' 'dng'
Please consider providing samples on <https://raw.pixls.us/>, thanks!
RawSpeed:Unable to find camera in database: 'Canon' 'Canon EOS 5D Mark III' 'dng'
Please consider providing samples on <https://raw.pixls.us/>, thanks!
RawSpeed:Unable to find camera in database: 'Canon' 'Canon EOS 5D Mark III' 'dng'
Please consider providing samples on <https://raw.pixls.us/>, thanks!
[dev_pixelpipe] module `white balance' min: (2047,000000) max: (33532,156250) [full]
[dev_pixelpipe] module `highlight reconstruction' min: (1,000000) max: (1,000000) [full]
[dev_pixelpipe] module `demosaic' min: (0,999999; 0,000000; 0,000000) max: (1,000001; 1,000001; 1,000001) [full]
[dev_pixelpipe] module `base curve' min: (0,999985; 0,000000; 0,000000) max: (0,999985; 0,999985; 0,999985) [full]
[dev_pixelpipe] module `input color profile' min: (99,998886; 0,000000; 0,000000) max: (99,998886; 0,000000; 0,000000) [full]
[dev_pixelpipe] module `sharpen' min: (99,998886; 0,000000; 0,000000) max: (99,998886; 0,000000; 0,000000) [full]
[dev_pixelpipe] module `output color profile' min: (0,999985; 0,000000; 0,000000) max: (0,999985; 0,999989; 0,999984) [full]
[dev_pixelpipe] module `white balance' min: (2046,999634) max: (33532,160156) [preview]
[dev_pixelpipe] module `highlight reconstruction' min: (1,000000) max: (1,000000) [preview]
[dev_pixelpipe] module `demosaic' min: (1,000000; 0,000000; 0,000000) max: (1,000000; 1,000000; 1,000000) [preview]
[dev_pixelpipe] module `base curve' min: (0,999985; 0,000000; 0,000000) max: (0,999985; 0,999985; 0,999985) [preview]
[dev_pixelpipe] module `input color profile' min: (99,998886; 0,000000; 0,000000) max: (99,998886; 0,000000; 0,000000) [preview]
[dev_pixelpipe] module `sharpen' min: (99,998886; 0,000000; 0,000000) max: (99,998886; 0,000000; 0,000000) [preview]
[dev_pixelpipe] module `output color profile' min: (0,999985; 0,000000; 0,000000) max: (0,999985; 0,999989; 0,999984) [preview]

LebedevRI · 2017-01-14T18:21:51Z

And now it does not. Gremlins.

LebedevRI

Ok, not counting the darktable part, which is TODO, this is good. rstest reports no regressions.

@anarsoul thank you!

axxel · 2017-01-15T01:08:29Z

@LebedevRI since you mentioned rstest: we'd need at least one of those DNGs in the test-set, even though they violate the "hasn't been modified in any way" rule.

LebedevRI · 2017-01-15T07:51:27Z

@axxel i know, but then they would be straight out of hdrmerge, completely generated by it. and that would be kinda-ok.
I'll upload

…l mem alloc. ================================================================= ==9166==ERROR: LeakSanitizer: detected memory leaks Direct leak of 144000000 byte(s) in 24 object(s) allocated from: #0 0x716db0 in operator new[](unsigned long) (/home/lebedevri/rawspeed/build-Clang-SANITIZE/src/utilities/rstest/rstest+0x716db0) #1 0xaae8ec in rawspeed::VC5Decompressor::Array2D<short>::create(unsigned int, unsigned int) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/VC5Decompressor.cpp:92:21 darktable-org#2 0xaab67e in rawspeed::VC5Decompressor::decode(unsigned int, unsigned int) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/VC5Decompressor.cpp:637:26 darktable-org#3 0xa8dab5 in rawspeed::AbstractDngDecompressor::decompressThreaded(rawspeed::RawDecompressorThread const*) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractDngDecompressor.cpp:138:11 darktable-org#4 0x8f64d8 in rawspeed::RawDecompressorThread::start_routine(void*) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractParallelizedDecompressor.h:67:22 darktable-org#5 0x8f3989 in rawspeed::AbstractParallelizedDecompressor::decompressOne(unsigned int) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractParallelizedDecompressor.cpp:39:3 darktable-org#6 0x8f40a4 in rawspeed::AbstractParallelizedDecompressor::startThreading(unsigned int) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractParallelizedDecompressor.cpp:55:12 darktable-org#7 0xa86672 in rawspeed::AbstractDngDecompressor::decompress() const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractDngDecompressor.cpp:44:3 darktable-org#8 0x9c4873 in rawspeed::DngDecoder::decodeData(rawspeed::TiffIFD const*, unsigned int) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decoders/DngDecoder.cpp:333:10 darktable-org#9 0x9c7cc6 in rawspeed::DngDecoder::decodeRawInternal() /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decoders/DngDecoder.cpp:415:3 darktable-org#10 0xc02508 in rawspeed::RawDecoder::decodeRaw() /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decoders/RawDecoder.cpp:260:20 darktable-org#11 0x7251ee in rawspeed::rstest::process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rawspeed::CameraMetaData const*, rawspeed::rstest::options const&) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/utilities/rstest/rstest.cpp:380:12 darktable-org#12 0x729552 in .omp_outlined._debug__ /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/utilities/rstest/rstest.cpp:544:17 darktable-org#13 0x72a5ce in .omp_outlined. /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/utilities/rstest/rstest.cpp:538:3 darktable-org#14 0x7f35f57d03c2 in __kmp_invoke_microtask (/usr/lib/x86_64-linux-gnu/libomp.so.5+0x883c2) darktable-org#15 0x7f35f5783929 in __kmp_fork_call (/usr/lib/x86_64-linux-gnu/libomp.so.5+0x3b929) darktable-org#16 0x7f35f5775570 in __kmpc_fork_call (/usr/lib/x86_64-linux-gnu/libomp.so.5+0x2d570) darktable-org#17 0x727ab5 in main /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/utilities/rstest/rstest.cpp:536:9 darktable-org#18 0x7f35f558ab16 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x22b16) SUMMARY: AddressSanitizer: 144000000 byte(s) leaked in 24 allocation(s). xargs: /home/lebedevri/rawspeed/build-Clang-SANITIZE/src/utilities/rstest/rstest: terminated by signal 6

../src/librawspeed/io/BitPumpJPEG.h:71:24: runtime error: implicit conversion from type 'int' of value -256 (32-bit, signed) to type 'unsigned long long' changed the value to 18446744073709551360 (64-bit, unsigned) #0 0x58250f in rawspeed::BitStream<rawspeed::JPEGBitPumpTag, rawspeed::BitStreamCacheRightInLeftOut>::fillCache(unsigned char const*) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/io/BitPumpJPEG.h:71:24 #1 0x581b6f in rawspeed::BitStream<rawspeed::JPEGBitPumpTag, rawspeed::BitStreamCacheRightInLeftOut>::fillSafe() /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/io/BitStream.h:124:14 darktable-org#2 0x581551 in rawspeed::BitStream<rawspeed::JPEGBitPumpTag, rawspeed::BitStreamCacheRightInLeftOut>::fill(unsigned int) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/io/BitStream.h:143:7 darktable-org#3 0x580a14 in int rawspeed::HuffmanTableLUT::decode<rawspeed::BitStream<rawspeed::JPEGBitPumpTag, rawspeed::BitStreamCacheRightInLeftOut>, true>(rawspeed::BitStream<rawspeed::JPEGBitPumpTag, rawspeed::BitStreamCacheRightInLeftOut>&) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/HuffmanTableLUT.h:192:8 darktable-org#4 0x580083 in int rawspeed::HuffmanTableLUT::decodeNext<rawspeed::BitStream<rawspeed::JPEGBitPumpTag, rawspeed::BitStreamCacheRightInLeftOut> >(rawspeed::BitStream<rawspeed::JPEGBitPumpTag, rawspeed::BitStreamCacheRightInLeftOut>&) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/HuffmanTableLUT.h:178:12 darktable-org#5 0x635327 in void rawspeed::LJpegDecompressor::decodeN<2, false>()::'lambda'(int)::operator()(int) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/LJpegDecompressor.cpp:193:37 darktable-org#6 0x625e41 in rawspeed::unroll_loop_t<void rawspeed::LJpegDecompressor::decodeN<2, false>()::'lambda'(int), 2ul>::repeat(void rawspeed::LJpegDecompressor::decodeN<2, false>()::'lambda'(int) const&) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/common/Common.h:232:5 darktable-org#7 0x625e41 in void rawspeed::unroll_loop<2ul, void rawspeed::LJpegDecompressor::decodeN<2, false>()::'lambda'(int)>(void rawspeed::LJpegDecompressor::decodeN<2, false>()::'lambda'(int) const&) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/common/Common.h:246 darktable-org#8 0x625e41 in void rawspeed::LJpegDecompressor::decodeN<2, false>() /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/LJpegDecompressor.cpp:192 darktable-org#9 0x620fb6 in rawspeed::LJpegDecompressor::decodeScan() /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/LJpegDecompressor.cpp:119:7 darktable-org#10 0x589995 in rawspeed::AbstractLJpegDecompressor::parseSOS(rawspeed::ByteStream) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractLJpegDecompressor.cpp:210:3 darktable-org#11 0x585e78 in rawspeed::AbstractLJpegDecompressor::decode() /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractLJpegDecompressor.cpp:101:7 darktable-org#12 0x61f7f7 in rawspeed::LJpegDecompressor::decode(unsigned int, unsigned int, unsigned int, unsigned int, bool) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/LJpegDecompressor.cpp:80:30 darktable-org#13 0x5fe5db in rawspeed::AbstractDngDecompressor::decompressThreaded(rawspeed::RawDecompressorThread const*) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractDngDecompressor.cpp:103:11 darktable-org#14 0x557894 in rawspeed::RawDecompressorThread::start_routine(void*) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractParallelizedDecompressor.h:67:22 darktable-org#15 0x555cd2 in rawspeed::AbstractParallelizedDecompressor::decompressOne(unsigned int) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractParallelizedDecompressor.cpp:39:3 darktable-org#16 0x55621e in rawspeed::AbstractParallelizedDecompressor::startThreading(unsigned int) const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractParallelizedDecompressor.cpp:55:12 darktable-org#17 0x5fc37b in rawspeed::AbstractDngDecompressor::decompress() const /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decompressors/AbstractDngDecompressor.cpp:44:3 darktable-org#18 0x5b12ba in rawspeed::DngDecoder::decodeData(rawspeed::TiffIFD const*, unsigned int) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decoders/DngDecoder.cpp:333:10 darktable-org#19 0x5b3201 in rawspeed::DngDecoder::decodeRawInternal() /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decoders/DngDecoder.cpp:415:3 darktable-org#20 0x6c072d in rawspeed::RawDecoder::decodeRaw() /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/librawspeed/decoders/RawDecoder.cpp:260:20 darktable-org#21 0x4c58a5 in rawspeed::rstest::process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rawspeed::CameraMetaData const*, rawspeed::rstest::options const&) /home/lebedevri/rawspeed/build-Clang-SANITIZE/../src/utilities/rstest/rstest.cpp:380:12

LebedevRI requested changes Jan 6, 2017

View reviewed changes

LebedevRI mentioned this pull request Jan 6, 2017

decodeFPDeltaRow() license? jcelaya/hdrmerge#103

Closed

LebedevRI requested changes Jan 7, 2017

View reviewed changes

LebedevRI added a commit to LebedevRI/rawspeed that referenced this pull request Jan 7, 2017

Add zlib into dockerfile and msys2 deps. For darktable-org#17

75ed7ee

LebedevRI reviewed Jan 7, 2017

View reviewed changes

LebedevRI reviewed Jan 10, 2017

View reviewed changes

axxel reviewed Jan 10, 2017

View reviewed changes

LebedevRI reviewed Jan 10, 2017

View reviewed changes

LebedevRI requested changes Jan 10, 2017

View reviewed changes

shoffmeister reviewed Jan 13, 2017

View reviewed changes

LebedevRI self-assigned this Jan 14, 2017

LebedevRI requested changes Jan 14, 2017

View reviewed changes

Rawspeed: DngDecoder: add support for deflate decompression

c06aa70

LebedevRI approved these changes Jan 14, 2017

View reviewed changes

LebedevRI merged commit c06aa70 into darktable-org:develop Jan 14, 2017

LebedevRI mentioned this pull request Jan 17, 2017

[Q] Option to choose DNG compression? jcelaya/hdrmerge#86

Closed

LebedevRI mentioned this pull request Sep 18, 2019

Compressed/Uncompressed DNGs jcelaya/hdrmerge#191

Open

Rawspeed: DngDecoder: add support for deflate decompression #17

Rawspeed: DngDecoder: add support for deflate decompression #17

Conversation

anarsoul commented Jan 6, 2017

houz commented Jan 6, 2017

LebedevRI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarsoul Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 7, 2017 • edited Loading

Current coverage is 5.61% (diff: 0.00%)

anarsoul commented Jan 7, 2017

anarsoul commented Jan 7, 2017

LebedevRI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LebedevRI Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LebedevRI Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarsoul commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axxel commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LebedevRI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anarsoul Jan 6, 2017 •

edited

Loading

codecov-io commented Jan 7, 2017 •

edited

Loading

LebedevRI Jan 7, 2017 •

edited

Loading

LebedevRI Jan 10, 2017 •

edited

Loading

shoffmeister Jan 13, 2017 •

edited

Loading