Support for Pentax pixel shift files

heckflosse commented 8 years ago

Pentax K70 supports pixel shift mode. In this mode 4 shifted frames are recorded and available in the raw file. How it works In first step we could allow access to each of the frames in rt. That's easy, as dcraw already supports that. We would need a frame selector somewhere in gui of the raw demosaic tab and pass the value (1,2,3,4) to dcraw.

I will make a new branch with a prototype to allow testing.

In second step we can define a new 'demosaic' method pixel shift which combines the 4 frames to one image.

agriggio commented 7 years ago

Disclaimer: haven't tried yet. But wouldn't be better to gray out the tab instead of hiding it? I would find it clearer...

Beep6581 commented 7 years ago

@heckflosse great!

We need to agree as a team on how to handle the Editor tab's toolbox regarding choices which are invalid at a given time - do we hide them or gray them out? Remember that the File Browser's toolbox shows all options.

I've been thinking about this sporadically for some time and I'm of the opinion that it's better to gray them out instead of hiding them. That means that the user won't be surprised by tools and options appearing and disappearing depending on the condition, i.e. tools which depend on type of raw file, tools which depend on raw vs non-raw file, tools which influence other tools.

agriggio commented 7 years ago

@Beep6581 I vote for grey out, FWIW

heckflosse commented 7 years ago

I'm ambivalent about hiding or graying out. I will push the changes (with hide mode) to psgtk3 branch soon. Then everybody can test and we can discuss.

Personally for the raw tab I prefer to gray out instead of hide when loading non raw files. For the sensor specific panels I prefer to hide instead of gray out because most users have either an xtrans cam or a bayer cam. Very few have both types.

Desmis commented 7 years ago

@heckflosse

I agree with your last proposition :)

heckflosse commented 7 years ago

I pushed my changes to psgtk3 branch.

For raw tab I used 'gray out' For sensor specific panels I used 'hide'

But of course we can change that :)

Here's a link to an xtrans file for testing.

And here you can find a file with 2 frames in case someone wants to test that the sub-image selector now corresponds to the number of sub-images in a file Hint: For whatever reason this file downloads with extension tif. You have to change the extension to cr2

agriggio commented 7 years ago

I prefer to hide instead of gray out because most users have either an xtrans cam or a bayer cam. Very few have both types.

I have to admit (for the nth time) that I'm really new to photography, but I had no idea that bayer vs xtrans was as polarizing as vi vs emacs :smile:

More seriously though, I still prefer greying out, but it's not a strong preference.

heckflosse commented 7 years ago

I guess (not tried yet) if we gray out for example the xtrans panel, we have to collapse it first, because once it's grayed out we can not close it anymore and that's not what we want. Or should we let the expander active and only gray out the corresponding frame?

agriggio commented 7 years ago

Or should we let the expander active and only gray out the corresponding frame?

I vote for this, yes.

Beep6581 commented 7 years ago

After testing the latest psgtk3 I admit that I like things the way they are, but there are certain benefits to not hiding the bayer/x-trans demosaicing tool entirely:

Seeing both but with only one usable means it's clear to the user that, as an example, enabling auto CA correction in one will not enable it in the other. When the X-Trans Demosaicing tool is hidden and the Bayer visible, or vice-versa, the user might think the two are the same tool and be confused.
Seeing the grayed-out sub-frame combo when loading a raw photo with only one frame means users will know that the option is there.
Consistency requires that we do the same to other tools/widgets based on whether they are usable with the currently loaded image. Currently we're only judging whether hiding demosaicing-related tools makes sense, but by making a decision here we are in fact (if we decide to go with consistency) committing to impose this decision on other tools which may or may not exist yet. That makes the choice quite difficult :)

Cons:

Graying out doesn't help reduce the UI. Hiding is cleaner.
Seeing a disabled sub-frame combo means they will have to figure out why it's disabled.

heckflosse commented 7 years ago

Seems neither me nor @agriggio nor @Beep6581 are really sure about the desired behaviour concerning hide/gray out of sensor specific tools. For that reason I would also like to get opinions from users: @michaelezra and @sguyader : Please comment

Hombre57 commented 7 years ago

It's easy to collapse a Tool before graying it out.

However if we look at some widgets in tools, we support both method with the following general rule (not sure it's always the case, but it will give an idea) :

if a combobox is swapping from one mode to another, the resulting widget displays the according widgets only, i.e. some widgets will be hidden or shown depending on the active mode
if a checkbox enable/disable something, that thing is made sensitive or unsensitive

We could consider the image type or sensor type as a mode, and then show the according widgets only. So with this scheme in mind, depending on the image type, the Raw tab should be hidden for standard images, and the unused XTrans or Bayer subframe too.

Note that Everything has to be shown (nothing hidden!) in the Batch Editor tools. It would be too complex to handle if you select multiple image type.

But that's just my 2 cents.

heckflosse commented 7 years ago

Ok, now we have 6 opinions.

I prefer to gray out the raw tab for non raw (tiff, jpeg) files and to hide the unused sensor specific panels.
@Hombre57 prefers to hide both
@Beep6581 seems to be ambivalent
@agriggio seems to prefer graying out both
@sguyader prefers hiding the unused sensor specific panels but is ambivalent about the raw tab
@Desmis prefers hiding the unused sensor specific panels but didn't comment about the raw tab

sguyader commented 7 years ago

I'm bit ambivalent too regarding the raw tab: At first I was in favor of hiding the raw tab altogether for kon-raw images, but now I wouldn't mind tinder it simply greyed out. I'm also in favor of hiding the unused sensor-specific panels.

heckflosse commented 7 years ago

@sguyader Sébastien, thanks for your comment! I updated my post

agriggio commented 7 years ago

After reading other people's opinions, and thinking about this a bit more, I slightly changed my mind :-) I vote for greying out the RAW tab, but hiding the sensor-specific stuff (and the choice about multiple frames for single-frame RAWs)

Beep6581 commented 7 years ago

I walked the dog, I had my morning coffee, I sat on a bench and watched the birds, I thought things over, concluded that what works best in one case does not work best in another, and I decided that as I'm unable to come up with one solution which is ideal in all cases I'm fine with whatever you choose.

gaaned92 commented 7 years ago

Sorry to interfere, but I put my one cent. I should prefer:

to hide functions or tab that are never used with a kind of image (e.g. raw tab for jpeg image, bayer sensor for xtrans...) in order to avoid to bloat the gui
to grey functions that are context sensitive (they are active or not depending on other settings) André

heckflosse commented 7 years ago

Ok, after reading all the comments again I came to the conclusion that the approach of psgtk3 branch (hiding unused sensor specific panels and graying out raw tab for non-raw images) is at least better than to show them always. I would like to merge psgtk3 into dev again soon. Any objections?

Desmis commented 7 years ago

I agree

No objections :) jacques

heckflosse commented 7 years ago

Merged!

Beep6581 commented 7 years ago

Saving a partial profile with nothing selected results in the file containing this extra data:

[RAW]
CAAutoStrength=2

[RAW Bayer]
PixelShiftMotion=0
PixelShiftMotionCorrection=5
PixelShiftMotionCorrectionMethod=1
pixelShiftStddevFactorGreen=5
pixelShiftStddevFactorRed=5
pixelShiftStddevFactorBlue=5
PixelShiftEperIso=0
PixelShiftNreadIso=0
PixelShiftPrnu=1
PixelShiftSigma=1
PixelShiftSum=3
PixelShiftRedBlueWeight=0.69999999999999996
PixelShiftShowMotion=false
PixelShiftShowMotionMaskOnly=false
pixelShiftAutomatic=true
pixelShiftNonGreenHorizontal=false
pixelShiftNonGreenVertical=false
pixelShiftHoleFill=true
pixelShiftMedian=false
pixelShiftMedian3=false
pixelShiftGreen=true
pixelShiftBlur=true
pixelShiftSmoothFactor=0.69999999999999996
pixelShiftExp0=false
pixelShiftLmmse=false
pixelShiftEqualBright=false
pixelShiftNonGreenCross=true
pixelShiftNonGreenCross2=false
pixelShiftNonGreenAmaze=false

heckflosse commented 7 years ago

Hmmm :(

heckflosse commented 7 years ago

@Hombre57 I need your help for the Pixel Shift partial profile issue reported by @Beep6581

Hombre57 commented 7 years ago

@heckflosse Looking...

heckflosse commented 7 years ago

@Hombre57 Thank you :+1:

heckflosse commented 7 years ago

@Hombre57 Feel free to push to dev. I've no changes on hold currtently.

Hombre57 commented 7 years ago

@heckflosse In the future, could you please keep some consistency when naming procparams values ? Some of them has a lower case "P" first letter in the pp3. I've changed 2 first letter capitalization in the code, but can't do that for the pp3 without breaking compatibility, and I don't want to clutter the ProcParams::load for this. I'll be ready to push in few minutes.

heckflosse commented 7 years ago

@Hombre57 Yes, I will do that in future!

Hombre57 commented 7 years ago

@heckflosse Patch committed.

heckflosse commented 7 years ago

@Hombre57 :+1: I knew that I can count on you! Thank you very much!!!

Hombre57 commented 7 years ago

It's fast when it's easy 😉 I have 3 opened patch but they're more complicated than expected...

heckflosse commented 7 years ago

:) What's easy for you is not easy for me (and vice versa)!

Beep6581 commented 7 years ago

but can't do that for the pp3 without breaking compatibility

There is no stable release with PS yet so it's fine to break compatibility in a dev build.

Hombre57 commented 7 years ago

@Beep6581 Could we mention in release notes of 5.1 that pp3 for pixelshift are not compatible to 5.0 dev version ? In that case I could clean that up, other better leave it as is.

Beep6581 commented 7 years ago

@Hombre57 we never promised to preserve PP3 compatibility across dev builds. In fact it is even explicitly stated in the dev release notes that compatibility will not be preserved (first section, second-last point). So go ahead and break.

May I add, bon anniversaire! :birthday: :tada:

michaelezra commented 6 years ago

It may be worthwhile extending this functionality to use any number of frames from any camera and create a super-resolution output image. Even simplest median averaging would be great! Advanced with suppression of moving artifacts is a "+"!:)

In the UI - user selects multiple images, right/click/super-resolution merge

Hombre57 commented 6 years ago

@michaelezra You mean stitching images to create a super-resolution image ? That would be nice, but please open a new issue.

Beep6581 / RawTherapee

Support for Pentax pixel shift files #3489