Dive/transcoding by BryonLewis · Pull Request #521 · Kitware/dive

BryonLewis · 2021-01-06T20:51:19Z

transcoding.mp4

Notes:

The big note is that the Windows version is using the VIAME ffmpeg included in the VIAME desktop installed version. The Linux version doesn't support libx264 so right now I have it using the user/system installed ffmpeg if it exists to do the conversion.
Not a fan of it, but using the update-job ipc to signal a conversion job is complete by looking at the baseJob data inside of it. This is used to indicate to the user that they can't click on a 'recents' item until it is complete.
JSON response from windows ffprobe is mixed with the setup_viame.bat file so you need to take the stdout and split it based on some included JSON so looking for the { and }. I don't like this but it works for right now using the VIAME version of ffmpeg.
Image conversion spawns a process one at a time for each image, but it remains under a single job in the display. Windows has limits on how many commands can be chained together, this way our limit is depth recursion instead of the much smaller windows limit. Also included some info about the updating process. NOTE: if one image needs conversion I convert all images in the folder (I don't want the pain of managing two lists).

Linux ffpemg Fallback:
Going to use the new ffmpeg in VIAME if it exists, then the system install ffmpeg and finally fallback to the h264 encoder with linux and cuda if it is installed on the system.

BryonLewis · 2021-01-12T19:27:32Z

Okay going to update this to have some proper failing message for Windows and Linux
Windows:

Sees if the kwiver installation exists, if it doesn't inform the user to download the VIAME toolkit tools.

Linux:

Attempt the kwiver installation folder with libx264
On failure attempt system ffmpeg version
On failure attmept kwiver using CUDA and nv_enc codec

After failure we note that they should have kwiver installed from the VIAME toolkit tools.

…into DIVE/transcoding

subdavis

Only got about halfway through, need to work on some other stuff today.

subdavis · 2021-01-19T14:21:12Z

  return jobBase;
 }

+function checkMedia(


Please make this async and use spawn(), then await a close or error event.

https://stackoverflow.com/questions/53204192/how-can-i-make-child-process-spawn-return-a-promise-in-my-typescript-module

The main thread has a lot of responsibilities, including an IPC server and an express.js server. While ffprobe is running, spawnSync will block the event loop.

https://nodejs.org/en/docs/guides/dont-block-the-event-loop/

No problem, I think this might of been a holdover from when I was initially coding and didn't have the 'converting...' stuff implemented and thought that displaying should be done immediately.

subdavis · 2021-01-19T14:26:28Z

-      } else if (websafeVideoTypes.includes(mimetype)) {
-        /* TODO: Kick off video inspection and maybe transcode */
+      } else if (websafeVideoTypes.includes(mimetype) || otherVideoTypes.includes(mimetype)) {
+        if (!checkMedia(settings, path) || otherVideoTypes.includes(mimetype)) {


await new async function

subdavis · 2021-01-19T14:27:35Z

+  updater: DesktopJobUpdater,
+  { checkMedia, convertMedia }: {
+  checkMedia: (settings: Settings, path: string) => boolean;
+  convertMedia: ConvertMedia;


I didn't think about it at the time, but this has the added benefit of being really nice to unit test.

subdavis · 2021-01-19T15:02:15Z

+      {
+        meta: jsonMeta,
+        mediaList: srcDstList,
+        type: datasetType,


type is a member of jsonMeta, this is redundant.

subdavis · 2021-01-19T15:30:04Z

+    const extension = datasetType === 'video' ? '.mp4' : '.png';
+    mediaConvertList.forEach((item) => {
+      const destLoc = item.replace(jsonMeta.originalBasePath, projectDirAbsPath);
+      const destExt = destLoc.replace(npath.extname(item), extension);


This is a confusing name. destExt isn't an ext, it's the entire absolute path. Could you make this a more expressive variable name?

subdavis · 2021-01-19T15:39:32Z

+  if (existing.transcodingJobKey === transcodingJobKey) {
+    existing.transcodingJobKey = undefined;
+  }
+  _saveAsJson(projectDirInfo.metaFileAbsPath, existing);


move save into conditional? Nothing to save otherwise.

Why would the keys not match? Should an error be thrown if this unexpected condition occurs?

Also, _saveAsJson bypasses lock acquisition, so it should not be used. Should probably absorb that function into saveMetadata.

Co-authored-by: Brandon Davis <git@subdavis.com>

subdavis

Tested, works great. After addressing async spawn, this is good to merge.

subdavis · 2021-01-19T21:36:56Z

+  if (result.error) {
+    throw result.error;
+  }
+  // TODO: Don't like this, but Windows needs this


could you elaborate on why windows needs this?

ffprobe is located in the context of the VIAME install. You need to run the setup_viame.bat file first to get the proper PATH setup and then you immediately run the ffprobe afterwards. The batch file has a bunch of commands it logs to stdout when running which is added to by the ffprobe. I could try some redirecting of the stdout for the batch file and then see if afterwards if I could get it back. I may look into that to clean this up a bit.

subdavis · 2021-01-19T21:37:48Z

+    }
+  }
+  //Now we need to test for a local install with libx264
+  const localffmpeg = spawnSync('ffmpeg -encoders', { shell: '/bin/bash' });


I missed this in my first pass, but this should be spawn() also. spawnSync should be avoided.

subdavis · 2021-01-19T21:39:53Z

+                  <span class="primary--text text--darken-1 text-decoration-none">
+                    {{ recent.name }}
+                  </span>
+                  <span>


Suggested change

<span>

<span class="pl-4">

Match padding on other side of converting status

subdavis · 2021-01-19T22:04:41Z

+          Close
+        </v-btn>
+      </template>
+    </v-snackbar>


kudos for using v-snackbar and not further entrenching the snackbar service.

This could in the future be moved outside of Recent.vue into a more global location for the Desktop version to return error messages.

Co-authored-by: Brandon Davis <git@subdavis.com>

BryonLewis · 2021-01-20T01:00:03Z

Added to the utils.ts the function spawnResult(command, shell, args=[]) considering how often I want to spawn a command and just want the stdout back without any other stuff. We could use this to refactor the nvidia CUDA check in the future if we want to simplify stuff. It is based on the nvidia-smi check anyways.
There is a redirect on the windows version of setup_viame.bat to push the stdout output to null when using it for ffmpeg or ffprobe. This doesn't block the subsequent command so I could go back to utilizing the standard JSON parse for the output from the command. If the parse fails it should throw the error right there and that will get propagated up to the snackbar.

subdavis · 2021-01-20T14:19:14Z

Couple remaining concerns that I really didn't notice until aa01b3c (which was an improvement and helped me read the code).

Failures in transcoding provide no means of debugging. If we had to work with a customer to debug transcoding problems, I would much rather ask for the job working directory than a screenshot of the last 10 lines of output.
I don't see any reason not to capture every single bit of output from any process we spawn and dump it into a log for later inspection. Having it in the debug console is OK, but I'd rather not have to describe to people how to launch VIAME through command prompt to access that console.

Also, these are sort of in conflict.

job.workingDir should be a safe place that any function or process should be free to dump whatever it wants without consequence. This should be documented in the interface, I didn't communicate that intention.
This tool should never modify originalBasePath.

In general this is what I'm proposing. Feel free to disagree.

Did it call spawn()? It should be a job.
All jobs should have a working directory and logging inside ${VIAME_DATA_PATH}/DIVE_Jobs/.

That way, users know exactly where to look when anything goes wrong. Exceptions are the checks that run on the settings page.

EDIT: also, in the near future when jobs persist through app restart, I'd like to recover logs from that working directory to show.

FWIW: with a trivial modification, all jobs could use createKwiverWorkingDirectory generically to initialize the job.workingDir.

BryonLewis · 2021-01-20T14:46:30Z

I don't see any reason not to capture every single bit of output from any process we spawn and dump it into a log for later inspection. Having it in the debug console is OK, but I'd rather not have to describe to people how to launch VIAME through command prompt to access that console. Sending it to /dev/null isn't ideal.

This is only the setup_viame.bat for windows in the specific instance of utilizing ffprobe or ffmpeg. If there is an issue in setup_viame.bat it you would never even get this far because it wouldn't think that VIAME is installed based on the settings page. Removing this would bring back that weird stdout search of { and } to convert to JSON for ffprobe. The output of the batch script is the echoing at bunch of variables that are being set. There are no actual commands being executed.

job.workingDir should be a safe place that any function or process should be free to dump whatever it wants without consequence. This should be documented in the interface, I didn't communicate that intention.

This tool should never modify originalBasePath.

I can modify it so it creates a workingDir, the only thing in it would be the runlog.txt for the conversion considering I'll just use absolute paths for the input/output of the media conversion. I think I didn't do this initially because the only item in there would be the log.txt and didn't know if a ffmpeg conversion log was necessary to save, but you have good arguments as to support. I may even suggest a open Folder link in the Job panel that will directly open the folder to make it easier to find.

Did it call spawn()? It should be a job.

All jobs should have a working directory and logging inside ${VIAME_DATA_PATH}/DIVE_Jobs/.
A lot of the ffmpegCommand setup as well as ffprobe (checkMedia) utilize spawn for creation (or more specifically my modified spawnResult). I think these small checks don't necessarily warrant a job folder but understand that actual media conversion could.

subdavis · 2021-01-20T14:58:00Z

I'll leave it up to you. I would like for transcoding jobs to have their own working dir in the jobs folder, but if we want to come back to that later, I'm fine with it.

I can test again and we can merge this.

BryonLewis · 2021-01-20T16:31:23Z

Completed testing on windows, it is properly creating the runlog.txt for conversions and properly converting now.

subdavis

Please merge this before I request more arbitrary changes.

BryonLewis added 3 commits January 4, 2021 15:07

initial testing and structure

b232653

initial testing and structure

23545b6

Transcoding working for videos

9801de9

BryonLewis force-pushed the DIVE/transcoding branch from 0ab5719 to 9801de9 Compare January 6, 2021 20:56

BryonLewis added 6 commits January 8, 2021 15:03

Merge branch 'master' into DIVE/transcoding

c3a49d2

test mods

25fad6e

Image support and windows support

7350043

Windows support

47a3fe1

linux nvenc

1206b82

Merge branch 'master' into DIVE/transcoding

210553e

BryonLewis and others added 7 commits January 12, 2021 22:05

Added in ffmpeg switching for linux and feedback

a022f45

prep for windows testing

9fc5d3f

self review update

addc738

Merge branch 'master' into DIVE/transcoding

102fe33

refactoring

58ec0c5

windows updates

0c5f7ce

Merge branch 'master' into DIVE/transcoding

338dbd5

BryonLewis marked this pull request as ready for review January 18, 2021 22:17

BryonLewis added 2 commits January 19, 2021 08:25

small fixes

e24a31d

Merge branch 'DIVE/transcoding' of https://github.com/VIAME/VIAME-Web …

3bc296b

…into DIVE/transcoding

BryonLewis requested a review from subdavis January 19, 2021 13:54

subdavis suggested changes Jan 19, 2021

View reviewed changes

Update client/platform/desktop/backend/native/common.ts

5345e89

Co-authored-by: Brandon Davis <git@subdavis.com>

subdavis suggested changes Jan 19, 2021

View reviewed changes

BryonLewis and others added 3 commits January 19, 2021 18:16

Update client/platform/desktop/backend/native/common.ts

03dedb5

Co-authored-by: Brandon Davis <git@subdavis.com>

async commands and other update

aa01b3c

Windows JSON fix

e0e4774

Merge branch 'master' into DIVE/transcoding

7b79adb

jobLog, need to test in windows

5c2933b

BryonLewis force-pushed the DIVE/transcoding branch from ac5411e to 5c2933b Compare January 20, 2021 15:10

subdavis approved these changes Jan 20, 2021

View reviewed changes

BryonLewis merged commit 763fc42 into master Jan 20, 2021

subdavis deleted the DIVE/transcoding branch January 21, 2021 02:15

This was linked to issues Jan 22, 2021

DIVE Desktop Functionality for Video transcoding[FEATURE] #508

Closed

Enable h264 code for VIAME linux #526

Closed

Conversation

BryonLewis commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryonLewis commented Jan 12, 2021

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis commented Jan 20, 2021

Uh oh!

subdavis commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryonLewis commented Jan 20, 2021

Uh oh!

subdavis commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryonLewis commented Jan 20, 2021

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BryonLewis commented Jan 6, 2021 •

edited

Loading

BryonLewis Jan 19, 2021 •

edited

Loading

subdavis commented Jan 20, 2021 •

edited

Loading

subdavis commented Jan 20, 2021 •

edited

Loading