-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add video track and attachment extraction #45
base: release/1.1
Are you sure you want to change the base?
Conversation
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.
Hi @chuong, thanks for submitting this PR! This functionality you wrote will be very useful once merged. I spent some time going over your changes and left some comments on some issues I am concerned with. Other than those I would suggest adding a few comments in the extraction functions to make a it a bit easier to follow what is happening. Let me know if you have any questions or need help making the changes I have requested.
new_attachment = MKVAttachment(file_path, | ||
name=attachment['file_name'], | ||
description=attachment['description']) | ||
if 'id' in attachment: | ||
new_attachment.id = attachment['id'] | ||
if 'size' in attachment: | ||
new_attachment.size = attachment['size'] | ||
if 'properties' in attachment and 'uid' in attachment['properties']: | ||
new_attachment.uid = attachment['properties']['uid'] |
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.
Looking at the mkvmerge output schema, it seems that "id", "size", and "properties" will always be present in an attachment's section of the info_json. Therefore, these should be added in the MKVAttachment constructor so they are documented and not just dynamically assigned.
Note that "uid" is not required within the "properties" section of the mkvmerge output schema. I recommend including "properties" as a parameter to the constructor, then within the constructor, check if "uid" exists and assign it to an attribute within the MKVAttachment object.
Perhaps an implementation that allows something like this:
new_attachment = MKVAttachment(
file_path,
name=attachment['file_name'],
description=attachment['description'],
id=attachment['id'],
size=attachment['size'],
properties=attachment['properties']
)
for track in self.tracks: | ||
if track._track_type == 'video': | ||
bname = splitext(basename(track.file_path))[0] | ||
name = '{}.mp4'.format(join(out_folder, bname + "_" + track.track_name)) |
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.
Three comments for this line:
-
Before using the out_folder parameter, you should run it through os.path.expanduser to ensure output directories like
~/data/
are compatible. -
track_name is an optional attribute in MKVTrack and will default to None. According to the mkvmerge output schema, "codec", "id", and "type" are the only required properties in a track. I would suggest combining these properties along with the filename to produce unique names for each video track.
-
Video tracks are not guaranteed to be compatible with mp4 containers. The mkvextract docs list the different types of video tracks here (the video tracks are prepended with a "V_"). I think the best solution for the time being is to create a dictionary that maps each video track type to a compatible container type and use this to decide on an extension.
name_args = [] | ||
for attachment in self.attachments: | ||
bname = splitext(basename(attachment.file_path))[0] | ||
name = join(out_folder, bname + "_" + attachment.name) |
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.
Same concerns as part 1 of the comment on line 688. As for the naming of attachment files, I think it would be more appropriate to keep the attachment name specified and not prepend the basename.
This pull request is to add a needed feature for people like me to extract color, depth and IR video tracks and calibration attachment from a recorded stream from Microsoft Azure Kinect. An example code using this new feature is like this:
My
AzureKinect.mkv
video file can be obtained from https://drive.google.com/open?id=1X62D8MX9jPfByfYp2l627WrvVXAk--AK