New BS.2076-3 features (tagList, profileList). #201
Conversation
5697cd0 to
4e5ed77
Compare
There was a problem hiding this comment.
Looks good, have added a few minor comments.
Only big issues are:
- Document copying: Currently this will fail as a tag in the copy will point to the elements in the original rather than the new document. Will need to update copy.cpp to fix this. I've added a (failing) test to highlight the issue
- Removal: Removing tagged elements from the document will result in tags pointing to elements that are no longer present. We could use weak_ptr instead of shared_ptr to hold the tag references which would solve that problem, but the tag groups themselves would then be invalid after the last reference was removed. Checking through all the tag groups every time an element is removed feels very inefficient. I guess we could store the tag groups as shared pointers on the elements rather than being owned by the tag list, and keep a weak_ptrs to the groups in the tag list, that way when the last reference to a group was removed the group would be destroyed. Downside is that it would make the setter / accessor methods for the tag group a bit more complicated. Feels like there should be a more elegant way to do it but I can't think of one right now.
| template <typename Element> | ||
| std::shared_ptr<const Element> getElement() const; | ||
|
|
||
| template <typename Element> | ||
| std::shared_ptr<Element> getElement(); | ||
|
|
There was a problem hiding this comment.
Unless I'm missing something specific about TagList and ProfileList, I'd be a bit inclined to remove getElement(), and instead add TagList and ProfileList to DocumentBase as OptionalElements - Partly for consistency with the rest of the API when retreiving individual sub elements (everywhere else uses get<T>()), partly so you get the usual has/get/set/isDefault behaviour for free. I think if we kept things as-it, a 2076-2 or below document would just return nullptr, which I don't think we do anywhere else to indicate an absence?
| bool TagGroup::addReference(std::shared_ptr<AudioProgramme> programme) { | ||
| auto it = | ||
| std::find(audioProgrammes_.begin(), audioProgrammes_.end(), programme); | ||
| if (it == audioProgrammes_.end()) { | ||
| audioProgrammes_.push_back(std::move(programme)); | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| bool TagGroup::addReference(std::shared_ptr<AudioContent> content) { | ||
| auto it = std::find(audioContents_.begin(), audioContents_.end(), content); | ||
| if (it == audioContents_.end()) { | ||
| audioContents_.push_back(std::move(content)); | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| bool TagGroup::addReference(std::shared_ptr<AudioObject> object) { | ||
| auto it = std::find(audioObjects_.begin(), audioObjects_.end(), object); | ||
| if (it == audioObjects_.end()) { | ||
| audioObjects_.push_back(std::move(object)); | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
If we're storing shared_ptrs to other elements, we'll need to handle removal from the tag list when those elements are removed from the document, and pointer-resassignment during a deep copy in copy.cpp
| void TagGroup::removeReference(std::shared_ptr<AudioProgramme> programme) { | ||
| auto it = | ||
| std::find(audioProgrammes_.begin(), audioProgrammes_.end(), programme); | ||
| if (it != audioProgrammes_.end()) { | ||
| audioProgrammes_.erase(it); | ||
| } | ||
| } | ||
|
|
||
| void TagGroup::removeReference(std::shared_ptr<AudioContent> content) { | ||
| auto it = std::find(audioContents_.begin(), audioContents_.end(), content); | ||
| if (it != audioContents_.end()) { | ||
| audioContents_.erase(it); | ||
| } | ||
| } | ||
|
|
||
| void TagGroup::removeReference(std::shared_ptr<AudioObject> object) { | ||
| auto it = std::find(audioObjects_.begin(), audioObjects_.end(), object); | ||
| if (it != audioObjects_.end()) { | ||
| audioObjects_.erase(it); | ||
| } | ||
| } |
There was a problem hiding this comment.
Better to pass the shared_ptr<> parameters as const& here as we're not storing a copy in this function, and their atomic reference counting has a (small) cost.
| std::shared_ptr<Document> parseFrame( | ||
| std::istream& stream, const FrameHeader& header, | ||
| xml::ParserOptions options, std::shared_ptr<Document> commonDefinitions) { | ||
| xml::DocumentParser parser(stream, options, commonDefinitions); | ||
| parser.setHeader(header); | ||
| return parser.parse(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Would be more clear if this was split out into a separate PR
| // DEBUG FUNCTIONS | ||
| int getTempId() { return temp_id_; }; | ||
| void setTempId(int n) { temp_id_ = n; }; |
There was a problem hiding this comment.
Should these be removed?
|
|
||
| std::shared_ptr<TagGroup> DocumentParser::parseTagGroup(NodePtr node) { | ||
| auto tagGroup = std::make_shared<TagGroup>(); | ||
| tagGroup->setTempId(rand() % 1000); |
There was a problem hiding this comment.
Should this tempId be removed?
| class TTag : private detail::TTagBase, | ||
| private detail::AddWrapperMethods<TTag> { |
There was a problem hiding this comment.
Could we not just call these Tag, TagValue TagClass? You can tell from context that they're associated with the TagGroup (i.e. they only ever appear in methods associated with the tag group)
| template <typename... Parameters> | ||
| explicit TagGroup(Parameters... namedArgs) { | ||
| detail::setNamedOptionHelper(this, std::move(namedArgs)...); | ||
| } |
There was a problem hiding this comment.
I think the spec says there must be at least one reference associated with a tag group, so it'd be good to enforce that as an invariant. I think you could do that by making 3 constructors:
template<typename... Parameters>
explicit TagGroup(std::shared_ptr<AudioObject> const& reference, Parameters... namedArgs);
template<typename... Parameters>
explicit TagGroup(std::shared_ptr<AudioContent> const& reference, Parameters... namedArgs);
template<typename... Parameters>
explicit TagGroup(std::shared_ptr<AudioProgramme> const& reference, Parameters... namedArgs);
And then making tag groups immutable. Alternatively you could make removing the last reference throw but that feels messier? What do you think?
| ADM_EXPORT bool add(std::shared_ptr<ProfileList> profileList); | ||
| /// @brief Add a tagList | ||
| ADM_EXPORT bool add(std::shared_ptr<TagList> tagList); |
There was a problem hiding this comment.
I think these should be set()? (you can't have >1 profileList or tagList in a document)
| std::shared_ptr<ProfileList> profileList_; | ||
| std::shared_ptr<TagList> tagList_; |
There was a problem hiding this comment.
These probably want to be boost::optional<> rather than std::shared_ptr<> as we don't need reference semantics I don't think, just the ability for the elements to not be present?
The new elements in BS.2076-3 - tagList and profileList - have been added. These are needed for the EBU Production Profile.
There's also a new 'parseFrame' routine to allow a S-ADM frame to be parsed without repeating the parsing of the common definitions.