Well, after some tests and thinking, it downed to me that I should just adjust the gamma curve, there is no point in multiplying everything with a constant multiplyer.
So my function is now like this, and it works (but not exactly how it does in RawTherapee)
I also want to pick a single gamma curve and apply those same remap value to hundreds or thousands of images. So having the ability to directly manipulate the libraw_colordata_t.curve[] data in lieu of defining gammas would be VERY nice. I find on 50mp canon cr2 images calculating the gamma curve remap per image is almost 15s per image on an ivy bridge xeon.
Hi Alex,
I'm the rawpy developer and just had a look at this. I'm using LibRaw::COLOR(row, col) to construct the color pattern matrix, which works fine for non-x-trans images. But for x-trans any value after the first row returns 0. so ::COLOR(1, 0) is 0, as well as ::COLOR(1,1) is 0. I'm using libraw 0.18.2 currently and noticed some x-trans related changes in newer versions: https://github.com/LibRaw/LibRaw/compare/0.18.2...0.18.5 But they seem to be related to security only.
My test image is: http://img.photographyblog.com/reviews/fujifilm_x_t1/photos/fujifilm_x_t...
Can you check whether the COLOR method works correctly here? I think there may be a problem.
Thanks!
Maik
Amazing, yes you are completely right, that's the reason it was always failing. In gcc, I added
LOCAL_CFLAGS += -fsigned-char
to the make file and it works perfectly and decodes like a charm! As I read, the default of a char is not really defined in c and depends on the architecture, so this error makes perfectly sens. But may there would be a way to explicitly init is as a signed char directly in code so that these things will not happen to others?
so after quite a lot of coffee, I found some kind of translation method to translate the stacktrace to real code lines. Here's the result:
********** Crash dump: **********
signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x7246192bb8
Routine fuji_decode_sample_odd(fuji_compressed_block*, fuji_compressed_params const*, unsigned short*, int, int_pair*) at libraw_fuji_compressed.cpp:490
Routine LibRaw::xtrans_decode_block(fuji_compressed_block*, fuji_compressed_params const*, int) at libraw_fuji_compressed.cpp:727
Routine LibRaw::fuji_decode_strip(fuji_compressed_params const*, int, long long, unsigned int) at libraw_fuji_compressed.cpp:919
Routine LibRaw::fuji_decode_loop(fuji_compressed_params const*, int, long long*, unsigned int*) at libraw_fuji_compressed.cpp:999
Routine LibRaw::fuji_compressed_load_raw() at libraw_fuji_compressed.cpp:983
Routine LibRaw::unpack() at libraw_cxx.cpp:2811 (discriminator 3)
Routine libraw_unpack at libraw_c_api.cpp:130
Routine Java_com_tssystems_Libraw_open at libraw.c:51
For me it seems like the access of the grads array via the gradient variable runs further than the array, but I'm not sure. What do you think? Does any output of the variables may help?
Just a note: The value of the gradient variable before it fails is 2272 which seems way higher than all others (they are around 250). Maybe there's something going wrong here?
// Edit: The really strange thing is that the Ra... Rg values change each time I run it and the crash finally ocurse. For instance, now I get
Ra 16383 Rb 0 Rc 0 Rd 0 Rg 0
or
Ra 0 Rb 0 Rc 0 Rd 0 Rg 849
before it will crash. Seems there is something really going wrong with the values?
For some other pretty strange reason it almost seems like something is running parallel, because I can't see the full var dump of the q_table in onces. But this could also be a transfering issue of the log in android since the buffer speed is somewhat limited? I don't have openmp enabled.
libraw_raw2image does NOT subtracts black level from image[] array, most likely this is the source of the problem (image looks dull and with magenta cast, right?)
My experience is very mixed with debugging c code on Android. However I’ll try to find a way to generate a dump or something similar to find out where it fails and will report back later.
Thank you very much for your fast response, looking at the data structure documentation for imgdata.idata.filters, I see this:
unsigned filters;
Bit mask describing the order of color pixels in the matrix (0 for full-color images). 32 bits of this field describe 16 pixels (8 rows with two pixels in each, from left to right and from top to bottom). Each two bits have values 0 to 3, which correspond to four possible colors. Convenient work with this field is ensured by the COLOR(row,column) function, which returns the number of the active color for a given pixel.
values less than 1000 are reserved as special cases:
1 - Leaf Catchlight with 16x16 bayer matrix;
9 - Fuji X-Trans (6x6 matrix)
3..8 and 10..999 - are unused.
Some points are not very clear to me:
-32 bits for 16 pixels, 2bits per pixels to code color, but what is the mapping ? (0->red, 1->green, 2->blue)
-What does this 1-->999 code means ?
Current libraw snapshot can run user-specified demosaic routine, just set interpolate_bayer pointer to point to your function.
Look into other demosaic functions code (e.g. lin_interpolate()) to see how to loop through all pixels.
To get bayer pattern, use imgdata.idata.filters variable, or COLOR(r,c) call.
I'm also a little irritated. I basically changed nothing and used the latest git (just from today). The only thing I changed for Android is to manually implement the "swab" function, but the implementation I use should be the same as the original from c. I could also see no calls on this function when the xtrans decoding is going on.
C++ standard defines arrays as dense arrays, no spacing or extra rows padding.
Also, because xtrans/_abs arrays are defined in LibRaw class definition, and it is possible that we compile two source files with different -O.. switches, array layout should not changed from -O0 (no warning) to -O2 (warning issued).
So, I think this is gcc problem: it should either show this warning with any -O.. setting, or do not show it with -O2
Anyway, this loop was already changed to to 6-wide loops in master branch with commit message 'to make gcc happy' :)
Yes, this will effectively map [0...max_value/exposure] to full output range.
Well, after some tests and thinking, it downed to me that I should just adjust the gamma curve, there is no point in multiplying everything with a constant multiplyer.
So my function is now like this, and it works (but not exactly how it does in RawTherapee)
void do_color_curve() { int i; double resulting_gamma; double max_color=original_exposure_max_channels*(1.0f/exposure); for(i=0;i<65535;i++) { double i_double=i; resulting_gamma=pow(i_double/(double)max_color,1.0f/gamma); if(resulting_gamma>1)resulting_gamma=1; color_curve[i]=resulting_gamma*255; } }I also want to pick a single gamma curve and apply those same remap value to hundreds or thousands of images. So having the ability to directly manipulate the libraw_colordata_t.curve[] data in lieu of defining gammas would be VERY nice. I find on 50mp canon cr2 images calculating the gamma curve remap per image is almost 15s per image on an ivy bridge xeon.
OK, looks like over-optimization in COLOR:
int COLOR(int row, int col) { return libraw_internal_data.internal_output_params.fuji_width ? FCF(row, col) : FC(row, col); } int FC(int row, int col) { return (imgdata.idata.filters >> (((row << 1 & 14) | (col & 1)) << 1) & 3); } int fcol(int row, int col);where FCF() is for 'fuji-rotated' images (S5 Pro, etc).
It should be
int COLOR(int row, int col) { return imgdata.idata.filters < 1000 ? fcol(row,col): (libraw_internal_data.internal_output_params.fuji_width ? FCF(row, col) : FC(row, col)); }To be fixed in 0.19 (0.18 is security-only-patches now)
Hi Alex,
I'm the rawpy developer and just had a look at this. I'm using LibRaw::COLOR(row, col) to construct the color pattern matrix, which works fine for non-x-trans images. But for x-trans any value after the first row returns 0. so ::COLOR(1, 0) is 0, as well as ::COLOR(1,1) is 0. I'm using libraw 0.18.2 currently and noticed some x-trans related changes in newer versions: https://github.com/LibRaw/LibRaw/compare/0.18.2...0.18.5 But they seem to be related to security only.
My test image is: http://img.photographyblog.com/reviews/fujifilm_x_t1/photos/fujifilm_x_t...
Can you check whether the COLOR method works correctly here? I think there may be a problem.
Thanks!
Maik
That was it, thank you very much!
I changed my code to:
libraw_raw2image(raw_struct);
libraw_subtract_black(raw_struct);
And it works very nicely :) (well, still have to do other things now, but the results are much better).
Yes, default char type (signed/unsigned) is undefined (although it is signed in all our test/production environments).
We'll change char definition to signed char (or, maybe, to int8_t, will see)
Thank you for reporting the bug.
Amazing, yes you are completely right, that's the reason it was always failing. In gcc, I added
LOCAL_CFLAGS += -fsigned-char
to the make file and it works perfectly and decodes like a charm! As I read, the default of a char is not really defined in c and depends on the architecture, so this error makes perfectly sens. But may there would be a way to explicitly init is as a signed char directly in code so that these things will not happen to others?
Thanks again!
it looks like I know the answer:
q_table is defined as:
char *q_table; /* quantization table */
So, signed char, from -127 to 128.
It is initialized with ..-4...+4 values at lines 110-130.
Is there any chance that your char type is by default unsigned char?
Hi Alex,
so after quite a lot of coffee, I found some kind of translation method to translate the stacktrace to real code lines. Here's the result:
For me it seems like the access of the grads array via the gradient variable runs further than the array, but I'm not sure. What do you think? Does any output of the variables may help?
Just a note: The value of the gradient variable before it fails is 2272 which seems way higher than all others (they are around 250). Maybe there's something going wrong here?
Thanks
Torsten
Hi,
here are some dumps:
// Edit: The really strange thing is that the Ra... Rg values change each time I run it and the crash finally ocurse. For instance, now I get
before it will crash. Seems there is something really going wrong with the values?
For some other pretty strange reason it almost seems like something is running parallel, because I can't see the full var dump of the q_table in onces. But this could also be a transfering issue of the log in android since the buffer speed is somewhat limited? I don't have openmp enabled.
Both 2272 and 250 are too high values for gradient variable b/c table size is only 41.
gradient is calculated by:
q_table contains values from -4 to +4, so maximum gradient value is 40.
Could you please try to dump q_table values? And, also, values used to calc gradient: Rb - Rf, Rc - Rb
Dear Sir:
libraw_raw2image does NOT subtracts black level from image[] array, most likely this is the source of the problem (image looks dull and with magenta cast, right?)
no_auto_scale completely disables LibRaw::scale_colors() step
This is not intended for common image processing using dcraw_process, this option is special-purpose
Hi Alex,
My experience is very mixed with debugging c code on Android. However I’ll try to find a way to generate a dump or something similar to find out where it fails and will report back later.
Thanks
Torsten
We always welcome raw samples.
To send us files, you may use any file sharing service (WeTransfer, Dropbox, Google Drive, etc) and send link to it to info@libraw.org.
Please use simple public links (available for anyone, who knows the URL), but not 'Share for account' feature
0...999 range is reserved for future custom layouts, like Fuji 6x6.
32 bits describes 16 pixels, 4x4 block. For all today cameras it is repeated 2x2 blocks.
00 - red, 01 - green, 10 - blue, 11 - green2 channels
(just use LibRaw::COLOR(row,col) call to get layout)
Thank you very much for your fast response, looking at the data structure documentation for imgdata.idata.filters, I see this:
unsigned filters;
Bit mask describing the order of color pixels in the matrix (0 for full-color images). 32 bits of this field describe 16 pixels (8 rows with two pixels in each, from left to right and from top to bottom). Each two bits have values 0 to 3, which correspond to four possible colors. Convenient work with this field is ensured by the COLOR(row,column) function, which returns the number of the active color for a given pixel.
values less than 1000 are reserved as special cases:
1 - Leaf Catchlight with 16x16 bayer matrix;
9 - Fuji X-Trans (6x6 matrix)
3..8 and 10..999 - are unused.
Some points are not very clear to me:
-32 bits for 16 pixels, 2bits per pixels to code color, but what is the mapping ? (0->red, 1->green, 2->blue)
-What does this 1-->999 code means ?
I will take a look at the source code tomorrow
Current libraw snapshot can run user-specified demosaic routine, just set interpolate_bayer pointer to point to your function.
Look into other demosaic functions code (e.g. lin_interpolate()) to see how to loop through all pixels.
To get bayer pattern, use imgdata.idata.filters variable, or COLOR(r,c) call.
Is there any way to get exact code point that fails with exception? maybe coredump or so?
Hi Alex,
yes, it's the line, I put it behind the bracket:
{__android_log_print(ANDROID_LOG_INFO,"libraw","decode_block %d %d %d",g_even_pos,g_odd_pos,line_width); ...This is the output I see:
I'm also a little irritated. I basically changed nothing and used the latest git (just from today). The only thing I changed for Android is to manually implement the "swab" function, but the implementation I use should be the same as the original from c. I could also see no calls on this function when the xtrans decoding is going on.
Regards
Torsten
Run the file through valgrind (under FreeBSD), do not see any fuji-decoder problems (buffer overrun, etc), so no idea.
BTW, what exact version do you use (in my working version line 712 is { bracket after while operator)?
1) If use_camera_matrix value suggests to use camera (raw metadata) color, than this color data is copied over rgb_cam array at open_file() stage.
2) If camera matrix is not available, embedded color data (from adobe_coeff() large array) is used.
Camera color matrix is parsed into imgdata.color.cmatrix
Sorry, wrong. If use_camera_matrix is set, so camera matrix should be used, camera color matrix is copied to rgb_cam
C++ standard defines arrays as dense arrays, no spacing or extra rows padding.
Also, because xtrans/_abs arrays are defined in LibRaw class definition, and it is possible that we compile two source files with different -O.. switches, array layout should not changed from -O0 (no warning) to -O2 (warning issued).
So, I think this is gcc problem: it should either show this warning with any -O.. setting, or do not show it with -O2
Anyway, this loop was already changed to to 6-wide loops in master branch with commit message 'to make gcc happy' :)
Pages