#5 MpvAddAssetRefValue doesn't duplicate metadata
As all the other list manipulation macros,
MpvAddAssetRefValue does a bulk structure copy as part
of its function.
However, metadata information, stored in the
pnmfMetadata and pmpvMetadata fields are *pointers* and
not structures themselves. So the pointers are copied,
rather than the metadata itself.
That's fine as far as it goes, but there's one awful
problem.
When MpvAssetRefFree is called, the metadata is
released via MpvFree. But this does not invalidate any
pointers that have been copied via MpvAddAssetRefValue,
whose copies have no reason to assume that the metadata
has been given back to the heap.
What's the impact of this?
If an asset Ref in an album has metadata, and a
MarkList is created via a routine like
MpvFindIndexAlbumAssets, then all the metadata
associated with that reference is removed when the
MarkList is freed, resulting in potetial bogus data
reads if another marklist is generated, or resulting in
double-free errors when the manifest itself is released.
What's ths solution?
I don't know what the intent of the designers is, but
I'd guess that either the metadata must be copied when
an assetRef is copied, or a reference counting
mechanism needs to be installed on the metadata.
Logged In: YES
user_id=903442
Ok, I've got a workaround in place. I wrote a couple of
functions that wipe the pnmfMetadata and pmpvMetadata
pointers from AssetRefs and MarkLists (just setting the
pointers to NULL, not actually releasing storage). I call
these functions just before I call the Mpv...Free calls, but
only when I'm calling these on data which I know is
duplicated from the manifest (and not actual manifest data).
This seems to be the most efficient solution to the problem,
since just about any other solution causes huge data bloat.
And the level of added complexity is actually relatively small.
Perhaps the mpvTools API needs to "Free" entry points: one
for "in manifest" data, and one for "copied from manifest"
data, where the "copied from" versions wipe the pointers
before calling the "in manifest" versions.
This is an API complexity issue, though, which would be nice
not to have.
-scole
Logged In: YES
user_id=640757
I'm one of the designers of the toolkit and I've reviewed
your comments. Good catch and analysis.
I would prefer not to perform a deep copy of the metadata
elements when populating an MpvMarkList within one of
the MpvFind... functions. As you say in your followup, that
would cause data bloat. I would instead propose changing
the behavior of the MpvMarkList structs that are populated
by the MpvFind... functions so that they don't own the
metadata elements that they point to. When freed they
wouldn't free the metadata elements. The metadata
elements would only be freed when the FileManifest struct
is freed.
This implies a dependency on the FileManifest struct, but
the dependency already exists due to the string pool
member of the FileManifest.
To achieve this change I would either introduce a new
MpvMarkList type or add a flag to the existing
MpvMarkList type that indicates whether it owns the
metadata elements that it points to. The new
MpvMarkList objects that you use with the MpvFind...
functions could then be created and freed with only one
restriction -- the FileManifest struct that owns all of the
metadata elements must be freed last.
This solution is actually quite similar to your workaround.
Would this work for you? Would you mind if the
MpvFind... functions are changed to use a new
MpvMarkList type?
Logged In: YES
user_id=903442
A different marklist type is fine, actually. We'd need
alternate types for anything the marklist includes, though,
I imagine. (In other words, the DependentMarkList doesn't
contain a standard AssetRefList, it contains a
DependentAssetRefList or something.)
I do like the idea of different types as opposed to a flag
in the data that says "dependent", mostly because I loathe
the idea of adding another field to the data structures and
bloating the load size. Though if it takes copious amounts
of code to handle that, then the trade-off is less clear.
Hopefully the code hit will be minimal.