Closed agriggio closed 6 years ago
@agriggio Alberto, great. Currently the whole image is detected as motion. To see the pixelshift result you have to set Motion correction
to off. Then you see that there is something wrong.
I will check that now.
@agriggio About the filters
variable. Currently it can fit only for one of the 4 frames.
The whole multi-frame code assumes that all frames have the same filter.
Assume you have this pattern in the first frame:
RGRG
GBGB
then you will have for example
GBGB
RGRG
in the second frame.
The simplest solution would be to shift the image in sony_arq_load_raw()
based on shot_select
so that all 4 images can use the same filters variable. Don't worry about the pixel shift code. If the frames are shifted the same way Pentax does, the code automatically will shift back correctly.
@agriggio Alberto, I played a bit with the arq files.
This:
void CLASS sony_arq_load_raw()
{
int row, col, bits=0;
ushort samples[4];
while (1 << ++bits < maximum);
for (row=0; row < raw_height; row++) {
int c0 = FC(row, 0);
int c1 = FC(row, 1);
int cng = c0 & 1 ? c1 : c0;
cng ^= 1;
for (col=0; col < raw_width; col++) {
read_shorts(samples, 4);
cng ^= 1;
RAW(row,col) = samples[cng];
if ((RAW(row,col) >>= load_flags) >> bits
&& (unsigned) (row-top_margin) < height
&& (unsigned) (col-left_margin) < width) derror();
}
}
}
gives a perfect first frame. Of course it works only for the first frame because it does not use shot_select. It's just a base ...
@heckflosse I arrived to the same conclusion, essentially :-) Here's my version:
void CLASS sony_arq_load_raw()
{
int row, col, bits=0;
ushort samples[4];
while (1 << ++bits < maximum);
for (row=0; row < raw_height; row++) {
for (col=0; col < raw_width; col++) {
read_shorts(samples, 4);
RAW(row,col) = samples[(shot_select + (2 * (row & 1)) + (col & 1)) % 4];
if ((RAW(row,col) >>= load_flags) >> bits
&& (unsigned) (row-top_margin) < height
&& (unsigned) (col-left_margin) < width) derror();
}
}
}
Now I'll play with the shifting as you suggested above!
@agriggio Alberto, hint: In the arq file I linked in the pixls.us thread there are at least two regions with motion. You can use this regions to check whether you combine the channels correctly for the individual frames. Currently frame 1 and 3 are combined correctly while frame 2 and 4 are not. Here's a screenshot which shows a region with motion:
@heckflosse Ingo, here's a version that works (should, at least :-) for all the 4 individual frames:
void CLASS sony_arq_load_raw()
{
int row, col, bits=0;
ushort samples[4];
while (1 << ++bits < maximum);
for (row=0; row < ((shot_select < 2) ? 1 : raw_height); row++) {
for (col=0; col < ((row == 0) ? raw_width : 1); col++) {
RAW(row,col) = 0;
}
}
for (row=0; row < raw_height; row++) {
int r = row + (shot_select & 1);
for (col=0; col < raw_width; col++) {
int c = col + ((shot_select >> 1) & 1);
read_shorts(samples, 4);
if (r < raw_height && c < raw_width) {
RAW(r,c) = samples[(2 * (r & 1)) + (c & 1)];
if ((RAW(r,c) >>= load_flags) >> bits
&& (unsigned) (row-top_margin) < height
&& (unsigned) (col-left_margin) < width) derror();
}
}
}
}
@heckflosse Ingo, I was too curious and so I started playing with the pixelshift code. I used the following hack to make it work... I hope you don't mind!
diff --git a/rtengine/pixelshift.cc b/rtengine/pixelshift.cc
--- a/rtengine/pixelshift.cc
+++ b/rtengine/pixelshift.cc
@@ -295,6 +295,43 @@
}
+class RawDataFrameReorder {
+public:
+ typedef array2D<float> *array2D_ptr;
+ RawDataFrameReorder(const std::string &model, array2D_ptr *rawDataFrames):
+ model_(model),
+ frames_(rawDataFrames)
+ {
+ if (model_ == "ILCE-7RM3") {
+ std::swap(frames_[2], frames_[3]);
+ }
+ }
+
+ ~RawDataFrameReorder()
+ {
+ if (model_ == "ILCE-7RM3") {
+ std::swap(frames_[2], frames_[3]);
+ }
+ }
+
+ unsigned int getframe(unsigned int frame)
+ {
+ if (model_ == "ILCE-7RM3") {
+ if (frame == 2) {
+ return 3;
+ } else if (frame == 3) {
+ return 2;
+ }
+ }
+ return frame;
+ }
+
+private:
+ std::string model_;
+ array2D_ptr *frames_;
+};
+
+
}
using namespace std;
@@ -307,6 +344,9 @@
return;
}
+ RawDataFrameReorder reorder_frames(model, rawDataFrames);
+ frame = reorder_frames.getframe(frame);
+
RAWParams::BayerSensor bayerParams = bayerParamsIn;
bayerParams.pixelShiftAutomatic = true;
@agriggio Alberto, great! I posted some screenshots in the pixls.us thread
@heckflosse thanks. I've also added a patch for basic pixel-shifting using the good old dcraw :-) (of course, no motion correction though)
Now we need that PS symbol on the thumbs of arq ps files...
@heckflosse should we create a arq-pixelshift
branch?
@agriggio Alberto, yes, please :)
@heckflosse just pushed branch sony-pixel-shift
@agriggio great, thank you. I will push a change to remove Pentax
from the bundled profiles Pentax Pixel Shift
folder
@heckflosse Ingo, would you consider exposing also the camera make (and not just the model) in RawImageSource::pixelshift
? That might be a bit more future-proof... (low priority of course)
@agriggio pushed
Here's a little change for better code locality and that might prevent static analyzer warnings concerning implicit copy and assignment operators:
diff --git a/rtengine/pixelshift.cc b/rtengine/pixelshift.cc
index 875f87a2..17572556 100644
--- a/rtengine/pixelshift.cc
+++ b/rtengine/pixelshift.cc
@@ -294,50 +294,55 @@ void calcFrameBrightnessFactor(unsigned int frame, uint32_t datalen, LUT<uint32_
}
+}
-class RawDataFrameReorder {
-public:
- typedef array2D<float> *array2D_ptr;
- RawDataFrameReorder(const std::string &model, array2D_ptr *rawDataFrames):
- model_(model),
- frames_(rawDataFrames)
- {
- if (model_ == "ILCE-7RM3") {
- std::swap(frames_[2], frames_[3]);
- }
- }
+using namespace std;
+using namespace rtengine;
- ~RawDataFrameReorder()
+void RawImageSource::pixelshift(int winx, int winy, int winw, int winh, const RAWParams::BayerSensor &bayerParamsIn, unsigned int frame, const std::string &make, const std::string &model, float rawWpCorrection)
+{
+ class RawDataFrameReorder :
+ public NonCopyable
{
- if (model_ == "ILCE-7RM3") {
- std::swap(frames_[2], frames_[3]);
- }
- }
+ public:
+ using array2DPtr = array2D<float>*;
- unsigned int getframe(unsigned int frame)
- {
- if (model_ == "ILCE-7RM3") {
- if (frame == 2) {
- return 3;
- } else if (frame == 3) {
- return 2;
+ RawDataFrameReorder(const std::string& model, array2DPtr* frames) :
+ model_(model),
+ frames_(frames)
+ {
+ if (model_ == "ILCE-7RM3") {
+ std::swap(frames_[2], frames_[3]);
}
}
- return frame;
- }
-private:
- std::string model_;
- array2D_ptr *frames_;
-};
+ ~RawDataFrameReorder()
+ {
+ if (model_ == "ILCE-7RM3") {
+ std::swap(frames_[2], frames_[3]);
+ }
+ }
+ unsigned int getFrameNumber(unsigned int frame_number)
+ {
+ if (model_ == "ILCE-7RM3") {
+ switch (frame_number) {
+ case 2: {
+ return 3;
+ }
-}
+ case 3: {
+ return 2;
+ }
+ }
+ }
+ return frame_number;
+ }
-using namespace std;
-using namespace rtengine;
-void RawImageSource::pixelshift(int winx, int winy, int winw, int winh, const RAWParams::BayerSensor &bayerParamsIn, unsigned int frame, const std::string &make, const std::string &model, float rawWpCorrection)
-{
+ private:
+ const std::string model_;
+ array2DPtr* const frames_;
+ };
if(numFrames != 4) { // fallback for non pixelshift files
amaze_demosaic_RT(winx, winy, winw, winh, rawData, red, green, blue);
@@ -345,7 +350,7 @@ void RawImageSource::pixelshift(int winx, int winy, int winw, int winh, const RA
}
RawDataFrameReorder reorder_frames(model, rawDataFrames);
- frame = reorder_frames.getframe(frame);
+ frame = reorder_frames.getFrameNumber(frame);
RAWParams::BayerSensor bayerParams = bayerParamsIn;
I didn't dare to turn RawDataFrameReorder::model_
into a const std::string&
as I don't have the files and time to test, but it should be feasible in this context.
Best, Flössie
Thanks @Floessie for the review. FWIW, I think the class is better put in the anonymous namespace than inside the body of the pixelshift
method, which is already quite long and complex.
I don't mind. Function local classes have their certain charm when a class is only used in that function (especially as they are allowed to call private functions of the surrounding class), but I think code- and linker-wise it's the same.
So here's another version, which is now also const
-correct:
diff --git a/rtengine/pixelshift.cc b/rtengine/pixelshift.cc
index 875f87a2..9997b8c6 100644
--- a/rtengine/pixelshift.cc
+++ b/rtengine/pixelshift.cc
@@ -21,11 +21,14 @@
////////////////////////////////////////////////////////////////
#include <cmath>
-#include "rawimagesource.h"
-#include "../rtgui/multilangmgr.h"
-#include "procparams.h"
+
#include "gauss.h"
#include "median.h"
+#include "noncopyable.h"
+#include "procparams.h"
+#include "rawimagesource.h"
+
+#include "../rtgui/multilangmgr.h"
namespace
{
@@ -294,13 +297,15 @@ void calcFrameBrightnessFactor(unsigned int frame, uint32_t datalen, LUT<uint32_
}
-
-class RawDataFrameReorder {
+class RawDataFrameReorder :
+ public rtengine::NonCopyable
+{
public:
- typedef array2D<float> *array2D_ptr;
- RawDataFrameReorder(const std::string &model, array2D_ptr *rawDataFrames):
+ using array2DPtr = array2D<float>*;
+
+ RawDataFrameReorder(const std::string& model, array2DPtr* frames) :
model_(model),
- frames_(rawDataFrames)
+ frames_(frames)
{
if (model_ == "ILCE-7RM3") {
std::swap(frames_[2], frames_[3]);
@@ -314,38 +319,41 @@ public:
}
}
- unsigned int getframe(unsigned int frame)
+ unsigned int getFrameNumber(unsigned int frame_number) const
{
if (model_ == "ILCE-7RM3") {
- if (frame == 2) {
- return 3;
- } else if (frame == 3) {
- return 2;
+ switch (frame_number) {
+ case 2: {
+ return 3;
+ }
+
+ case 3: {
+ return 2;
+ }
}
}
- return frame;
+ return frame_number;
}
private:
- std::string model_;
- array2D_ptr *frames_;
+ const std::string model_;
+ array2DPtr* const frames_;
};
-
}
using namespace std;
using namespace rtengine;
+
void RawImageSource::pixelshift(int winx, int winy, int winw, int winh, const RAWParams::BayerSensor &bayerParamsIn, unsigned int frame, const std::string &make, const std::string &model, float rawWpCorrection)
{
-
if(numFrames != 4) { // fallback for non pixelshift files
amaze_demosaic_RT(winx, winy, winw, winh, rawData, red, green, blue);
return;
}
- RawDataFrameReorder reorder_frames(model, rawDataFrames);
- frame = reorder_frames.getframe(frame);
+ const RawDataFrameReorder reorder_frames(model, rawDataFrames);
+ frame = reorder_frames.getFrameNumber(frame);
RAWParams::BayerSensor bayerParams = bayerParamsIn;
Best, Flössie
@heckflosse Ingo, on this file: https://filebin.net/w6vabcr6ndktteo7/Sony_a7RIII_motion_pixelshift_longexposure.ARQ
reordering of the frames is not performed because the camera is wrongly reported as ILCE-7RM2
inside pixelshif
. Yet, RT shows it to be ILCE-7RM3
in the info panel... any idea what is going on?
-TIFF-IFD0:Model=ILCE-7RM3
-TIFF-IFD0:Software=ILCE-7RM3 v1.00
-TIFF-IFD0-ExifIFD-MakerNotes:SonyModelID=ILCE-7RM2
-TIFF-IFD1:Model=ILCE-7RM3
-TIFF-IFD1:Software=ILCE-7RM3 v1.00
Could it be that someone changed the SonyModelID
tag intentionally, as suggested here?
http://www.fredmiranda.com/forum/topic/1514359/74#14265492
Note that you need the latest version of ExifTool (not sure exactly which - it doesn't work in 10.64 but works in 10.68) if you want to set the tag to ILCE-7RM3
, but ILCE-7RM2
can be set in older versions.
Or you can probably do it in an older version by not using PrintConv, knowing that
ILCE-7RM2
= 347
and ILCE-7RM3
= 362
.
So:
./exiftool -overwrite_original_in_place -SonyModelID="ILCE-7RM3" Sony_a7RIII_motion_pixelshift_longexposure.ARQ
@Beep6581 ah that could definitely be. I've seen that trick being suggested on several places indeed... :frowning_face:
Ok I worked around it. However, there's still something we are missing I think. I tried with the Sony software, and although the result is full of motion artifacts, on areas without motion it shows an incredible amount of detail. I just cannot get there with RT, and I tried hard... here's an example:
RT:
Sony
Here's my pp3. Any hint is appreciated! Sony_a7RIII_motion_pixelshift_longexposure.txt
@agriggio Alberto, at a first look the noise reduction median clearly destroys some detail
@heckflosse but if I don't do that I get an annoying grid pattern.... I really tried a bunch of things, and I'm still doing...
@agriggio Alberto, here's my pp3 Sony_a7RIII_motion_pixelshift_longexposure_ingo.ARQ.txt
Left is with my pp3, right is with yours
@agriggio false colour suppression is your friend. It removes a lot of the grid pattern
@heckflosse Ingo, the pp3 you linked is identical to mine -- are you sure you uploaded the right one? Anyway, looking at your screenshot I agree it's more detailed, but also noisier. I see a lot of white pixels. I still think we are missing something compared to sony (which is totally understandable at this stage, of course). I just want to understand what, exactly :-)
false colour suppression is your friend. It removes a lot of the grid pattern
A-HA!! I thought this was totally useless for pixelshift, I would have never considered it! Here's a new comparison, old vs new (disable median filter, enable 1 step of false color suppression):
OLD
NEW
Now we're talking ;-)
BTW, here's my take on the reordering cleanup, which hopefully makes @Floessie happy :yum:
diff --git a/rtengine/dcraw.cc b/rtengine/dcraw.cc
--- a/rtengine/dcraw.cc
+++ b/rtengine/dcraw.cc
@@ -2353,19 +2353,21 @@
// RT
void CLASS sony_arq_load_raw()
{
+ static unsigned frame2pos[] = { 0, 1, 3, 2 };
int row, col, bits=0;
ushort samples[4];
+ unsigned frame = frame2pos[shot_select];
while (1 << ++bits < maximum);
- for (row=0; row < ((shot_select < 2) ? 1 : raw_height); row++) {
+ for (row=0; row < ((frame < 2) ? 1 : raw_height); row++) {
for (col=0; col < ((row == 0) ? raw_width : 1); col++) {
RAW(row,col) = 0;
}
}
for (row=0; row < raw_height; row++) {
- int r = row + (shot_select & 1);
+ int r = row + (frame & 1);
for (col=0; col < raw_width; col++) {
- int c = col + ((shot_select >> 1) & 1);
+ int c = col + ((frame >> 1) & 1);
read_shorts(samples, 4);
if (r < raw_height && c < raw_width) {
RAW(r,c) = samples[(2 * (r & 1)) + (c & 1)];
diff --git a/rtengine/pixelshift.cc b/rtengine/pixelshift.cc
--- a/rtengine/pixelshift.cc
+++ b/rtengine/pixelshift.cc
@@ -294,44 +294,6 @@
}
-
-class RawDataFrameReorder {
-public:
- typedef array2D<float> *array2D_ptr;
- RawDataFrameReorder(const std::string &model, array2D_ptr *rawDataFrames):
- model_(model),
- frames_(rawDataFrames)
- {
- if (model_ == "ILCE-7RM3") {
- std::swap(frames_[2], frames_[3]);
- }
- }
-
- ~RawDataFrameReorder()
- {
- if (model_ == "ILCE-7RM3") {
- std::swap(frames_[2], frames_[3]);
- }
- }
-
- unsigned int getframe(unsigned int frame)
- {
- if (model_ == "ILCE-7RM3") {
- if (frame == 2) {
- return 3;
- } else if (frame == 3) {
- return 2;
- }
- }
- return frame;
- }
-
-private:
- std::string model_;
- array2D_ptr *frames_;
-};
-
-
}
using namespace std;
@@ -344,9 +306,6 @@
return;
}
- RawDataFrameReorder reorder_frames(model, rawDataFrames);
- frame = reorder_frames.getframe(frame);
-
RAWParams::BayerSensor bayerParams = bayerParamsIn;
bayerParams.pixelShiftAutomatic = true;
@agriggio it was the wrong file. Basically I used false colour suppression, a bit of cbdl and in custom motion correction I set the blur radius to 5 and ISO adaption to 2. The white pixels are caused by too strong settings for microcontrast.
Edit: I also used Equalize brightness of frames
in custom mode.
@heckflosse thanks Ingo, as I wrote above just replacing a median (denoise) with another one (false colour suppression) did the trick :wink:
@agriggio
BTW, here's my take on the reordering cleanup, which hopefully makes @Floessie happy 😋
Well, not completely 😉. If you change the static
in static unsigned frame2pos[] = { 0, 1, 3, 2 };
into constexpr
you'll save a whopping 72 bytes in the release build binary. 😋
user@machine:~/src/rawtherapee/build$ ls -la release/rawtherapee
-rwxr-xr-x 1 user user 13227992 Dez 13 15:36 release/rawtherapee
user@machine:~/src/rawtherapee/build$ ls -la release/rawtherapee
-rwxr-xr-x 1 user user 13227920 Dez 13 15:37 release/rawtherapee
@agriggio
which hopefully makes @Floessie happy
me too, because now the shift of the frames when using e.g. Amaze and switching frames is the same as for Pentax PS files (down, right, up)
me too, because now the shift of the frames when using e.g. Amaze and switching frames is the same as for Pentax PS files (down, right, up)
Well I hope @Floessie doesn't get upset if I admit this (i.e. consistent behaviour) was the main reason for the change :-)
Absolutely not! 😄 The best days are those when verbose code is replaced by terse but readable one. And consistency is 👍.
Hi guys .. very nice so far .. since I just managed to recover my ability to compile RT I'll be more active to contribute my part :)
As it is now RT use the nRead & ePerIso data from PentaxK70 which is a bit wrong to apply on ILCE-7RM3. Bill Claff provides preliminary data so I copied them .. http://www.photonstophotos.net/Charts/RN_e.htm#Sony%20ILCE-7RM3_14
Please insert at line 507 ..
// preliminary ILCE-7RM3 data, good fidelity except from A) small innaccuracy at places
// due to integer scaling quantization, B) much different noise behavior of PDAF pixels
static const float nReadILCE7RM3[] = { 4.2f, // ISO 100
3.9f, // ISO 125
3.6f, // ISO 160
3.55f, // ISO 200
3.5f, // ISO 250
3.45f, // ISO 320
3.35f, // ISO 400
3.3f, // ISO 500
1.3f, // ISO 640
1.2f, // ISO 800
1.2f, // ISO 1000
1.2f, // ISO 1250
1.15f, // ISO 1600
1.2f, // ISO 2000
1.15f, // ISO 2500
1.15f, // ISO 3200
1.1f, // ISO 4000
1.1f, // ISO 5000
1.05f, // ISO 6400
1.05f, // ISO 8000
1.05f, // ISO 10000
1.0f, // ISO 12800
1.0f, // ISO 16000
1.0f, // ISO 20000
1.0f, // ISO 25600
1.0f, // ISO 32000
1.0f, // ISO 40000
1.0f, // ISO 51200
1.1f, // ISO 64000
1.1f, // ISO 80000
1.1f, // ISO 102400
};
static const float ePerIsoILCE7RM3 = 0.8f;
Also insert at line 533
} else if(model.find("ILCE-7RM3") != string::npos) {
nRead = nReadILCE7RM3[nReadIndex];
eperIsoModel = ePerIsoILCE7RM3;
.. and the related camconst.json item ..
{ // Quality C,
"make_model": "Sony ILCE-7RM3",
"dcraw_matrix": [ 6640,-1847,-503,-5238,13010,2474,-993,1673,6527 ], // DNG_v10.1 D65
"raw_crop": [ 0, 0, -36, 0 ], // full raw frame 8000x5320 - 36 rightmost columns are garbage
"ranges": { "black": 512, "white": 16300 }
},
@iliasg
since I just managed to recover my ability to compile RT I'll be more active to contribute my part :)
That's really good news 👍
RawTherapee now supports ARQ files and @agriggio even wrote the python program to generate them - this is a 5.4 blocker, can we close?
Could you summarize the status of RT's ARQ support - is anything missing?
we can close for me. we have full support for arq (at least based on the samples I could try). what is missing is a GUI for combining 4 arw into one arq (or for letting RT work on 4 arw directly), but as you said I wrote a script to do that. not ideal, but seems to do the job
Great, thanks.
I started playing with the ARQ file that was submitted via pixls yesterday. I was able to make some progress but I'm not 100% there yet. This patch makes RT load the file and successfully identify that it has 4 frames. The colours are still wrong however. I suspect I'm not setting the value for dcraw's
filters
variable correctly. But hopefully someone more knowledgeable can figure it out?BTW, I'm using this entry for
camconst.json
(basically copied from A7RII):Just as a sign of hope, here's a screenshot:
Running pixelshift doesn't crash, but it doesn't seem to do anything either (I mean I see no improvement in detail).