Allow users to generate IMF metadata files by passing in a simple timeline of tracks of media files #363
Conversation
tcase-netflix
requested review from
svenkatrav,
davidt-netflix and
danielhdz13-netflix
| public List<MediaFile> mediaFiles; | ||
| } | ||
|
|
||
| public static class MediaFile { |
There was a problem hiding this comment.
I would suggest that we rename this to something other than 'MediaFile'. The term 'file' may be misleading, because what this class actually holds is more than just a file. It also includes timing and edit information used to define an entry in the track. Maybe the name 'TrackEntry' would be more appropriate?
In IMF terminology, within a CPL this is called a Resource. A resource identifies the specific MXF file ('TrackFile'), but also how it is being used within the track for this particular instance. Multiple Resources can point to the same MXF file. Example:
- Resource1:
mainVideo_01.mxf, frames 0-500 // Uses the first 500 frames of one file - Resource2:
videoInsert.mxf, frames 0-48 // Adds the first 48 frames from a different file - Resource3:
mainVideo_01.mxf, frames 548-3000 // Back to the first file again, skipping the next 48 frames
In the above example, mainVideo_01.mxf is used twice. The first and third entries in the track are different Resources, but they reference the same 'TrackFile'.
If I understand correctly, your definition of MediaFile is trying to do something similar to the CPL Resource, right? It is possible for the same MXF file to be present in multiple MediaFiles per Track, correct? If that is the case, it might make sense to avoid using the word 'file' in the class name, just for clarity.
There was a problem hiding this comment.
Thanks @davidt-netflix I'll rename MediaFile to TrackEntry.. It is possible to use the same file multiple times in the track - the unit test does this. I'll update the class name.
There was a problem hiding this comment.
@davidt-netflix , I overwrote the previous commit and addressed the issues you mentioned. Please let me know what you think!
| logger.info("Adding file to resources: {}..", mediaFile.getFile().getName()); | ||
| resources.add( | ||
| new IMFTrackFileResourceType( | ||
| UUIDHelper.fromUUID(UUID.randomUUID()), |
There was a problem hiding this comment.
This seems pretty minor, but everywhere else in Photon uses this same utility class for generating UUIDs
IMFUUIDGenerator.getInstance().generateUUID()
I looked at the code, and it seems that method is just internally calling UUID.RandomUUID() anyway, with an additional check to make sure that we aren't reusing the same UUID again. I'm not familiar with the original context for the decision to wrap this in a utility... maybe there was a problem where the random UUID generator could hand out the same value multiple times? Maybe this is just tech debt?
But for consistency, it might just be best to switch this to using the Photon utility, rather than the normal java generator.
There was a problem hiding this comment.
Thank you, will update!
| PackingList packingList = new PackingList(pklOutputFile); | ||
| for (PackingList.Asset asset : packingList.getAssets()) { | ||
| logger.debug("Asset from packing list: {}", asset); | ||
| // is there a better way to identify a CPL? |
There was a problem hiding this comment.
This isn't a great way to identify the CPL, but technically it will work for your specific usage.
The IMF standards do not dictate that CPL file names follow the pattern "CPL-" + uuid + ".xml". That is just part of Photon's IMPBuilder implementation. Now since (internally) you are using Photon's IMPBuilder, technically, your CPL's always should follow that pattern. But it is a fragile implementation, and may break if someone changes the file name generator code elsewhere.
I am also somewhat hesitant to do things that reinforce Photon's implementation as a "de-facto standard". So if we can avoid adding code that makes assumptions based on file names, I think that is the best bet.
Finding which files contain the CPL requires actually looking into the individual files. But you already have the PKL here, and that tells you the MIME type of each file. You can filter to only files of 'text/xml', and then just look for an XML document that has 'CompositionPlaylist' as the document element. Here is an
example where this logic already exists to find the CPL.
There was a problem hiding this comment.
Thank you for the reference, I'll use that method.
| mediaFile.getEntryPoint(), // defaults to 0 if null | ||
| mediaFile.getDuration(), // defaults to intrinsic duration if null | ||
| mediaFile.getRepeatCount(), // defaults to 1 if null | ||
| UUIDHelper.fromUUID(UUID.randomUUID()), // used as the essence descriptor id |
There was a problem hiding this comment.
There is a (minor) risk here, of simply using a random UUID for SourceEncoding, without doing additional record keeping.
Within a CPL, there should be a one-to-one mapping between SourceEncoding (specific Essence Descriptors) and TrackFileId (individual essence file).
But in a CPL, multiple Resources can reference the same TrackFile. In these cases, when an essence file is reused, the different Resource entries would be expected to have the same SourceEncoding value. By assigning this as a random UUID per Resource, this will create duplicate Essence Descriptors in the CPL.
This is actually a relatively common bug, and most IMF parsers can handle it okay. But I believe the correct way to manage this would be to create some sort of dictionary, mapping essence files (TrackFileId) to essence descriptors (SourceEncoding).
Here is an example where this is being done elsewhere in Photon.
There was a problem hiding this comment.
Good catch! I'll add a lookup check. Looking at the CPL generated in the unit test where the same TrackFile is used twice, only one essence descriptor is created per track file, (and the Resource entries that point to the same TrackFile contain the same SourceEncoding) as hoped.
… in a simple timeline of tracks.
tcase-netflix
force-pushed
the
create-imp-from-essences
branch
from
8b6d46d
to
cca7a84
Compare
No description provided.