Skip to content

Commit 4eca0ef

Browse files
authored
Fix checks for mismatched image attributes. (#466)
* Rejig the tests. * Ensure image oetf attribute is initialized. * Add tests. * Add warning() that accepts std:string.
1 parent 14284e7 commit 4eca0ef

5 files changed

Lines changed: 172 additions & 65 deletions

File tree

tests/toktx-tests.cmake

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,31 @@ add_test( NAME toktx-invalid-target-type
5858
COMMAND toktx --target_type RGBH a b
5959
)
6060

61+
add_test( NAME toktx-set-oetf-second-file-no-error
62+
COMMAND toktx --lower_left_maps_to_s0t0 --mipmap --nometadata --assign_oetf linear -- - ../srcimages/level0.ppm ../srcimages/level1.ppm ../srcimages/level2.ppm ../srcimages/level3.ppm ../srcimages/level4.ppm ../srcimages/level5.ppm ../srcimages/level6.ppm
63+
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/testimages
64+
)
65+
66+
add_test( NAME toktx-convert-oetf-second-file-no-error
67+
COMMAND toktx --lower_left_maps_to_s0t0 --mipmap --nometadata --convert_oetf linear -- - ../srcimages/level0.ppm ../srcimages/level1.ppm ../srcimages/level2.ppm ../srcimages/level3.ppm ../srcimages/level4.ppm ../srcimages/level5.ppm ../srcimages/level6.ppm
68+
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/testimages
69+
)
70+
71+
add_test( NAME toktx-change-target-type-second-file-no-error
72+
COMMAND toktx --lower_left_maps_to_s0t0 --mipmap --nometadata --target_type RGBA -- - ../srcimages/level0.ppm ../srcimages/level1.ppm ../srcimages/level2.ppm ../srcimages/level3.ppm ../srcimages/level4.ppm ../srcimages/level5.ppm ../srcimages/level6.ppm
73+
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/testimages
74+
)
75+
76+
add_test( NAME toktx-different-colortype-second-file-error
77+
COMMAND toktx --lower_left_maps_to_s0t0 --mipmap --nometadata -- - ../srcimages/level0.ppm ../srcimages/level1-alpha.pam ../srcimages/level2.ppm ../srcimages/level3.ppm ../srcimages/level4.ppm ../srcimages/level5.ppm ../srcimages/level6.ppm
78+
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/testimages
79+
)
80+
81+
add_test( NAME toktx-different-colortype-second-file-warning
82+
COMMAND toktx --lower_left_maps_to_s0t0 --mipmap --nometadata --target_type RGBA -- - ../srcimages/level0.ppm ../srcimages/level1-alpha.pam ../srcimages/level2.ppm ../srcimages/level3.ppm ../srcimages/level4.ppm ../srcimages/level5.ppm ../srcimages/level6.ppm
83+
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/testimages
84+
)
85+
6186
set_tests_properties(
6287
toktx-test-foobar
6388
toktx-automipmap-mipmaps
@@ -72,10 +97,17 @@ set_tests_properties(
7297
toktx-swizzle-gt-4
7398
toktx-invalid-swizzle-char
7499
toktx-invalid-target-type
100+
toktx-different-colortype-second-file-error
75101
PROPERTIES
76102
WILL_FAIL TRUE
77103
)
78104

105+
set_tests_properties(
106+
toktx-different-colortype-second-file-warning
107+
PROPERTIES
108+
PASS_REGULAR_EXPRESSION "toktx warning! \"../srcimages/level1-alpha.pam\" has a different colortype_e"
109+
)
110+
79111
function( gencmpktx test_name reference source args env files )
80112
if(files)
81113
add_test( NAME toktx-cmp-${test_name}

tools/toktx/image.hpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -321,9 +321,10 @@ class Image {
321321
virtual Image& copyToRGBA(Image&) = 0;
322322

323323
protected:
324-
Image() : width(0), height(0), primaries(KHR_DF_PRIMARIES_BT709) { }
324+
Image() : Image(0, 0) { }
325325
Image(uint32_t w, uint32_t h)
326-
: width(w), height(h), primaries(KHR_DF_PRIMARIES_BT709) { }
326+
: width(w), height(h), oetf(KHR_DF_TRANSFER_UNSPECIFIED),
327+
primaries(KHR_DF_PRIMARIES_BT709) { }
327328

328329
uint32_t width, height; // In pixels
329330
colortype_e colortype;

tools/toktx/npbmimage.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,8 @@ createFromPPM(FILE* src, bool transformOETF, Image::rescale_e rescale)
200200
image->transformOETF(decode_bt709, encode_linear);
201201
image->setOetf(KHR_DF_TRANSFER_LINEAR);
202202
}
203+
} else {
204+
image->setOetf(KHR_DF_TRANSFER_ITU);
203205
}
204206
return image;
205207
}
@@ -261,6 +263,8 @@ createFromPGM(FILE* src, bool transformOETF, Image::rescale_e rescale)
261263
image->transformOETF(decode_bt709, encode_linear);
262264
image->setOetf(KHR_DF_TRANSFER_LINEAR);
263265
}
266+
} else {
267+
image->setOetf(KHR_DF_TRANSFER_ITU);
264268
}
265269
return image;
266270
}
@@ -402,6 +406,8 @@ createFromPAM(FILE* src, bool transformOETF, Image::rescale_e rescale)
402406
image->transformOETF(decode_bt709, encode_linear);
403407
image->setOetf(KHR_DF_TRANSFER_LINEAR);
404408
}
409+
} else {
410+
image->setOetf(KHR_DF_TRANSFER_ITU);
405411
}
406412

407413
return image;

tools/toktx/pngimage.cc

Lines changed: 26 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -213,11 +213,6 @@ Image::CreateFromPNG(FILE* src, bool transformOETF, Image::rescale_e rescale)
213213
break;
214214
}
215215

216-
if (!transformOETF) {
217-
// User is overriding color space info from file.
218-
return image;
219-
}
220-
221216
// state will have been updated with the rest of the file info.
222217

223218
// Here is the priority of the color space info in PNG:
@@ -260,28 +255,34 @@ Image::CreateFromPNG(FILE* src, bool transformOETF, Image::rescale_e rescale)
260255
delete image;
261256
throw std::runtime_error("PNG file has gAMA of 0.");
262257
}
263-
// What PNG calls gamma is the power to use for encoding. Elsewhere
264-
// gamma is commonly used for the power to use for decoding.
265-
// For example by spec. the value in the PNG file is
266-
// gamma * 100000 so gamma of 45455 is .45455. The power for
267-
// decoding is the inverse, i.e 1 / .45455 which is 2.2.
268-
// The variable gamma below is for decoding and is 1 / gAMA.
269-
float gamma = (float) 100000 / state.info_png.gama_gamma;
270-
// 1.6667 is a very arbitrary cutoff.
271-
if (componentBits == 8 && gamma > 1.6667f) {
272-
image->transformOETF(decode_gamma, encode_sRGB, gamma);
273-
image->setOetf(KHR_DF_TRANSFER_SRGB);
274-
if (gamma > 3.3333f) {
275-
warning("Transformed PNG image with gamma of %f to sRGB"
276-
" gamma (~2.2)", gamma);
258+
if (transformOETF) {
259+
// What PNG calls gamma is the power to use for encoding.
260+
// Elsewhere gamma is commonly used for the power to use for
261+
// decoding. For example by spec. the value in the PNG file is
262+
// gamma * 100000 so gamma of 45455 is .45455. The power for
263+
// decoding is the inverse, i.e 1 / .45455 which is 2.2.
264+
// The variable gamma below is for decoding and is 1 / gAMA.
265+
float gamma = (float) 100000 / state.info_png.gama_gamma;
266+
// 1.6667 is a very arbitrary cutoff.
267+
if (componentBits == 8 && gamma > 1.6667f) {
268+
image->transformOETF(decode_gamma, encode_sRGB, gamma);
269+
image->setOetf(KHR_DF_TRANSFER_SRGB);
270+
if (gamma > 3.3333f) {
271+
warning("Transformed PNG image with gamma of %f to sRGB"
272+
" gamma (~2.2)", gamma);
273+
}
274+
} else {
275+
image->transformOETF(decode_gamma, encode_linear, gamma);
276+
image->setOetf(KHR_DF_TRANSFER_LINEAR);
277+
if (gamma > 1.3) {
278+
warning("Transformed PNG image with gamma of %f to"
279+
" linear", gamma);
280+
}
277281
}
278282
} else {
279-
image->transformOETF(decode_gamma, encode_linear, gamma);
280-
image->setOetf(KHR_DF_TRANSFER_LINEAR);
281-
if (gamma > 1.3) {
282-
warning("Transformed PNG image with gamma of %f to"
283-
" linear", gamma);
284-
}
283+
// User is overriding color space info from file.
284+
image->setOetf(KHR_DF_TRANSFER_UNSPECIFIED);
285+
return image;
285286
}
286287
}
287288
} else {

tools/toktx/toktx.cc

Lines changed: 105 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,7 @@ class toktxApp : public scApp {
337337

338338
void warning(const char *pFmt, va_list args);
339339
void warning(const char *pFmt, ...);
340+
void warning(const string&);
340341

341342
protected:
342343
virtual bool processOption(argparser& parser, int opt);
@@ -688,10 +689,22 @@ toktxApp::main(int argc, _TCHAR *argv[])
688689
unsigned int levelWidth=0, levelHeight=0, levelDepth=0;
689690
// These initializations are to avoid compiler warnings.
690691
khr_df_transfer_e chosenOETF = KHR_DF_TRANSFER_UNSPECIFIED;
691-
khr_df_transfer_e firstImageOETF = KHR_DF_TRANSFER_UNSPECIFIED;
692692
khr_df_primaries_e chosenPrimaries = KHR_DF_PRIMARIES_UNSPECIFIED;
693-
khr_df_primaries_e firstImagePrimaries = KHR_DF_PRIMARIES_UNSPECIFIED;
694-
Image::colortype_e firstImageColortype = Image::eRGB;
693+
struct _imageAttribs {
694+
khr_df_transfer_e oetf;
695+
khr_df_primaries_e primaries;
696+
uint32_t componentCount;
697+
bool oetfWarned;
698+
bool primariesWarned;
699+
bool componentCountWarned;
700+
} expectedAttribs = {
701+
KHR_DF_TRANSFER_UNSPECIFIED,
702+
KHR_DF_PRIMARIES_UNSPECIFIED,
703+
3,
704+
false,
705+
false,
706+
false
707+
};
695708
string defaultSwizzle;
696709

697710
processEnvOptions();
@@ -735,16 +748,20 @@ toktxApp::main(int argc, _TCHAR *argv[])
735748
// If input is > 8bit and user wants LDR issue quality loss warning
736749
if (options.astc && image->getComponentSize() > 1
737750
&& options.astcopts.mode == KTX_PACK_ASTC_ENCODER_MODE_LDR) {
738-
cerr << name << ": Warning! input file is 16bit but LDR option is specified."
739-
<< " Expect quality loss in the output."
740-
<< endl;
751+
stringstream msg;
752+
msg << "Input file is 16-bit but LDR option is specified. "
753+
<< "Expect quality loss in the output."
754+
<< endl;
755+
warning(msg.str());
741756
}
742757

743758
// If input is < 8bit and user wants HDR issue warning
744759
if (options.astc && image->getComponentSize() <= 1 &&
745760
options.astcopts.mode == KTX_PACK_ASTC_ENCODER_MODE_HDR) {
746-
cerr << name << ": Warning! input file is not 16bit but HDR option is specified."
747-
<< endl;
761+
stringstream msg;
762+
msg << "Input file is not 16-bit but HDR option is specified."
763+
<< endl;
764+
warning(msg.str());
748765
}
749766

750767
// If no astc mode option is specified and
@@ -756,11 +773,69 @@ toktxApp::main(int argc, _TCHAR *argv[])
756773
options.astcopts.mode = KTX_PACK_ASTC_ENCODER_MODE_HDR;
757774
}
758775

776+
// Check that all input files have matching oetf, primaries and
777+
// component count. Raise error or warning depending on attribute
778+
// and assign or convert options.
759779
if (i == 0) {
760780
// First file.
761-
firstImageOETF = image->getOetf();
762-
firstImagePrimaries = image->getPrimaries();
763-
firstImageColortype = image->getColortype();
781+
expectedAttribs.oetf = image->getOetf();
782+
expectedAttribs.primaries = image->getPrimaries();
783+
expectedAttribs.componentCount = image->getComponentCount();
784+
} else {
785+
// Subsequent files.
786+
if (image->getOetf() != expectedAttribs.oetf
787+
&& options.convert_oetf == KHR_DF_TRANSFER_UNSPECIFIED)
788+
{
789+
stringstream msg;
790+
msg << "\"" << infile << "\" is encoded with a "
791+
"different transfer function (OETF) than preceding "
792+
"file(s)." << endl;
793+
if (options.assign_oetf == KHR_DF_TRANSFER_UNSPECIFIED) {
794+
cerr << name << ": " << msg.str();
795+
exitCode = 1;
796+
goto cleanup;
797+
} else if (!expectedAttribs.oetfWarned) {
798+
warning(msg.str());
799+
expectedAttribs.oetfWarned = true;
800+
}
801+
// Don't warn when convert_oetf is set as proper conversions
802+
// will be done so all images will be in the same space.
803+
}
804+
if (image->getPrimaries() != expectedAttribs.primaries) {
805+
stringstream msg;
806+
msg << "\"" << infile << "\" has different color "
807+
"primaries than preceding file(s)." << endl;
808+
if (options.assign_primaries == KHR_DF_PRIMARIES_UNSPECIFIED) {
809+
cerr << name << ": " << msg.str();
810+
exitCode = 1;
811+
goto cleanup;
812+
} else if (!expectedAttribs.primariesWarned) {
813+
warning(msg.str());
814+
expectedAttribs.primariesWarned = true;
815+
}
816+
// There is no convert_primaries option.
817+
}
818+
if (image->getComponentCount() != expectedAttribs.componentCount) {
819+
stringstream msg;
820+
msg << "\"" << infile
821+
<< "\" has a different colortype_e"
822+
<< " (component count) than preceding file(s)."
823+
<< endl;
824+
if (options.targetType == commandOptions::eUnspecified) {
825+
cerr << name << ": " << msg.str();
826+
exitCode = 1;
827+
goto cleanup;
828+
} else if (!expectedAttribs.componentCountWarned) {
829+
msg << "The components of the level or layer derived "
830+
<< "from this file will likely be significantly "
831+
<< "different"
832+
<< endl
833+
<< "from those in other levels or layers."
834+
<< endl;
835+
warning(msg.str());
836+
expectedAttribs.componentCountWarned = true;
837+
}
838+
}
764839
}
765840

766841
if (options.assign_oetf != KHR_DF_TRANSFER_UNSPECIFIED) {
@@ -832,8 +907,9 @@ toktxApp::main(int argc, _TCHAR *argv[])
832907
if (options.targetType != commandOptions::eUnspecified) {
833908
if (options.targetType != (int)image->getComponentCount()) {
834909
Image* newImage = nullptr;
835-
// The following casts only work because the only case that will
836-
// be taken at runtime is the one where image is the same
910+
// The casts in the following copyTo* definitions only work
911+
// because, thanks to the switch, at runtime we always pass
912+
// the image type being cast to.
837913
if (image->getComponentSize() == 2) {
838914
switch (options.targetType) {
839915
case commandOptions::eR:
@@ -1084,26 +1160,6 @@ toktxApp::main(int argc, _TCHAR *argv[])
10841160
goto cleanup;
10851161
}
10861162
} else {
1087-
// Subsequent files.
1088-
if (image->getOetf() != firstImageOETF) {
1089-
cerr << name << ": \"" << infile << "\" is encoded with a "
1090-
"different transfer function (OETF) than preceding files."
1091-
<< endl;
1092-
exitCode = 1;
1093-
goto cleanup;
1094-
}
1095-
if (image->getPrimaries() != firstImagePrimaries) {
1096-
cerr << name << ": \"" << infile << "\" has different color "
1097-
"primaries than preceding files." << endl;
1098-
exitCode = 1;
1099-
goto cleanup;
1100-
}
1101-
if (image->getColortype() != firstImageColortype) {
1102-
cerr << name << ": \"" << infile << "\" has a different colortype_e"
1103-
<< " (component count) than preceding files." << endl;
1104-
exitCode = 1;
1105-
goto cleanup;
1106-
}
11071163
// Input file order is layer, faceSlice, level. This seems easier for
11081164
// a human to manage than the order in a KTX file. It keeps the
11091165
// base level images and their mip levels together.
@@ -1611,7 +1667,7 @@ toktxApp::processOption(argparser& parser, int opt)
16111667

16121668
void toktxApp::warning(const char *pFmt, va_list args) {
16131669
if (options.warn) {
1614-
cerr << name << " warning: ";
1670+
cerr << name << " warning! ";
16151671
vfprintf(stderr, pFmt, args);
16161672
cerr << endl;
16171673
}
@@ -1627,12 +1683,23 @@ void toktxApp::warning(const char *pFmt, ...) {
16271683
}
16281684
}
16291685

1686+
void toktxApp::warning(const string& msg) {
1687+
if (options.warn) {
1688+
cerr << name << " warning! ";
1689+
cerr << msg;
1690+
}
1691+
}
1692+
16301693
void warning(const char *pFmt, ...) {
1631-
va_list args;
1632-
va_start(args, pFmt);
1694+
va_list args;
1695+
va_start(args, pFmt);
1696+
1697+
theApp.warning(pFmt, args);
1698+
va_end(args);
1699+
}
16331700

1634-
theApp.warning(pFmt, args);
1635-
va_end(args);
1701+
void warning(const string& msg) {
1702+
theApp.warning(msg);
16361703
}
16371704

16381705
static ktx_uint32_t

0 commit comments

Comments
 (0)