Data conversion for raw volumes #356
Conversation
There was a problem hiding this comment.
The code looks ok, but could you please add in the commit message and documentation what is the expected benefit of this feature from a user perspective? Even after reviewing it I have no idea how or why I would want to use that. Is a very lossy way to save some GPU memory?
| * with format being one of: char, int8, unsigned char, uint8, short, int16, | ||
| * unsigned short, uint16, int, int32, unsigned int, | ||
| * uint32, float | ||
| * with formats being one of: char, int8, unsigned char, uint8, short, int16, |
There was a problem hiding this comment.
not clear now, please rename formats. does it refer to format or inputFormat from the line above?
When should one specify the inputFormat, when is it optional?
There was a problem hiding this comment.
format is for both. Changed to 'input format' and documented defaults
There was a problem hiding this comment.
OK, just (?output=format) which should be inside () just like (,input format) to show that it is optional
| if( _inputType == DT_UINT32 && _outputType == DT_UINT8 ) | ||
| return _scale< uint8_t, uint32_t >( ptr, size ); | ||
| if( _inputType == DT_UINT32 && _outputType == DT_UINT16 ) | ||
| return _scale< uint16_t, uint32_t >( ptr, size ); |
There was a problem hiding this comment.
reverse the order: template< class O, class I > -> template< class I, class O> so that it reads in the logical order (uint32_t ->uint16_t ) ?
How is hopefully clear from the doxygen. Why: If you need it, you'll find it. This is only an increment to a full solution, which I needed to render a uint16 data set. The pressing issue was that this produces a black image today in Livre (and @chevtche was not around), but it also is faster since it uses 1.5GB instead of 3GB. Visually I would guess it doesn't make a big difference, since the images I got are very nice already. And 1.5 vs 3GB is also a bit more then 'some GPU memory' ;). The full solution would be a generic decorator for all data source doing the conversion, but this is a bigger can of worms. |
There was a problem hiding this comment.
Thanks for the information, I was missing parts of that. Having it in the commit message makes the reviewer's life easier than trying to reverse-engineer the programmer's intentions :-) And in this case the doxygen changes were rather minimalistic. As you also point out, the uses cases for this feature are limited, and mostly motivated as a work-around for a rendering bug. The memory gain offered by the requantization only helps if the input volume is in the range [1x; 2x] the GPU memory, beyond that an LOD structure is needed anyway. And not all volumes will support such a hard compression. It is probably fine on very continuous fields such as LFP, but could have bad effects on a medical image stack where the extra bits code for different types of tissue or structures. It is probably better to change the volume generation toolchain to output an 8-bit volume directly in the cases where the 16-bits of precision are not necessary, but I guess that's up to the user in the end.
| * with format being one of: char, int8, unsigned char, uint8, short, int16, | ||
| * unsigned short, uint16, int, int32, unsigned int, | ||
| * uint32, float | ||
| * with formats being one of: char, int8, unsigned char, uint8, short, int16, |
There was a problem hiding this comment.
OK, just (?output=format) which should be inside () just like (,input format) to show that it is optional
|
Maybe we should fix the 16-bit bug instead of downscaling ? It's very sad to lose informations... I just tested 16-bit volume with the memory data source and it's working. I think the bug is in the raw datasource loader. |
|
Both. Sometimes downscaling is no issue, as the visual result is still good enough at 2x the performance:
|
No description provided.