From 2863c51c8fa2bfbd4dbd43718733f271047c5dc0 Mon Sep 17 00:00:00 2001 From: Ayush Singh Date: Sun, 18 Oct 2026 20:23:37 +0000 Subject: [PATCH 1/6] fix(xml): handle rational types in ImageSpec roundtrip Fixes #5571. 1) TypeDesc::fromstring() now recognizes 'rational2i' and 'rational2f'. 2) ParamValue parsing now replaces the '/' in rational XML values with ',' so the numbers load correctly. Added tests for both. Signed-off-by: Ayush Singh --- src/libOpenImageIO/imagespec_test.cpp | 15 +++++++++++++++ src/libutil/paramlist.cpp | 10 +++++++++- src/libutil/typedesc.cpp | 20 ++++++++++---------- src/libutil/typedesc_test.cpp | 2 ++ 4 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/libOpenImageIO/imagespec_test.cpp b/src/libOpenImageIO/imagespec_test.cpp index a233758c65..4ddb4d6852 100644 --- a/src/libOpenImageIO/imagespec_test.cpp +++ b/src/libOpenImageIO/imagespec_test.cpp @@ -47,6 +47,7 @@ const char* imagespec_xml_string = R"EOF( 0, 0 1 1 +30000/1001 )EOF"; @@ -358,6 +359,20 @@ test_imagespec_from_xml() OIIO_CHECK_EQUAL(spec.format, TypeFloat); OIIO_CHECK_EQUAL(spec.get_string_attribute("oiio:ColorSpace"), "scene_linear"); + OIIO_CHECK_EQUAL(spec.getattributetype("FramesPerSecond"), TypeRational); + OIIO_CHECK_EQUAL_THRESH(spec.get_float_attribute("FramesPerSecond", -1.0f), + 30000.0f / 1001.0f, 1.0e-5f); + + std::string xml = spec.to_xml(); + OIIO_CHECK_ASSERT(xml.find("name=\"FramesPerSecond\" type=\"rational2i\"") + != std::string::npos); + ImageSpec roundtripped; + roundtripped.from_xml(xml.c_str()); + OIIO_CHECK_EQUAL(roundtripped.getattributetype("FramesPerSecond"), + TypeRational); + OIIO_CHECK_EQUAL_THRESH(roundtripped.get_float_attribute("FramesPerSecond", + -1.0f), + 30000.0f / 1001.0f, 1.0e-5f); } diff --git a/src/libutil/paramlist.cpp b/src/libutil/paramlist.cpp index 28ee3ab365..0a32afc42b 100644 --- a/src/libutil/paramlist.cpp +++ b/src/libutil/paramlist.cpp @@ -130,7 +130,15 @@ parse_elements(string_view value, ParamValue& p) ParamValue::ParamValue(string_view name, TypeDesc type, string_view value) : ParamValue(name, type, 1, cspan()) { - if (type.basetype == TypeDesc::INT) { + if (type == TypeRational || type == TypeURational) { + // Accept either "x,y" or "x/y" here + std::string rational_values(value); + std::replace(rational_values.begin(), rational_values.end(), '/', ','); + if (type.basetype == TypeDesc::INT) + parse_elements(rational_values, *this); + else if (type.basetype == TypeDesc::UINT) + parse_elements(rational_values, *this); + } else if (type.basetype == TypeDesc::INT) { parse_elements(value, *this); } else if (type.basetype == TypeDesc::UINT) { parse_elements(value, *this); diff --git a/src/libutil/typedesc.cpp b/src/libutil/typedesc.cpp index 99dc09fed7..618c4570d8 100644 --- a/src/libutil/typedesc.cpp +++ b/src/libutil/typedesc.cpp @@ -214,6 +214,14 @@ TypeDesc::c_str() const result = Strutil::fmt::format("matrix{}", basetype_code[basetype]); break; } + } else if (vecsemantics == RATIONAL && (*this == TypeRational || *this == TypeURational)) { + result = (basetype == TypeDesc::INT) ? "rational" : "urational"; + } else if (vecsemantics == BOX) { + result = Strutil::fmt::format("box{}{}", int(aggregate), + basetype == FLOAT + ? "" + : basetype_code[basetype]); + alen = arraylen > 2 ? (arraylen / 2) : (arraylen < 0 ? -1 : 0); } else { // Special names for vector semantics const char* vec = ""; @@ -237,14 +245,6 @@ TypeDesc::c_str() const if (basetype != FLOAT) result += basetype_code[basetype]; } - // More unusual cases - if (vecsemantics == BOX) { - result = Strutil::fmt::format("box{}{}", int(aggregate), - basetype == FLOAT - ? "" - : basetype_code[basetype]); - alen = arraylen > 2 ? (arraylen / 2) : (arraylen < 0 ? -1 : 0); - } if (alen > 0) result += Strutil::fmt::format("[{}]", alen); else if (alen < 0) @@ -323,9 +323,9 @@ TypeDesc::fromstring(string_view typestring) t = OIIO::TypeFloat4; else if (type == "timecode") t = OIIO::TypeTimeCode; - else if (type == "rational") + else if (type == "rational" || type == "rational2i") t = OIIO::TypeRational; - else if (type == "urational") + else if (type == "urational" || type == "rational2u") t = OIIO::TypeURational; else if (type == "box2i") t = OIIO::TypeBox2i; diff --git a/src/libutil/typedesc_test.cpp b/src/libutil/typedesc_test.cpp index d8dfa7b324..c0abca4e7b 100644 --- a/src/libutil/typedesc_test.cpp +++ b/src/libutil/typedesc_test.cpp @@ -202,6 +202,8 @@ main(int /*argc*/, char* /*argv*/[]) TypeDesc(TypeDesc::INT, TypeDesc::VEC2, TypeDesc::RATIONAL), TypeRational, i2, "1/2"); + OIIO_CHECK_EQUAL(TypeDesc("rational2i"), TypeRational); + OIIO_CHECK_EQUAL(TypeDesc("rational2u"), TypeURational); uint32_t ui2[2] = { 1, 2 }; test_type("urational", TypeDesc(TypeDesc::UINT32, TypeDesc::VEC2, From 902ff666de1c1aa4f7410b774573568c14ae63c2 Mon Sep 17 00:00:00 2001 From: Ayush Singh Date: Tue, 20 Oct 2026 05:46:04 +0530 Subject: [PATCH 2/6] Apply suggestion from @lgritz Co-authored-by: Larry Gritz Signed-off-by: Ayush Singh --- src/libutil/paramlist.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libutil/paramlist.cpp b/src/libutil/paramlist.cpp index 0a32afc42b..eed075e4c1 100644 --- a/src/libutil/paramlist.cpp +++ b/src/libutil/paramlist.cpp @@ -131,6 +131,7 @@ ParamValue::ParamValue(string_view name, TypeDesc type, string_view value) : ParamValue(name, type, 1, cspan()) { if (type == TypeRational || type == TypeURational) { + // Accept either "x,y" or "x/y" here // Accept either "x,y" or "x/y" here std::string rational_values(value); std::replace(rational_values.begin(), rational_values.end(), '/', ','); From d753a8fd301578adbb4ecfdbb1957289d9e20595 Mon Sep 17 00:00:00 2001 From: Ayush Singh Date: Tue, 20 Oct 2026 05:48:54 +0530 Subject: [PATCH 3/6] Update paramlist.cpp Signed-off-by: Ayush Singh --- src/libutil/paramlist.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libutil/paramlist.cpp b/src/libutil/paramlist.cpp index eed075e4c1..0a32afc42b 100644 --- a/src/libutil/paramlist.cpp +++ b/src/libutil/paramlist.cpp @@ -131,7 +131,6 @@ ParamValue::ParamValue(string_view name, TypeDesc type, string_view value) : ParamValue(name, type, 1, cspan()) { if (type == TypeRational || type == TypeURational) { - // Accept either "x,y" or "x/y" here // Accept either "x,y" or "x/y" here std::string rational_values(value); std::replace(rational_values.begin(), rational_values.end(), '/', ','); From 065a2f765d926d9ccdcd32e389b56a1fd2776f98 Mon Sep 17 00:00:00 2001 From: Ayush Singh Date: Tue, 20 Oct 2026 15:10:02 +0530 Subject: [PATCH 4/6] Apply suggestion from @lgritz Co-authored-by: Larry Gritz Signed-off-by: Ayush Singh --- src/libOpenImageIO/imagespec_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libOpenImageIO/imagespec_test.cpp b/src/libOpenImageIO/imagespec_test.cpp index 4ddb4d6852..548c51aeb3 100644 --- a/src/libOpenImageIO/imagespec_test.cpp +++ b/src/libOpenImageIO/imagespec_test.cpp @@ -47,7 +47,7 @@ const char* imagespec_xml_string = R"EOF( 0, 0 1 1 -30000/1001 +30000/1001 )EOF"; From 13ebaab73c4ce3ba7a0ef842645c309ffedf088a Mon Sep 17 00:00:00 2001 From: Ayush Singh Date: Tue, 20 Oct 2026 15:10:22 +0530 Subject: [PATCH 5/6] Apply suggestion from @lgritz Co-authored-by: Larry Gritz Signed-off-by: Ayush Singh --- src/libOpenImageIO/imagespec_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libOpenImageIO/imagespec_test.cpp b/src/libOpenImageIO/imagespec_test.cpp index 548c51aeb3..986caf648d 100644 --- a/src/libOpenImageIO/imagespec_test.cpp +++ b/src/libOpenImageIO/imagespec_test.cpp @@ -364,7 +364,7 @@ test_imagespec_from_xml() 30000.0f / 1001.0f, 1.0e-5f); std::string xml = spec.to_xml(); - OIIO_CHECK_ASSERT(xml.find("name=\"FramesPerSecond\" type=\"rational2i\"") + OIIO_CHECK_ASSERT(xml.find("name=\"FramesPerSecond\" type=\"rational\"") != std::string::npos); ImageSpec roundtripped; roundtripped.from_xml(xml.c_str()); From 88325ce367318e91c7be771312351ff947160cc9 Mon Sep 17 00:00:00 2001 From: Ayush Singh Date: Tue, 20 Oct 2026 15:10:33 +0530 Subject: [PATCH 6/6] Apply suggestion from @lgritz Co-authored-by: Larry Gritz Signed-off-by: Ayush Singh --- src/libutil/typedesc_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libutil/typedesc_test.cpp b/src/libutil/typedesc_test.cpp index c0abca4e7b..328f4fc757 100644 --- a/src/libutil/typedesc_test.cpp +++ b/src/libutil/typedesc_test.cpp @@ -202,8 +202,8 @@ main(int /*argc*/, char* /*argv*/[]) TypeDesc(TypeDesc::INT, TypeDesc::VEC2, TypeDesc::RATIONAL), TypeRational, i2, "1/2"); - OIIO_CHECK_EQUAL(TypeDesc("rational2i"), TypeRational); - OIIO_CHECK_EQUAL(TypeDesc("rational2u"), TypeURational); + OIIO_CHECK_EQUAL(TypeDesc("rational"), TypeRational); + OIIO_CHECK_EQUAL(TypeDesc("urational"), TypeURational); uint32_t ui2[2] = { 1, 2 }; test_type("urational", TypeDesc(TypeDesc::UINT32, TypeDesc::VEC2,