From 2b003c2081544558bd0d51073d56d04f727cb8b3 Mon Sep 17 00:00:00 2001 From: Richard Bailey Date: Thu, 23 Apr 2026 19:57:44 +0100 Subject: [PATCH 1/9] Add Cartesian subelement on Direct Speakers block format Mirrors semantics on Objects type block format regarding interaction with the used coordinate type fix formatting --- include/adm/elements.hpp | 1 + .../audio_block_format_direct_speakers.hpp | 14 ++++ .../elements/audio_block_format_objects.hpp | 5 -- include/adm/elements/cartesian.hpp | 11 +++ include/adm/elements/common_parameters.hpp | 1 + .../audio_block_format_direct_speakers.cpp | 50 +++++++++++++- src/private/document_parser.cpp | 1 + src/private/rapidxml_formatter.cpp | 1 + ...dio_block_format_direct_speakers_tests.cpp | 68 +++++++++++++++++++ ...ite_default_cartesian_speaker.accepted.xml | 1 + ...e_specified_cartesian_speaker.accepted.xml | 1 + ...pecified_headphone_virtualise.accepted.xml | 1 + ...block_format_direct_speakers_cartesian.xml | 2 + ..._speakers_cartesian_spherical_mismatch.xml | 17 +++++ ..._speakers_spherical_cartesian_mismatch.xml | 17 +++++ ...dio_block_format_direct_speakers_tests.cpp | 51 ++++++++++++++ 16 files changed, 236 insertions(+), 6 deletions(-) create mode 100644 include/adm/elements/cartesian.hpp create mode 100644 tests/test_data/xml_parser/audio_block_format_direct_speakers_cartesian_spherical_mismatch.xml create mode 100644 tests/test_data/xml_parser/audio_block_format_direct_speakers_spherical_cartesian_mismatch.xml diff --git a/include/adm/elements.hpp b/include/adm/elements.hpp index 9fdf9366..cfea6c39 100644 --- a/include/adm/elements.hpp +++ b/include/adm/elements.hpp @@ -48,6 +48,7 @@ #include "adm/elements/time.hpp" #include "adm/elements/audio_programme_ref_screen.hpp" +#include "adm/elements/cartesian.hpp" #include "adm/elements/channel_lock.hpp" #include "adm/elements/dialogue.hpp" #include "adm/elements/format_descriptor.hpp" diff --git a/include/adm/elements/audio_block_format_direct_speakers.hpp b/include/adm/elements/audio_block_format_direct_speakers.hpp index 59faba92..bfd1618e 100644 --- a/include/adm/elements/audio_block_format_direct_speakers.hpp +++ b/include/adm/elements/audio_block_format_direct_speakers.hpp @@ -68,6 +68,8 @@ namespace adm { * +---------------------+------------------------------------+----------------------------+ * | initializeBlock | :type:`InitializeBlock` | :class:`OptionalParameter` | * +---------------------+------------------------------------+----------------------------+ + * | cartesian | :type:`Cartesian` | custom, see below | + * +---------------------+------------------------------------+----------------------------+ * | position | - :type:`SpeakerPosition` | :class:`VariantParameter` | * | | - :type:`SphericalSpeakerPosition` | | * | | - :type:`CartesianSpeakerPosition` | :class:`RequiredParameter` | @@ -83,6 +85,11 @@ namespace adm { * | speakerLabel | :type:`SpeakerLabels` | :class:`VectorParameter` | * +---------------------+------------------------------------+----------------------------+ * \endrst + * + * ``cartesian`` and ``position`` attributes are linked; see + * :func:`void set(Cartesian)`, :func:`void set(SpeakerPosition)`, + * :func:`void set(CartesianSpeakerPosition)` and + * :func:`void set(SphericalSpeakerPosition)`. * * @warning not all methods are implemented for speakerLabel */ @@ -138,6 +145,8 @@ namespace adm { ADM_EXPORT void set(Rtime rtime); /// @brief Duration setter ADM_EXPORT void set(Duration duration); + /// @brief Cartesian setter + ADM_EXPORT void set(Cartesian cartesian); /// @brief CartesianSpeakerPosition setter ADM_EXPORT void set(CartesianSpeakerPosition speakerPosition); /// @brief SphericalSpeakerPosition setter @@ -174,6 +183,7 @@ namespace adm { ADM_EXPORT Duration get(detail::ParameterTraits::tag) const; ADM_EXPORT SpeakerLabels get(detail::ParameterTraits::tag) const; + ADM_EXPORT Cartesian get(detail::ParameterTraits::tag) const; ADM_EXPORT CartesianSpeakerPosition get(detail::ParameterTraits::tag) const; ADM_EXPORT SphericalSpeakerPosition @@ -183,6 +193,7 @@ namespace adm { ADM_EXPORT bool has(detail::ParameterTraits::tag) const; ADM_EXPORT bool has(detail::ParameterTraits::tag) const; ADM_EXPORT bool has(detail::ParameterTraits::tag) const; + ADM_EXPORT bool has(detail::ParameterTraits::tag) const; ADM_EXPORT bool has( detail::ParameterTraits::tag) const; ADM_EXPORT bool has( @@ -192,15 +203,18 @@ namespace adm { bool isDefault(Tag) const { return false; } + ADM_EXPORT bool isDefault(detail::ParameterTraits::tag) const; ADM_EXPORT void unset(detail::ParameterTraits::tag); ADM_EXPORT void unset(detail::ParameterTraits::tag); ADM_EXPORT void unset(detail::ParameterTraits::tag); + ADM_EXPORT void unset(detail::ParameterTraits::tag); AudioBlockFormatId id_; boost::optional rtime_; boost::optional duration_; SpeakerLabels speakerLabels_; + boost::optional cartesian_; SpeakerPosition speakerPosition_; }; diff --git a/include/adm/elements/audio_block_format_objects.hpp b/include/adm/elements/audio_block_format_objects.hpp index 4c57faeb..afdd4a8e 100644 --- a/include/adm/elements/audio_block_format_objects.hpp +++ b/include/adm/elements/audio_block_format_objects.hpp @@ -18,11 +18,6 @@ namespace adm { class Document; - - /// @brief Tag for NamedType ::Cartesian - struct CartesianTag {}; - /// @brief NamedType for cartesian parameter - using Cartesian = detail::NamedType; /// @brief Tag for NamedType ::Width struct WidthTag {}; /// @brief NamedType for width parameter diff --git a/include/adm/elements/cartesian.hpp b/include/adm/elements/cartesian.hpp new file mode 100644 index 00000000..bce91911 --- /dev/null +++ b/include/adm/elements/cartesian.hpp @@ -0,0 +1,11 @@ +/// @file cartesian.hpp +#pragma once + +#include "adm/detail/named_type.hpp" + +namespace adm { + /// @brief Tag for NamedType ::Cartesian + struct CartesianTag {}; + /// @brief NamedType for cartesian parameter + using Cartesian = detail::NamedType; +} // namespace adm diff --git a/include/adm/elements/common_parameters.hpp b/include/adm/elements/common_parameters.hpp index 84a03c9e..ebf58446 100644 --- a/include/adm/elements/common_parameters.hpp +++ b/include/adm/elements/common_parameters.hpp @@ -1,6 +1,7 @@ #pragma once #include "adm/detail/auto_base.hpp" #include "adm/elements/audio_block_format_id.hpp" +#include "adm/elements/cartesian.hpp" #include "adm/elements/gain.hpp" #include "adm/elements/headphone_virtualise.hpp" #include "adm/elements/head_locked.hpp" diff --git a/src/elements/audio_block_format_direct_speakers.cpp b/src/elements/audio_block_format_direct_speakers.cpp index 86ad5352..ede9fec8 100644 --- a/src/elements/audio_block_format_direct_speakers.cpp +++ b/src/elements/audio_block_format_direct_speakers.cpp @@ -24,6 +24,18 @@ namespace adm { detail::ParameterTraits::tag) const { return speakerLabels_; } + Cartesian AudioBlockFormatDirectSpeakers::get( + detail::ParameterTraits::tag) const { + if (cartesian_ != boost::none) { + return cartesian_.get(); + } else { + if (has()) { + return Cartesian(false); + } else { + return Cartesian(true); + } + } + } CartesianSpeakerPosition AudioBlockFormatDirectSpeakers::get( detail::ParameterTraits::tag) const { return boost::get(speakerPosition_); @@ -50,6 +62,10 @@ namespace adm { detail::ParameterTraits::tag) const { return speakerLabels_.size() > 0; } + bool AudioBlockFormatDirectSpeakers::has( + detail::ParameterTraits::tag) const { + return true; + } bool AudioBlockFormatDirectSpeakers::has( detail::ParameterTraits::tag) const { return (boost::get(&speakerPosition_)); @@ -64,6 +80,10 @@ namespace adm { detail::ParameterTraits::tag) const { return duration_ == boost::none; } + bool AudioBlockFormatDirectSpeakers::isDefault( + detail::ParameterTraits::tag) const { + return cartesian_ == boost::none; + } // ---- Setter ---- // void AudioBlockFormatDirectSpeakers::set(AudioBlockFormatId id) { id_ = id; } @@ -71,16 +91,37 @@ namespace adm { void AudioBlockFormatDirectSpeakers::set(Duration duration) { duration_ = duration; } + void AudioBlockFormatDirectSpeakers::set(Cartesian cartesian) { + cartesian_ = cartesian; + + if (cartesian.get()) { + if (has()) { + speakerPosition_ = CartesianSpeakerPosition{}; + } + } else { + if (has()) { + speakerPosition_ = SphericalSpeakerPosition{}; + } + } + } void AudioBlockFormatDirectSpeakers::set( CartesianSpeakerPosition speakerPosition) { speakerPosition_ = speakerPosition; + cartesian_ = Cartesian(true); } void AudioBlockFormatDirectSpeakers::set( SphericalSpeakerPosition speakerPosition) { speakerPosition_ = speakerPosition; + if (cartesian_ != boost::none) { + cartesian_ = Cartesian(false); + } } void AudioBlockFormatDirectSpeakers::set(SpeakerPosition speakerPosition) { - speakerPosition_ = speakerPosition; + if (speakerPosition.which() == 0) { + set(boost::get(speakerPosition)); + } else if (speakerPosition.which() == 1) { + set(boost::get(speakerPosition)); + } } // ---- Unsetter ---- // @@ -96,6 +137,13 @@ namespace adm { detail::ParameterTraits::tag) { speakerLabels_.clear(); } + void AudioBlockFormatDirectSpeakers::unset( + detail::ParameterTraits::tag) { + cartesian_ = boost::none; + if (!has()) { + set(SphericalSpeakerPosition{}); + } + } // ---- Add ---- // bool AudioBlockFormatDirectSpeakers::add(SpeakerLabel speakerLabel) { diff --git a/src/private/document_parser.cpp b/src/private/document_parser.cpp index cb2635b8..58c0603d 100644 --- a/src/private/document_parser.cpp +++ b/src/private/document_parser.cpp @@ -599,6 +599,7 @@ namespace adm { setOptionalAttribute(node, "audioBlockFormatID", audioBlockFormat, &parseAudioBlockFormatId); addTimeParametersToBlock(node, audioBlockFormat, timeReference); setOptionalAttribute(node, "initializeBlock", audioBlockFormat); + setOptionalElement(node, "cartesian", audioBlockFormat); setMultiElement(node, "position", audioBlockFormat, &parseSpeakerPosition); addOptionalElements(node, "speakerLabel", audioBlockFormat, &parseSpeakerLabel); setOptionalElement(node, "headLocked", audioBlockFormat); diff --git a/src/private/rapidxml_formatter.cpp b/src/private/rapidxml_formatter.cpp index eed2d7bf..2b0028f0 100644 --- a/src/private/rapidxml_formatter.cpp +++ b/src/private/rapidxml_formatter.cpp @@ -349,6 +349,7 @@ namespace adm { if(audioBlock.has()) { node.addMultiElement(&audioBlock, "position", &formatCartesianSpeakerPosition); } + node.addOptionalElement(&audioBlock, "cartesian"); node.addOptionalElement(&audioBlock, "headLocked"); node.addOptionalElement(&audioBlock, "headphoneVirtualise", &formatHeadphoneVirtualise); diff --git a/tests/audio_block_format_direct_speakers_tests.cpp b/tests/audio_block_format_direct_speakers_tests.cpp index b0b422a4..5804c19d 100644 --- a/tests/audio_block_format_direct_speakers_tests.cpp +++ b/tests/audio_block_format_direct_speakers_tests.cpp @@ -10,11 +10,14 @@ TEST_CASE("DirectSpeakers block format common subelements") { REQUIRE(blockFormat.has() == true); REQUIRE(blockFormat.has() == false); REQUIRE(blockFormat.has() == false); + REQUIRE(blockFormat.has() == true); REQUIRE(blockFormat.isDefault() == true); + REQUIRE(blockFormat.isDefault() == true); auto defaultRtime = std::chrono::seconds{0}; REQUIRE(blockFormat.get().get() == defaultRtime); + REQUIRE(blockFormat.get() == false); auto rTime = std::chrono::seconds{1}; auto duration = std::chrono::seconds{10}; @@ -60,6 +63,8 @@ TEST_CASE("DirectSpeakers block format with Spherical coordinates") { defaultPosition.get()); REQUIRE(blockFormat.get().get() == defaultPosition.get()); + REQUIRE(blockFormat.get() == false); + REQUIRE(blockFormat.isDefault() == true); auto speakerPosition = SphericalSpeakerPosition(Azimuth(30), Elevation(10), Distance(0.5)); @@ -70,6 +75,8 @@ TEST_CASE("DirectSpeakers block format with Spherical coordinates") { Approx(10)); REQUIRE(blockFormat.get().get() == Approx(0.5)); + REQUIRE(blockFormat.get() == false); + REQUIRE(blockFormat.isDefault() == true); } } @@ -85,4 +92,65 @@ TEST_CASE("DirectSpeakers block format with Cartesian coordinates") { REQUIRE(retrievedPosition.get() == speakerPosition.get()); REQUIRE(retrievedPosition.get() == speakerPosition.get()); REQUIRE(retrievedPosition.get() == speakerPosition.get()); + REQUIRE(blockFormat.get() == true); + REQUIRE(blockFormat.isDefault() == false); +} + +TEST_CASE("DirectSpeakers block format cartesian interactions") { + using namespace adm; + + SECTION("spherical speaker position does not set cartesian when unset") { + auto blockFormat = AudioBlockFormatDirectSpeakers{}; + blockFormat.set(SphericalSpeakerPosition{Azimuth{30.0f}, Elevation{5.0f}}); + + REQUIRE(blockFormat.has() == true); + REQUIRE(blockFormat.has() == false); + REQUIRE(blockFormat.get() == false); + REQUIRE(blockFormat.isDefault() == true); + } + + SECTION( + "unsetting cartesian with cartesian position sets default spherical") { + auto blockFormat = AudioBlockFormatDirectSpeakers{}; + blockFormat.set(CartesianSpeakerPosition{X{0.8f}, Y{-0.3f}, Z{0.2f}}); + + REQUIRE(blockFormat.has() == true); + REQUIRE(blockFormat.get() == true); + + blockFormat.unset(); + + REQUIRE(blockFormat.has() == true); + REQUIRE(blockFormat.has() == false); + REQUIRE(blockFormat.get().get() == + Approx(0.0f)); + REQUIRE(blockFormat.get().get() == + Approx(0.0f)); + REQUIRE(blockFormat.get().has() == + false); + REQUIRE(blockFormat.get() == false); + REQUIRE(blockFormat.isDefault() == true); + } + + SECTION( + "setting cartesian true with spherical position sets default cartesian") { + auto blockFormat = AudioBlockFormatDirectSpeakers{}; + blockFormat.set(SphericalSpeakerPosition{Azimuth{10.0f}, Elevation{15.0f}, + Distance{1.0f}}); + + REQUIRE(blockFormat.has() == true); + REQUIRE(blockFormat.get().get() == + Approx(10.0f)); + + blockFormat.set(Cartesian{true}); + + REQUIRE(blockFormat.has() == true); + REQUIRE(blockFormat.has() == false); + REQUIRE(blockFormat.get().get() == + Approx(0.0f)); + REQUIRE(blockFormat.get().get() == + Approx(0.0f)); + REQUIRE(blockFormat.get().has() == false); + REQUIRE(blockFormat.get() == true); + REQUIRE(blockFormat.isDefault() == false); + } } diff --git a/tests/test_data/write_default_cartesian_speaker.accepted.xml b/tests/test_data/write_default_cartesian_speaker.accepted.xml index 44013271..269af8c9 100644 --- a/tests/test_data/write_default_cartesian_speaker.accepted.xml +++ b/tests/test_data/write_default_cartesian_speaker.accepted.xml @@ -7,6 +7,7 @@ 0.000000 0.000000 + 1 diff --git a/tests/test_data/write_specified_cartesian_speaker.accepted.xml b/tests/test_data/write_specified_cartesian_speaker.accepted.xml index 99959e97..bbce659e 100644 --- a/tests/test_data/write_specified_cartesian_speaker.accepted.xml +++ b/tests/test_data/write_specified_cartesian_speaker.accepted.xml @@ -15,6 +15,7 @@ 0.500000 0.400000 0.600000 + 1 1 0.500000 5 diff --git a/tests/test_data/write_specified_headphone_virtualise.accepted.xml b/tests/test_data/write_specified_headphone_virtualise.accepted.xml index daee63a0..eb81eda1 100644 --- a/tests/test_data/write_specified_headphone_virtualise.accepted.xml +++ b/tests/test_data/write_specified_headphone_virtualise.accepted.xml @@ -16,6 +16,7 @@ 0.000000 0.000000 + 1 diff --git a/tests/test_data/xml_parser/audio_block_format_direct_speakers_cartesian.xml b/tests/test_data/xml_parser/audio_block_format_direct_speakers_cartesian.xml index c4ee285d..fc7fafdb 100644 --- a/tests/test_data/xml_parser/audio_block_format_direct_speakers_cartesian.xml +++ b/tests/test_data/xml_parser/audio_block_format_direct_speakers_cartesian.xml @@ -15,6 +15,7 @@ 0.500000 0.400000 0.600000 + 1 @@ -22,3 +23,4 @@ + diff --git a/tests/test_data/xml_parser/audio_block_format_direct_speakers_cartesian_spherical_mismatch.xml b/tests/test_data/xml_parser/audio_block_format_direct_speakers_cartesian_spherical_mismatch.xml new file mode 100644 index 00000000..45f422a4 --- /dev/null +++ b/tests/test_data/xml_parser/audio_block_format_direct_speakers_cartesian_spherical_mismatch.xml @@ -0,0 +1,17 @@ + + + + + + + + 0.0 + 0.0 + 0.5 + 0 + + + + + + diff --git a/tests/test_data/xml_parser/audio_block_format_direct_speakers_spherical_cartesian_mismatch.xml b/tests/test_data/xml_parser/audio_block_format_direct_speakers_spherical_cartesian_mismatch.xml new file mode 100644 index 00000000..60370e99 --- /dev/null +++ b/tests/test_data/xml_parser/audio_block_format_direct_speakers_spherical_cartesian_mismatch.xml @@ -0,0 +1,17 @@ + + + + + + + + 30.0 + 0.0 + 1.0 + 1 + + + + + + diff --git a/tests/xml_parser_audio_block_format_direct_speakers_tests.cpp b/tests/xml_parser_audio_block_format_direct_speakers_tests.cpp index 6d3b8151..6e5b266a 100644 --- a/tests/xml_parser_audio_block_format_direct_speakers_tests.cpp +++ b/tests/xml_parser_audio_block_format_direct_speakers_tests.cpp @@ -27,6 +27,8 @@ TEST_CASE("xml_parser/audio_block_format_direct_speakers") { 30.0f); REQUIRE(firstBlockFormat.get().get() == 0.0f); + REQUIRE(firstBlockFormat.get() == false); + REQUIRE(firstBlockFormat.isDefault() == true); REQUIRE(firstBlockFormat.get().get() == false); REQUIRE(firstBlockFormat.get() .get() == Approx(60)); @@ -84,9 +86,58 @@ TEST_CASE("xml_parser/audio_block_format_direct_speakers_cartesian") { REQUIRE(speakerPosition.has()); auto edgeLock = speakerPosition.get(); REQUIRE(edgeLock.get().get() == "left"); + REQUIRE(firstBlockFormat.get() == true); + REQUIRE(firstBlockFormat.isDefault() == false); } } +TEST_CASE( + "xml_parser/" + "audio_block_format_direct_speakers_spherical_cartesian_mismatch") { + using namespace adm; + auto document = parseXml( + "xml_parser/audio_block_format_direct_speakers_spherical_cartesian_" + "mismatch.xml"); + auto channelFormat = + document->lookup(parseAudioChannelFormatId("AC_00011001")); + REQUIRE(channelFormat); + + auto firstBlockFormat = + *(channelFormat->getElements().begin()); + REQUIRE(firstBlockFormat.has() == true); + REQUIRE(firstBlockFormat.has() == false); + auto speakerPosition = firstBlockFormat.get(); + REQUIRE(speakerPosition.get() == Approx(30.0f)); + REQUIRE(speakerPosition.get() == Approx(0.0f)); + REQUIRE(speakerPosition.get() == Approx(1.0f)); + REQUIRE(firstBlockFormat.get() == false); + REQUIRE(firstBlockFormat.isDefault() == false); +} + +TEST_CASE( + "xml_parser/" + "audio_block_format_direct_speakers_cartesian_spherical_mismatch") { + using namespace adm; + auto document = parseXml( + "xml_parser/audio_block_format_direct_speakers_cartesian_spherical_" + "mismatch.xml"); + auto channelFormat = + document->lookup(parseAudioChannelFormatId("AC_00011001")); + REQUIRE(channelFormat); + + auto firstBlockFormat = + *(channelFormat->getElements().begin()); + REQUIRE(firstBlockFormat.has() == true); + REQUIRE(firstBlockFormat.has() == false); + auto speakerPosition = firstBlockFormat.get(); + REQUIRE(speakerPosition.get() == Approx(0.0f)); + REQUIRE(speakerPosition.get() == Approx(0.0f)); + REQUIRE(speakerPosition.has()); + REQUIRE(speakerPosition.get() == Approx(0.5f)); + REQUIRE(firstBlockFormat.get() == true); + REQUIRE(firstBlockFormat.isDefault() == false); +} + TEST_CASE("xml_parser/audio_block_format_direct_speakers_cartesian_bad_bound") { using namespace adm; REQUIRE_THROWS_AS( From ea6bff8fc81f6200b92de29fb3e956c5e6d12127 Mon Sep 17 00:00:00 2001 From: Richard Bailey Date: Thu, 23 Apr 2026 20:19:50 +0100 Subject: [PATCH 2/9] Fix CI We were using a very old vcpkg revision that attempted to download a version of 7zip that is no longer available. Updating to the latest action version while we're touching the build file Were also experiencing a build failure due to msys2 / gcc being used on Windows, so have added the ilammy/msvc-dev-cmd action to setup an MSVC environment. The build failure was possibly exposing a legitimate issue (using dllimport on template functions), but out of scope for this. --- .github/workflows/build.yml | 9 +++++---- vcpkg.json | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) create mode 100644 vcpkg.json diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1d00c6fa..0e23cc46 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -41,13 +41,14 @@ jobs: uses: lukka/get-cmake@latest - name: Restore artifacts, or run vcpkg, build and cache artifacts - uses: lukka/run-vcpkg@v7 + uses: lukka/run-vcpkg@v11 id: runvcpkg with: - vcpkgArguments: 'boost-variant boost-optional boost-format boost-functional boost-range boost-iterator boost-rational' - vcpkgTriplet: '${{ matrix.triplet }}' vcpkgDirectory: '${{ runner.workspace }}/b/vcpkg' - vcpkgGitCommitId: '7ad236f60f5f7197e93c4d7f0807622f4899076d' + vcpkgGitCommitId: 'b46d9050a9d40d54d24cac3ef8d50402d421f598' + + # Setup MSVC environment on windows, otherwise we get minsys2 / gcc + - uses: ilammy/msvc-dev-cmd@v1 - name: 'Install ubuntu dependencies' if: matrix.os == 'ubuntu-latest' diff --git a/vcpkg.json b/vcpkg.json new file mode 100644 index 00000000..9f9482f0 --- /dev/null +++ b/vcpkg.json @@ -0,0 +1,15 @@ +{ + "name": "libadm", + "version": "0.14.0", + "homepage": "https://github.com/ebu/libadm", + "license": "Apache-2.0", + "dependencies": [ + "boost-variant", + "boost-optional", + "boost-format", + "boost-functional", + "boost-range", + "boost-iterator", + "boost-rational" + ] +} \ No newline at end of file From 1c46509701e10fe5d05bfc86fbaf1813aabfbbf9 Mon Sep 17 00:00:00 2001 From: Richard Bailey Date: Fri, 10 Jul 2026 15:38:19 +0100 Subject: [PATCH 3/9] feat(tag-list): add TagList and ProfileList to Document [copilot] Introduce Tag, TagGroup, TagList wiring in the public API and XML parser/formatter, reuse ProfileList from SADM in ADM Document Review-depth: high Review-reason: Adds new user-visible ADM elements and XML behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- include/adm/document.hpp | 40 ++- include/adm/elements.hpp | 1 + include/adm/elements/audio_channel_format.hpp | 2 +- include/adm/elements/profile_list.hpp | 4 +- include/adm/elements/tag_list.hpp | 300 ++++++++++++++++++ include/adm/helper/element_range.hpp | 12 + include/adm/private/document_parser.hpp | 6 + include/adm/private/rapidxml_formatter.hpp | 3 + include/adm/private/rapidxml_wrapper.hpp | 14 + src/CMakeLists.txt | 1 + src/document.cpp | 36 +++ src/elements/tag_list.cpp | 102 ++++++ src/private/document_parser.cpp | 128 ++++++++ src/private/rapidxml_formatter.cpp | 20 ++ src/private/xml_writer.cpp | 2 + tests/CMakeLists.txt | 1 + tests/tag_list_tests.cpp | 121 +++++++ tests/test_data/tag_list.accepted.xml | 32 ++ 18 files changed, 815 insertions(+), 10 deletions(-) create mode 100644 include/adm/elements/tag_list.hpp create mode 100644 src/elements/tag_list.cpp create mode 100644 tests/tag_list_tests.cpp create mode 100644 tests/test_data/tag_list.accepted.xml diff --git a/include/adm/document.hpp b/include/adm/document.hpp index 3e605782..da5681e7 100644 --- a/include/adm/document.hpp +++ b/include/adm/document.hpp @@ -20,19 +20,29 @@ namespace adm { namespace detail { extern template class ADM_EXPORT_TEMPLATE_METHODS OptionalParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + OptionalParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + OptionalParameter; - using DocumentBase = HasParameters>; + using DocumentBase = + HasParameters, OptionalParameter, + OptionalParameter>; } // namespace detail /** * @brief Class representation of a whole ADM document * * \rst - * +---------------+-----------------+----------------------------+ - * | ADM Parameter | Parameter Type | Pattern Type | - * +===============+=================+============================+ - * | version | :type:`Version` | :class:`OptionalParameter` | - * +---------------+-----------------+----------------------------+ + * +---------------+---------------------+----------------------------+ + * | ADM Parameter | Parameter Type | Pattern Type | + * +===============+=====================+============================+ + * | version | :type:`Version` | :class:`OptionalParameter` | + * +---------------+---------------------+----------------------------+ + * | tagList | :type:`TagList` | :class:`OptionalParameter` | + * +---------------+---------------------+----------------------------+ + * | profileList | :type:`ProfileList` | :class:`OptionalParameter` | + * +---------------+---------------------+----------------------------+ * \endrst * * Note that: @@ -236,6 +246,23 @@ namespace adm { using detail::AddWrapperMethods::isDefault; using detail::AddWrapperMethods::unset; + /** + * @brief Set the document's tagList. + * + * Each TagGroup's audioProgramme/audioContent/audioObject references are + * validated against the document: + * * if a referenced element already belongs to a *different* document, + * the document is left unmodified and `false` is returned; + * * if a referenced element is not yet attached to any document, it is + * added to this document (mirroring the auto-add behaviour of + * `Document::add(...)` for nested references); + * * elements already belonging to this document are left untouched. + * + * @return `true` on success, `false` if any reference belongs to another + * document. + */ + ADM_EXPORT bool set(TagList tagList); + private: ADM_EXPORT Document(); ADM_EXPORT Document(const Document &) = default; @@ -316,5 +343,4 @@ namespace adm { typedef typename detail::ParameterTraits::tag Tag; return getElements(Tag()); } - } // namespace adm diff --git a/include/adm/elements.hpp b/include/adm/elements.hpp index cfea6c39..c51ba9f2 100644 --- a/include/adm/elements.hpp +++ b/include/adm/elements.hpp @@ -27,6 +27,7 @@ #include "adm/elements/audio_stream_format.hpp" #include "adm/elements/audio_track_uid.hpp" #include "adm/elements/profile_list.hpp" +#include "adm/elements/tag_list.hpp" #include "adm/elements/audio_block_format_direct_speakers.hpp" #include "adm/elements/audio_block_format_matrix.hpp" diff --git a/include/adm/elements/audio_channel_format.hpp b/include/adm/elements/audio_channel_format.hpp index 45df467e..aa474157 100644 --- a/include/adm/elements/audio_channel_format.hpp +++ b/include/adm/elements/audio_channel_format.hpp @@ -381,7 +381,7 @@ namespace adm { return previous == 0u || current == previous.get() + 1u; } } - + template void AudioChannelFormat::assignId(BlockFormat &blockFormat, BlockFormat *previousBlock) { diff --git a/include/adm/elements/profile_list.hpp b/include/adm/elements/profile_list.hpp index a49faf0d..ec19a79e 100644 --- a/include/adm/elements/profile_list.hpp +++ b/include/adm/elements/profile_list.hpp @@ -85,12 +85,12 @@ namespace adm { using ProfileListBase = HasParameters>; } // namespace detail - struct ProfileceListTag {}; + struct ProfileListTag {}; class ProfileList : private detail::ProfileListBase, private detail::AddWrapperMethods { public: - using tag = ProfileceListTag; + using tag = ProfileListTag; template explicit ProfileList(Parameters... namedArgs) { diff --git a/include/adm/elements/tag_list.hpp b/include/adm/elements/tag_list.hpp new file mode 100644 index 00000000..136fe469 --- /dev/null +++ b/include/adm/elements/tag_list.hpp @@ -0,0 +1,300 @@ +#pragma once +#include +#include "adm/detail/auto_base.hpp" +#include "adm/elements/audio_programme.hpp" +#include "adm/elements_fwd.hpp" +#include "adm/detail/named_option_helper.hpp" +#include "adm/detail/optional_comparison.hpp" +#include "adm/errors.hpp" + +namespace adm { + + class TagList; + + struct TagValueTag {}; + using TagValue = detail::NamedType; + + struct TagClassTag {}; + using TagClass = detail::NamedType; + + struct TagTag {}; + + namespace detail { + extern template class ADM_EXPORT_TEMPLATE_METHODS + RequiredParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + OptionalParameter; + + using TagBase = + HasParameters, OptionalParameter>; + } // namespace detail + + class Tag : private detail::TagBase, private detail::AddWrapperMethods { + public: + using tag = TagTag; + + template + explicit Tag(Parameters... namedArgs) { + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + + ADM_EXPORT explicit Tag(std::string str) : Tag(TagValue(std::move(str))) {} + ADM_EXPORT explicit Tag(const char *s); + + ADM_EXPORT void print(std::ostream &os) const; + + using detail::TagBase::set; + using detail::TagBase::unset; + using detail::AddWrapperMethods::get; + using detail::AddWrapperMethods::has; + using detail::AddWrapperMethods::isDefault; + using detail::AddWrapperMethods::unset; + + private: + using detail::TagBase::get; + using detail::TagBase::has; + + friend class detail::AddWrapperMethods; + }; + + struct TagsTag {}; + + using Tags = std::vector; + ADD_TRAIT(Tags, TagsTag); + + inline bool operator==(const Tag &a, const Tag &b) { + return detail::optionalsEqual(a, b); + } + + inline bool operator!=(const Tag &a, const Tag &b) { return !(a == b); } + + struct TagGroupTag {}; + + namespace detail { + extern template class ADM_EXPORT_TEMPLATE_METHODS VectorParameter; + + using TagGroupBase = HasParameters>; + } // namespace detail + + class TagGroup : private detail::TagGroupBase, + private detail::AddWrapperMethods { + public: + enum class RemoveResult { + Success, + LastReferenceError, // A TagGroup must always have at least one reference + NotFound + }; + using tag = TagGroupTag; + + TagGroup() = default; + + template + explicit TagGroup(std::shared_ptr const &reference, + Parameters... namedArgs) { + addReference(reference); + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + template + explicit TagGroup(std::shared_ptr const &reference, + Parameters... namedArgs) { + addReference(reference); + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + template + explicit TagGroup(std::shared_ptr const &reference, + Parameters... namedArgs) { + addReference(reference); + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + + /// @brief Add reference to an AudioProgramme + ADM_EXPORT bool addReference(std::shared_ptr programme); + + /// @brief Add reference to an AudioContent + ADM_EXPORT bool addReference(std::shared_ptr content); + + /// @brief Add reference to an AudioObject + ADM_EXPORT bool addReference(std::shared_ptr object); + + template + ElementRange getReferences(); + + template + ElementRange getReferences() const; + + /// @brief Remove reference to an AudioProgramme + ADM_EXPORT RemoveResult + removeReference(std::shared_ptr programme); + + /// @brief Remove reference to an AudioContent + ADM_EXPORT RemoveResult + removeReference(std::shared_ptr content); + + /// @brief Remove reference to an AudioObject + ADM_EXPORT RemoveResult + removeReference(std::shared_ptr object); + + template + void clearReferences(); + + using AddWrapperMethods::get; + using AddWrapperMethods::has; + using AddWrapperMethods::isDefault; + using AddWrapperMethods::unset; + using detail::TagGroupBase::add; + using detail::TagGroupBase::remove; + using detail::TagGroupBase::set; + + template + bool has() const { + return has(typename detail::ParameterTraits::tag{}); + } + + private: + using detail::TagGroupBase::get; + using detail::TagGroupBase::has; + using detail::TagGroupBase::isDefault; + using detail::TagGroupBase::unset; + + friend class detail::AddWrapperMethods; + friend class Document; + friend class TagList; + + bool invalid() const; + + ADM_EXPORT ElementRange getReferences( + detail::ParameterTraits::tag) const; + ADM_EXPORT ElementRange getReferences( + detail::ParameterTraits::tag); + ADM_EXPORT ElementRange getReferences( + detail::ParameterTraits::tag) const; + ADM_EXPORT ElementRange getReferences( + detail::ParameterTraits::tag); + ADM_EXPORT ElementRange getReferences( + detail::ParameterTraits::tag) const; + ADM_EXPORT ElementRange getReferences( + detail::ParameterTraits::tag); + + std::vector> audioProgrammes_; + std::vector> audioContents_; + std::vector> audioObjects_; + }; + + inline bool operator==(const TagGroup &a, const TagGroup &b) { + return detail::optionalsEqual(a, b) && + detail::elementRangeEqual( + a.getReferences(), + b.getReferences()) && + detail::elementRangeEqual( + a.getReferences(), + b.getReferences()) && + detail::elementRangeEqual( + a.getReferences(), + b.getReferences()); + } + + inline bool operator!=(const TagGroup &a, const TagGroup &b) { + return !(a == b); + } + + template + ElementRange TagGroup::getReferences() const { + typedef typename detail::ParameterTraits::tag Tag; + return getReferences(Tag()); + } + + template + ElementRange TagGroup::getReferences() { + typedef typename detail::ParameterTraits::tag Tag; + return getReferences(Tag()); + } + + inline ElementRange TagGroup::getReferences( + detail::ParameterTraits::tag) const { + return ElementRange(audioProgrammes_.begin(), + audioProgrammes_.end()); + } + + inline ElementRange TagGroup::getReferences( + detail::ParameterTraits::tag) const { + return ElementRange(audioContents_.begin(), + audioContents_.end()); + } + + inline ElementRange TagGroup::getReferences( + detail::ParameterTraits::tag) const { + return ElementRange(audioObjects_.begin(), + audioObjects_.end()); + } + + inline ElementRange TagGroup::getReferences( + detail::ParameterTraits::tag) { + return ElementRange(audioProgrammes_.begin(), + audioProgrammes_.end()); + } + + inline ElementRange TagGroup::getReferences( + detail::ParameterTraits::tag) { + return ElementRange(audioContents_.begin(), + audioContents_.end()); + } + + inline ElementRange TagGroup::getReferences( + detail::ParameterTraits::tag) { + return ElementRange(audioObjects_.begin(), + audioObjects_.end()); + } + + template + void TagGroup::clearReferences() { + typedef typename detail::ParameterTraits::tag Tag; + clearReferences(Tag()); + } + + struct TagGroupsTag {}; + + using TagGroups = std::vector; + ADD_TRAIT(TagGroups, TagGroupsTag); + + namespace detail { + extern template class ADM_EXPORT_TEMPLATE_METHODS + VectorParameter; + + using TagListBase = HasParameters>; + } // namespace detail + + struct TagListTag {}; + + class TagList : private detail::TagListBase, + private detail::AddWrapperMethods, + public std::enable_shared_from_this { + public: + template + std::shared_ptr create(Parameters... namedArgs) { + return std::make_shared( + std::forward(namedArgs...)); + } + using tag = TagListTag; + using detail::TagListBase::set; + using detail::AddWrapperMethods::get; + using detail::AddWrapperMethods::has; + using detail::AddWrapperMethods::isDefault; + using detail::AddWrapperMethods::unset; + using detail::TagListBase::remove; + + ADM_EXPORT bool add(TagGroup group); + + template + explicit TagList(Parameters... namedArgs) { + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + + private: + using detail::TagListBase::get; + using detail::TagListBase::has; + using detail::TagListBase::isDefault; + using detail::TagListBase::unset; + + friend class detail::AddWrapperMethods; + }; +} // namespace adm diff --git a/include/adm/helper/element_range.hpp b/include/adm/helper/element_range.hpp index 1cd1678a..8d170942 100644 --- a/include/adm/helper/element_range.hpp +++ b/include/adm/helper/element_range.hpp @@ -197,6 +197,18 @@ namespace adm { [](std::weak_ptr w) { return w.lock(); }); return result; } + template + bool elementRangeEqual(ElementRange a, + ElementRange b) { + if (a.size() != b.size()) { + return false; + } + bool equal = true; + for (std::size_t i = 0; i != a.size() && equal; ++i) { + equal = a[i] == b[i]; + } + return equal; + } } // namespace detail diff --git a/include/adm/private/document_parser.hpp b/include/adm/private/document_parser.hpp index ee2ac5c2..57ef5b7f 100644 --- a/include/adm/private/document_parser.hpp +++ b/include/adm/private/document_parser.hpp @@ -70,6 +70,7 @@ namespace adm { NodePtr node, boost::optional timeReference); Profile parseProfile(NodePtr node); ProfileList parseProfileList(NodePtr node); + Tag parseTTag(NodePtr node); NodePtr findAudioFormatExtendedNodeEbuCore(NodePtr root); NodePtr findAudioFormatExtendedNodeFullRecursive(NodePtr root); @@ -105,6 +106,8 @@ namespace adm { std::shared_ptr parseAudioPackFormat(NodePtr node); std::shared_ptr parseAudioTrackUid(NodePtr node); std::shared_ptr parseAudioChannelFormat(NodePtr node); + std::shared_ptr parseTagGroup(NodePtr node); + TagList parseTagList(NodePtr node); rapidxml::file<> xmlFile_; ParserOptions options_; @@ -127,6 +130,9 @@ namespace adm { std::map, AudioChannelFormatId> streamFormatChannelFormatRef_; std::map, AudioPackFormatId> streamFormatPackFormatRef_; std::map, std::vector> streamFormatTrackFormatRefs_; + std::map, std::vector> tagGroupProgrammeRefs_; + std::map, std::vector> tagGroupContentRefs_; + std::map, std::vector> tagGroupObjectRefs_; // clang-format on /// used to keep track of element IDs ourselves to avoid having it diff --git a/include/adm/private/rapidxml_formatter.hpp b/include/adm/private/rapidxml_formatter.hpp index df06671e..4ba22a3e 100644 --- a/include/adm/private/rapidxml_formatter.hpp +++ b/include/adm/private/rapidxml_formatter.hpp @@ -55,6 +55,9 @@ namespace adm { XmlNode &node, const std::shared_ptr trackUid); void formatProfileList(XmlNode &node, const ProfileList &profileList); void formatProfile(XmlNode &node, const Profile &profile); + void formatTagList(XmlNode &node, const TagList &tagList); + void formatTagGroup(XmlNode &node, const TagGroup &tagGroup); + void formatTag(XmlNode &node, const Tag &tag); void formatBlockFormatDirectSpeakers( XmlNode &node, const AudioBlockFormatDirectSpeakers &audioBlock, diff --git a/include/adm/private/rapidxml_wrapper.hpp b/include/adm/private/rapidxml_wrapper.hpp index 5d4ce89a..65a9af99 100644 --- a/include/adm/private/rapidxml_wrapper.hpp +++ b/include/adm/private/rapidxml_wrapper.hpp @@ -102,6 +102,10 @@ namespace adm { void addBaseElements(const Source &src, const std::string &name, Callable formatter); + template + void addBaseElement(const Source &src, const std::string &name, + Callable formatter); + template void addReference(const Source &src, const std::string &name); @@ -262,6 +266,16 @@ namespace adm { } } + template + void XmlNode::addBaseElement(const Source &src, const std::string &name, + Callable formatter) { + auto admElement = src->template getElement(); + if (admElement) { + auto node = addNode(name); + formatter(node, *admElement); + } + } + template void XmlNode::addReference(const Source &src, const std::string &name) { addElement(src->template getReference(), name); diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index b11f800f..7e4d98d3 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -54,6 +54,7 @@ add_library(adm elements/type_descriptor.cpp elements/format_descriptor.cpp elements/headphone_virtualise.cpp + elements/tag_list.cpp utilities/block_duration_assignment.cpp utilities/copy.cpp utilities/id_assignment.cpp diff --git a/src/document.cpp b/src/document.cpp index 85759cdd..3b59119f 100644 --- a/src/document.cpp +++ b/src/document.cpp @@ -12,6 +12,8 @@ namespace adm { namespace detail { template class OptionalParameter; + template class OptionalParameter; + template class OptionalParameter; } // namespace detail Document::Document() { idAssigner_.document(this); } @@ -256,6 +258,40 @@ namespace adm { return false; } + namespace { + template + bool tagGroupRefsBelongToOtherDoc( + Document const& doc, + std::vector> const& refs) { + for (auto const& ref : refs) { + auto parent = ref->getParent().lock(); + if (parent && parent.get() != &doc) return true; + } + return false; + } + } // namespace + + bool Document::set(TagList tagList) { + // Validate every TagGroup reference against this document up-front so a + // failure leaves the document unmodified. + for (auto const& group : tagList.get()) { + if (tagGroupRefsBelongToOtherDoc(*this, group.audioProgrammes_) || + tagGroupRefsBelongToOtherDoc(*this, group.audioContents_) || + tagGroupRefsBelongToOtherDoc(*this, group.audioObjects_)) { + return false; + } + } + // Adopt any unparented references into this document. Elements already + // belonging to this document are short-circuited by checkParent() inside + // add(). + for (auto const& group : tagList.get()) { + for (auto const& p : group.audioProgrammes_) add(p); + for (auto const& c : group.audioContents_) add(c); + for (auto const& o : group.audioObjects_) add(o); + } + detail::DocumentBase::set(std::move(tagList)); + return true; + } bool Document::remove(std::shared_ptr packFormat) { auto it = std::find(audioPackFormats_.begin(), audioPackFormats_.end(), packFormat); diff --git a/src/elements/tag_list.cpp b/src/elements/tag_list.cpp new file mode 100644 index 00000000..f76c16bf --- /dev/null +++ b/src/elements/tag_list.cpp @@ -0,0 +1,102 @@ +#include "adm/elements/tag_list.hpp" +#include + +namespace adm { + Tag::Tag(const char* s) { + // to avoid UB from std::string + if (!s) { + throw error::AdmGenericRuntimeError{ + "Cannot construct Tag from null const char*"}; + } + set(TagValue{std::string{s}}); + } + + template + bool add_reference(std::vector>& references, + std::shared_ptr ref) { + auto it = std::find(references.begin(), references.end(), ref); + if (it == references.end()) { + references.push_back(ref); + return true; + } + return false; + } + + // ---- References ---- // + bool TagGroup::addReference(std::shared_ptr programme) { + return add_reference(audioProgrammes_, std::move(programme)); + } + + bool TagGroup::addReference(std::shared_ptr content) { + return add_reference(audioContents_, std::move(content)); + } + + bool TagGroup::addReference(std::shared_ptr object) { + return add_reference(audioObjects_, std::move(object)); + } + + template + TagGroup::RemoveResult remove_reference( + std::vector>& references, + std::shared_ptr const& ref) { + auto it = std::find(references.begin(), references.end(), ref); + if (it == references.end()) { + return TagGroup::RemoveResult::NotFound; + } + references.erase(it); + return TagGroup::RemoveResult::Success; + } + + TagGroup::RemoveResult TagGroup::removeReference( + std::shared_ptr programme) { + if (remove_reference(audioProgrammes_, programme) == + RemoveResult::NotFound) { + return RemoveResult::NotFound; + } + if (invalid()) { + addReference(std::move(programme)); + return RemoveResult::LastReferenceError; + } + return RemoveResult::Success; + } + + TagGroup::RemoveResult TagGroup::removeReference( + std::shared_ptr content) { + if (remove_reference(audioContents_, content) == RemoveResult::NotFound) { + return RemoveResult::NotFound; + } + if (invalid()) { + addReference(std::move(content)); + return RemoveResult::LastReferenceError; + } + return RemoveResult::Success; + } + + TagGroup::RemoveResult TagGroup::removeReference( + std::shared_ptr object) { + if (remove_reference(audioObjects_, object) == RemoveResult::NotFound) { + return RemoveResult::NotFound; + } + if (invalid()) { + addReference(std::move(object)); + return RemoveResult::LastReferenceError; + } + return RemoveResult::Success; + } + + bool TagGroup::invalid() const { + return audioContents_.empty() && audioObjects_.empty() && + audioProgrammes_.empty(); + } + + bool TagList::add(TagGroup group) { + return detail::TagListBase::add(std::move(group)); + } + + namespace detail { + template class RequiredParameter; + template class OptionalParameter; + template class VectorParameter; + template class VectorParameter; + } // namespace detail +} // namespace adm diff --git a/src/private/document_parser.cpp b/src/private/document_parser.cpp index 58c0603d..20e67e29 100644 --- a/src/private/document_parser.cpp +++ b/src/private/document_parser.cpp @@ -3,6 +3,7 @@ #include "adm/private/xml_parser_helper.hpp" #include "adm/detail/named_type_validators.hpp" #include "adm/errors.hpp" + namespace adm { namespace xml { @@ -111,6 +112,18 @@ namespace adm { resolveReference(streamFormatPackFormatRef_); resolveReferences(streamFormatTrackFormatRefs_); + // add other ADM elements to ADM document + for (NodePtr node = root->first_node(); node; + node = node->next_sibling()) { + std::string nodeName(node->name(), node->name_size()); + if (nodeName == "profileList") { + // Can't use the local add function as that contains an ID setting + document_->set(parseProfileList(node)); + } else if (nodeName == "tagList") { + // Can't use the local add function as that contains an ID setting + document_->set(parseTagList(node)); + } + } } else { throw error::XmlParsingError("audioFormatExtended node not found"); } @@ -540,6 +553,121 @@ namespace adm { return profileList; } + Tag parseTTag(NodePtr node) { + Tag ttag; + setValue(node, ttag); + setOptionalAttribute(node, "class", ttag); + return ttag; + } + + struct TagGroupBuilder { + TagGroupBuilder(std::vector programmeNodes, + std::vector contentNodes, + std::vector objectNodes) { + for (auto n : programmeNodes) { + programmeIds.push_back(parseAudioProgrammeId(n->value())); + } + for (auto n : contentNodes) { + contentIds.push_back(parseAudioContentId(n->value())); + } + for (auto n : objectNodes) { + objectIds.push_back(parseAudioObjectId(n->value())); + } + } + + bool valid_ids() const { + return !(programmeIds.empty() && contentIds.empty() && + objectIds.empty()); + } + + void resolveReferences(adm::detail::IDMap& map) { + for (auto const& id : programmeIds) { + if (auto element = map.lookup(id)) { + programmes.push_back(element); + } + } + for (auto const& id : contentIds) { + if (auto element = map.lookup(id)) { + contents.push_back(element); + } + } + for (auto const& id : objectIds) { + if (auto element = map.lookup(id)) { + objects.push_back(element); + } + } + } + + bool valid_references() { + return !(programmes.empty() && contents.empty() && objects.empty()); + } + + template + void add_all(std::vector> const& refs, + TagGroup& group) { + for (auto const& r : refs) { + group.addReference(r); + } + } + + template + std::shared_ptr create_with( + std::vector>& refs) { + auto last = refs.back(); + auto group = std::make_shared(last); + refs.pop_back(); + add_all(refs, *group); + refs.clear(); + return group; + } + + std::shared_ptr build(adm::detail::IDMap& id_map) { + std::shared_ptr group; + if (!valid_ids()) return group; + resolveReferences(id_map); + if (!valid_references()) return group; + if (!programmes.empty()) { + group = create_with(programmes); + } else if (!contents.empty()) { + group = create_with(contents); + } else if (!objects.empty()) { + group = create_with(objects); + } + add_all(programmes, *group); + add_all(contents, *group); + add_all(objects, *group); + return group; + } + + std::vector programmeIds; + std::vector contentIds; + std::vector objectIds; + std::vector> programmes; + std::vector> contents; + std::vector> objects; + }; + + std::shared_ptr DocumentParser::parseTagGroup(NodePtr node) { + TagGroupBuilder builder(detail::findElements(node, "audioProgrammeIDRef"), + detail::findElements(node, "audioContentIDRef"), + detail::findElements(node, "audioObjectIDRef")); + auto tagGroup = builder.build(idMap_); + if (!tagGroup) { + throw std::runtime_error("Error parsing tag group"); + } + addOptionalElements(node, "tag", tagGroup, &parseTTag); + return tagGroup; + } + + TagList DocumentParser::parseTagList(NodePtr node) { + TagList tagList; + auto elements = detail::findElements(node, "tagGroup"); + for (auto& element : elements) { + detail::invokeAdd(tagList, TagGroup(*parseTagGroup(element))); + } + return tagList; + } + namespace { template void addTimeParametersToBlock( diff --git a/src/private/rapidxml_formatter.cpp b/src/private/rapidxml_formatter.cpp index 2b0028f0..7534bd50 100644 --- a/src/private/rapidxml_formatter.cpp +++ b/src/private/rapidxml_formatter.cpp @@ -790,5 +790,25 @@ namespace adm { node.addAttribute(&profile, "profileVersion"); node.addAttribute(&profile, "profileLevel"); } + + void formatTagList(XmlNode &node, const TagList &tagList) { + node.addVectorElements(&tagList, "tagGroup", &formatTagGroup); + } + + void formatTagGroup(XmlNode &node, const TagGroup &tagGroup) { + node.addVectorElements(&tagGroup, "tag", &formatTag); + node.addReferences( + &tagGroup, "audioProgrammeIDRef"); + node.addReferences(&tagGroup, + "audioContentIDRef"); + node.addReferences(&tagGroup, + "audioObjectIDRef"); + } + + void formatTag(XmlNode &node, const Tag &tag) { + node.setValue(tag.get()); + node.addOptionalAttribute(&tag, "class"); + } + } // namespace xml } // namespace adm diff --git a/src/private/xml_writer.cpp b/src/private/xml_writer.cpp index 6ebc608a..401c45af 100644 --- a/src/private/xml_writer.cpp +++ b/src/private/xml_writer.cpp @@ -30,6 +30,8 @@ namespace adm { TimeReference timeReference = TimeReference::TOTAL) { // clang-format off audioFormatExtended.addOptionalAttribute(document, "version"); + audioFormatExtended.addOptionalElement(document, "profileList", &formatProfileList); + audioFormatExtended.addOptionalElement(document, "tagList", &formatTagList); audioFormatExtended.addBaseElements(document, "audioProgramme", &formatAudioProgramme); audioFormatExtended.addBaseElements(document, "audioContent", &formatAudioContent); audioFormatExtended.addBaseElements(document, "audioObject", &formatAudioObject); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0fef76a9..0315572e 100755 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -72,6 +72,7 @@ add_adm_test("screen_edge_lock_tests") add_adm_test("speaker_position_tests") add_adm_test("type_descriptor_tests") add_adm_test("version_tests") +add_adm_test("tag_list_tests") add_adm_test("xml_audio_block_format_objects_tests") add_adm_test("xml_loudness_metadata_tests") add_adm_test("xml_parser_audio_block_format_direct_speakers_tests") diff --git a/tests/tag_list_tests.cpp b/tests/tag_list_tests.cpp new file mode 100644 index 00000000..017def73 --- /dev/null +++ b/tests/tag_list_tests.cpp @@ -0,0 +1,121 @@ +#include +#include "helper/parameter_checks.hpp" +#include "adm/document.hpp" +#include "adm/utilities/object_creation.hpp" +#include "adm/parse.hpp" +#include "adm/write.hpp" +#include "helper/file_comparator.hpp" +#include "adm/elements/tag_list.hpp" + +#include + +using namespace adm; +using namespace adm_test; + +TEST_CASE("Tag parameters") { + Tag tag{TagValue("value")}; + + check_optional_param(tag, canBeSetTo(TagClass{"class2"})); + check_required_param(tag, hasDefaultOf(TagValue{"value"}), + canBeSetTo(TagValue{"value2"})); +} + +TEST_CASE("TagGroup parameters") { + TagGroup tagGroup; + Tag tag{TagClass("class"), TagValue("value")}; + + check_vector_param(tagGroup, canBeSetTo(Tags{tag})); +} + +TEST_CASE("TagList parameters") { + Tag tag{TagClass("class"), TagValue("value")}; + TagGroup tagGroup; + tagGroup.add(tag); + TagList tagList; + + check_vector_param(tagList, canBeSetTo(TagGroups{tagGroup})); +} + +TEST_CASE("adm xml/taglist") { + auto doc = parseXml("tag_list.accepted.xml"); + + REQUIRE(doc->has()); + auto tag_list = doc->get(); + auto tag_groups = tag_list.get(); + REQUIRE(tag_groups.size() == 1); + + auto tag_group = tag_groups.at(0); + auto tags = tag_group.get(); + auto tag = tags.at(0); + CHECK(tag.get() == "class1"); + CHECK(tag.get() == "value1"); + + std::stringstream xml; + writeXml(xml, doc); + CHECK_THAT(xml.str(), EqualsXmlFile("tag_list")); +} +TEST_CASE( + "TagList referencing elements owned by another document is rejected") { + auto otherDoc = Document::create(); + auto holder = addSimpleObjectTo(otherDoc, "Other"); + + TagGroup tagGroup{holder.audioObject}; + tagGroup.add(Tag{TagValue("v")}); + TagList tagList; + tagList.add(tagGroup); + + auto doc = Document::create(); + REQUIRE(doc->set(tagList) == false); + REQUIRE(!doc->has()); +} + +TEST_CASE( + "TagList referencing unparented elements adopts them into the document") { + auto doc = Document::create(); + auto programme = AudioProgramme::create(AudioProgrammeName{"P"}); + auto content = AudioContent::create(AudioContentName{"C"}); + auto object = AudioObject::create(AudioObjectName{"O"}); + + TagGroup tagGroup{programme}; + tagGroup.addReference(content); + tagGroup.addReference(object); + tagGroup.add(Tag{TagValue("v")}); + TagList tagList; + tagList.add(tagGroup); + + doc->set(tagList); + REQUIRE(doc->has()); + // The previously-unparented elements should now belong to the document. + REQUIRE(programme->getParent().lock() == doc); + REQUIRE(content->getParent().lock() == doc); + REQUIRE(object->getParent().lock() == doc); + // ... and be reachable through the usual element collections. + auto programmes = doc->getElements(); + REQUIRE(std::find(programmes.begin(), programmes.end(), programme) != + programmes.end()); + auto contents = doc->getElements(); + REQUIRE(std::find(contents.begin(), contents.end(), content) != + contents.end()); + auto objects = doc->getElements(); + REQUIRE(std::find(objects.begin(), objects.end(), object) != objects.end()); +} + +TEST_CASE( + "TagGroups with same tag are only equal if references are also equal") { + auto object = adm::AudioObject::create(AudioObjectName("First")); + auto programme = adm::AudioProgramme::create(AudioProgrammeName("Second")); + Tag tag("Duplicate"); + TagGroup first(object); + first.add(tag); + + TagGroup second(programme); + second.add(tag); + + TagGroup third(object); + third.add(tag); + + REQUIRE(first != second); + REQUIRE(first == first); + REQUIRE(second == second); + REQUIRE(first == third); +} \ No newline at end of file diff --git a/tests/test_data/tag_list.accepted.xml b/tests/test_data/tag_list.accepted.xml new file mode 100644 index 00000000..4c349e5d --- /dev/null +++ b/tests/test_data/tag_list.accepted.xml @@ -0,0 +1,32 @@ + + + + + + + + value1 + APR_1001 + ACO_1001 + AO_1001 + + + + ACO_1001 + + + AO_1001 + + + AP_00010001 + ATU_00000001 + + + AT_00010003_01 + AP_00010001 + + + + + + From 362af423533617653f6d3622ec410b0556dbad03 Mon Sep 17 00:00:00 2001 From: Richard Bailey Date: Fri, 10 Jul 2026 15:53:34 +0100 Subject: [PATCH 4/9] feat(tag-list): add deepCopy remapping and remove-side pruning [copilot] Move TagList/ProfileList auxiliary copying into shared copy machinery, remap TagGroup references during deepCopy, and centralize TagGroup pruning with template-based remove-side logic. Review-depth: high Review-reason: Alters deep-copy semantics and document remove side effects for TagList references. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- include/adm/document.hpp | 6 ++ include/adm/private/copy.hpp | 39 ++++++++++ src/document.cpp | 43 ++++++++++- src/private/copy.cpp | 59 +++++++++++---- src/utilities/copy.cpp | 4 +- tests/profile_list_tests.cpp | 37 +++++++++ tests/tag_list_tests.cpp | 140 ++++++++++++++++++++++++++++++++++- 7 files changed, 307 insertions(+), 21 deletions(-) diff --git a/include/adm/document.hpp b/include/adm/document.hpp index da5681e7..96dd009d 100644 --- a/include/adm/document.hpp +++ b/include/adm/document.hpp @@ -96,6 +96,12 @@ namespace adm { * * References from and to the ADM element will automatically be removed * too. + * + * Additional side effects: + * - removing an AudioProgramme, AudioContent, or AudioObject prunes + * TagGroup entries in tagList that reference the removed element + * - If the TagList becomes empty due to pruning, the list itself is + * removed as it is only valid when it contains 1 or more TagGroups */ ///@{ /// @brief Remove an AudioProgramme diff --git a/include/adm/private/copy.hpp b/include/adm/private/copy.hpp index 9fa6e544..3c464d22 100644 --- a/include/adm/private/copy.hpp +++ b/include/adm/private/copy.hpp @@ -9,9 +9,48 @@ namespace adm { + /** + * @brief Per-element-kind shared_ptr mapping from a source Document's + * elements to their copies. Populated by `copyAllElements` and consumed + * by reference-resolution helpers such as `resolveReferences` and + * `copyAuxiliary`. + */ + struct ElementMapping { + // clang-format off + std::unordered_map, std::shared_ptr> audioProgramme; + std::unordered_map, std::shared_ptr> audioContent; + std::unordered_map, std::shared_ptr> audioObject; + std::unordered_map, std::shared_ptr> audioPackFormat; + std::unordered_map, std::shared_ptr> audioChannelFormat; + std::unordered_map, std::shared_ptr> audioStreamFormat; + std::unordered_map, std::shared_ptr> audioTrackFormat; + std::unordered_map, std::shared_ptr> audioTrackUid; + // clang-format on + }; + std::vector copyAllElements( std::shared_ptr document); + /** + * @brief Like `copyAllElements`, but also reports the source-to-copy + * mapping for every element kind, so callers can translate references + * stored in document-level parameters (e.g. tagList). + */ + std::vector copyAllElements( + std::shared_ptr document, ElementMapping& mapping); + + /** + * @brief Copy document-level auxiliary parameters (ProfileList, TagList) + * from `src` to `dest`, translating any element references in the + * TagList through `mapping`. + * + * Must be called *after* the copied elements have been added to `dest`, + * so that `Document::set(TagList)` can adopt them via paternity checks. + */ + void copyAuxiliary(std::shared_ptr src, + std::shared_ptr dest, + ElementMapping const& mapping); + template class AddTo : public boost::static_visitor<> { public: diff --git a/src/document.cpp b/src/document.cpp index 3b59119f..ffcd43e5 100644 --- a/src/document.cpp +++ b/src/document.cpp @@ -10,6 +10,40 @@ #include namespace adm { + namespace { + template + bool pruneIf(ContainerT& container, Predicate predicate) { + auto end = std::remove_if(container.begin(), container.end(), predicate); + if (end == container.end()) { + return false; + } + container.erase(end, container.end()); + return true; + } + + template + void pruneTagGroupsReferencing( + Document& document, + std::shared_ptr const& removedElement) { + if (!document.has()) return; + auto list = document.get(); + auto groups = list.get(); + auto pruned = pruneIf(groups, [&](TagGroup const& group) { + auto refs = group.template getReferences(); + return std::find(refs.begin(), refs.end(), removedElement) != + refs.end(); + }); + if (!pruned) return; + if (groups.empty()) { + document.unset(); + return; + } + TagList newList; + for (auto& group : groups) newList.add(group); + document.set(newList); + } + } // namespace + namespace detail { template class OptionalParameter; template class OptionalParameter; @@ -33,8 +67,8 @@ namespace adm { copy->audioTrackFormats_.reserve(audioTrackFormats_.size()); copy->audioTrackUids_.reserve(audioTrackUids_.size()); - auto elements = copyAllElements(shared_from_this()); - if (has()) copy->set(get()); + ElementMapping mapping; + auto elements = copyAllElements(shared_from_this(), mapping); for (auto& e : elements) { if (auto v = boost::get>(&e)) { AudioProgrammeAttorney::setParent(*v, copy); @@ -62,6 +96,7 @@ namespace adm { copy->audioTrackUids_.push_back(*v); } } + copyAuxiliary(shared_from_this(), copy, mapping); return copy; } @@ -224,6 +259,7 @@ namespace adm { if (it != audioProgrammes_.end()) { audioProgrammes_.erase(it); AudioProgrammeAttorney::setParent(programme, {}); + pruneTagGroupsReferencing(*this, programme); return true; } return false; @@ -237,6 +273,7 @@ namespace adm { for (auto& audioProgramme : audioProgrammes_) { audioProgramme->removeReference(content); } + pruneTagGroupsReferencing(*this, content); return true; } return false; @@ -253,6 +290,7 @@ namespace adm { for (auto& audioContent : audioContents_) { audioContent->removeReference(object); } + pruneTagGroupsReferencing(*this, object); return true; } return false; @@ -292,6 +330,7 @@ namespace adm { detail::DocumentBase::set(std::move(tagList)); return true; } + bool Document::remove(std::shared_ptr packFormat) { auto it = std::find(audioPackFormats_.begin(), audioPackFormats_.end(), packFormat); diff --git a/src/private/copy.cpp b/src/private/copy.cpp index cf56b01e..55bdea7f 100644 --- a/src/private/copy.cpp +++ b/src/private/copy.cpp @@ -3,22 +3,8 @@ namespace adm { - struct ElementMapping { - // clang-format off - std::unordered_map, std::shared_ptr> audioProgramme; - std::unordered_map, std::shared_ptr> audioContent; - std::unordered_map, std::shared_ptr> audioObject; - std::unordered_map, std::shared_ptr> audioPackFormat; - std::unordered_map, std::shared_ptr> audioChannelFormat; - std::unordered_map, std::shared_ptr> audioStreamFormat; - std::unordered_map, std::shared_ptr> audioTrackFormat; - std::unordered_map, std::shared_ptr> audioTrackUid; - // clang-format on - }; - std::vector copyAllElements( - std::shared_ptr document) { - ElementMapping mapping; + std::shared_ptr document, ElementMapping& mapping) { std::vector copiedElements; // copy for (const auto& element : document->getElements()) { @@ -95,4 +81,47 @@ namespace adm { return copiedElements; } + std::vector copyAllElements( + std::shared_ptr document) { + ElementMapping mapping; + return copyAllElements(std::move(document), mapping); + } + + void copyAuxiliary(std::shared_ptr src, + std::shared_ptr dest, + ElementMapping const& mapping) { + if (src->has()) dest->set(src->get()); + if (src->has()) dest->set(src->get()); + if (!src->has()) return; + + auto srcTagList = src->get(); + TagList newTagList; + for (auto const& srcGroup : srcTagList.get()) { + // Translate each ref through the mapping. The source document is + // assumed valid: Document::set(TagList) and Document::remove() keep + // every TagGroup ref attached to the document, so it is guaranteed + // to be in the mapping (mirrors the assumption used by + // resolveReferences for ordinary cross-references). TagGroup has no + // default ctor, so the first translated ref seeds the new group. + std::unique_ptr newGroup; + auto translate = [&](auto const& srcRefs, auto const& mappingMap) { + for (auto const& r : srcRefs) { + auto const& mapped = mappingMap.at(r); + if (!newGroup) + newGroup.reset(new TagGroup(mapped)); + else + newGroup->addReference(mapped); + } + }; + translate(srcGroup.getReferences(), mapping.audioObject); + translate(srcGroup.getReferences(), mapping.audioContent); + translate(srcGroup.getReferences(), + mapping.audioProgramme); + if (!newGroup) continue; // not possible for a valid source document + for (auto const& tag : srcGroup.get()) newGroup->add(tag); + newTagList.add(*newGroup); + } + dest->set(std::move(newTagList)); + } + } // namespace adm diff --git a/src/utilities/copy.cpp b/src/utilities/copy.cpp index f6fa1db1..28ae3807 100644 --- a/src/utilities/copy.cpp +++ b/src/utilities/copy.cpp @@ -10,8 +10,10 @@ namespace adm { void deepCopyTo(std::shared_ptr src, std::shared_ptr dest) { - auto copiedElements = copyAllElements(src); + ElementMapping mapping; + auto copiedElements = copyAllElements(src, mapping); addElements(copiedElements, dest); + copyAuxiliary(src, dest, mapping); } } // namespace adm diff --git a/tests/profile_list_tests.cpp b/tests/profile_list_tests.cpp index 887ae81d..e5361902 100644 --- a/tests/profile_list_tests.cpp +++ b/tests/profile_list_tests.cpp @@ -66,3 +66,40 @@ TEST_CASE("sadm xml/profilelist") { writeXml(xml, document, header); CHECK_THAT(xml.str(), EqualsXmlFile("profile_list_frame_header")); } + +TEST_CASE("document deep copy preserves profile list") { + auto document = Document::create(); + + ProfileList profileList; + profileList.add(Profile{ProfileValue{"value1"}, ProfileName{"name1"}, + ProfileVersion{"version1"}, ProfileLevel{"level1"}}); + profileList.add(Profile{ProfileValue{"value2"}, ProfileName{"name2"}, + ProfileVersion{"version2"}, ProfileLevel{"level2"}}); + document->set(profileList); + + auto documentCopy = document->deepCopy(); + REQUIRE(documentCopy->has()); + auto copiedProfiles = documentCopy->get().get(); + REQUIRE(copiedProfiles.size() == 2); + + CHECK(copiedProfiles.at(0).get() == "value1"); + CHECK(copiedProfiles.at(0).get() == "name1"); + CHECK(copiedProfiles.at(0).get() == "version1"); + CHECK(copiedProfiles.at(0).get() == "level1"); + + CHECK(copiedProfiles.at(1).get() == "value2"); + CHECK(copiedProfiles.at(1).get() == "name2"); + CHECK(copiedProfiles.at(1).get() == "version2"); + CHECK(copiedProfiles.at(1).get() == "level2"); + + ProfileList updatedProfileList; + updatedProfileList.add( + Profile{ProfileValue{"changed"}, ProfileName{"changed"}, + ProfileVersion{"changed"}, ProfileLevel{"changed"}}); + document->set(updatedProfileList); + + auto copiedProfilesAfterUpdate = + documentCopy->get().get(); + REQUIRE(copiedProfilesAfterUpdate.size() == 2); + CHECK(copiedProfilesAfterUpdate.at(0).get() == "value1"); +} diff --git a/tests/tag_list_tests.cpp b/tests/tag_list_tests.cpp index 017def73..eea54421 100644 --- a/tests/tag_list_tests.cpp +++ b/tests/tag_list_tests.cpp @@ -1,11 +1,11 @@ #include #include "helper/parameter_checks.hpp" #include "adm/document.hpp" -#include "adm/utilities/object_creation.hpp" #include "adm/parse.hpp" #include "adm/write.hpp" #include "helper/file_comparator.hpp" #include "adm/elements/tag_list.hpp" +#include "adm/utilities/object_creation.hpp" #include @@ -21,15 +21,17 @@ TEST_CASE("Tag parameters") { } TEST_CASE("TagGroup parameters") { - TagGroup tagGroup; + auto programme = AudioProgramme::create(AudioProgrammeName{"Test"}); + TagGroup tagGroup{programme}; Tag tag{TagClass("class"), TagValue("value")}; check_vector_param(tagGroup, canBeSetTo(Tags{tag})); } TEST_CASE("TagList parameters") { + auto programme = AudioProgramme::create(AudioProgrammeName{"Test"}); Tag tag{TagClass("class"), TagValue("value")}; - TagGroup tagGroup; + TagGroup tagGroup{programme}; tagGroup.add(tag); TagList tagList; @@ -54,6 +56,138 @@ TEST_CASE("adm xml/taglist") { writeXml(xml, doc); CHECK_THAT(xml.str(), EqualsXmlFile("tag_list")); } + +TEST_CASE("document copy updates tagList references") { + auto doc = Document::create(); + auto holder = addSimpleObjectTo(doc, "Test"); + Tag tag{TagClass("class"), TagValue("value")}; + auto programme = AudioProgramme::create(AudioProgrammeName{"Test"}); + TagGroup tagGroup{programme}; + tagGroup.add(tag); + tagGroup.addReference(holder.audioObject); + auto tagList = TagList{}; + tagList.add(tagGroup); + doc->set(tagList); + + auto doc_copy = doc->deepCopy(); + auto copied_object = doc_copy->getElements().front(); + REQUIRE(doc_copy->has()); + auto copied_tag_groups = doc_copy->get().get(); + REQUIRE(!copied_tag_groups.empty()); + auto copied_tagged_object_refs = + copied_tag_groups.front().getReferences(); + REQUIRE(!copied_tagged_object_refs.empty()); + auto tagged_object_ref = copied_tagged_object_refs.front(); + REQUIRE(copied_object == tagged_object_ref); +} + +TEST_CASE("document copy remaps tagList programme/content/object references") { + auto doc = Document::create(); + auto programme = AudioProgramme::create(AudioProgrammeName{"Programme"}); + auto content = AudioContent::create(AudioContentName{"Content"}); + auto object = AudioObject::create(AudioObjectName{"Object"}); + doc->add(programme); + doc->add(content); + doc->add(object); + + TagGroup tagGroup{programme}; + tagGroup.addReference(content); + tagGroup.addReference(object); + tagGroup.add(Tag{TagClass("class"), TagValue("value")}); + TagList tagList{}; + tagList.add(tagGroup); + doc->set(tagList); + + auto docCopy = doc->deepCopy(); + REQUIRE(docCopy->has()); + + auto copiedProgramme = docCopy->getElements().front(); + auto copiedContent = docCopy->getElements().front(); + auto copiedObject = docCopy->getElements().front(); + + auto copiedGroups = docCopy->get().get(); + REQUIRE(copiedGroups.size() == 1); + + auto copiedProgrammeRefs = + copiedGroups.front().getReferences(); + REQUIRE(copiedProgrammeRefs.size() == 1); + REQUIRE(copiedProgrammeRefs.front() == copiedProgramme); + REQUIRE(copiedProgrammeRefs.front() != programme); + + auto copiedContentRefs = copiedGroups.front().getReferences(); + REQUIRE(copiedContentRefs.size() == 1); + REQUIRE(copiedContentRefs.front() == copiedContent); + REQUIRE(copiedContentRefs.front() != content); + + auto copiedObjectRefs = copiedGroups.front().getReferences(); + REQUIRE(copiedObjectRefs.size() == 1); + REQUIRE(copiedObjectRefs.front() == copiedObject); + REQUIRE(copiedObjectRefs.front() != object); +} + +TEST_CASE( + "removing last referenced element from document removes TagGroup and " + "transitively TagList") { + auto doc = Document::create(); + auto holder = addSimpleObjectTo(doc, "Test"); + Tag tag{TagClass("class"), TagValue("value")}; + auto programme = AudioProgramme::create(AudioProgrammeName{"Test"}); + TagGroup tagGroup{programme}; + tagGroup.add(tag); + tagGroup.addReference(holder.audioObject); + auto tagList = TagList{}; + tagList.add(tagGroup); + doc->set(tagList); + REQUIRE(doc->has()); + REQUIRE(doc->get().get().size() == 1); + doc->remove(holder.audioObject); + REQUIRE(!doc->has()); +} + +TEST_CASE( + "removing referenced AudioProgramme from document removes TagGroup and " + "transitively TagList") { + auto doc = Document::create(); + auto programme = AudioProgramme::create(AudioProgrammeName{"Programme"}); + auto content = AudioContent::create(AudioContentName{"Content"}); + doc->add(programme); + doc->add(content); + + TagGroup tagGroup{programme}; + tagGroup.add(Tag{TagClass("class"), TagValue("value")}); + tagGroup.addReference(content); + TagList tagList{}; + tagList.add(tagGroup); + doc->set(tagList); + + REQUIRE(doc->has()); + REQUIRE(doc->get().get().size() == 1); + doc->remove(programme); + REQUIRE(!doc->has()); +} + +TEST_CASE( + "removing referenced AudioContent from document removes TagGroup and " + "transitively TagList") { + auto doc = Document::create(); + auto programme = AudioProgramme::create(AudioProgrammeName{"Programme"}); + auto content = AudioContent::create(AudioContentName{"Content"}); + doc->add(programme); + doc->add(content); + + TagGroup tagGroup{programme}; + tagGroup.add(Tag{TagClass("class"), TagValue("value")}); + tagGroup.addReference(content); + TagList tagList{}; + tagList.add(tagGroup); + doc->set(tagList); + + REQUIRE(doc->has()); + REQUIRE(doc->get().get().size() == 1); + doc->remove(content); + REQUIRE(!doc->has()); +} + TEST_CASE( "TagList referencing elements owned by another document is rejected") { auto otherDoc = Document::create(); From ab10d407b6cb31c0e855f19949d6d855b54956cd Mon Sep 17 00:00:00 2001 From: Richard Bailey Date: Fri, 10 Jul 2026 15:57:51 +0100 Subject: [PATCH 5/9] feat(loudness-renderer): add renderer metadata[copilot] Introduce loudnessMetadata.renderer support, shared renderer parameter types, and parser/formatter integration with dedicated unit and XML tests. Review-depth: high Review-reason: Adds new ADM element behavior and serialization semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- include/adm/elements/coordinate_mode.hpp | 16 +++++ include/adm/elements/loudness_metadata.hpp | 8 +++ include/adm/elements/loudness_renderer.hpp | 72 +++++++++++++++++++ .../adm/elements/renderer_common_types.hpp | 37 ++++++++++ include/adm/elements_fwd.hpp | 1 + src/CMakeLists.txt | 1 + src/elements/loudness_metadata.cpp | 14 ++++ src/elements/loudness_renderer.cpp | 40 +++++++++++ src/private/document_parser.cpp | 22 ++++++ src/private/rapidxml_formatter.cpp | 16 +++++ tests/CMakeLists.txt | 2 + tests/loudness_renderer_tests.cpp | 60 ++++++++++++++++ .../test_data/loudness_renderer.accepted.xml | 20 ++++++ .../xml_parser/loudness_renderer.xml | 19 +++++ tests/xml_loudness_renderer_tests.cpp | 35 +++++++++ 15 files changed, 363 insertions(+) create mode 100644 include/adm/elements/coordinate_mode.hpp create mode 100644 include/adm/elements/loudness_renderer.hpp create mode 100644 include/adm/elements/renderer_common_types.hpp create mode 100644 src/elements/loudness_renderer.cpp create mode 100644 tests/loudness_renderer_tests.cpp create mode 100644 tests/test_data/loudness_renderer.accepted.xml create mode 100644 tests/test_data/xml_parser/loudness_renderer.xml create mode 100644 tests/xml_loudness_renderer_tests.cpp diff --git a/include/adm/elements/coordinate_mode.hpp b/include/adm/elements/coordinate_mode.hpp new file mode 100644 index 00000000..2a6cce85 --- /dev/null +++ b/include/adm/elements/coordinate_mode.hpp @@ -0,0 +1,16 @@ +/// @file coordinate_mode.hpp +#pragma once + +#include +#include "adm/detail/named_type.hpp" + +namespace adm { + + /// @brief Tag for NamedType ::CoordinateMode + struct CoordinateModeTag {}; + /// @brief NamedType for the coordinateMode attribute + /// + /// Allowable values are "polar" or "cartesian". + using CoordinateMode = detail::NamedType; + +} // namespace adm diff --git a/include/adm/elements/loudness_metadata.hpp b/include/adm/elements/loudness_metadata.hpp index e0ef37e1..ed41b64a 100644 --- a/include/adm/elements/loudness_metadata.hpp +++ b/include/adm/elements/loudness_metadata.hpp @@ -3,6 +3,7 @@ #include "adm/detail/named_type.hpp" #include "adm/detail/auto_base.hpp" #include "adm/detail/optional_comparison.hpp" +#include "adm/elements/loudness_renderer.hpp" #include "adm/export.h" #include #include @@ -115,6 +116,8 @@ namespace adm { ADM_EXPORT void set(MaxShortTerm maxShortTerm); /// @brief DialogueLoudness setter ADM_EXPORT void set(DialogueLoudness dialogueLoudness); + /// @brief LoudnessRenderer setter + ADM_EXPORT void set(LoudnessRenderer renderer); /** * @brief ADM parameter unset template @@ -149,6 +152,8 @@ namespace adm { get(detail::ParameterTraits::tag) const; ADM_EXPORT DialogueLoudness get(detail::ParameterTraits::tag) const; + ADM_EXPORT LoudnessRenderer + get(detail::ParameterTraits::tag) const; ADM_EXPORT bool has(detail::ParameterTraits::tag) const; ADM_EXPORT bool has(detail::ParameterTraits::tag) const; @@ -160,6 +165,7 @@ namespace adm { ADM_EXPORT bool has(detail::ParameterTraits::tag) const; ADM_EXPORT bool has(detail::ParameterTraits::tag) const; ADM_EXPORT bool has(detail::ParameterTraits::tag) const; + ADM_EXPORT bool has(detail::ParameterTraits::tag) const; template bool isDefault(Tag) const { @@ -175,6 +181,7 @@ namespace adm { ADM_EXPORT void unset(detail::ParameterTraits::tag); ADM_EXPORT void unset(detail::ParameterTraits::tag); ADM_EXPORT void unset(detail::ParameterTraits::tag); + ADM_EXPORT void unset(detail::ParameterTraits::tag); boost::optional loudnessMethod_; boost::optional loudnessRecType_; @@ -185,6 +192,7 @@ namespace adm { boost::optional maxMomentary_; boost::optional maxShortTerm_; boost::optional dialogueLoudness_; + boost::optional renderer_; }; // ---- Implementation ---- // diff --git a/include/adm/elements/loudness_renderer.hpp b/include/adm/elements/loudness_renderer.hpp new file mode 100644 index 00000000..025453a8 --- /dev/null +++ b/include/adm/elements/loudness_renderer.hpp @@ -0,0 +1,72 @@ +/// @file loudness_renderer.hpp +#pragma once + +#include + +#include "adm/detail/auto_base.hpp" +#include "adm/detail/named_option_helper.hpp" +#include "adm/elements/renderer_common_types.hpp" +#include "adm/export.h" + +namespace adm { + + /// @brief Tag for LoudnessRenderer class + struct LoudnessRendererTag {}; + + namespace detail { + extern template class ADM_EXPORT_TEMPLATE_METHODS + OptionalParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + OptionalParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + OptionalParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + OptionalParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + VectorParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + VectorParameter; + + using LoudnessRendererBase = HasParameters< + OptionalParameter, OptionalParameter, + OptionalParameter, OptionalParameter, + VectorParameter, + VectorParameter>; + } // namespace detail + + /** + * @brief Class representation of the renderer sub-element of a + * loudnessMetadata element (BS.2076-3 Tables A1-39 / A1-40). + */ + class LoudnessRenderer : private detail::LoudnessRendererBase, + private detail::AddWrapperMethods { + public: + using tag = LoudnessRendererTag; + + template + explicit LoudnessRenderer(Parameters... namedArgs) { + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + + using detail::LoudnessRendererBase::add; + using detail::LoudnessRendererBase::remove; + using detail::LoudnessRendererBase::set; + using detail::AddWrapperMethods::get; + using detail::AddWrapperMethods::has; + using detail::AddWrapperMethods::isDefault; + using detail::AddWrapperMethods::unset; + + ADM_EXPORT void print(std::ostream& os) const; + + private: + using detail::LoudnessRendererBase::get; + using detail::LoudnessRendererBase::has; + using detail::LoudnessRendererBase::isDefault; + using detail::LoudnessRendererBase::unset; + + friend class detail::AddWrapperMethods; + }; + + ADD_TRAIT(LoudnessRenderer, LoudnessRendererTag); + +} // namespace adm diff --git a/include/adm/elements/renderer_common_types.hpp b/include/adm/elements/renderer_common_types.hpp new file mode 100644 index 00000000..8920157f --- /dev/null +++ b/include/adm/elements/renderer_common_types.hpp @@ -0,0 +1,37 @@ +/// @file renderer_common_types.hpp +#pragma once + +#include +#include + +#include "adm/detail/named_type.hpp" +#include "adm/elements/audio_object_id.hpp" +#include "adm/elements/audio_pack_format_id.hpp" +#include "adm/elements/coordinate_mode.hpp" + +namespace adm { + + /// @brief Tag for NamedType ::RendererUri + struct RendererUriTag {}; + /// @brief NamedType for the renderer uri attribute + using RendererUri = detail::NamedType; + + /// @brief Tag for NamedType ::RendererName + struct RendererNameTag {}; + /// @brief NamedType for the renderer name attribute + using RendererName = detail::NamedType; + + /// @brief Tag for NamedType ::RendererVersion + struct RendererVersionTag {}; + /// @brief NamedType for the renderer version attribute + using RendererVersion = detail::NamedType; + + /// @brief Vector of audioPackFormatIDRef values used by a renderer + using RendererPackFormatIdRefs = std::vector; + ADD_TRAIT(RendererPackFormatIdRefs, RendererPackFormatIdRefsTag); + + /// @brief Vector of audioObjectIDRef values used by a renderer + using RendererObjectIdRefs = std::vector; + ADD_TRAIT(RendererObjectIdRefs, RendererObjectIdRefsTag); + +} // namespace adm diff --git a/include/adm/elements_fwd.hpp b/include/adm/elements_fwd.hpp index 518bdb2e..f4a14604 100644 --- a/include/adm/elements_fwd.hpp +++ b/include/adm/elements_fwd.hpp @@ -47,5 +47,6 @@ namespace adm { class AudioTrackUidId; class LoudnessMetadata; + class LoudnessRenderer; class AudioProgrammeReferenceScreen; } // namespace adm diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 7e4d98d3..30d99c84 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -44,6 +44,7 @@ add_library(adm elements/jump_position.cpp elements/label.cpp elements/loudness_metadata.cpp + elements/loudness_renderer.cpp elements/object_divergence.cpp elements/position.cpp elements/position_offset.cpp diff --git a/src/elements/loudness_metadata.cpp b/src/elements/loudness_metadata.cpp index c441b939..b9535554 100644 --- a/src/elements/loudness_metadata.cpp +++ b/src/elements/loudness_metadata.cpp @@ -45,6 +45,10 @@ namespace adm { detail::ParameterTraits::tag) const { return dialogueLoudness_.get(); } + LoudnessRenderer LoudnessMetadata::get( + detail::ParameterTraits::tag) const { + return renderer_.get(); + } // ---- Has ---- // bool LoudnessMetadata::has( @@ -80,6 +84,10 @@ namespace adm { detail::ParameterTraits::tag) const { return dialogueLoudness_ != boost::none; } + bool LoudnessMetadata::has( + detail::ParameterTraits::tag) const { + return renderer_ != boost::none; + } // ---- Setter ---- // void LoudnessMetadata::set(LoudnessMethod loudnessMethod) { @@ -109,6 +117,9 @@ namespace adm { void LoudnessMetadata::set(DialogueLoudness dialogueLoudness) { dialogueLoudness_ = dialogueLoudness; } + void LoudnessMetadata::set(LoudnessRenderer renderer) { + renderer_ = std::move(renderer); + } // ---- Unsetter ---- // void LoudnessMetadata::unset(detail::ParameterTraits::tag) { @@ -140,6 +151,9 @@ namespace adm { void LoudnessMetadata::unset(detail::ParameterTraits::tag) { dialogueLoudness_ = boost::none; } + void LoudnessMetadata::unset(detail::ParameterTraits::tag) { + renderer_ = boost::none; + } void LoudnessMetadata::print(std::ostream& os) const { os << "("; diff --git a/src/elements/loudness_renderer.cpp b/src/elements/loudness_renderer.cpp new file mode 100644 index 00000000..3b0fc8ff --- /dev/null +++ b/src/elements/loudness_renderer.cpp @@ -0,0 +1,40 @@ +#include "adm/elements/loudness_renderer.hpp" + +namespace adm { + + void LoudnessRenderer::print(std::ostream& os) const { + os << "("; + bool first = true; + auto sep = [&]() { + if (!first) os << ", "; + first = false; + }; + if (has()) { + sep(); + os << "uri=" << get(); + } + if (has()) { + sep(); + os << "name=" << get(); + } + if (has()) { + sep(); + os << "version=" << get(); + } + if (has()) { + sep(); + os << "coordinateMode=" << get(); + } + os << ")"; + } + + namespace detail { + template class OptionalParameter; + template class OptionalParameter; + template class OptionalParameter; + template class OptionalParameter; + template class VectorParameter; + template class VectorParameter; + } // namespace detail + +} // namespace adm diff --git a/src/private/document_parser.cpp b/src/private/document_parser.cpp index 20e67e29..118a54e1 100644 --- a/src/private/document_parser.cpp +++ b/src/private/document_parser.cpp @@ -1072,6 +1072,26 @@ namespace adm { return jumpPosition; } + LoudnessRenderer parseLoudnessRenderer(NodePtr node) { + LoudnessRenderer renderer; + setOptionalAttribute(node, "uri", renderer); + setOptionalAttribute(node, "name", renderer); + setOptionalAttribute(node, "version", renderer); + setOptionalAttribute(node, "coordinateMode", renderer); + RendererPackFormatIdRefs packRefs; + for (auto& packNode : + detail::findElements(node, "audioPackFormatIDRef")) { + packRefs.push_back(parseAudioPackFormatId(packNode->value())); + } + if (!packRefs.empty()) renderer.set(std::move(packRefs)); + RendererObjectIdRefs objectRefs; + for (auto& objectNode : detail::findElements(node, "audioObjectIDRef")) { + objectRefs.push_back(parseAudioObjectId(objectNode->value())); + } + if (!objectRefs.empty()) renderer.set(std::move(objectRefs)); + return renderer; + } + LoudnessMetadata parseLoudnessMetadata(NodePtr node) { LoudnessMetadata loudnessMetadata; setOptionalAttribute(node, "loudnessMethod", @@ -1089,6 +1109,8 @@ namespace adm { setOptionalElement(node, "maxShortTerm", loudnessMetadata); setOptionalElement(node, "dialogueLoudness", loudnessMetadata); + setOptionalElement(node, "renderer", loudnessMetadata, + &parseLoudnessRenderer); return loudnessMetadata; } diff --git a/src/private/rapidxml_formatter.cpp b/src/private/rapidxml_formatter.cpp index 7534bd50..d94ac7b2 100644 --- a/src/private/rapidxml_formatter.cpp +++ b/src/private/rapidxml_formatter.cpp @@ -93,6 +93,20 @@ namespace adm { // clang-format on } + void formatLoudnessRenderer(XmlNode &node, + const LoudnessRenderer &renderer) { + node.addOptionalAttribute(&renderer, "uri"); + node.addOptionalAttribute(&renderer, "name"); + node.addOptionalAttribute(&renderer, "version"); + node.addOptionalAttribute(&renderer, "coordinateMode"); + for (auto const &packId : renderer.get()) { + node.addElement("audioPackFormatIDRef", formatId(packId)); + } + for (auto const &objectId : renderer.get()) { + node.addElement("audioObjectIDRef", formatId(objectId)); + } + } + void formatLoudnessMetadata(XmlNode &node, const LoudnessMetadata loudnessMetadata) { node.addOptionalAttribute(&loudnessMetadata, @@ -110,6 +124,8 @@ namespace adm { node.addOptionalElement(&loudnessMetadata, "maxShortTerm"); node.addOptionalElement(&loudnessMetadata, "dialogueLoudness"); + node.addOptionalElement(&loudnessMetadata, "renderer", + &formatLoudnessRenderer); } void formatAudioContent(XmlNode &node, diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0315572e..ccbd03a7 100755 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -60,6 +60,7 @@ add_adm_test("gain_tests") add_adm_test("jump_position_tests") add_adm_test("label_tests") add_adm_test("loudness_metadata_tests") +add_adm_test("loudness_renderer_tests") add_adm_test("named_type_tests") add_adm_test("object_creation_tests") add_adm_test("object_divergence_tests") @@ -75,6 +76,7 @@ add_adm_test("version_tests") add_adm_test("tag_list_tests") add_adm_test("xml_audio_block_format_objects_tests") add_adm_test("xml_loudness_metadata_tests") +add_adm_test("xml_loudness_renderer_tests") add_adm_test("xml_parser_audio_block_format_direct_speakers_tests") add_adm_test("xml_parser_audio_block_format_hoa_tests") add_adm_test("xml_parser_audio_block_format_binaural_tests") diff --git a/tests/loudness_renderer_tests.cpp b/tests/loudness_renderer_tests.cpp new file mode 100644 index 00000000..8dd7fdaf --- /dev/null +++ b/tests/loudness_renderer_tests.cpp @@ -0,0 +1,60 @@ +#include +#include "adm/elements/loudness_renderer.hpp" +#include "adm/elements/audio_pack_format_id.hpp" +#include "adm/elements/audio_object_id.hpp" + +using namespace adm; + +TEST_CASE("loudness_renderer/empty") { + LoudnessRenderer renderer; + REQUIRE(renderer.has() == false); + REQUIRE(renderer.has() == false); + REQUIRE(renderer.has() == false); + REQUIRE(renderer.has() == false); + REQUIRE(renderer.get().empty()); + REQUIRE(renderer.get().empty()); +} + +TEST_CASE("loudness_renderer/set_unset") { + LoudnessRenderer renderer; + renderer.set(RendererUri("urn:itu:bs:2127:0:itu_adm_renderer")); + renderer.set(RendererName("Rec. ITU-R BS.2127")); + renderer.set(RendererVersion("1.0.0")); + renderer.set(CoordinateMode("polar")); + + REQUIRE(renderer.get() == + std::string{"urn:itu:bs:2127:0:itu_adm_renderer"}); + REQUIRE(renderer.get() == std::string{"Rec. ITU-R BS.2127"}); + REQUIRE(renderer.get() == std::string{"1.0.0"}); + REQUIRE(renderer.get() == std::string{"polar"}); + + renderer.unset(); + renderer.unset(); + renderer.unset(); + renderer.unset(); + + REQUIRE(renderer.has() == false); + REQUIRE(renderer.has() == false); + REQUIRE(renderer.has() == false); + REQUIRE(renderer.has() == false); +} + +TEST_CASE("loudness_renderer/id_refs") { + LoudnessRenderer renderer; + RendererPackFormatIdRefs packs{parseAudioPackFormatId("AP_00010002")}; + RendererObjectIdRefs objects{parseAudioObjectId("AO_1001"), + parseAudioObjectId("AO_1002")}; + renderer.set(packs); + renderer.set(objects); + + REQUIRE(renderer.get().size() == 1); + REQUIRE(renderer.get().size() == 2); +} + +TEST_CASE("loudness_renderer/named_args_constructor") { + LoudnessRenderer renderer{RendererUri("urn:itu:bs:2127:0:itu_adm_renderer"), + CoordinateMode("cartesian")}; + REQUIRE(renderer.get() == + std::string{"urn:itu:bs:2127:0:itu_adm_renderer"}); + REQUIRE(renderer.get() == std::string{"cartesian"}); +} diff --git a/tests/test_data/loudness_renderer.accepted.xml b/tests/test_data/loudness_renderer.accepted.xml new file mode 100644 index 00000000..5fd64b7a --- /dev/null +++ b/tests/test_data/loudness_renderer.accepted.xml @@ -0,0 +1,20 @@ + + + + + + + + -23.000000 + + AP_00010002 + AO_1001 + AO_1002 + + + + + + + + diff --git a/tests/test_data/xml_parser/loudness_renderer.xml b/tests/test_data/xml_parser/loudness_renderer.xml new file mode 100644 index 00000000..aaffb6e9 --- /dev/null +++ b/tests/test_data/xml_parser/loudness_renderer.xml @@ -0,0 +1,19 @@ + + + + + + + + -23.0 + + AP_00010002 + AO_1001 + AO_1002 + + + + + + + diff --git a/tests/xml_loudness_renderer_tests.cpp b/tests/xml_loudness_renderer_tests.cpp new file mode 100644 index 00000000..658f2d48 --- /dev/null +++ b/tests/xml_loudness_renderer_tests.cpp @@ -0,0 +1,35 @@ +#include +#include +#include "adm/document.hpp" +#include "adm/elements/audio_content.hpp" +#include "adm/elements/audio_programme.hpp" +#include "adm/elements/loudness_metadata.hpp" +#include "adm/elements/loudness_renderer.hpp" +#include "adm/parse.hpp" +#include "adm/write.hpp" +#include "helper/file_comparator.hpp" + +using namespace adm; + +TEST_CASE("xml/loudness_renderer") { + auto document = parseXml("xml_parser/loudness_renderer.xml"); + + auto programme = document->lookup(parseAudioProgrammeId("APR_1001")); + REQUIRE(programme->has()); + auto const& lms = programme->get(); + REQUIRE(lms.size() == 1); + auto const& lm = lms.at(0); + REQUIRE(lm.has()); + auto renderer = lm.get(); + REQUIRE(renderer.get() == + std::string{"urn:itu:bs:2127:0:itu_adm_renderer"}); + REQUIRE(renderer.get() == std::string{"Rec. ITU-R BS.2127"}); + REQUIRE(renderer.get() == std::string{"1.0.0"}); + REQUIRE(renderer.get() == std::string{"polar"}); + REQUIRE(renderer.get().size() == 1); + REQUIRE(renderer.get().size() == 2); + + std::stringstream xml; + writeXml(xml, document); + CHECK_THAT(xml.str(), EqualsXmlFile("loudness_renderer")); +} From 67d59d65971d195d23efbdf819be79c78dbfa168 Mon Sep 17 00:00:00 2001 From: Richard Bailey Date: Fri, 10 Jul 2026 16:01:48 +0100 Subject: [PATCH 6/9] feat(authoring-information): add reference-screen metadata and renderer interop [copilot] Add authoringInformation and coordinateMode support on audioProgrammeReferenceScreen, wire renderer interop APIs, and integrate parser/formatter/copy behavior with tests. Review-depth: high Review-reason: Introduces new programme metadata structures and cross-element interop behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- include/adm/elements.hpp | 1 + include/adm/elements/audio_programme.hpp | 9 + .../elements/audio_programme_ref_screen.hpp | 45 ++- .../adm/elements/authoring_information.hpp | 158 ++++++++++ include/adm/elements/loudness_metadata.hpp | 4 + include/adm/elements/loudness_renderer.hpp | 9 + .../adm/elements/renderer_common_types.hpp | 33 ++- include/adm/elements_fwd.hpp | 2 + include/adm/private/document_parser.hpp | 20 ++ src/CMakeLists.txt | 1 + src/elements/audio_programme.cpp | 15 + src/elements/authoring_information.cpp | 46 +++ src/elements/loudness_metadata.cpp | 4 + src/elements/loudness_renderer.cpp | 42 +++ src/private/copy.cpp | 125 ++++++++ src/private/document_parser.cpp | 269 ++++++++++++++++-- src/private/rapidxml_formatter.cpp | 53 +++- tests/CMakeLists.txt | 4 + tests/audio_programme_ref_screen_tests.cpp | 15 + tests/authoring_information_tests.cpp | 38 +++ tests/loudness_renderer_tests.cpp | 13 +- tests/renderer_interop_tests.cpp | 82 ++++++ .../authoring_information.accepted.xml | 22 ++ .../test_data/loudness_renderer.accepted.xml | 2 + .../xml_parser/authoring_information.xml | 21 ++ .../xml_parser/loudness_renderer.xml | 2 + tests/xml_authoring_information_tests.cpp | 37 +++ 27 files changed, 1029 insertions(+), 43 deletions(-) create mode 100644 include/adm/elements/authoring_information.hpp create mode 100644 src/elements/authoring_information.cpp create mode 100644 tests/audio_programme_ref_screen_tests.cpp create mode 100644 tests/authoring_information_tests.cpp create mode 100644 tests/renderer_interop_tests.cpp create mode 100644 tests/test_data/authoring_information.accepted.xml create mode 100644 tests/test_data/xml_parser/authoring_information.xml create mode 100644 tests/xml_authoring_information_tests.cpp diff --git a/include/adm/elements.hpp b/include/adm/elements.hpp index c51ba9f2..87aa24b1 100644 --- a/include/adm/elements.hpp +++ b/include/adm/elements.hpp @@ -49,6 +49,7 @@ #include "adm/elements/time.hpp" #include "adm/elements/audio_programme_ref_screen.hpp" +#include "adm/elements/authoring_information.hpp" #include "adm/elements/cartesian.hpp" #include "adm/elements/channel_lock.hpp" #include "adm/elements/dialogue.hpp" diff --git a/include/adm/elements/audio_programme.hpp b/include/adm/elements/audio_programme.hpp index a1728aa2..f29dd88f 100644 --- a/include/adm/elements/audio_programme.hpp +++ b/include/adm/elements/audio_programme.hpp @@ -9,6 +9,7 @@ #include "adm/elements/audio_content.hpp" #include "adm/elements/audio_programme_id.hpp" #include "adm/elements/audio_programme_ref_screen.hpp" +#include "adm/elements/authoring_information.hpp" #include "adm/elements/loudness_metadata.hpp" #include "adm/elements_fwd.hpp" #include "adm/helper/element_range.hpp" @@ -158,6 +159,8 @@ namespace adm { ADM_EXPORT void set(MaxDuckingDepth depth); /// @brief AudioProgrammeReferenceScreen setter ADM_EXPORT void set(AudioProgrammeReferenceScreen refScreen); + /// @brief AuthoringInformation setter + ADM_EXPORT void set(AuthoringInformation authoringInformation); /** * @brief ADM parameter unset template @@ -237,6 +240,8 @@ namespace adm { get(detail::ParameterTraits::tag) const; ADM_EXPORT AudioProgrammeReferenceScreen get(detail::ParameterTraits::tag) const; + ADM_EXPORT AuthoringInformation + get(detail::ParameterTraits::tag) const; ADM_EXPORT bool has(detail::ParameterTraits::tag) const; ADM_EXPORT bool has(detail::ParameterTraits::tag) const; @@ -247,6 +252,8 @@ namespace adm { ADM_EXPORT bool has(detail::ParameterTraits::tag) const; ADM_EXPORT bool has( detail::ParameterTraits::tag) const; + ADM_EXPORT bool has( + detail::ParameterTraits::tag) const; template bool isDefault(Tag) const { @@ -261,6 +268,7 @@ namespace adm { ADM_EXPORT void unset(detail::ParameterTraits::tag); ADM_EXPORT void unset( detail::ParameterTraits::tag); + ADM_EXPORT void unset(detail::ParameterTraits::tag); ADM_EXPORT ElementRange getReferences( detail::ParameterTraits::tag) const; @@ -283,6 +291,7 @@ namespace adm { std::vector> audioContents_; boost::optional maxDuckingDepth_; boost::optional refScreen_; + boost::optional authoringInformation_; }; ///@} diff --git a/include/adm/elements/audio_programme_ref_screen.hpp b/include/adm/elements/audio_programme_ref_screen.hpp index 5b44673e..8a0b6bbc 100644 --- a/include/adm/elements/audio_programme_ref_screen.hpp +++ b/include/adm/elements/audio_programme_ref_screen.hpp @@ -1,20 +1,51 @@ #pragma once #include +#include "adm/detail/auto_base.hpp" +#include "adm/detail/named_option_helper.hpp" +#include "adm/elements/coordinate_mode.hpp" +#include "adm/export.h" namespace adm { struct AudioProgrammeReferenceScreenTag {}; - class AudioProgrammeReferenceScreen { + namespace detail { + using AudioProgrammeReferenceScreenBase = + HasParameters>; + } // namespace detail + + class AudioProgrammeReferenceScreen + : private detail::AudioProgrammeReferenceScreenBase, + private detail::AddWrapperMethods { public: - typedef AudioProgrammeReferenceScreenTag tag; + using tag = AudioProgrammeReferenceScreenTag; + + template + explicit AudioProgrammeReferenceScreen(Parameters... namedArgs) { + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + + using detail::AudioProgrammeReferenceScreenBase::set; + using detail::AddWrapperMethods::get; + using detail::AddWrapperMethods::has; + using detail::AddWrapperMethods::isDefault; + using detail::AddWrapperMethods::unset; + + void print(std::ostream &os) const { + os << "("; + if (has()) { + os << "coordinateMode=" << get(); + } + os << ")"; + } - AudioProgrammeReferenceScreen() {} + private: + using detail::AudioProgrammeReferenceScreenBase::get; + using detail::AudioProgrammeReferenceScreenBase::has; + using detail::AudioProgrammeReferenceScreenBase::isDefault; + using detail::AudioProgrammeReferenceScreenBase::unset; - /** - * @brief Print overview to ostream - */ - void print(std::ostream &os) const { os << "()"; }; + friend class detail::AddWrapperMethods; }; } // namespace adm diff --git a/include/adm/elements/authoring_information.hpp b/include/adm/elements/authoring_information.hpp new file mode 100644 index 00000000..a6441601 --- /dev/null +++ b/include/adm/elements/authoring_information.hpp @@ -0,0 +1,158 @@ +/// @file authoring_information.hpp +#pragma once + +#include +#include + +#include "adm/detail/auto_base.hpp" +#include "adm/detail/named_option_helper.hpp" +#include "adm/detail/named_type.hpp" +#include "adm/detail/optional_comparison.hpp" +#include "adm/elements/audio_pack_format_id.hpp" +#include "adm/elements/renderer_common_types.hpp" +#include "adm/export.h" + +namespace adm { + + class LoudnessRenderer; + + /// @brief Tag for ::ReferenceLayout named-type + struct ReferenceLayoutTag {}; + /// @brief NamedType wrapping the audioPackFormatIDRef of a referenceLayout + /// sub-element of authoringInformation (BS.2076-3 Table A1-51). + using ReferenceLayout = + detail::NamedType; + + /// @brief Vector of ReferenceLayout + using ReferenceLayouts = std::vector; + ADD_TRAIT(ReferenceLayouts, ReferenceLayoutsTag); + + /// @brief Tag for Renderer class + struct RendererTag {}; + + namespace detail { + extern template class ADM_EXPORT_TEMPLATE_METHODS + VectorParameter; + + using RendererBase = HasParameters< + RequiredParameter, OptionalParameter, + OptionalParameter, OptionalParameter, + VectorParameter>; + } // namespace detail + + /** + * @brief Class representation of the renderer sub-element of an + * authoringInformation element (BS.2076-3 Tables A1-52 / A1-53). + */ + class Renderer : private detail::RendererBase, + private detail::AddWrapperMethods { + public: + using tag = RendererTag; + + template + explicit Renderer(RendererUri uri, Parameters... namedArgs) { + this->set(std::move(uri)); + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + + using detail::RendererBase::add; + using detail::RendererBase::remove; + using detail::RendererBase::set; + using detail::AddWrapperMethods::get; + using detail::AddWrapperMethods::has; + using detail::AddWrapperMethods::isDefault; + using detail::AddWrapperMethods::unset; + + /// @brief Convert to LoudnessRenderer preserving all shared parameters. + ADM_EXPORT LoudnessRenderer toLoudnessRenderer() const; + + ADM_EXPORT void print(std::ostream &os) const; + + private: + using detail::RendererBase::get; + using detail::RendererBase::has; + using detail::RendererBase::isDefault; + using detail::RendererBase::unset; + + friend class detail::AddWrapperMethods; + }; + + ADD_TRAIT(Renderer, RendererTag); + + inline bool operator==(const Renderer &a, const Renderer &b) { + if (!detail::optionalsEqual(a, b)) { + return false; + } + + if (a.has() != + b.has()) { + return false; + } + + if (!a.has()) { + return true; + } + + return a.get() == + b.get(); + } + inline bool operator!=(const Renderer &a, const Renderer &b) { + return !(a == b); + } + + /// @brief Vector of Renderer + using Renderers = std::vector; + ADD_TRAIT(Renderers, RenderersTag); + + /// @brief Tag for AuthoringInformation class + struct AuthoringInformationTag {}; + + namespace detail { + extern template class ADM_EXPORT_TEMPLATE_METHODS + VectorParameter; + extern template class ADM_EXPORT_TEMPLATE_METHODS + VectorParameter; + + using AuthoringInformationBase = + HasParameters, + VectorParameter>; + } // namespace detail + + /** + * @brief Class representation of the authoringInformation sub-element of an + * audioProgramme element (BS.2076-3 §5.8.6). + */ + class AuthoringInformation + : private detail::AuthoringInformationBase, + private detail::AddWrapperMethods { + public: + using tag = AuthoringInformationTag; + + template + explicit AuthoringInformation(Parameters... namedArgs) { + detail::setNamedOptionHelper(this, std::move(namedArgs)...); + } + + using detail::AuthoringInformationBase::add; + using detail::AuthoringInformationBase::remove; + using detail::AuthoringInformationBase::set; + using detail::AddWrapperMethods::get; + using detail::AddWrapperMethods::has; + using detail::AddWrapperMethods::isDefault; + using detail::AddWrapperMethods::unset; + + ADM_EXPORT void print(std::ostream &os) const; + + private: + using detail::AuthoringInformationBase::get; + using detail::AuthoringInformationBase::has; + using detail::AuthoringInformationBase::isDefault; + using detail::AuthoringInformationBase::unset; + + friend class detail::AddWrapperMethods; + }; + + ADD_TRAIT(AuthoringInformation, AuthoringInformationTag); + +} // namespace adm diff --git a/include/adm/elements/loudness_metadata.hpp b/include/adm/elements/loudness_metadata.hpp index ed41b64a..f951d9e3 100644 --- a/include/adm/elements/loudness_metadata.hpp +++ b/include/adm/elements/loudness_metadata.hpp @@ -12,6 +12,8 @@ namespace adm { + class Renderer; + /// @brief Tag for NamedType ::LoudnessMethod struct loudnessMethodTag {}; /// @brief NamedType for loudnessMethod parameter @@ -118,6 +120,8 @@ namespace adm { ADM_EXPORT void set(DialogueLoudness dialogueLoudness); /// @brief LoudnessRenderer setter ADM_EXPORT void set(LoudnessRenderer renderer); + /// @brief Renderer setter (converted to LoudnessRenderer) + ADM_EXPORT void set(Renderer renderer); /** * @brief ADM parameter unset template diff --git a/include/adm/elements/loudness_renderer.hpp b/include/adm/elements/loudness_renderer.hpp index 025453a8..8c564663 100644 --- a/include/adm/elements/loudness_renderer.hpp +++ b/include/adm/elements/loudness_renderer.hpp @@ -10,6 +10,8 @@ namespace adm { + class Renderer; + /// @brief Tag for LoudnessRenderer class struct LoudnessRendererTag {}; @@ -56,6 +58,13 @@ namespace adm { using detail::AddWrapperMethods::isDefault; using detail::AddWrapperMethods::unset; + /// @brief Create a LoudnessRenderer from an authoring Renderer. + ADM_EXPORT static LoudnessRenderer fromRenderer(Renderer const& renderer); + + /// @brief Convert to Renderer, using provided uri and explicitly dropping audioObjectIDRef values. + /// uri is required as it is optional in a Loudness renderer but required in authoring renderer + ADM_EXPORT Renderer toRendererDroppingObjectRefs(RendererUri uri) const; + ADM_EXPORT void print(std::ostream& os) const; private: diff --git a/include/adm/elements/renderer_common_types.hpp b/include/adm/elements/renderer_common_types.hpp index 8920157f..e141c090 100644 --- a/include/adm/elements/renderer_common_types.hpp +++ b/include/adm/elements/renderer_common_types.hpp @@ -1,13 +1,15 @@ /// @file renderer_common_types.hpp #pragma once +#include +#include #include #include +#include "adm/detail/auto_base.hpp" #include "adm/detail/named_type.hpp" -#include "adm/elements/audio_object_id.hpp" -#include "adm/elements/audio_pack_format_id.hpp" #include "adm/elements/coordinate_mode.hpp" +#include "adm/elements_fwd.hpp" namespace adm { @@ -26,12 +28,31 @@ namespace adm { /// @brief NamedType for the renderer version attribute using RendererVersion = detail::NamedType; - /// @brief Vector of audioPackFormatIDRef values used by a renderer - using RendererPackFormatIdRefs = std::vector; + /// @brief Vector of audioPackFormat references used by a renderer + using RendererPackFormatIdRefs = + std::vector>; ADD_TRAIT(RendererPackFormatIdRefs, RendererPackFormatIdRefsTag); - /// @brief Vector of audioObjectIDRef values used by a renderer - using RendererObjectIdRefs = std::vector; + /// @brief Vector of audioObject references used by a renderer + using RendererObjectIdRefs = std::vector>; ADD_TRAIT(RendererObjectIdRefs, RendererObjectIdRefsTag); + namespace detail { + template <> + struct ParameterCompare { + static bool compare(RendererPackFormatIdRefs const& lhs, + RendererPackFormatIdRefs const& rhs) { + return lhs == rhs; + } + }; + + template <> + struct ParameterCompare { + static bool compare(RendererObjectIdRefs const& lhs, + RendererObjectIdRefs const& rhs) { + return lhs == rhs; + } + }; + } // namespace detail + } // namespace adm diff --git a/include/adm/elements_fwd.hpp b/include/adm/elements_fwd.hpp index f4a14604..e5044501 100644 --- a/include/adm/elements_fwd.hpp +++ b/include/adm/elements_fwd.hpp @@ -48,5 +48,7 @@ namespace adm { class LoudnessMetadata; class LoudnessRenderer; + class Renderer; + class AuthoringInformation; class AudioProgrammeReferenceScreen; } // namespace adm diff --git a/include/adm/private/document_parser.hpp b/include/adm/private/document_parser.hpp index 57ef5b7f..65276444 100644 --- a/include/adm/private/document_parser.hpp +++ b/include/adm/private/document_parser.hpp @@ -45,6 +45,9 @@ namespace adm { LoudnessMetadatas parseLoudnessMetadatas(const std::vector& nodes); AudioProgrammeReferenceScreen parseAudioProgrammeReferenceScreen( NodePtr node); + Renderer parseRenderer(NodePtr node); + ReferenceLayout parseReferenceLayout(NodePtr node); + AuthoringInformation parseAuthoringInformation(NodePtr node); Label parseLabel(NodePtr node); AudioBlockFormatObjects parseAudioBlockFormatObjects( NodePtr node, boost::optional timeReference); @@ -160,6 +163,10 @@ namespace adm { const std::map, std::vector>& map); + void resolveProgrammeAuthoringRendererReferences(); + void resolveProgrammeLoudnessRendererReferences(); + void resolveContentLoudnessRendererReferences(); + template void resolveReference(const std::map& map) { for (const auto& entry : map) { @@ -173,6 +180,19 @@ namespace adm { } void setCommonProperties(std::shared_ptr audioPackFormat, NodePtr node); + + struct RendererNestedIds { + std::vector packFormatIds; + std::vector objectIds; + }; + + std::map, + std::vector>> + programmeAuthoringRendererPackFormatRefs_; + std::map, std::vector> + programmeLoudnessRendererRefs_; + std::map, std::vector> + contentLoudnessRendererRefs_; }; } // namespace xml diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 30d99c84..37e71fa0 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -36,6 +36,7 @@ add_library(adm elements/audio_block_format_binaural.cpp elements/audio_object_interaction.cpp elements/audio_pack_format_hoa.cpp + elements/authoring_information.cpp elements/common_parameters.cpp elements/time.cpp elements/channel_lock.cpp diff --git a/src/elements/audio_programme.cpp b/src/elements/audio_programme.cpp index cbb59b31..5ee6ef47 100644 --- a/src/elements/audio_programme.cpp +++ b/src/elements/audio_programme.cpp @@ -45,6 +45,10 @@ namespace adm { detail::ParameterTraits::tag) const { return refScreen_.get(); } + AuthoringInformation AudioProgramme::get( + detail::ParameterTraits::tag) const { + return authoringInformation_.get(); + } // ---- Has ---- // bool AudioProgramme::has( @@ -73,6 +77,10 @@ namespace adm { detail::ParameterTraits::tag) const { return refScreen_ != boost::none; } + bool AudioProgramme::has( + detail::ParameterTraits::tag) const { + return authoringInformation_ != boost::none; + } // ---- isDefault ---- // bool AudioProgramme::isDefault(detail::ParameterTraits::tag) const { @@ -101,6 +109,9 @@ namespace adm { void AudioProgramme::set(AudioProgrammeReferenceScreen refScreen) { refScreen_ = refScreen; } + void AudioProgramme::set(AuthoringInformation authoringInformation) { + authoringInformation_ = std::move(authoringInformation); + } // ---- Unsetter ---- // void AudioProgramme::unset( @@ -120,6 +131,10 @@ namespace adm { detail::ParameterTraits::tag) { refScreen_ = boost::none; } + void AudioProgramme::unset( + detail::ParameterTraits::tag) { + authoringInformation_ = boost::none; + } // ---- References ---- // bool AudioProgramme::addReference(std::shared_ptr content) { diff --git a/src/elements/authoring_information.cpp b/src/elements/authoring_information.cpp new file mode 100644 index 00000000..d9067f3b --- /dev/null +++ b/src/elements/authoring_information.cpp @@ -0,0 +1,46 @@ +#include "adm/elements/authoring_information.hpp" +#include "adm/elements/loudness_renderer.hpp" + +namespace adm { + + LoudnessRenderer Renderer::toLoudnessRenderer() const { + return LoudnessRenderer::fromRenderer(*this); + } + + void Renderer::print(std::ostream& os) const { + os << "("; + bool first = true; + auto sep = [&]() { + if (!first) os << ", "; + first = false; + }; + if (has()) { + sep(); + os << "uri=" << get(); + } + if (has()) { + sep(); + os << "name=" << get(); + } + if (has()) { + sep(); + os << "version=" << get(); + } + if (has()) { + sep(); + os << "coordinateMode=" << get(); + } + os << ")"; + } + + void AuthoringInformation::print(std::ostream& os) const { + os << "(referenceLayouts=" << get().size() + << ", renderers=" << get().size() << ")"; + } + + namespace detail { + template class VectorParameter; + template class VectorParameter; + } // namespace detail + +} // namespace adm diff --git a/src/elements/loudness_metadata.cpp b/src/elements/loudness_metadata.cpp index b9535554..32a19396 100644 --- a/src/elements/loudness_metadata.cpp +++ b/src/elements/loudness_metadata.cpp @@ -1,4 +1,5 @@ #include "adm/elements/loudness_metadata.hpp" +#include "adm/elements/authoring_information.hpp" #include @@ -120,6 +121,9 @@ namespace adm { void LoudnessMetadata::set(LoudnessRenderer renderer) { renderer_ = std::move(renderer); } + void LoudnessMetadata::set(Renderer renderer) { + renderer_ = renderer.toLoudnessRenderer(); + } // ---- Unsetter ---- // void LoudnessMetadata::unset(detail::ParameterTraits::tag) { diff --git a/src/elements/loudness_renderer.cpp b/src/elements/loudness_renderer.cpp index 3b0fc8ff..aebe5322 100644 --- a/src/elements/loudness_renderer.cpp +++ b/src/elements/loudness_renderer.cpp @@ -1,7 +1,49 @@ #include "adm/elements/loudness_renderer.hpp" +#include "adm/elements/authoring_information.hpp" namespace adm { + LoudnessRenderer LoudnessRenderer::fromRenderer(Renderer const& renderer) { + LoudnessRenderer loudnessRenderer; + if (renderer.has()) { + loudnessRenderer.set(renderer.get()); + } + if (renderer.has()) { + loudnessRenderer.set(renderer.get()); + } + if (renderer.has()) { + loudnessRenderer.set(renderer.get()); + } + if (renderer.has()) { + loudnessRenderer.set(renderer.get()); + } + if (renderer.has()) { + loudnessRenderer.set(renderer.get()); + } + return loudnessRenderer; + } + + Renderer LoudnessRenderer::toRendererDroppingObjectRefs( + RendererUri uri) const { + Renderer renderer{std::move(uri)}; + if (has()) { + renderer.set(get()); + } + if (has()) { + renderer.set(get()); + } + if (has()) { + renderer.set(get()); + } + if (has()) { + renderer.set(get()); + } + if (has()) { + renderer.set(get()); + } + return renderer; + } + void LoudnessRenderer::print(std::ostream& os) const { os << "("; bool first = true; diff --git a/src/private/copy.cpp b/src/private/copy.cpp index 55bdea7f..a71a8886 100644 --- a/src/private/copy.cpp +++ b/src/private/copy.cpp @@ -3,6 +3,119 @@ namespace adm { + namespace { + RendererPackFormatIdRefs remapPackFormatRefs( + RendererPackFormatIdRefs const& refs, ElementMapping const& mapping) { + RendererPackFormatIdRefs remapped; + remapped.reserve(refs.size()); + for (auto const& ref : refs) { + auto it = mapping.audioPackFormat.find(ref); + if (it != mapping.audioPackFormat.end()) { + remapped.push_back(it->second); + } else { + remapped.push_back(ref); + } + } + return remapped; + } + + RendererObjectIdRefs remapObjectRefs(RendererObjectIdRefs const& refs, + ElementMapping const& mapping) { + RendererObjectIdRefs remapped; + remapped.reserve(refs.size()); + for (auto const& ref : refs) { + auto it = mapping.audioObject.find(ref); + if (it != mapping.audioObject.end()) { + remapped.push_back(it->second); + } else { + remapped.push_back(ref); + } + } + return remapped; + } + + void remapAuthoringRendererReferences( + std::shared_ptr const& programme, + ElementMapping const& mapping) { + if (!programme->has()) { + return; + } + + auto info = programme->get(); + if (!info.has()) { + return; + } + + auto renderers = info.get(); + bool changed = false; + for (auto& renderer : renderers) { + if (!renderer.has()) { + continue; + } + + auto remapped = remapPackFormatRefs( + renderer.get(), mapping); + if (remapped.empty()) { + renderer.unset(); + } else { + renderer.set(std::move(remapped)); + } + changed = true; + } + + if (changed) { + info.set(std::move(renderers)); + programme->set(std::move(info)); + } + } + + template + void remapLoudnessRendererReferences(std::shared_ptr const& owner, + ElementMapping const& mapping) { + if (!owner->template has()) { + return; + } + + auto loudnessMetadatas = owner->template get(); + bool changed = false; + for (auto& loudnessMetadata : loudnessMetadatas) { + if (!loudnessMetadata.template has()) { + continue; + } + + auto renderer = loudnessMetadata.template get(); + + if (renderer.template has()) { + auto remapped = remapPackFormatRefs( + renderer.template get(), mapping); + if (remapped.empty()) { + renderer.template unset(); + } else { + renderer.set(std::move(remapped)); + } + changed = true; + } + + if (renderer.template has()) { + auto remapped = remapObjectRefs( + renderer.template get(), mapping); + if (remapped.empty()) { + renderer.template unset(); + } else { + renderer.set(std::move(remapped)); + } + changed = true; + } + + loudnessMetadata.set(std::move(renderer)); + } + + if (changed) { + owner->set(std::move(loudnessMetadatas)); + } + } + } // namespace + std::vector copyAllElements( std::shared_ptr document, ElementMapping& mapping) { std::vector copiedElements; @@ -78,6 +191,18 @@ namespace adm { resolveReference(element, mapping.audioTrackUid, mapping.audioChannelFormat); } + + for (const auto& element : document->getElements()) { + auto copiedProgramme = mapping.audioProgramme.at(element); + remapAuthoringRendererReferences(copiedProgramme, mapping); + remapLoudnessRendererReferences(copiedProgramme, mapping); + } + + for (const auto& element : document->getElements()) { + auto copiedContent = mapping.audioContent.at(element); + remapLoudnessRendererReferences(copiedContent, mapping); + } + return copiedElements; } diff --git a/src/private/document_parser.cpp b/src/private/document_parser.cpp index 118a54e1..d1dda1c6 100644 --- a/src/private/document_parser.cpp +++ b/src/private/document_parser.cpp @@ -4,6 +4,8 @@ #include "adm/detail/named_type_validators.hpp" #include "adm/errors.hpp" +#include + namespace adm { namespace xml { @@ -111,6 +113,9 @@ namespace adm { resolveReference(streamFormatChannelFormatRef_); resolveReference(streamFormatPackFormatRef_); resolveReferences(streamFormatTrackFormatRefs_); + resolveProgrammeAuthoringRendererReferences(); + resolveProgrammeLoudnessRendererReferences(); + resolveContentLoudnessRendererReferences(); // add other ADM elements to ADM document for (NodePtr node = root->first_node(); node; @@ -235,10 +240,46 @@ namespace adm { setOptionalMultiElement(node, "loudnessMetadata", audioProgramme, &parseLoudnessMetadatas); setOptionalElement(node, "audioProgrammeReferenceScreen", audioProgramme, &parseAudioProgrammeReferenceScreen); + setOptionalElement(node, "authoringInformation", audioProgramme, &parseAuthoringInformation); addOptionalReferences(node, "audioContentIDRef", audioProgramme, programmeContentRefs_, &parseAudioContentId); addOptionalElements + + diff --git a/tests/test_data/xml_parser/authoring_information.xml b/tests/test_data/xml_parser/authoring_information.xml new file mode 100644 index 00000000..e26acccd --- /dev/null +++ b/tests/test_data/xml_parser/authoring_information.xml @@ -0,0 +1,21 @@ + + + + + + + + + AP_00010003 + + + AP_00010003 + AP_00010017 + + + + + + + + diff --git a/tests/test_data/xml_parser/loudness_renderer.xml b/tests/test_data/xml_parser/loudness_renderer.xml index aaffb6e9..f17df930 100644 --- a/tests/test_data/xml_parser/loudness_renderer.xml +++ b/tests/test_data/xml_parser/loudness_renderer.xml @@ -13,6 +13,8 @@ + + diff --git a/tests/xml_authoring_information_tests.cpp b/tests/xml_authoring_information_tests.cpp new file mode 100644 index 00000000..9965032a --- /dev/null +++ b/tests/xml_authoring_information_tests.cpp @@ -0,0 +1,37 @@ +#include +#include +#include "adm/document.hpp" +#include "adm/elements/audio_programme.hpp" +#include "adm/elements/audio_programme_ref_screen.hpp" +#include "adm/elements/authoring_information.hpp" +#include "adm/elements/coordinate_mode.hpp" +#include "adm/parse.hpp" +#include "adm/write.hpp" +#include "helper/file_comparator.hpp" + +using namespace adm; + +TEST_CASE("xml/authoring_information") { + auto document = parseXml("xml_parser/authoring_information.xml"); + + auto programme = document->lookup(parseAudioProgrammeId("APR_1001")); + REQUIRE(programme->has()); + auto info = programme->get(); + REQUIRE(info.get().size() == 1); + auto renderers = info.get(); + REQUIRE(renderers.size() == 1); + auto const& r = renderers.at(0); + REQUIRE(r.get() == + std::string{"urn:itu:bs:2127:0:itu_adm_renderer"}); + REQUIRE(r.get() == std::string{"polar"}); + REQUIRE(r.get().size() == 2); + + REQUIRE(programme->has()); + auto screen = programme->get(); + REQUIRE(screen.has()); + REQUIRE(screen.get() == std::string{"cartesian"}); + + std::stringstream xml; + writeXml(xml, document); + CHECK_THAT(xml.str(), EqualsXmlFile("authoring_information")); +} From 51ed3993bee9a6e05ae5e1c136397d7ce6b823dd Mon Sep 17 00:00:00 2001 From: Richard Bailey Date: Wed, 22 Jul 2026 13:55:21 +0100 Subject: [PATCH 7/9] feat (authoring-infomation) Add ReferenceLayout, RecursionGuard, ref forward parsing [copilot] - Add ReferenceLayout type - Remap renderer references on document deepCopy() + tests - Ensure renderer references located after renderer parse correctly - Guard against very deeply nested documents overflowing the stack on add() --- include/adm/document.hpp | 3 + .../adm/elements/authoring_information.hpp | 33 +- include/adm/private/document_parser.hpp | 3 +- src/document.cpp | 181 ++++++++ src/private/copy.cpp | 57 ++- src/private/document_parser.cpp | 111 +++-- src/private/rapidxml_formatter.cpp | 3 +- tests/adm_document_tests.cpp | 389 ++++++++++++++++++ tests/authoring_information_tests.cpp | 13 +- ...audio_stream_format_forward_track_refs.xml | 16 + ...uthoring_information_forward_pack_refs.xml | 23 ++ ...horing_information_identical_renderers.xml | 19 + ...thoring_information_multiple_renderers.xml | 19 + .../loudness_renderer_forward_refs.xml | 35 ++ tests/xml_authoring_information_tests.cpp | 80 +++- tests/xml_loudness_renderer_tests.cpp | 37 ++ .../xml_parser_audio_stream_format_tests.cpp | 15 + 17 files changed, 970 insertions(+), 67 deletions(-) create mode 100644 tests/test_data/xml_parser/audio_stream_format_forward_track_refs.xml create mode 100644 tests/test_data/xml_parser/authoring_information_forward_pack_refs.xml create mode 100644 tests/test_data/xml_parser/authoring_information_identical_renderers.xml create mode 100644 tests/test_data/xml_parser/authoring_information_multiple_renderers.xml create mode 100644 tests/test_data/xml_parser/loudness_renderer_forward_refs.xml diff --git a/include/adm/document.hpp b/include/adm/document.hpp index 96dd009d..34d52436 100644 --- a/include/adm/document.hpp +++ b/include/adm/document.hpp @@ -102,6 +102,9 @@ namespace adm { * TagGroup entries in tagList that reference the removed element * - If the TagList becomes empty due to pruning, the list itself is * removed as it is only valid when it contains 1 or more TagGroups + * - removing an AudioPackFormat or AudioObject also removes matching + * renderer/referenceLayout ID references in nested + * authoringInformation/loudnessMetadata structures. */ ///@{ /// @brief Remove an AudioProgramme diff --git a/include/adm/elements/authoring_information.hpp b/include/adm/elements/authoring_information.hpp index a6441601..edae6307 100644 --- a/include/adm/elements/authoring_information.hpp +++ b/include/adm/elements/authoring_information.hpp @@ -8,7 +8,6 @@ #include "adm/detail/named_option_helper.hpp" #include "adm/detail/named_type.hpp" #include "adm/detail/optional_comparison.hpp" -#include "adm/elements/audio_pack_format_id.hpp" #include "adm/elements/renderer_common_types.hpp" #include "adm/export.h" @@ -18,10 +17,34 @@ namespace adm { /// @brief Tag for ::ReferenceLayout named-type struct ReferenceLayoutTag {}; - /// @brief NamedType wrapping the audioPackFormatIDRef of a referenceLayout - /// sub-element of authoringInformation (BS.2076-3 Table A1-51). - using ReferenceLayout = - detail::NamedType; + /** + * @brief Reference to audioPackFormat used by a referenceLayout + * sub-element of authoringInformation (BS.2076-3 Table A1-51). + */ + class ReferenceLayout { + public: + using tag = ReferenceLayoutTag; + + ReferenceLayout() = default; + explicit ReferenceLayout(std::shared_ptr packFormat) + : packFormat_(std::move(packFormat)) {} + + std::shared_ptr const &get() const { return packFormat_; } + + private: + std::shared_ptr packFormat_; + }; + + ADD_TRAIT(ReferenceLayout, ReferenceLayoutTag); + + inline bool operator==(ReferenceLayout const &lhs, + ReferenceLayout const &rhs) { + return lhs.get() == rhs.get(); + } + inline bool operator!=(ReferenceLayout const &lhs, + ReferenceLayout const &rhs) { + return !(lhs == rhs); + } /// @brief Vector of ReferenceLayout using ReferenceLayouts = std::vector; diff --git a/include/adm/private/document_parser.hpp b/include/adm/private/document_parser.hpp index 65276444..cfad83f8 100644 --- a/include/adm/private/document_parser.hpp +++ b/include/adm/private/document_parser.hpp @@ -46,7 +46,6 @@ namespace adm { AudioProgrammeReferenceScreen parseAudioProgrammeReferenceScreen( NodePtr node); Renderer parseRenderer(NodePtr node); - ReferenceLayout parseReferenceLayout(NodePtr node); AuthoringInformation parseAuthoringInformation(NodePtr node); Label parseLabel(NodePtr node); AudioBlockFormatObjects parseAudioBlockFormatObjects( @@ -186,6 +185,8 @@ namespace adm { std::vector objectIds; }; + std::map, std::vector> + programmeAuthoringReferenceLayoutPackFormatRefs_; std::map, std::vector>> programmeAuthoringRendererPackFormatRefs_; diff --git a/src/document.cpp b/src/document.cpp index ffcd43e5..aa9f9cc7 100644 --- a/src/document.cpp +++ b/src/document.cpp @@ -8,6 +8,7 @@ #include "adm/private/copy.hpp" #include +#include namespace adm { namespace { @@ -50,6 +51,175 @@ namespace adm { template class OptionalParameter; } // namespace detail + namespace { + // Bound the recursion depth of Document::add() to prevent a stack + // overflow when the cross-reference graph is pathologically deep + // (e.g. an adversarial XML with thousands of nested + // audioPackFormatIDRefs). Re-entering an already-added element is + // already short-circuited by checkParent(), so this only fires on + // genuinely deep, unique chains. The limit is far above any realistic + // ADM document. + constexpr int kMaxAddRecursionDepth = 1000; + thread_local int g_addRecursionDepth = 0; + + struct AddRecursionGuard { + AddRecursionGuard() { + if (g_addRecursionDepth >= kMaxAddRecursionDepth) { + throw std::runtime_error( + "Document::add: cross-reference recursion depth exceeded " + "(possible deeply-nested or malformed input)"); + } + ++g_addRecursionDepth; + } + ~AddRecursionGuard() { --g_addRecursionDepth; } + AddRecursionGuard(const AddRecursionGuard&) = delete; + AddRecursionGuard& operator=(const AddRecursionGuard&) = delete; + }; + + template + bool pruneRendererRefs( + RendererType& renderer, + std::shared_ptr const& removedElement) { + if (!renderer.template has()) { + return false; + } + auto const removedId = removedElement->template get(); + auto refs = renderer.template get(); + pruneIf(refs, [&removedId, &removedElement](auto const& ref) { + if (ref == removedElement) { + return true; + } + bool removed = ref->template get() == + removedId; + return removed; + }); + + if (refs.empty()) { + renderer.template unset(); + } else { + renderer.set(std::move(refs)); + } + return true; + } + + template + bool pruneLoudnessMetadataIdRefs( + LoudnessMetadatas& data, + std::shared_ptr removedElement) { + bool changed = false; + for (auto& loudnessMetadata : data) { + if (!loudnessMetadata.has()) { + continue; + } + auto renderer = loudnessMetadata.get(); + if (!pruneRendererRefs>>( + renderer, + removedElement)) { + continue; + } + loudnessMetadata.set(std::move(renderer)); + changed = true; + } + return changed; + + } + + template + void pruneElementLoudnessIdRefs( + std::shared_ptr const& element, + std::shared_ptr const& removedElement) { + if (!element->template has()) { + return; + } + auto loudnessMetadatas = element->template get(); + if (!pruneLoudnessMetadataIdRefs(loudnessMetadatas, removedElement)) { + return; + } + element->set(std::move(loudnessMetadatas)); + } + + bool pruneAuthoringInformationPackFormatIdRefs( + AuthoringInformation& info, + AudioPackFormatId const& removedId, + std::shared_ptr const& removedPackFormat) { + bool changed = false; + + if (info.has()) { + auto renderers = info.get(); + bool renderersChanged = false; + for (auto& renderer : renderers) { + renderersChanged |= + pruneRendererRefs( + renderer, + removedPackFormat); + } + if (renderersChanged) { + if (renderers.empty()) { + info.unset(); + } else { + info.set(std::move(renderers)); + } + changed = true; + } + } + + if (info.has()) { + auto referenceLayouts = info.get(); + auto pruned = pruneIf(referenceLayouts, [&](ReferenceLayout const& layout) { + auto const& ref = layout.get(); + if (ref == removedPackFormat) { + return true; + } + return ref->get() == removedId; + }); + if (pruned) { + if (referenceLayouts.empty()) { + info.unset(); + } else { + info.set(std::move(referenceLayouts)); + } + changed = true; + } + } + + return changed; + } + + void prunePackFormatIdRefs( + Document& document, + AudioPackFormatId const& removedId, + std::shared_ptr const& removedPackFormat) { + for (auto const& programme : document.getElements()) { + if (programme->has()) { + auto info = programme->get(); + if (pruneAuthoringInformationPackFormatIdRefs( + info, + removedId, + removedPackFormat)) { + programme->set(std::move(info)); + } + } + pruneElementLoudnessIdRefs(programme, removedPackFormat); + } + + for (auto const& content : document.getElements()) { + pruneElementLoudnessIdRefs(content, removedPackFormat); + } + } + + void pruneObjectIdRefs( + Document& document, + std::shared_ptr const& removedObject) { + for (auto const& programme : document.getElements()) { + pruneElementLoudnessIdRefs(programme, removedObject); + } + + for (auto const& content : document.getElements()) { + pruneElementLoudnessIdRefs(content, removedObject); + } + } + } // namespace + Document::Document() { idAssigner_.document(this); } std::shared_ptr Document::create() { @@ -102,6 +272,7 @@ namespace adm { // ---- add elements ---- // bool Document::add(std::shared_ptr programme) { + AddRecursionGuard guard; if (!checkParent(programme, "AudioProgramme")) { idAssigner_.assignId(*programme); AudioProgrammeAttorney::setParent(programme, shared_from_this()); @@ -116,6 +287,7 @@ namespace adm { } bool Document::add(std::shared_ptr content) { + AddRecursionGuard guard; if (!checkParent(content, "AudioContent")) { idAssigner_.assignId(*content); AudioContentAttorney::setParent(content, shared_from_this()); @@ -130,6 +302,7 @@ namespace adm { } bool Document::add(std::shared_ptr object) { + AddRecursionGuard guard; if (!checkParent(object, "AudioObject")) { idAssigner_.assignId(*object); AudioObjectAttorney::setParent(object, shared_from_this()); @@ -152,6 +325,7 @@ namespace adm { } } bool Document::add(std::shared_ptr packFormat) { + AddRecursionGuard guard; if (!checkParent(packFormat, "AudioPackFormat")) { idAssigner_.assignId(*packFormat); AudioPackFormatAttorney::setParent(packFormat, shared_from_this()); @@ -169,6 +343,7 @@ namespace adm { } bool Document::add(std::shared_ptr channelFormat) { + AddRecursionGuard guard; if (!checkParent(channelFormat, "AudioChannelFormat")) { idAssigner_.assignId(*channelFormat); AudioChannelFormatAttorney::setParent(channelFormat, shared_from_this()); @@ -180,6 +355,7 @@ namespace adm { } bool Document::add(std::shared_ptr streamFormat) { + AddRecursionGuard guard; if (!checkParent(streamFormat, "AudioStreamFormat")) { idAssigner_.assignId(*streamFormat); AudioStreamFormatAttorney::setParent(streamFormat, shared_from_this()); @@ -207,6 +383,7 @@ namespace adm { } bool Document::add(std::shared_ptr trackFormat) { + AddRecursionGuard guard; if (!checkParent(trackFormat, "AudioTrackFormat")) { // NOTE: That the id assignment works properly the AudioStreamFormats // have to be added before the AudioTrackFormat. @@ -229,6 +406,7 @@ namespace adm { } bool Document::add(std::shared_ptr trackUid) { + AddRecursionGuard guard; if (!checkParent(trackUid, "AudioTrackUid")) { idAssigner_.assignId(*trackUid); AudioTrackUidAttorney::setParent(trackUid, shared_from_this()); @@ -291,6 +469,7 @@ namespace adm { audioContent->removeReference(object); } pruneTagGroupsReferencing(*this, object); + pruneObjectIdRefs(*this, object); return true; } return false; @@ -335,6 +514,7 @@ namespace adm { auto it = std::find(audioPackFormats_.begin(), audioPackFormats_.end(), packFormat); if (it != audioPackFormats_.end()) { + auto removedPackId = packFormat->get(); audioPackFormats_.erase(it); AudioPackFormatAttorney::setParent(packFormat, {}); for (auto& audioPackFormat : audioPackFormats_) { @@ -353,6 +533,7 @@ namespace adm { audioTrackUid->removeReference(); } } + prunePackFormatIdRefs(*this, removedPackId, packFormat); return true; } return false; diff --git a/src/private/copy.cpp b/src/private/copy.cpp index a71a8886..295eb5a2 100644 --- a/src/private/copy.cpp +++ b/src/private/copy.cpp @@ -34,7 +34,7 @@ namespace adm { return remapped; } - void remapAuthoringRendererReferences( + void remapAuthoringInformationReferences( std::shared_ptr const& programme, ElementMapping const& mapping) { if (!programme->has()) { @@ -42,31 +42,52 @@ namespace adm { } auto info = programme->get(); - if (!info.has()) { - return; - } - - auto renderers = info.get(); bool changed = false; - for (auto& renderer : renderers) { - if (!renderer.has()) { - continue; + if (info.has()) { + auto layouts = info.get(); + ReferenceLayouts remappedLayouts; + remappedLayouts.reserve(layouts.size()); + for (auto const& refLayout : layouts) { + auto const& ref = refLayout.get(); + auto it = mapping.audioPackFormat.find(ref); + if (it != mapping.audioPackFormat.end()) { + remappedLayouts.push_back(ReferenceLayout{it->second}); + } else { + remappedLayouts.push_back(ReferenceLayout{ref}); + } } - auto remapped = remapPackFormatRefs( - renderer.get(), mapping); - if (remapped.empty()) { - renderer.unset(); + if (remappedLayouts.empty()) { + info.unset(); } else { - renderer.set(std::move(remapped)); + info.set(std::move(remappedLayouts)); } changed = true; } - if (changed) { - info.set(std::move(renderers)); - programme->set(std::move(info)); + if (info.has()) { + auto renderers = info.get(); + for (auto& renderer : renderers) { + if (!renderer.has()) { + continue; + } + + auto remapped = remapPackFormatRefs( + renderer.get(), mapping); + if (remapped.empty()) { + renderer.unset(); + } else { + renderer.set(std::move(remapped)); + } + changed = true; + } + + if (changed) { + info.set(std::move(renderers)); + } } + + if (changed) programme->set(std::move(info)); } template @@ -194,7 +215,7 @@ namespace adm { for (const auto& element : document->getElements()) { auto copiedProgramme = mapping.audioProgramme.at(element); - remapAuthoringRendererReferences(copiedProgramme, mapping); + remapAuthoringInformationReferences(copiedProgramme, mapping); remapLoudnessRendererReferences(copiedProgramme, mapping); } diff --git a/src/private/document_parser.cpp b/src/private/document_parser.cpp index d1dda1c6..1b8a9708 100644 --- a/src/private/document_parser.cpp +++ b/src/private/document_parser.cpp @@ -247,6 +247,21 @@ namespace adm { addOptionalElements