Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save 8 bit b/w images using PseudoGrey Plus #3244

Closed
wants to merge 2 commits into from
Closed

Save 8 bit b/w images using PseudoGrey Plus #3244

wants to merge 2 commits into from

Conversation

Floessie
Copy link
Collaborator

@Floessie Floessie commented Apr 7, 2016

I rediscovered Pat David's article on PseudoGrey recently and came across PseudoGrey Plus on Rich Franzen's site. I implemented a little stand-alone converter first and tried it successfully on 16 bit TIFFs exported from RT. Now here's an implementation directly for RT. It might be a nice (though small) USP for what is already an excellent piece of software.

PseudoGrey Plus by Rich Franzen is an algorithm to store 3110 levels of gray in 24 bit color images. This is simply done by adding 1 or 2 to the R, G, and/or B values to adjust the pixel's luminance. It's a subtle but perceivable effect and helps to prevent banding.

This implementation adds a new method isBW() to IImage, which is only implemented for Image16 and defaults to false. It's only called once per saving and has a shortcut to determine that the image is not b/w.

The minimally invasive core algorithm was added to rtengine/imageio.cc and is only called when saving an 8 bit image that is b/w.

Please note that when saving as JPEG, compression squashes the effect and a quality of 95+ and 4:4:4 subsampling is recommended. Many PseudoGrey sample image on the net suffer from this and do not really show the benefit.

return res;
}

void scanlineToPseudoGrey (const unsigned short *in16, unsigned char *out8, unsigned long width)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be more appropriate to use the fixed-width integer types from the cstdint header here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. As I saw no <cstdint> include, but the use of unsigned short, I went for the latter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So no reason to repeat mistakes of the past? I would really prefer this to use fixed-width types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's okay for you, then it's okay for me. :) I'll change that to the better.

PseudoGrey Plus by Rich Franzen is an algorithm to store 3110 levels of
gray in 24 bit color images. This is simply done by adding 1 or 2 to the
R, G, and/or B values to adjust the pixel's luminance. It's a subtle but
perceivable effect and helps to prevent banding.

This implementation adds a new method isBW() to IImage, which is only
implemented for Image16 and defaults to false. It's only called once per
saving and has a shortcut to determine that the image is NOT b/w.

The minimally invasive core algorithm was added to rtengine/imageio.cc
and is only called when saving an 8 bit image that is b/w.

Please note that when saving as JPEG, compression squashes the effect
and a quality of 95+ and 4:4:4 subsampling is recommended. Many
PseudoGrey sample image on the net suffer from this and do not really
show the benefit.
- use `std::uint16_t`
- move `addSat()` to `rt_math`
- de-inline `ImageDatas`
- fix `Image16::isBW()`
- remove `volatile` from `Image16::savePNG()`
template<
typename T,
typename = typename std::enable_if<std::is_integral<T>::value>::type,
typename = typename std::enable_if<std::is_unsigned<T>::value>::type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! :-)

@heckflosse
Copy link
Collaborator

I had a look at processing times. 'scanlineToPseudoGrey' adds about 10 to 15% to the processing time of the jpeg save loop. I think that's fine as it is now. There are still more important parts to speed up in rt ;-)

Ingo

@heckflosse
Copy link
Collaborator

Currently it seems like pseudogray implementation is mandatory for 8-bit per channel output. We really should have to make it optional for backwards compatibility and for those who want really monochrome output. Any Ideas where to place the gui control (checkbox) are welcome

@adamreichold
Copy link
Contributor

@heckflosse @Floessie AFAIU, the only consumers that would see reduced quality due to this are people who are doing numerical processing with the resulting images, i.e. scientific applications. But I doubt that all the image processing heuristics employed within RT make it suitable for that task anyway.
Is there any downside to doing this with images intended for human consumption? If so, I think the performance & quality tab of the preferences dialog would be suitable. But even then, I would probably enable it by default.
While we are there: There is an option to "serialize" TIFF image I/O which maps to options.serializeTiffRead in the code which leads to a static mutex being taken in ImageIO::loadTIFF. Isn't this PROTECT_VECTORS all over again? Meaning either the code is correct without locking or it isn't. Or are there really different libTIFF variants in use, some of which are thread-safe and some are not?

@Floessie
Copy link
Collaborator Author

Floessie commented Apr 9, 2016

@heckflosse: I have to hook up with @adamreichold in this matter. But if there has to be a checkbox, I would expect to see it in the "Save" dialogs below or besides the target format selection and only enabled for 8 bit formats. Or in the place, @adamreichold mentioned.

@heckflosse
Copy link
Collaborator

@adamreichold About the serialized tiff read: #2704

@heckflosse
Copy link
Collaborator

@adamreichold @Floessie There was an Issue some time ago about b&w images not being completely monochrome. For that reason we have this force b&w code. For me an option in preferences would do it. I'm also fine with enabling it by default.

@adamreichold
Copy link
Contributor

@heckflosse The forum links from #2704 seem to be dead so I cannot follow up on the story. Was it about performance or about correctness? And from the comments I wonder under what circumstances one would disable the setting?

@adamreichold
Copy link
Contributor

@heckflosse Besides the question on in the intention of the issue about images not being mathematically grayscale and the quite heavy handed solution of forcing the pixel values, shouldn't we then rather extend the B&W parameters with an explicit force option which would enable the mentioned code and disable the pseudo-gray code as well?
But again without that earlier discussion it is hard for me to decide whether we really should do that forcing at all. What were the problems that led to the forcing? Coloured artifacts from other processing steps? The need to numerically post-process?

@heckflosse
Copy link
Collaborator

@adamreichold I don't know about the reasons for the b&w forcing. But without it, pseudobw isBW() check would not be possible ;-) For that reason, your suggested solution (disabling pseudogray when explicit force option is set) is not possible too.
I'll try to find the Issue which led to the b&w force code.

The SerializeTiffread was about performance. When opening a folder full of tiff files or opening more than one tiff at once in METM, reading one tiff per thread from HD is much slower than the serialized read. On SSD that's not a problem. For that reason I made it optional.

@heckflosse
Copy link
Collaborator

@adamreichold I found the Issue
Currently 'pseudogray' only works with b&w tool in rt (because only for b&w tool b&w is forced). It would be nice to have pseudogray also for b&w film simulations and the other methods to generate a b&w image.

@Floessie
Copy link
Collaborator Author

Floessie commented Apr 9, 2016

@heckflosse: An option in preferences sounds like a sane solution.

@adamreichold
Copy link
Contributor

@heckflosse While isBW does indeed need perfect grayscale input, I rather was thinking of hooking this directly into the B&W effect, i.e. if B&W is enabled but forcing is not, image is always saved using pseudo-gray, if B&W and forcing are enabled, pseudo-gray is not used, and if B&W is disabled pseudo gray is not used (or it could still be done using isBW but RT will probably never produce perfectly grayscale images without B&W and forcing enabled anyway?).

One open issue I see here is whether the pseudo-gray algorithm will work properly with practically but not mathematical grayscale input?

Concerning the method of grayscale coercion, I was wondering if we should use something that is perceptibly weighted like qGray instead of just setting r = b = g?

Concerning the serialized TIFF read, in other I/O heavy processing tools, I have seen options to limit the number of I/O threads used. We could use something similar, at least controlling the number of concurrent I/O tasks we perform, as even SSD have only so many ports internally and also spinning disks can sustain concurrent streams in some RAID configurations? But that's probably a wholly different topic...

@adamreichold
Copy link
Contributor

@heckflosse If we make pseudo-gray an explicit choice based on the processing parameters we could also enable it for B&W film simulation as long as we are able to determine if the HaldCLUT profile in use is actually simulating B&W. So we would probably be more flexible in general compared to use something like isBW.

@Floessie
Copy link
Collaborator Author

Floessie commented Apr 9, 2016

@adamreichold: As to the open issue of mathematical grayscale input: PseudoGrey takes only 12 bits into account. The lowest nibble is discarded anyway. So relaxing isBW() (or adding the number of significant bits as a parameter) could help.

@heckflosse: If the outcome of film simultation is not absolutely R == B == G, how do we know, this wasn't intended by the HaldCLUT? One could test the HaldCLUT for isBW(), but that's too extensive for my taste. Why not stick to the isBW() metric (perhaps relaxed to 12 significant bits) to keep things simple. PseudoGrey isn't that much of a benefit to kick up a storm.

@heckflosse
Copy link
Collaborator

@Floessie Testing HaldCLUT for isBW() could be done without much overhead here after loading the file and before colour space conversion. At this point we should know whether the HaldCLUT is intended to be b&w. Processing time of this check should be quite low because size of HaldCLUT files usually is less than 3 Mp. Additionally when checking HaldCLUT for isBW() and not checking the image we want to be b&w for isBW() but making pseudo-gray based on processing parameters (as suggested by @adamreichold) instead we would save more processing time than we need to check isBW() for the HaldCLUT.
@adamreichold About the qGray. AFAIK the current force bw code is made to eliminate very small differences between channels in b&w images. So setting r = b = g should do it. But maybe I'm wrong here.

Ingo

@Beep6581
Copy link
Owner

Hello

As stated already, some users explicitly asked that RawTherapee's grayscale images use R=G=B, something to do with printing, possibly the printer using color ink instead of black when it detects colors in what looks like a grayscale image.

I don't mind the merge, but:

  • it would have to be optional, I would expect to see such an option in the Save As window (Grayscale/PseudoGray) where it can be used on a per-image basis rather than in Preferences,
  • it would be nice to see a convincing argument for its use. Does having 1786 or 3110 "unique" colors/shades of pseudo-gray vs 256 really matter? I don't see banding in an 8-bit R=G=B grayscale image on the monitor. If the usefulness only comes out when strongly manipulating curves or levels in GIMP, would not salting the image with slight noise in GIMP accomplish a similar result? Who would want to strongly manipulate an 8-bit TIFF image anyway? This won't benefit users saving JPEG for web. It would be nice to see an actual example rather than text.

@adamreichold

The forum links from #2704 seem to be dead so I cannot follow up on the story.

Try this: http://rawtherapee.com/oldforum/viewtopic.php?f=1&t=5949

There are more ways of achieving B&W than just using the B&W tool. HaldCLUT has already been mentioned, but there's also desaturation via the RGB Saturation slider, via the L_a_b* Saturation slider and CC/CL curves, etc. Also remember that you can use Film Simulation to achieve B&W which you then subsequently color-tone... Putting this in the Save As window seems more simple to me.

@Floessie
Copy link
Collaborator Author

Hello DrSlony,

here are some files for you to judge. The questions you pose are absolutely justified. Depending on the image I perceived a little effect, but whether or not that's enough for integrating the feature into RT is up to you, of course. Perhaps we could ask Pat, what he thinks about PseudoGrey as he has obviously tested it.

When I first wrote the code, I wondered why RT didn't save grayscale as 8 bit files instead of 24 bit ones and had the impression RT could do more with the 24 bits. As I had read about PseudoGrey on Pat's site long before pixls.us started, I thought, that could be beneficial. Well, later it turned out, that you actively clamp grayscale to R=G=B for the reasons you mentioned above, so I do no longer know if PseudoGrey really has a stand here.

Best,
Flössie

@Beep6581
Copy link
Owner

I took a look at those files. A colorcube analysis shows that the PG version of the fort photo has 3271 unique colors while GS has 255. Despite the numbers, I see no visual difference.

Setting the white point to 80 out of 255 (31%) in a Levels tool while working in 8-bit precision leads to this:
GS:
scrot_2016-04-13 233937

PG:
scrot_2016-04-13 233946

Doing the same in 32-bit floating point mode, setting Levels white point to 31%, leads to:
GS:
scrot_2016-04-13 234947

PG:
scrot_2016-04-13 235002

Again no visual difference, with the same hair comb in the histogram. As a result, I see no compelling reason for implementing this.

@Floessie
Copy link
Collaborator Author

Thanks for your thorough analysis and @adamreichold and @heckflosse for looking into it! So PseudoGrey seemingly doesn't live up to its promise.

See you,
Flössie

@Floessie Floessie closed this Apr 14, 2016
@Floessie Floessie deleted the pseudogrey branch April 14, 2016 10:55
@Beep6581
Copy link
Owner

I will run one more test on a downscaled image later today, just because I'm very curious, but it seems like a case of trying very hard to find a problem for a solution instead of a solution which helps with a common problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants