Skip to content

Commit 40642eb

Browse files
lgritzlgritz
committed
dpx: some refactoring for test coverage and clarity
Looking at line-by-line test coverage for the dpx reader, I noticed some refactoring I wanted to do. Switch to a table lookup template rather than complex case statements. (Less "code", simple static table lookups from data.) get_keycode_values() had opportunity to greatly simplify using some of our string functions. Signed-off-by: Larry Gritz <[email protected]>
1 parent 024c856 commit 40642eb

File tree

1 file changed

+117
-141
lines changed

1 file changed

+117
-141
lines changed

‎src/dpx.imageio/dpxinput.cpp‎

Lines changed: 117 additions & 141 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
// SPDX-License-Identifier: Apache-2.0
33
// https://github.com/AcademySoftwareFoundation/OpenImageIO
44

5+
#include <algorithm>
56
#include <cmath>
67
#include <iomanip>
78
#include <memory>
9+
#include <utility>
810

911
#include <OpenEXR/ImfTimeCode.h> //For TimeCode support
1012

@@ -86,7 +88,7 @@ class DPXInput final : public ImageInput {
8688

8789
/// Helper function - fill int array with KeyCode values
8890
///
89-
void get_keycode_values(int* array);
91+
void get_keycode_values(span<int> array);
9092

9193
/// Helper function - convert Imf::TimeCode to string;
9294
///
@@ -174,6 +176,22 @@ DPXInput::open(const std::string& name, ImageSpec& newspec,
174176

175177

176178

179+
// Helper, given key and a span of (KEY,VAL) pairs, look up which item
180+
// matches. If no key is found, then return the defaultval.
181+
template<typename KEY, typename VAL>
182+
const VAL&
183+
lookup(const KEY& key, cspan<std::pair<KEY, VAL>> values, const VAL& defaultval)
184+
{
185+
OIIO_ASSERT(values.size() >= 2);
186+
auto found = std::find_if(values.begin(), values.end(),
187+
[&key](const std::pair<KEY, VAL>& v) {
188+
return v.first == key;
189+
});
190+
return (found != values.end()) ? found->second : defaultval;
191+
}
192+
193+
194+
177195
bool
178196
DPXInput::seek_subimage(int subimage, int miplevel)
179197
{
@@ -294,18 +312,18 @@ DPXInput::seek_subimage(int subimage, int miplevel)
294312
// bits per pixel
295313
m_spec.attribute("oiio:BitsPerSample", m_dpx.header.BitDepth(subimage));
296314
// image orientation - see appendix B.2 of the OIIO documentation
297-
int orientation;
298-
switch (m_dpx.header.ImageOrientation()) {
299-
case dpx::kLeftToRightTopToBottom: orientation = 1; break;
300-
case dpx::kRightToLeftTopToBottom: orientation = 2; break;
301-
case dpx::kLeftToRightBottomToTop: orientation = 4; break;
302-
case dpx::kRightToLeftBottomToTop: orientation = 3; break;
303-
case dpx::kTopToBottomLeftToRight: orientation = 5; break;
304-
case dpx::kTopToBottomRightToLeft: orientation = 6; break;
305-
case dpx::kBottomToTopLeftToRight: orientation = 8; break;
306-
case dpx::kBottomToTopRightToLeft: orientation = 7; break;
307-
default: orientation = 0; break;
308-
}
315+
static const std::pair<dpx::Orientation, int> orientation_table[]
316+
= { { dpx::kLeftToRightTopToBottom, 1 },
317+
{ dpx::kRightToLeftTopToBottom, 2 },
318+
{ dpx::kLeftToRightBottomToTop, 4 },
319+
{ dpx::kRightToLeftBottomToTop, 3 },
320+
{ dpx::kTopToBottomLeftToRight, 5 },
321+
{ dpx::kTopToBottomRightToLeft, 6 },
322+
{ dpx::kBottomToTopLeftToRight, 8 },
323+
{ dpx::kBottomToTopRightToLeft, 7 } };
324+
int orientation
325+
= lookup<dpx::Orientation, int>(m_dpx.header.ImageOrientation(),
326+
orientation_table, 1);
309327
m_spec.attribute("Orientation", orientation);
310328

311329
m_spec.attribute("oiio:subimages", (int)m_dpx.header.ImageElementCount());
@@ -495,51 +513,31 @@ DPXInput::seek_subimage(int subimage, int miplevel)
495513
m_spec.attribute("dpx:FilmEdgeCode", filmedge);
496514
}
497515

498-
tmpstr.clear();
499-
switch (m_dpx.header.Signal()) {
500-
case dpx::kUndefined: tmpstr = "Undefined"; break;
501-
case dpx::kNTSC: tmpstr = "NTSC"; break;
502-
case dpx::kPAL: tmpstr = "PAL"; break;
503-
case dpx::kPAL_M: tmpstr = "PAL-M"; break;
504-
case dpx::kSECAM: tmpstr = "SECAM"; break;
505-
case dpx::k525LineInterlace43AR:
506-
tmpstr = "YCbCr ITU-R 601-5 525i, 4:3";
507-
break;
508-
case dpx::k625LineInterlace43AR:
509-
tmpstr = "YCbCr ITU-R 601-5 625i, 4:3";
510-
break;
511-
case dpx::k525LineInterlace169AR:
512-
tmpstr = "YCbCr ITU-R 601-5 525i, 16:9";
513-
break;
514-
case dpx::k625LineInterlace169AR:
515-
tmpstr = "YCbCr ITU-R 601-5 625i, 16:9";
516-
break;
517-
case dpx::k1050LineInterlace169AR: tmpstr = "YCbCr 1050i, 16:9"; break;
518-
case dpx::k1125LineInterlace169AR_274:
519-
tmpstr = "YCbCr 1125i, 16:9 (SMPTE 274M)";
520-
break;
521-
case dpx::k1250LineInterlace169AR: tmpstr = "YCbCr 1250i, 16:9"; break;
522-
case dpx::k1125LineInterlace169AR_240:
523-
tmpstr = "YCbCr 1125i, 16:9 (SMPTE 240M)";
524-
break;
525-
case dpx::k525LineProgressive169AR: tmpstr = "YCbCr 525p, 16:9"; break;
526-
case dpx::k625LineProgressive169AR: tmpstr = "YCbCr 625p, 16:9"; break;
527-
case dpx::k750LineProgressive169AR:
528-
tmpstr = "YCbCr 750p, 16:9 (SMPTE 296M)";
529-
break;
530-
case dpx::k1125LineProgressive169AR:
531-
tmpstr = "YCbCr 1125p, 16:9 (SMPTE 274M)";
532-
break;
533-
case dpx::k255:
534-
// don't set the attribute at all
535-
break;
536-
default:
537-
tmpstr = Strutil::fmt::format("Undefined {}",
538-
(int)m_dpx.header.Signal());
539-
break;
540-
}
541-
if (!tmpstr.empty())
542-
m_spec.attribute("dpx:Signal", tmpstr);
516+
static const std::pair<dpx::VideoSignal, const char*> signal_table[] = {
517+
{ dpx::kUndefined, "Undefined" },
518+
{ dpx::kNTSC, "NTSC" },
519+
{ dpx::kPAL, "PAL" },
520+
{ dpx::kPAL_M, "PAL-M" },
521+
{ dpx::kSECAM, "SECAM" },
522+
{ dpx::k525LineInterlace43AR, "YCbCr ITU-R 601-5 525i, 4:3" },
523+
{ dpx::k625LineInterlace43AR, "YCbCr ITU-R 601-5 625i, 4:3" },
524+
{ dpx::k525LineInterlace169AR, "YCbCr ITU-R 601-5 525i, 16:9" },
525+
{ dpx::k625LineInterlace169AR, "YCbCr ITU-R 601-5 625i, 16:9" },
526+
{ dpx::k1050LineInterlace169AR, "YCbCr 1050i, 16:9" },
527+
{ dpx::k1125LineInterlace169AR_274, "YCbCr 1125i, 16:9 (SMPTE 274M)" },
528+
{ dpx::k1250LineInterlace169AR, "YCbCr 1250i, 16:9" },
529+
{ dpx::k1125LineInterlace169AR_240, "YCbCr 1125i, 16:9 (SMPTE 240M)" },
530+
{ dpx::k525LineProgressive169AR, "YCbCr 525p, 16:9" },
531+
{ dpx::k625LineProgressive169AR, "YCbCr 625p, 16:9" },
532+
{ dpx::k750LineProgressive169AR, "YCbCr 750p, 16:9 (SMPTE 296M)" },
533+
{ dpx::k1125LineProgressive169AR, "YCbCr 1125p, 16:9 (SMPTE 274M)" },
534+
{ dpx::k255, nullptr /* don't set the attribute at all */ },
535+
};
536+
const char* signal
537+
= lookup<dpx::VideoSignal, const char*>(m_dpx.header.Signal(),
538+
signal_table, "Undefined");
539+
if (signal)
540+
m_spec.attribute("dpx:Signal", signal);
543541

544542
// read in user data; don't bother if the buffer is already filled (user
545543
// data is per-file, not per-element)
@@ -619,125 +617,103 @@ DPXInput::read_native_scanlines(int subimage, int miplevel, int ybegin,
619617
std::string
620618
DPXInput::get_characteristic_string(dpx::Characteristic c)
621619
{
622-
switch (c) {
623-
case dpx::kUserDefined: return "User defined";
624-
case dpx::kPrintingDensity: return "Printing density";
625-
case dpx::kLinear: return "Linear";
626-
case dpx::kLogarithmic: return "Logarithmic";
627-
case dpx::kUnspecifiedVideo: return "Unspecified video";
628-
case dpx::kSMPTE274M: return "SMPTE 274M";
629-
case dpx::kITUR709: return "ITU-R 709-4";
630-
case dpx::kITUR601: return "ITU-R 601-5 system B or G";
631-
case dpx::kITUR602: return "ITU-R 601-5 system M";
632-
case dpx::kNTSCCompositeVideo: return "NTSC composite video";
633-
case dpx::kPALCompositeVideo: return "PAL composite video";
634-
case dpx::kZLinear: return "Z depth linear";
635-
case dpx::kZHomogeneous: return "Z depth homogeneous";
636-
case dpx::kADX: return "ADX";
637-
case dpx::kUndefinedCharacteristic:
638-
default: return "Undefined";
639-
}
620+
// using Val = std::pair<dpx::Characteristic, const char*>
621+
static const std::pair<dpx::Characteristic, const char*> table[] {
622+
{ dpx::kUserDefined, "User defined" },
623+
{ dpx::kPrintingDensity, "Printing density" },
624+
{ dpx::kLinear, "Linear" },
625+
{ dpx::kLogarithmic, "Logarithmic" },
626+
{ dpx::kUnspecifiedVideo, "Unspecified video" },
627+
{ dpx::kSMPTE274M, "SMPTE 274M" },
628+
{ dpx::kITUR709, "ITU-R 709-4" },
629+
{ dpx::kITUR601, "ITU-R 601-5 system B or G" },
630+
{ dpx::kITUR602, "ITU-R 601-5 system M" },
631+
{ dpx::kNTSCCompositeVideo, "NTSC composite video" },
632+
{ dpx::kPALCompositeVideo, "PAL composite video" },
633+
{ dpx::kZLinear, "Z depth linear" },
634+
{ dpx::kZHomogeneous, "Z depth homogeneous" },
635+
{ dpx::kADX, "ADX" },
636+
{ dpx::kUndefinedCharacteristic, "Undefined" }
637+
};
638+
return lookup<dpx::Characteristic, const char*>(c, table, "Undefined");
640639
}
641640

642641

643642

644643
std::string
645644
DPXInput::get_descriptor_string(dpx::Descriptor c)
646645
{
647-
switch (c) {
648-
case dpx::kUserDefinedDescriptor:
649-
case dpx::kUserDefined2Comp:
650-
case dpx::kUserDefined3Comp:
651-
case dpx::kUserDefined4Comp:
652-
case dpx::kUserDefined5Comp:
653-
case dpx::kUserDefined6Comp:
654-
case dpx::kUserDefined7Comp:
655-
case dpx::kUserDefined8Comp: return "User defined";
656-
case dpx::kRed: return "Red";
657-
case dpx::kGreen: return "Green";
658-
case dpx::kBlue: return "Blue";
659-
case dpx::kAlpha: return "Alpha";
660-
case dpx::kLuma: return "Luma";
661-
case dpx::kColorDifference: return "Color difference";
662-
case dpx::kDepth: return "Depth";
663-
case dpx::kCompositeVideo: return "Composite video";
664-
case dpx::kRGB: return "RGB";
665-
case dpx::kRGBA: return "RGBA";
666-
case dpx::kABGR: return "ABGR";
667-
case dpx::kCbYCrY: return "CbYCrY";
668-
case dpx::kCbYACrYA: return "CbYACrYA";
669-
case dpx::kCbYCr: return "CbYCr";
670-
case dpx::kCbYCrA: return "CbYCrA";
671-
//case dpx::kUndefinedDescriptor:
672-
default: return "Undefined";
673-
}
646+
static const std::pair<dpx::Descriptor, const char*> table[] {
647+
{ dpx::kUserDefinedDescriptor, "User defined" },
648+
{ dpx::kUserDefined2Comp, "User defined" },
649+
{ dpx::kUserDefined3Comp, "User defined" },
650+
{ dpx::kUserDefined4Comp, "User defined" },
651+
{ dpx::kUserDefined5Comp, "User defined" },
652+
{ dpx::kUserDefined6Comp, "User defined" },
653+
{ dpx::kUserDefined7Comp, "User defined" },
654+
{ dpx::kUserDefined8Comp, "User defined" },
655+
{ dpx::kRed, "Red" },
656+
{ dpx::kGreen, "Green" },
657+
{ dpx::kBlue, "Blue" },
658+
{ dpx::kAlpha, "Alpha" },
659+
{ dpx::kLuma, "Luma" },
660+
{ dpx::kColorDifference, "Color difference" },
661+
{ dpx::kDepth, "Depth" },
662+
{ dpx::kCompositeVideo, "Composite video" },
663+
{ dpx::kRGB, "RGB" },
664+
{ dpx::kRGBA, "RGBA" },
665+
{ dpx::kABGR, "ABGR" },
666+
{ dpx::kCbYCrY, "CbYCrY" },
667+
{ dpx::kCbYACrYA, "CbYACrYA" },
668+
{ dpx::kCbYCr, "CbYCr" },
669+
{ dpx::kCbYCrA, "CbYCrA" }
670+
};
671+
return lookup<dpx::Descriptor, const char*>(c, table, "Undefined");
674672
}
675673

676674

677675

678676
void
679-
DPXInput::get_keycode_values(int* array)
677+
DPXInput::get_keycode_values(span<int> array)
680678
{
681-
std::stringstream ss;
682-
679+
OIIO_ASSERT(array.size() == 7);
683680
// Manufacturer code
684-
ss << std::string(m_dpx.header.filmManufacturingIdCode, 2);
685-
ss >> array[0];
686-
ss.clear();
687-
ss.str("");
688-
681+
array[0] = Strutil::stoi(string_view(m_dpx.header.filmManufacturingIdCode, 2));
689682
// Film type
690-
ss << std::string(m_dpx.header.filmType, 2);
691-
ss >> array[1];
692-
ss.clear();
693-
ss.str("");
694-
683+
array[1] = Strutil::stoi(string_view(m_dpx.header.filmType, 2));
695684
// Prefix
696-
ss << std::string(m_dpx.header.prefix, 6);
697-
ss >> array[2];
698-
ss.clear();
699-
ss.str("");
700-
685+
array[2] = Strutil::stoi(string_view(m_dpx.header.prefix, 6));
701686
// Count
702-
ss << std::string(m_dpx.header.count, 4);
703-
ss >> array[3];
704-
ss.clear();
705-
ss.str("");
706-
687+
array[3] = Strutil::stoi(string_view(m_dpx.header.count, 4));
707688
// Perforation Offset
708-
ss << std::string(m_dpx.header.perfsOffset, 2);
709-
ss >> array[4];
710-
ss.clear();
711-
ss.str("");
689+
array[4] = Strutil::stoi(string_view(m_dpx.header.perfsOffset, 2));
712690

713691
// Format
714692
std::string format(m_dpx.header.format, 32);
715-
int& perfsPerFrame = array[5];
716-
int& perfsPerCount = array[6];
717-
718-
// default values
719-
perfsPerFrame = 4;
720-
perfsPerCount = 64;
721-
693+
int perfsPerFrame = 4; // default values
694+
int perfsPerCount = 64;
695+
using Strutil::starts_with;
722696
if (format == "8kimax") {
723697
perfsPerFrame = 15;
724698
perfsPerCount = 120;
725-
} else if (format.substr(0, 4) == "2kvv" || format.substr(0, 4) == "4kvv") {
699+
} else if (starts_with(format, "2kvv") || starts_with(format, "4kvv")) {
726700
perfsPerFrame = 8;
727701
} else if (format == "VistaVision") {
728702
perfsPerFrame = 8;
729-
} else if (format.substr(0, 4) == "2k35" || format.substr(0, 4) == "4k35") {
703+
} else if (starts_with(format, "2k35") || starts_with(format, "4k35")) {
730704
perfsPerFrame = 4;
731705
} else if (format == "Full Aperture") {
732706
perfsPerFrame = 4;
733707
} else if (format == "Academy") {
734708
perfsPerFrame = 4;
735-
} else if (format.substr(0, 7) == "2k3perf"
736-
|| format.substr(0, 7) == "4k3perf") {
709+
} else if (starts_with(format, "2k3perf")
710+
|| starts_with(format, "4k3perf")) {
737711
perfsPerFrame = 3;
738712
} else if (format == "3perf") {
739713
perfsPerFrame = 3;
740714
}
715+
array[5] = perfsPerFrame;
716+
array[6] = perfsPerCount;
741717
}
742718

743719

0 commit comments

Comments
 (0)