While digging through the image reading code for PGM and PPM files, I 
noticed something strange regarding the handling of the "maxvalue" 
header field:


(A) In PPM files, the value appears to be consistently interpreted as 
the value that would represent 1.0. The same applies to PGM files with a 
"maxvalue" of 255 or less.

(B) For PGM files with a "maxvalue" of 256 or higher however, maxvalue 
appears to be effectively ignored, and the values interpreted as if 
65535 would represent 1.0.


Can someone confirm this in practice?

Any suggestions how to handle this? I would consider (B) a bug and 
change it - even at the cost of potentially breaking existing scenes, 
speculating that most files with a "maxvalue" > 255 do have a value of 
65535 anyway.

Post a reply to this message

clipka wrote:
> (A) In PPM files, the value appears to be consistently interpreted as 
> the value that would represent 1.0. The same applies to PGM files with a 
> "maxvalue" of 255 or less.
> 
> (B) For PGM files with a "maxvalue" of 256 or higher however, maxvalue 
> appears to be effectively ignored, and the values interpreted as if 
> 65535 would represent 1.0.
> 
> 
> Can someone confirm this in practice?
> 
> Any suggestions how to handle this? I would consider (B) a bug and 
> change it - even at the cost of potentially breaking existing scenes, 
> speculating that most files with a "maxvalue" > 255 do have a value of 
> 65535 anyway.

B is a bug and your speculation is valid.

There are indeed some (but very rare) PNM files that seem to assume 
"maxvalue" is the maximal value that *appears* within the image out of 
the full range of 0-65535 but this is a bug within the used PNM-writer 
and shouldn't become our problem.

-Ive

Post a reply to this message

Ive schrieb:
> There are indeed some (but very rare) PNM files that seem to assume 
> "maxvalue" is the maximal value that *appears* within the image out of 
> the full range of 0-65535 but this is a bug within the used PNM-writer 
> and shouldn't become our problem.

Good.

Another thing: In the various file readers, I frequently encounter the 
following construct:

   floatColor = ((intColor * 255) / intMax) / 255.0;

where intMax is derived from the bit depth of the color component.
Does this make any sense to you? Wouldn't it be more faithful to the 
encoded data to decode it as

   floatColor = float(intColor) / float(intMax) ?

My initial assumption was that it's just lazy coding, with the /255.0 
having been added when refactoring the code to use float values, but the 
frequent appearance makes me uneasy.

Post a reply to this message

clipka wrote:

> Another thing: In the various file readers, I frequently encounter the 
> following construct:
> 
>   floatColor = ((intColor * 255) / intMax) / 255.0;
> 
> where intMax is derived from the bit depth of the color component.
> Does this make any sense to you? Wouldn't it be more faithful to the 
> encoded data to decode it as
> 
>   floatColor = float(intColor) / float(intMax) ?
> 
> My initial assumption was that it's just lazy coding, with the /255.0 
> having been added when refactoring the code to use float values, but the 
> frequent appearance makes me uneasy.


Assuming floatColor was once a byteColor it really looks like a relict 
of the once used MulDiv approach. I cannot see a reason for keeping that.


Ahem, and in case you came across tiff.cpp, could you please change the line

if (PhotometricInterpretation == PHOTOMETRIC_PALETTE)

to

if ((PhotometricInterpretation == PHOTOMETRIC_PALETTE) && 
(TIFFIsTiled(tif) == 0))


This will prevent POV-Ray from having a few pointers running wild when 
someone tries to use a color palette TIF-file that happens to be tiled 
and is not arranged in strips.

-Ive

Post a reply to this message

Ive schrieb:

> Ahem, and in case you came across tiff.cpp, could you please change the 
> line
> 
> if (PhotometricInterpretation == PHOTOMETRIC_PALETTE)
> 
> to
> 
> if ((PhotometricInterpretation == PHOTOMETRIC_PALETTE) && 
> (TIFFIsTiled(tif) == 0))

Done. I have not much of an idea what it is for, but I trust you on this 
one :-)

I'll include that with the gamma handling stuff.

Post a reply to this message