Support 2076-2-style structures in reassignIds #184
Conversation
This implementation uses the ID value of the ATUID as part of the ACF ID. This means assigned ACF ID's will not necessarily be consecutive, (since some ATUID's may use common definitions ACFs which don't need defining) but they will be unique.... ... in most cases. It may throw in ID set methods due to conflicts when 2076-2-style references are mixed with 2076-1 references in the same document. This is because ACF ids mimic the associated ASF and ATF ID's which are assigned in a simple loop with an incrementing counter.
Don't respond as if the actual comparison failed
Not sure why it was commented out. Makes sense to have it in, and it works. Was commented out since "initial commit". Perhaps it wasn't working back then?
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #184 +/- ##
==========================================
+ Coverage 89.56% 89.91% +0.34%
==========================================
Files 125 125
Lines 5781 5812 +31
==========================================
+ Hits 5178 5226 +48
+ Misses 603 586 -17 ☔ View full report in Codecov by Sentry. |
| @@ -93,4 +97,48 @@ namespace adm { | |||
| } | |||
| ///@} | |||
|
|
|||
| class IdReassigner { | |||
There was a problem hiding this comment.
Could this go in the implementation file? It doesn't look like it should be used by users.
It might be worth a quick comment on IdReassigner and IdIssuer to say what they are for (especially in the presence if IdAssigner, which could also do with explaining itself).
I'm fine with the structure as it is, but I would be tempted to merge these into one class, and perhaps pass the document to the reassignXIds functions instead of storing it.
There was a problem hiding this comment.
I think passing document to IdReassigner::reassignAllIds might suggest that a single IdReassigner instance can be used to process multiple documents, which is fine, but it would mean reconstructing IdIssuer on each passed in document (also fine). Since that IdIssuer instance is associated with just that one document, they should be passed to functions together. I think then the code would become less clean. That said, I have no strong feeling either way...
There was a problem hiding this comment.
I guess we could wrap a Document with its IdIssuer in a struct and pass that around
There was a problem hiding this comment.
I think passing document to IdReassigner::reassignAllIds might suggest that a single IdReassigner instance can be used to process multiple documents
Oh yeah, true, it looks good to me as-is.
Avoid unnecessary incrementation
since just an int anyway
Closes #181
See issue ticket for more details
The approach has been to process ACFs stemming from ATUIDs in the
reassignAudioTrackUidIds. If an ACF is found, apply an available ACF ID.However, other ACF IDs (and ATF and ASF IDs) were applied in the
reassignAudioStreamFormatIdsfunction which just blindly applies IDs sequentially, because previously this was fine and logical to do - it was the only function assigning IDs to ACF elements. Therefore this function could now inadvertently assign the same ID to more than one ACF because there were no checks to see if the ID was available.Therefore the newgetAvailableIdValuefunction was written will find an available ID value to use with ACF, ATF and ASF groups. It ensures the ID value is available for all 3 element types so that they can share the same value and make the ADM XML more readable and easy to follow.The disadvantage of this approach is that available ID lookup has exponential complexity based on ACF count. However, given thatreassignIdsis not expected to be called repeatedly, and that even a "large" ADM document only has maybe 128 ACFs requiring ID assignment, this is probably fine.Update:
The whole reassignIds logic has been wrapped in a class, with an IdIssuer class used to track IDs and ensure only unique ID's are issued to elements. This avoids the need for lookups and reduces to linear complexity. Note that a common ID value is used for groups of ACF, ATF and ASF elements so that the ADM XML is more readable and easy to follow.