Skip to content

Commit 42fd804

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.) Also had opportunity to greatly simplify the timecode utilities using some of our string functions. Signed-off-by: Larry Gritz <[email protected]>
1 parent 02f9013 commit 42fd804

File tree

1 file changed

+123
-162
lines changed

1 file changed

+123
-162
lines changed

‎src/dpx.imageio/dpxinput.cpp‎

Lines changed: 123 additions & 162 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

@@ -77,20 +79,16 @@ class DPXInput final : public ImageInput {
7779
}
7880

7981
/// Helper function - retrieve string for libdpx characteristic
80-
///
8182
std::string get_characteristic_string(dpx::Characteristic c);
8283

8384
/// Helper function - retrieve string for libdpx descriptor
84-
///
8585
std::string get_descriptor_string(dpx::Descriptor c);
8686

8787
/// Helper function - fill int array with KeyCode values
88-
///
89-
void get_keycode_values(int* array);
88+
void get_keycode_values(span<int> array);
9089

9190
/// Helper function - convert Imf::TimeCode to string;
92-
///
93-
std::string get_timecode_string(Imf::TimeCode& tc);
91+
std::string get_timecode_string(const Imf::TimeCode& tc);
9492
};
9593

9694

@@ -174,6 +172,22 @@ DPXInput::open(const std::string& name, ImageSpec& newspec,
174172

175173

176174

175+
// Helper, given key and a span of (KEY,VAL) pairs, look up which item
176+
// matches. If no key is found, then return the defaultval.
177+
template<typename KEY, typename VAL>
178+
const VAL&
179+
lookup(const KEY& key, cspan<std::pair<KEY, VAL>> values, const VAL& defaultval)
180+
{
181+
OIIO_ASSERT(values.size() >= 2);
182+
auto found = std::find_if(values.begin(), values.end(),
183+
[&key](const std::pair<KEY, VAL>& v) {
184+
return v.first == key;
185+
});
186+
return (found != values.end()) ? found->second : defaultval;
187+
}
188+
189+
190+
177191
bool
178192
DPXInput::seek_subimage(int subimage, int miplevel)
179193
{
@@ -294,18 +308,18 @@ DPXInput::seek_subimage(int subimage, int miplevel)
294308
// bits per pixel
295309
m_spec.attribute("oiio:BitsPerSample", m_dpx.header.BitDepth(subimage));
296310
// 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-
}
311+
static const std::pair<dpx::Orientation, int> orientation_table[]
312+
= { { dpx::kLeftToRightTopToBottom, 1 },
313+
{ dpx::kRightToLeftTopToBottom, 2 },
314+
{ dpx::kLeftToRightBottomToTop, 4 },
315+
{ dpx::kRightToLeftBottomToTop, 3 },
316+
{ dpx::kTopToBottomLeftToRight, 5 },
317+
{ dpx::kTopToBottomRightToLeft, 6 },
318+
{ dpx::kBottomToTopLeftToRight, 8 },
319+
{ dpx::kBottomToTopRightToLeft, 7 } };
320+
int orientation
321+
= lookup<dpx::Orientation, int>(m_dpx.header.ImageOrientation(),
322+
orientation_table, 1);
309323
m_spec.attribute("Orientation", orientation);
310324

311325
m_spec.attribute("oiio:subimages", (int)m_dpx.header.ImageElementCount());
@@ -495,51 +509,31 @@ DPXInput::seek_subimage(int subimage, int miplevel)
495509
m_spec.attribute("dpx:FilmEdgeCode", filmedge);
496510
}
497511

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);
512+
static const std::pair<dpx::VideoSignal, const char*> signal_table[] = {
513+
{ dpx::kUndefined, "Undefined" },
514+
{ dpx::kNTSC, "NTSC" },
515+
{ dpx::kPAL, "PAL" },
516+
{ dpx::kPAL_M, "PAL-M" },
517+
{ dpx::kSECAM, "SECAM" },
518+
{ dpx::k525LineInterlace43AR, "YCbCr ITU-R 601-5 525i, 4:3" },
519+
{ dpx::k625LineInterlace43AR, "YCbCr ITU-R 601-5 625i, 4:3" },
520+
{ dpx::k525LineInterlace169AR, "YCbCr ITU-R 601-5 525i, 16:9" },
521+
{ dpx::k625LineInterlace169AR, "YCbCr ITU-R 601-5 625i, 16:9" },
522+
{ dpx::k1050LineInterlace169AR, "YCbCr 1050i, 16:9" },
523+
{ dpx::k1125LineInterlace169AR_274, "YCbCr 1125i, 16:9 (SMPTE 274M)" },
524+
{ dpx::k1250LineInterlace169AR, "YCbCr 1250i, 16:9" },
525+
{ dpx::k1125LineInterlace169AR_240, "YCbCr 1125i, 16:9 (SMPTE 240M)" },
526+
{ dpx::k525LineProgressive169AR, "YCbCr 525p, 16:9" },
527+
{ dpx::k625LineProgressive169AR, "YCbCr 625p, 16:9" },
528+
{ dpx::k750LineProgressive169AR, "YCbCr 750p, 16:9 (SMPTE 296M)" },
529+
{ dpx::k1125LineProgressive169AR, "YCbCr 1125p, 16:9 (SMPTE 274M)" },
530+
{ dpx::k255, nullptr /* don't set the attribute at all */ },
531+
};
532+
const char* signal
533+
= lookup<dpx::VideoSignal, const char*>(m_dpx.header.Signal(),
534+
signal_table, "Undefined");
535+
if (signal)
536+
m_spec.attribute("dpx:Signal", signal);
543537

544538
// read in user data; don't bother if the buffer is already filled (user
545539
// data is per-file, not per-element)
@@ -619,147 +613,114 @@ DPXInput::read_native_scanlines(int subimage, int miplevel, int ybegin,
619613
std::string
620614
DPXInput::get_characteristic_string(dpx::Characteristic c)
621615
{
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-
}
616+
// using Val = std::pair<dpx::Characteristic, const char*>
617+
static const std::pair<dpx::Characteristic, const char*> table[] {
618+
{ dpx::kUserDefined, "User defined" },
619+
{ dpx::kPrintingDensity, "Printing density" },
620+
{ dpx::kLinear, "Linear" },
621+
{ dpx::kLogarithmic, "Logarithmic" },
622+
{ dpx::kUnspecifiedVideo, "Unspecified video" },
623+
{ dpx::kSMPTE274M, "SMPTE 274M" },
624+
{ dpx::kITUR709, "ITU-R 709-4" },
625+
{ dpx::kITUR601, "ITU-R 601-5 system B or G" },
626+
{ dpx::kITUR602, "ITU-R 601-5 system M" },
627+
{ dpx::kNTSCCompositeVideo, "NTSC composite video" },
628+
{ dpx::kPALCompositeVideo, "PAL composite video" },
629+
{ dpx::kZLinear, "Z depth linear" },
630+
{ dpx::kZHomogeneous, "Z depth homogeneous" },
631+
{ dpx::kADX, "ADX" },
632+
{ dpx::kUndefinedCharacteristic, "Undefined" }
633+
};
634+
return lookup<dpx::Characteristic, const char*>(c, table, "Undefined");
640635
}
641636

642637

643638

644639
std::string
645640
DPXInput::get_descriptor_string(dpx::Descriptor c)
646641
{
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-
}
642+
static const std::pair<dpx::Descriptor, const char*> table[] {
643+
{ dpx::kUserDefinedDescriptor, "User defined" },
644+
{ dpx::kUserDefined2Comp, "User defined" },
645+
{ dpx::kUserDefined3Comp, "User defined" },
646+
{ dpx::kUserDefined4Comp, "User defined" },
647+
{ dpx::kUserDefined5Comp, "User defined" },
648+
{ dpx::kUserDefined6Comp, "User defined" },
649+
{ dpx::kUserDefined7Comp, "User defined" },
650+
{ dpx::kUserDefined8Comp, "User defined" },
651+
{ dpx::kRed, "Red" },
652+
{ dpx::kGreen, "Green" },
653+
{ dpx::kBlue, "Blue" },
654+
{ dpx::kAlpha, "Alpha" },
655+
{ dpx::kLuma, "Luma" },
656+
{ dpx::kColorDifference, "Color difference" },
657+
{ dpx::kDepth, "Depth" },
658+
{ dpx::kCompositeVideo, "Composite video" },
659+
{ dpx::kRGB, "RGB" },
660+
{ dpx::kRGBA, "RGBA" },
661+
{ dpx::kABGR, "ABGR" },
662+
{ dpx::kCbYCrY, "CbYCrY" },
663+
{ dpx::kCbYACrYA, "CbYACrYA" },
664+
{ dpx::kCbYCr, "CbYCr" },
665+
{ dpx::kCbYCrA, "CbYCrA" }
666+
};
667+
return lookup<dpx::Descriptor, const char*>(c, table, "Undefined");
674668
}
675669

676670

677671

678672
void
679-
DPXInput::get_keycode_values(int* array)
673+
DPXInput::get_keycode_values(span<int> array)
680674
{
681-
std::stringstream ss;
682-
675+
OIIO_ASSERT(array.size() == 7);
683676
// Manufacturer code
684-
ss << std::string(m_dpx.header.filmManufacturingIdCode, 2);
685-
ss >> array[0];
686-
ss.clear();
687-
ss.str("");
688-
677+
array[0] = Strutil::stoi(
678+
string_view(m_dpx.header.filmManufacturingIdCode, 2));
689679
// Film type
690-
ss << std::string(m_dpx.header.filmType, 2);
691-
ss >> array[1];
692-
ss.clear();
693-
ss.str("");
694-
680+
array[1] = Strutil::stoi(string_view(m_dpx.header.filmType, 2));
695681
// Prefix
696-
ss << std::string(m_dpx.header.prefix, 6);
697-
ss >> array[2];
698-
ss.clear();
699-
ss.str("");
700-
682+
array[2] = Strutil::stoi(string_view(m_dpx.header.prefix, 6));
701683
// Count
702-
ss << std::string(m_dpx.header.count, 4);
703-
ss >> array[3];
704-
ss.clear();
705-
ss.str("");
706-
684+
array[3] = Strutil::stoi(string_view(m_dpx.header.count, 4));
707685
// Perforation Offset
708-
ss << std::string(m_dpx.header.perfsOffset, 2);
709-
ss >> array[4];
710-
ss.clear();
711-
ss.str("");
686+
array[4] = Strutil::stoi(string_view(m_dpx.header.perfsOffset, 2));
712687

713688
// Format
714689
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-
690+
int perfsPerFrame = 4; // default values
691+
int perfsPerCount = 64;
692+
using Strutil::starts_with;
722693
if (format == "8kimax") {
723694
perfsPerFrame = 15;
724695
perfsPerCount = 120;
725-
} else if (format.substr(0, 4) == "2kvv" || format.substr(0, 4) == "4kvv") {
696+
} else if (starts_with(format, "2kvv") || starts_with(format, "4kvv")) {
726697
perfsPerFrame = 8;
727698
} else if (format == "VistaVision") {
728699
perfsPerFrame = 8;
729-
} else if (format.substr(0, 4) == "2k35" || format.substr(0, 4) == "4k35") {
700+
} else if (starts_with(format, "2k35") || starts_with(format, "4k35")) {
730701
perfsPerFrame = 4;
731702
} else if (format == "Full Aperture") {
732703
perfsPerFrame = 4;
733704
} else if (format == "Academy") {
734705
perfsPerFrame = 4;
735-
} else if (format.substr(0, 7) == "2k3perf"
736-
|| format.substr(0, 7) == "4k3perf") {
706+
} else if (starts_with(format, "2k3perf")
707+
|| starts_with(format, "4k3perf")) {
737708
perfsPerFrame = 3;
738709
} else if (format == "3perf") {
739710
perfsPerFrame = 3;
740711
}
712+
array[5] = perfsPerFrame;
713+
array[6] = perfsPerCount;
741714
}
742715

743716

744717

745718
std::string
746-
DPXInput::get_timecode_string(Imf::TimeCode& tc)
719+
DPXInput::get_timecode_string(const Imf::TimeCode& tc)
747720
{
748-
int values[] = { tc.hours(), tc.minutes(), tc.seconds(), tc.frame() };
749-
std::stringstream ss;
750-
for (int i = 0; i < 4; i++) {
751-
std::ostringstream padded;
752-
padded << std::setw(2) << std::setfill('0') << values[i];
753-
ss << padded.str();
754-
if (i != 3) {
755-
if (i == 2) {
756-
tc.dropFrame() ? ss << ';' : ss << ':';
757-
} else {
758-
ss << ':';
759-
}
760-
}
761-
}
762-
return ss.str();
721+
return Strutil::fmt::format("{:02d}:{:02d}:{:02d}{}{:02d}", tc.hours(),
722+
tc.minutes(), tc.seconds(),
723+
(tc.dropFrame() ? ';' : ':'), tc.frame());
763724
}
764725

765726
OIIO_PLUGIN_NAMESPACE_END

0 commit comments

Comments
 (0)