Skip to content
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.

Sign up for GitHub

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

Extend file type support #72

Open
wants to merge 1 commit into
base: release/1.0.9
Choose a base branch
from
Open

Extend file type support #72

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 29 additions & 31 deletions pymkv/MKVFile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,38 +93,36 @@ def __init__(self, file_path=None, title=None, mkvmerge_path='mkvmerge'):
self._link_to_next_file = None
self.tracks = []
self.attachments = []
if file_path is not None and verify_matroska(file_path, mkvmerge_path=self.mkvmerge_path):
# add file title
file_path = expanduser(file_path)
info_json = json.loads(sp.check_output([self.mkvmerge_path, '-J', file_path]).decode())
if self.title is None and 'title' in info_json['container']['properties']:
self.title = info_json['container']['properties']['title']

# add tracks with info
for track in info_json['tracks']:
new_track = MKVTrack(file_path, track_id=track['id'], mkvmerge_path=self.mkvmerge_path)
if 'track_name' in track['properties']:
new_track.track_name = track['properties']['track_name']
if 'language' in track['properties']:
new_track.language = track['properties']['language']
if 'language_ietf' in track['properties']:
new_track.language_ietf = track['properties']['language_ietf']
if 'default_track' in track['properties']:
new_track.default_track = track['properties']['default_track']
if 'forced_track' in track['properties']:
new_track.forced_track = track['properties']['forced_track']
if 'flag_commentary' in track['properties']:
new_track.flag_commentary = track['properties']['flag_commentary']
if 'flag_hearing_impaired' in track['properties']:
new_track.flag_hearing_impaired = track['properties']['flag_hearing_impaired']
if 'flag_visual_impaired' in track['properties']:
new_track.flag_visual_impaired = track['properties']['flag_visual_impaired']
if 'flag_original' in track['properties']:
new_track.flag_original = track['properties']['flag_original']
self.add_track(new_track)

# split options
self._split_options = []
if not verify_mkvmerge(mkvmerge_path=mkvmerge_path):
raise FileNotFoundError('mkvmerge is not at the specified path, add it there or change'
'the mkvmerge_path property')
if file_path is not None:
file_path = expanduser(file_path)
info = json.loads(sp.check_output([self.mkvmerge_path, '-J', file_path]).decode())
if info['container']['recognized'] is True and info['container']['supported'] is True:
Comment on lines +97 to +103
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the info['container']['recognized'] check belongs to the verify_matroska function, so we can reuse it in other checks down the road. And in that case, I think we would still need to use the verify_matroska check here since we are "importing" a file.

Copy link
Contributor Author

@Rubo3 Rubo3 Aug 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in verify_recognized, but I thought that check here is not enough, the file must also be supported, hence the and info['container']['supported'] is True. I removed verify_matroska because I found your package can work with file types other than MKV (I have tested it with MP4, but ideally it should work with the other supported file types, too) and then produce an MKV, and I wouldn't call the verify_supported because we already have the JSON dictionary, we can query it directly.

I also find the code in Verifications.py to be rather duplicated, and I think the info dictionary could be useful to other users, so I have come up with this, a new function identify_file (or maybe just identify) which does the heavy lifting for verify_matroska, verify_recognized and verify_supported:

def identify_file(file_path, mkvmerge_path='mkvmerge'):
    """Get information about about the source file. Same as `mvkmerge -J <file_path>`."""
    if not isinstance(file_path, (str, os.PathLike)):
        raise TypeError(f'"{file_path}" is not of type str or os.PathLike')
    if not isfile(file_path):
        raise FileNotFoundError(f'"{file_path}" does not exist')
    try:
        info = json.loads(sp.check_output([mkvmerge_path, '-J', file_path]).decode())
    except sp.CalledProcessError:
        raise ValueError(f'"{file_path}" could not be opened')
    return info


def verify_matroska(file_path, mkvmerge_path='mkvmerge'):
    """Verify if a file is a Matroska file.

    file_path (str):
        Path of the file to be verified.
    mkvmerge_path (str):
        Alternate path to mkvmerge if it is not already in the $PATH variable.
    """
    info = identify_file(file_path, mkvmerge_path)
    return info['container']['type'] == 'Matroska'


def verify_recognized(file_path, mkvmerge_path='mkvmerge'):
    """Verify a file is recognized by mkvmerge.

    file_path (str):
        Path to the file to be verified.
    mkvmerge_path (str):
        Alternate path to mkvmerge if it is not already in the $PATH variable.
    """
    info = identify_file(file_path, mkvmerge_path)
    return info['container']['recognized']


def verify_supported(file_path, mkvmerge_path='mkvmerge'):
    """Verify a file is supported by mkvmerge.

    file_path (str):
        Path to the file to be verified.
    mkvmerge_path (str):
        Alternate path to mkvmerge if it is not already in the $PATH variable.
    """
    info = identify_file(file_path, mkvmerge_path)
    return info['container']['supported']

I have removed the verify_mkvmerge check because I think it is not necessary here, the user can check it when they need, before using other functions which depend on mkvmerge, instead of having the package check it every time. (I have also not updated the documentation.)
What do you think?

# add tracks with info
for track in info['tracks']:
new_track = MKVTrack(file_path, track_id=track['id'], mkvmerge_path=self.mkvmerge_path)
if 'track_name' in track['properties']:
new_track.track_name = track['properties']['track_name']
if 'language' in track['properties']:
new_track.language = track['properties']['language']
if 'language_ietf' in track['properties']:
new_track.language_ietf = track['properties']['language_ietf']
if 'default_track' in track['properties']:
new_track.default_track = track['properties']['default_track']
if 'forced_track' in track['properties']:
new_track.forced_track = track['properties']['forced_track']
if 'flag_commentary' in track['properties']:
new_track.flag_commentary = track['properties']['flag_commentary']
if 'flag_hearing_impaired' in track['properties']:
new_track.flag_hearing_impaired = track['properties']['flag_hearing_impaired']
if 'flag_visual_impaired' in track['properties']:
new_track.flag_visual_impaired = track['properties']['flag_visual_impaired']
if 'flag_original' in track['properties']:
new_track.flag_original = track['properties']['flag_original']
self.add_track(new_track)

def __repr__(self):
return repr(self.__dict__)
Expand Down