Skip to content

Adding in forcing transcode option#928

Merged
BryonLewis merged 4 commits intomainfrom
desktop/force-transcode
Sep 21, 2021
Merged

Adding in forcing transcode option#928
BryonLewis merged 4 commits intomainfrom
desktop/force-transcode

Conversation

@BryonLewis
Copy link
Collaborator

fixes #922

Added in new Transcode option under the advanced import options.
Transcode will add the video to the mediaConvertList (The list that will determine if items are not web safe and should be transcoded, or transcoded for non-square pixels/ frame misalignment).
Selection_354

Selection_355

Selection_356

@BryonLewis BryonLewis marked this pull request as ready for review September 16, 2021 12:49

const forceMediaTranscode = (event: boolean) => {
if (event) {
const mediaFile = `${argCopy.value.jsonMeta.originalBasePath}/${argCopy.value.jsonMeta.originalVideoFile}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm conflicted about this line. Nowhere else in the client code is privy to the details of how a disk path is constructed. Everywhere else, the client allows the server to provide a resource URL to media files and does not attempt to make assumptions about disk layout from information in DatasetMeta.

Also, you can't guarantee originalBasePath doesn't end with /, leaving you with "/basepath//filepath.mp4"; You need to use import npath from 'path'; npath.join(), but this is a nodejs only library, so it can't be done on the client.

Does this work on windows? Wouldn't you end up with "C:\\some\base\path/video.mp4"? And if not, what if native/common.ts changed in the future such that it was? That change should only affect the node backend code, not client code.

I think you need a new boolean force transcode flag on the import payload.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a great point. I was avoiding the npath import but didn't fully think of the implications especially when it comes to windows. I'll change it to an option on the importayload

@subdavis subdavis self-requested a review September 20, 2021 14:15
Copy link
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

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

  • I liked the effect in the previous iteration where turning on force transcoding enabled the blue transcoding alert message.
  • Maybe add a short description to the switch to explain what this toggle does and why it might be necessary, though if you think this is overkill, feel free to ignore.

Dataset will not be available until transcoding is complete.
</v-alert>
<v-alert
v-if="importData.mediaConvertList.length === 0 && argCopy.mediaConvertList.length"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is out of date.

@BryonLewis
Copy link
Collaborator Author

I noticed during large media file imports the screen was empty for a few seconds while the checkMedia was doing the ffprobe frame check. The length of time seems to be dependent on the file and the container. Instead of leaving the screen empty I added the dialog with a Calculating... progress to let the user know that at least something is happening and it wasn't freezing up.

To replicate import any of the DIVA AVI videos to see the delay.

@subdavis subdavis self-requested a review September 21, 2021 17:14
Copy link
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

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

Works as expected. Any solution to the ffprobe issue should be a separate PR since it isn't caused by this change.

@BryonLewis BryonLewis merged commit 66325b5 into main Sep 21, 2021
@subdavis subdavis deleted the desktop/force-transcode branch September 21, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Force transcode option in DIVE Desktop

2 participants

Comments