-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sadm support #182
Sadm support #182
Conversation
b202c6c
to
e1aa399
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #182 +/- ##
==========================================
- Coverage 89.91% 88.76% -1.15%
==========================================
Files 125 141 +16
Lines 5812 6251 +439
==========================================
+ Hits 5226 5549 +323
- Misses 586 702 +116 ☔ View full report in Codecov by Sentry. |
5e167f3
to
83b6667
Compare
a34a98c
to
8da73d6
Compare
Sorry about the commit history, I wasn't sure how to slice it in a more digestable way so just left the full horror. Strongly suggest we do a squash/merge rather than dumping all this onto master. |
I'm not sure about InitializeBlock. Ideally we wouldn't add it to every audioBlockFormat, but just flag it on audioChannelFormat, but it's not clear (to me) whether it needs to be present on every audioBlockFormat, or just the first, and this way it's on the user rather than us mandating a (potentially wrong) approach during write/parse. The intent for the local/total timereference seems to be that it should be consistent throughout the frame (I think this is actually mentioned in the emission profile document?), which is why I implemented that via checking the header rather than having it on every block. It might have been nice to replace Rtime with Start (or added it as an alias?) in this context, as that name is closer to the semantic meaning for both local and total time, and works in both contexts. I didn't want to break the API (by removing Rtime) or do something confusing by having two types alias the same parameter in only this place (although now I think of it, we do sort of do that with TypeDefinition). |
I wasn't sure whether to make parsing stricter, and require audioFormatExtended nodes within a frame to be parsed using the method that takes a frame header. In the end I opted to allow it (by adding a flag that allows you to disable checking time-reference consistency, as parsing using the original parseXml method would otherwise fail if you encountered an lstart/lduration), but maybe there's not a use-case for that? I remember thinking there was at the time, but now... |
One disadvantage relative to the v2 implementation is the fact that you parse twice for SADM (once for the header, once for the document) - I don't think this is an issue from a performance point of view, but it might be a useability issue from the point of view of using input streams - you'd need to reset them between calls. If that's an issue I don't think it's a big job to rethink the parsing interface to return a handle to the xml representation that could be passed to the two functions to avoid doing the first bit twice, or to provide a wrapper that returns both together and does that internally. |
For initializeBlock, I think there should be one for the first audioBlockFormat in each frame. So if there's multiple audioBlockFormats within the frame, then only the first one should have it. |
include/adm/detail/id_parser.hpp
Outdated
throw std::runtime_error(errorString.str()); | ||
} | ||
} | ||
|
||
void check_prefix(const char *prefix, size_t size) const { | ||
assert(prefix && prefix[size] == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this results in an unused parameter warning in non-debug builds
I'm not sure about this change -- the original intent was to avoid allocating a std::string for every ID that is parsed, and this undoes that, while also having the length passed for no reason.
Perhaps it would be better to just use check_prefix(std::string const &prefix)
, and put the prefixes somewhere as static std::string
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a bit nerd-sniped by this. b0ab49a and 1a3f6dd rework the id parsing. The first one was just an attempt at 'putting the prefixes somewhere', which led on to the second one via 'hey we could just..'
Lets you declare the id formatting via a couple of traits classes then let the parser work out what to do with that.
I think the end result makes the ID classes look nicer, with less duplication and a consistent order for validation/parsing, at the expense of making the id_parser more complicated. Not sure if it's a good thing or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libadm/src/elements/audio_block_format_id.cpp
Lines 105 to 138 in 1a3f6dd
namespace detail { | |
template <> | |
struct IdTraits<AudioBlockFormatId> { | |
static constexpr const char* name{"audioBlockFormatID"}; | |
static constexpr const char* format{"AB_yyyyxxxx_zzzzzzzz"}; | |
static constexpr std::size_t sections{3}; | |
}; | |
template <> | |
struct IdSection<AudioBlockFormatId, 0> { | |
using type = TypeDescriptor; | |
static constexpr char identifier{'y'}; | |
}; | |
template <> | |
struct IdSection<AudioBlockFormatId, 1> { | |
using type = AudioBlockFormatIdValue; | |
static constexpr char identifier{'x'}; | |
}; | |
template <> | |
struct IdSection<AudioBlockFormatId, 2> { | |
using type = AudioBlockFormatIdCounter; | |
static constexpr char identifier{'z'}; | |
}; | |
} // namespace detail | |
AudioBlockFormatId parseAudioBlockFormatId(const std::string& id) { | |
detail::IDParser<AudioBlockFormatId> parser{id}; | |
parser.validate(); | |
return parser.parse(); | |
} | |
std::string formatId(const AudioBlockFormatId& id) { | |
return detail::formatId(id); | |
} |
is an example of what I mean.
The ID benchmarks show the same result as on master for parsing (within the variation I get between runs), and slightly better for formatting. Doesn't significantly impact performance either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable, though it's quite a bit more code.
My ultra-simple idea was to base everything on the template/format string at run-time -- like this but without templates -- you can check the length, prefix/underscores (by checking that all non-lowercase letters match), and use the placeholder letters to parse/fill in hex values.
// somewhere global
const std::sting format = "AB_yyyyxxxx_zzzzzzzz";
// parse
check_format(format, id); // checks length, AB_ and second _
int index = parse_hex(format, id, 'z'); // extracts z bit, repeat for y, x, z
// format
std::sting format = "AB_yyyyxxxx_zzzzzzzz";
std::strng id = format;
fill_hex(format, id, 'x', value) // fills in x part
Technically this means iterating over the strings a few more times, but maybe that's better than a lot of templates? The performance isn't really a problem, it just needs to not be very slow (like the regex-and-iostreams version).
This version is neat and looks good to me though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, that's a lot of code!
It seems good, i prefer this new way of dealing with the time references.
I might fix a few of the more nit-picky things myself, and leave commit references in the comments.
e965c0f
to
1a3f6dd
Compare
Adds SADM support to libadm
New features
frameHeader
element introduced in BS.2125-1 and all its childrenparseFrameHeader()
method introduced to parse this new elementInitialiseBlock
parameter added to allaudioBlockFormat
typesparseXml
andwriteXml
methods added to support parsing and writing of xml frames using the additional information from aFrameHeader
obect, together with theaudioFormatExtended
node represented by aDocument
lstart
/lduration
onaudioBlockFormat
now supported by settingTimeReference
in theFrameFormat
subelement ofFrameHeader
, then using the new writeXml / parseXml overloads.status
,timeReference
,frameType
) are represented withenum class
rather thanstd::string
basedNamedType
s (used elsewhere in libadm), to reduce scope for error. A newformatValue()
function is added for serialising these to the strings used in the standard, to mirrorformatId()
.Misc changes
WRONG_00000001
rather thanAP_00000001
for anaudioPackFormatID
, you would now get a parse error telling you the prefix is incorrect, rather than one telling you the ID in the wrong length, for example.Changes relative to SADM-v2 branch
Frame
class removed andDocument
inheritance heirachy reverted.XMLParser
SADMParser
inheritance heirachy removed,XMLParser
renamedDocumentParser
and used to parse theaudioFormatExtended
node to aDocument
in both ADM and SADM documents. SeparateFrameHeaderParser
used for parsing xml to aFrameHeader
frameID
formats by use of two parameters,FrameIndex
andChunkIndex
Start
andDuration
types rather than introducing newFrameStart
andFrameDuration
types, as the attribute names and semantics match.NumSubFrame
/FrameSkip
/FrameShift
types removed as no longer present in standardLStart
LDuration
elements removed fromaudioBlockFormat
classes and instead inferred from value ofTimeReference
. There is an option to disable checks during parsing.CountToSameChunk
/NumMetadataChunks
/ProfileList
/ChangedIds
representations addedCartesian
flag in more block format types,ProfileList
inDocument
)FrameFormat get<FrameFormat>();
rather thanFrameFormat& frameFormat();
VectorParameter<>
used over iterator-based apis for elements that have multiples, as all of these things are likely to be relatively small (1s-100s, not 10000s). The iterator based APIs are error-prone (this has cropped up a few times when editing audioBlockFormats, for example)auto_base.hpp
where possible to reduce scope for copy/paste errors / inconsistent behaviour of above standard methods